Message ID | b8160b6dbaf4899ff95928a7af006a126baa8f9c.1707756045.git.josef@toxicpanda.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] fstests: add a regression test for fiemap into an mmap range | expand |
On 2/12/24 22:11, Josef Bacik wrote: > Btrfs had a deadlock that you could trigger by mmap'ing a large file and > using that as the buffer for fiemap. This test adds a c program to do > this, and the fstest creates a large enough file and then runs the > reproducer on the file. Without the fix btrfs deadlocks, with the fix > we pass fine. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > v1->v2: > - Add the fiemap group to the test. > - Actually include the reproducer helper program. > > src/Makefile | 2 +- > src/fiemap-fault.c | 73 +++++++++++++++++++++++++++++++++++++++++++ > tests/generic/740 | 41 ++++++++++++++++++++++++ > tests/generic/740.out | 2 ++ > 4 files changed, 117 insertions(+), 1 deletion(-) > create mode 100644 src/fiemap-fault.c > create mode 100644 tests/generic/740 > create mode 100644 tests/generic/740.out > > diff --git a/src/Makefile b/src/Makefile > index d79015ce..916f6755 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -34,7 +34,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \ > fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \ > detached_mounts_propagation ext4_resize t_readdir_3 splice2pipe \ > - uuid_ioctl > + uuid_ioctl fiemap-fault > > EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \ > btrfs_crc32c_forged_name.py popdir.pl popattr.py \ > diff --git a/src/fiemap-fault.c b/src/fiemap-fault.c > new file mode 100644 > index 00000000..27081188 > --- /dev/null > +++ b/src/fiemap-fault.c Pls add fiemap-fault file to .gitignore more below. > @@ -0,0 +1,73 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2024 Meta Platforms, Inc. All Rights Reserved. > + */ > + > +#include <sys/ioctl.h> > +#include <sys/mman.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <linux/fs.h> > +#include <linux/types.h> > +#include <linux/fiemap.h> > +#include <err.h> > +#include <errno.h> > +#include <fcntl.h> > +#include <stdio.h> > +#include <string.h> > +#include <unistd.h> > + > +int prep_mmap_buffer(int fd, void **addr) > +{ > + struct stat st; > + int ret; > + > + ret = fstat(fd, &st); > + if (ret) > + err(1, "failed to stat %d", fd); > + > + *addr = mmap(NULL, st.st_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0); > + if (*addr == MAP_FAILED) > + err(1, "failed to mmap %d", fd); > + > + return st.st_size; > +} > + > +int main(int argc, char *argv[]) > +{ > + struct fiemap *fiemap; > + size_t sz, last = 0; > + void *buf = NULL; > + int ret, fd; > + > + if (argc != 2) > + errx(1, "no in and out file name arguments given"); > + > + fd = open(argv[1], O_RDWR, 0666); > + if (fd == -1) > + err(1, "failed to open %s", argv[1]); > + > + sz = prep_mmap_buffer(fd, &buf); > + > + fiemap = (struct fiemap *)buf; > + fiemap->fm_flags = 0; > + fiemap->fm_extent_count = (sz - sizeof(struct fiemap)) / > + sizeof(struct fiemap_extent); > + > + while (last < sz) { > + int i; > + > + fiemap->fm_start = last; > + fiemap->fm_length = sz - last; > + > + ret = ioctl(fd, FS_IOC_FIEMAP, (unsigned long)fiemap); > + if (ret < 0) > + err(1, "fiemap failed %d (%s)", errno, strerror(errno)); > + for (i = 0; i < fiemap->fm_mapped_extents; i++) > + last = fiemap->fm_extents[i].fe_logical + > + fiemap->fm_extents[i].fe_length; > + } > + munmap() is missing. a few more below. > + close(fd); > + return 0; > +} > diff --git a/tests/generic/740 b/tests/generic/740 > new file mode 100644 > index 00000000..30ace1dd > --- /dev/null > +++ b/tests/generic/740 > @@ -0,0 +1,41 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2023 Meta Platforms, Inc. All Rights Reserved. > +# > +# FS QA Test 708 > +# > +# Test fiemap into an mmaped buffer of the same file > +# > +# Create a reasonably large file, then run a program which mmaps it and uses > +# that as a buffer for an fiemap call. This is a regression test for btrfs > +# where we used to hold a lock for the duration of the fiemap call which would > +# result in a deadlock if we page faulted. > +# > +. ./common/preamble > +_begin_fstest quick auto fiemap > +[ $FSTYP == "btrfs" ] && \ > + _fixed_by_kernel_commit xxxxxxxxxxxx \ > + "btrfs: fix deadlock with fiemap and extent locking" > + > +# real QA test starts here > +_supported_fs generic > +_require_test > +_require_odirect > +_require_test_program fiemap-fault > +dst=$TEST_DIR/fiemap-fault-$seq Could we pls use: dst=$TEST_DIR/$seq/fiemap-fault mkdir -p $dst Keeps $TEST_DIR organized. > + > +echo "Silence is golden" > + > +for i in $(seq 0 2 1000) > +do > + $XFS_IO_PROG -d -f -c "pwrite -q $((i * 4096)) 4096" $dst > +done > + > +$here/src/fiemap-fault $dst > /dev/null || _fail "failed doing fiemap" > + > +rm -f $dst Pls move this to custom clean up at _cleanup() within the testcase. Thanks, Anand > + > +# success, all done > +status=$? > +exit > + > diff --git a/tests/generic/740.out b/tests/generic/740.out > new file mode 100644 > index 00000000..3f841e60 > --- /dev/null > +++ b/tests/generic/740.out > @@ -0,0 +1,2 @@ > +QA output created by 740 > +Silence is golden
On Mon, 12 Feb 2024 11:41:14 -0500, Josef Bacik wrote: > Btrfs had a deadlock that you could trigger by mmap'ing a large file and > using that as the buffer for fiemap. This test adds a c program to do > this, and the fstest creates a large enough file and then runs the > reproducer on the file. Without the fix btrfs deadlocks, with the fix > we pass fine. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> With a .gitignore entry added for the fiemap-fault binary: Revewed-by: David Disseldorp <ddiss@suse.de> A few other minor nits below... > --- > v1->v2: > - Add the fiemap group to the test. > - Actually include the reproducer helper program. > > src/Makefile | 2 +- > src/fiemap-fault.c | 73 +++++++++++++++++++++++++++++++++++++++++++ > tests/generic/740 | 41 ++++++++++++++++++++++++ > tests/generic/740.out | 2 ++ > 4 files changed, 117 insertions(+), 1 deletion(-) > create mode 100644 src/fiemap-fault.c > create mode 100644 tests/generic/740 > create mode 100644 tests/generic/740.out > > diff --git a/src/Makefile b/src/Makefile > index d79015ce..916f6755 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -34,7 +34,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \ > fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \ > detached_mounts_propagation ext4_resize t_readdir_3 splice2pipe \ > - uuid_ioctl > + uuid_ioctl fiemap-fault Nit: FWIW, this hunk collides with 3a0381a4 ("btrfs: test snapshotting a deleted subvolume") > EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \ > btrfs_crc32c_forged_name.py popdir.pl popattr.py \ > diff --git a/src/fiemap-fault.c b/src/fiemap-fault.c > new file mode 100644 > index 00000000..27081188 > --- /dev/null > +++ b/src/fiemap-fault.c > @@ -0,0 +1,73 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2024 Meta Platforms, Inc. All Rights Reserved. > + */ > + > +#include <sys/ioctl.h> > +#include <sys/mman.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <linux/fs.h> > +#include <linux/types.h> > +#include <linux/fiemap.h> > +#include <err.h> > +#include <errno.h> > +#include <fcntl.h> > +#include <stdio.h> > +#include <string.h> > +#include <unistd.h> > + > +int prep_mmap_buffer(int fd, void **addr) > +{ > + struct stat st; > + int ret; > + > + ret = fstat(fd, &st); > + if (ret) > + err(1, "failed to stat %d", fd); > + > + *addr = mmap(NULL, st.st_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0); > + if (*addr == MAP_FAILED) > + err(1, "failed to mmap %d", fd); > + > + return st.st_size; > +} > + > +int main(int argc, char *argv[]) > +{ > + struct fiemap *fiemap; > + size_t sz, last = 0; > + void *buf = NULL; > + int ret, fd; > + > + if (argc != 2) > + errx(1, "no in and out file name arguments given"); > + > + fd = open(argv[1], O_RDWR, 0666); > + if (fd == -1) > + err(1, "failed to open %s", argv[1]); > + > + sz = prep_mmap_buffer(fd, &buf); > + > + fiemap = (struct fiemap *)buf; > + fiemap->fm_flags = 0; > + fiemap->fm_extent_count = (sz - sizeof(struct fiemap)) / > + sizeof(struct fiemap_extent); > + > + while (last < sz) { > + int i; > + > + fiemap->fm_start = last; > + fiemap->fm_length = sz - last; > + > + ret = ioctl(fd, FS_IOC_FIEMAP, (unsigned long)fiemap); > + if (ret < 0) > + err(1, "fiemap failed %d (%s)", errno, strerror(errno)); Nit: err() should already append the strerror() string. > + for (i = 0; i < fiemap->fm_mapped_extents; i++) > + last = fiemap->fm_extents[i].fe_logical + > + fiemap->fm_extents[i].fe_length; > + } > + > + close(fd); > + return 0; > +} > diff --git a/tests/generic/740 b/tests/generic/740 > new file mode 100644 > index 00000000..30ace1dd > --- /dev/null > +++ b/tests/generic/740 > @@ -0,0 +1,41 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2023 Meta Platforms, Inc. All Rights Reserved. > +# > +# FS QA Test 708 Test 740 > +# > +# Test fiemap into an mmaped buffer of the same file > +# > +# Create a reasonably large file, then run a program which mmaps it and uses > +# that as a buffer for an fiemap call. This is a regression test for btrfs > +# where we used to hold a lock for the duration of the fiemap call which would > +# result in a deadlock if we page faulted. > +# > +. ./common/preamble > +_begin_fstest quick auto fiemap > +[ $FSTYP == "btrfs" ] && \ > + _fixed_by_kernel_commit xxxxxxxxxxxx \ > + "btrfs: fix deadlock with fiemap and extent locking" > + > +# real QA test starts here > +_supported_fs generic > +_require_test > +_require_odirect > +_require_test_program fiemap-fault > +dst=$TEST_DIR/fiemap-fault-$seq > + > +echo "Silence is golden" > + > +for i in $(seq 0 2 1000) > +do > + $XFS_IO_PROG -d -f -c "pwrite -q $((i * 4096)) 4096" $dst > +done > + > +$here/src/fiemap-fault $dst > /dev/null || _fail "failed doing fiemap" Nit: the error paths print errors, so the explicit _fail should be unnecessary, or you could turn it into an echo. > + > +rm -f $dst > + > +# success, all done > +status=$? > +exit > + > diff --git a/tests/generic/740.out b/tests/generic/740.out > new file mode 100644 > index 00000000..3f841e60 > --- /dev/null > +++ b/tests/generic/740.out > @@ -0,0 +1,2 @@ > +QA output created by 740 > +Silence is golden
On Mon, Feb 12, 2024 at 11:41:14AM -0500, Josef Bacik wrote: > Btrfs had a deadlock that you could trigger by mmap'ing a large file and > using that as the buffer for fiemap. This test adds a c program to do > this, and the fstest creates a large enough file and then runs the > reproducer on the file. Without the fix btrfs deadlocks, with the fix > we pass fine. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > v1->v2: > - Add the fiemap group to the test. > - Actually include the reproducer helper program. Looks like this patch is missed (except the author changed the patch subject:), correct me if I'm wrong. The last review point hoped to add "src/fiemap-fault" to .gitignore. > > src/Makefile | 2 +- > src/fiemap-fault.c | 73 +++++++++++++++++++++++++++++++++++++++++++ > tests/generic/740 | 41 ++++++++++++++++++++++++ > tests/generic/740.out | 2 ++ > 4 files changed, 117 insertions(+), 1 deletion(-) > create mode 100644 src/fiemap-fault.c > create mode 100644 tests/generic/740 > create mode 100644 tests/generic/740.out > [snip] > diff --git a/tests/generic/740 b/tests/generic/740 > new file mode 100644 > index 00000000..30ace1dd > --- /dev/null > +++ b/tests/generic/740 > @@ -0,0 +1,41 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2023 Meta Platforms, Inc. All Rights Reserved. > +# > +# FS QA Test 708 > +# > +# Test fiemap into an mmaped buffer of the same file > +# > +# Create a reasonably large file, then run a program which mmaps it and uses > +# that as a buffer for an fiemap call. This is a regression test for btrfs > +# where we used to hold a lock for the duration of the fiemap call which would > +# result in a deadlock if we page faulted. > +# > +. ./common/preamble > +_begin_fstest quick auto fiemap > +[ $FSTYP == "btrfs" ] && \ > + _fixed_by_kernel_commit xxxxxxxxxxxx \ This commit id is "b0ad381fa769" now. > + "btrfs: fix deadlock with fiemap and extent locking" > + > +# real QA test starts here > +_supported_fs generic > +_require_test > +_require_odirect > +_require_test_program fiemap-fault > +dst=$TEST_DIR/fiemap-fault-$seq > + > +echo "Silence is golden" > + > +for i in $(seq 0 2 1000) > +do > + $XFS_IO_PROG -d -f -c "pwrite -q $((i * 4096)) 4096" $dst > +done _require_sparse_files ? > + > +$here/src/fiemap-fault $dst > /dev/null || _fail "failed doing fiemap" > + > +rm -f $dst If you'd like to remove this file, do it in _cleanup(), e.g. _cleanup() { rm -f $dst cd / rm -r -f $tmp.* } Could you rebase this patch on latest fstests for-next branch with above changes, then send a V2 please? I'd like to have this patch in next fstests release. Thanks, Zorro > + > +# success, all done > +status=$? > +exit > + > diff --git a/tests/generic/740.out b/tests/generic/740.out > new file mode 100644 > index 00000000..3f841e60 > --- /dev/null > +++ b/tests/generic/740.out > @@ -0,0 +1,2 @@ > +QA output created by 740 > +Silence is golden > -- > 2.43.0 > >
On Wed, Mar 13, 2024 at 11:48:51PM +0800, Zorro Lang wrote: > On Mon, Feb 12, 2024 at 11:41:14AM -0500, Josef Bacik wrote: > > Btrfs had a deadlock that you could trigger by mmap'ing a large file and > > using that as the buffer for fiemap. This test adds a c program to do > > this, and the fstest creates a large enough file and then runs the > > reproducer on the file. Without the fix btrfs deadlocks, with the fix > > we pass fine. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > v1->v2: > > - Add the fiemap group to the test. > > - Actually include the reproducer helper program. > > Looks like this patch is missed (except the author changed the patch subject:), > correct me if I'm wrong. The last review point hoped to add "src/fiemap-fault" > to .gitignore. > > > > > src/Makefile | 2 +- > > src/fiemap-fault.c | 73 +++++++++++++++++++++++++++++++++++++++++++ > > tests/generic/740 | 41 ++++++++++++++++++++++++ > > tests/generic/740.out | 2 ++ > > 4 files changed, 117 insertions(+), 1 deletion(-) > > create mode 100644 src/fiemap-fault.c > > create mode 100644 tests/generic/740 > > create mode 100644 tests/generic/740.out > > > > [snip] > > > diff --git a/tests/generic/740 b/tests/generic/740 > > new file mode 100644 > > index 00000000..30ace1dd > > --- /dev/null > > +++ b/tests/generic/740 > > @@ -0,0 +1,41 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (c) 2023 Meta Platforms, Inc. All Rights Reserved. > > +# > > +# FS QA Test 708 > > +# > > +# Test fiemap into an mmaped buffer of the same file > > +# > > +# Create a reasonably large file, then run a program which mmaps it and uses > > +# that as a buffer for an fiemap call. This is a regression test for btrfs > > +# where we used to hold a lock for the duration of the fiemap call which would > > +# result in a deadlock if we page faulted. > > +# > > +. ./common/preamble > > +_begin_fstest quick auto fiemap > > +[ $FSTYP == "btrfs" ] && \ > > + _fixed_by_kernel_commit xxxxxxxxxxxx \ > > This commit id is "b0ad381fa769" now. > > > + "btrfs: fix deadlock with fiemap and extent locking" > > + > > +# real QA test starts here > > +_supported_fs generic > > +_require_test > > +_require_odirect > > +_require_test_program fiemap-fault > > +dst=$TEST_DIR/fiemap-fault-$seq > > + > > +echo "Silence is golden" > > + > > +for i in $(seq 0 2 1000) > > +do > > + $XFS_IO_PROG -d -f -c "pwrite -q $((i * 4096)) 4096" $dst > > +done > > _require_sparse_files ? > This is the only comment I didn't incorporate, I'm not looking for sparse files, I'm just looking for a lot of extents. If smb fills in the holes I suppose this will only be a problem if it fills them in contiguously, but it'll just create a 4gib file instead of a 2gib file. Thanks, Josef
diff --git a/src/Makefile b/src/Makefile index d79015ce..916f6755 100644 --- a/src/Makefile +++ b/src/Makefile @@ -34,7 +34,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \ fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \ detached_mounts_propagation ext4_resize t_readdir_3 splice2pipe \ - uuid_ioctl + uuid_ioctl fiemap-fault EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \ btrfs_crc32c_forged_name.py popdir.pl popattr.py \ diff --git a/src/fiemap-fault.c b/src/fiemap-fault.c new file mode 100644 index 00000000..27081188 --- /dev/null +++ b/src/fiemap-fault.c @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2024 Meta Platforms, Inc. All Rights Reserved. + */ + +#include <sys/ioctl.h> +#include <sys/mman.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <linux/fs.h> +#include <linux/types.h> +#include <linux/fiemap.h> +#include <err.h> +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <string.h> +#include <unistd.h> + +int prep_mmap_buffer(int fd, void **addr) +{ + struct stat st; + int ret; + + ret = fstat(fd, &st); + if (ret) + err(1, "failed to stat %d", fd); + + *addr = mmap(NULL, st.st_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0); + if (*addr == MAP_FAILED) + err(1, "failed to mmap %d", fd); + + return st.st_size; +} + +int main(int argc, char *argv[]) +{ + struct fiemap *fiemap; + size_t sz, last = 0; + void *buf = NULL; + int ret, fd; + + if (argc != 2) + errx(1, "no in and out file name arguments given"); + + fd = open(argv[1], O_RDWR, 0666); + if (fd == -1) + err(1, "failed to open %s", argv[1]); + + sz = prep_mmap_buffer(fd, &buf); + + fiemap = (struct fiemap *)buf; + fiemap->fm_flags = 0; + fiemap->fm_extent_count = (sz - sizeof(struct fiemap)) / + sizeof(struct fiemap_extent); + + while (last < sz) { + int i; + + fiemap->fm_start = last; + fiemap->fm_length = sz - last; + + ret = ioctl(fd, FS_IOC_FIEMAP, (unsigned long)fiemap); + if (ret < 0) + err(1, "fiemap failed %d (%s)", errno, strerror(errno)); + for (i = 0; i < fiemap->fm_mapped_extents; i++) + last = fiemap->fm_extents[i].fe_logical + + fiemap->fm_extents[i].fe_length; + } + + close(fd); + return 0; +} diff --git a/tests/generic/740 b/tests/generic/740 new file mode 100644 index 00000000..30ace1dd --- /dev/null +++ b/tests/generic/740 @@ -0,0 +1,41 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2023 Meta Platforms, Inc. All Rights Reserved. +# +# FS QA Test 708 +# +# Test fiemap into an mmaped buffer of the same file +# +# Create a reasonably large file, then run a program which mmaps it and uses +# that as a buffer for an fiemap call. This is a regression test for btrfs +# where we used to hold a lock for the duration of the fiemap call which would +# result in a deadlock if we page faulted. +# +. ./common/preamble +_begin_fstest quick auto fiemap +[ $FSTYP == "btrfs" ] && \ + _fixed_by_kernel_commit xxxxxxxxxxxx \ + "btrfs: fix deadlock with fiemap and extent locking" + +# real QA test starts here +_supported_fs generic +_require_test +_require_odirect +_require_test_program fiemap-fault +dst=$TEST_DIR/fiemap-fault-$seq + +echo "Silence is golden" + +for i in $(seq 0 2 1000) +do + $XFS_IO_PROG -d -f -c "pwrite -q $((i * 4096)) 4096" $dst +done + +$here/src/fiemap-fault $dst > /dev/null || _fail "failed doing fiemap" + +rm -f $dst + +# success, all done +status=$? +exit + diff --git a/tests/generic/740.out b/tests/generic/740.out new file mode 100644 index 00000000..3f841e60 --- /dev/null +++ b/tests/generic/740.out @@ -0,0 +1,2 @@ +QA output created by 740 +Silence is golden
Btrfs had a deadlock that you could trigger by mmap'ing a large file and using that as the buffer for fiemap. This test adds a c program to do this, and the fstest creates a large enough file and then runs the reproducer on the file. Without the fix btrfs deadlocks, with the fix we pass fine. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- v1->v2: - Add the fiemap group to the test. - Actually include the reproducer helper program. src/Makefile | 2 +- src/fiemap-fault.c | 73 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/740 | 41 ++++++++++++++++++++++++ tests/generic/740.out | 2 ++ 4 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 src/fiemap-fault.c create mode 100644 tests/generic/740 create mode 100644 tests/generic/740.out