* [PATCH v3] generic/808: add a regression test for fiemap into an mmap range
@ 2024-03-20 14:36 Josef Bacik
2024-03-20 14:52 ` Filipe Manana
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Josef Bacik @ 2024-03-20 14:36 UTC (permalink / raw)
To: fstests, linux-btrfs, kernel-team
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>
---
v2->v3:
- Add fiemap-fault to .gitignore
- Added a _cleanup() helper
- Just let the output of fiemap-fault go instead of using || _fail
- Added the munmap
- Moved $dst to $TEST_DIR/$seq
.gitignore | 1 +
src/Makefile | 2 +-
src/fiemap-fault.c | 74 +++++++++++++++++++++++++++++++++++++++++++
tests/generic/808 | 48 ++++++++++++++++++++++++++++
tests/generic/808.out | 2 ++
5 files changed, 126 insertions(+), 1 deletion(-)
create mode 100644 src/fiemap-fault.c
create mode 100755 tests/generic/808
create mode 100644 tests/generic/808.out
diff --git a/.gitignore b/.gitignore
index 3b160209..f0fb72bd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -205,6 +205,7 @@ tags
/src/vfs/mount-idmapped
/src/log-writes/replay-log
/src/perf/*.pyc
+/src/filemap-fault
# Symlinked files
/tests/generic/035.out
diff --git a/src/Makefile b/src/Makefile
index e7442487..ab98a06f 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 t_snapshot_deleted_subvolume
+ uuid_ioctl t_snapshot_deleted_subvolume 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..73260068
--- /dev/null
+++ b/src/fiemap-fault.c
@@ -0,0 +1,74 @@
+// 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", errno);
+ for (i = 0; i < fiemap->fm_mapped_extents; i++)
+ last = fiemap->fm_extents[i].fe_logical +
+ fiemap->fm_extents[i].fe_length;
+ }
+
+ munmap(buf, sz);
+ close(fd);
+ return 0;
+}
diff --git a/tests/generic/808 b/tests/generic/808
new file mode 100755
index 00000000..36015f35
--- /dev/null
+++ b/tests/generic/808
@@ -0,0 +1,48 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2024 Meta Platforms, Inc. All Rights Reserved.
+#
+# FS QA Test 808
+#
+# 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 b0ad381fa769 \
+ "btrfs: fix deadlock with fiemap and extent locking"
+
+_cleanup()
+{
+ rm -f $dst
+ cd /
+ rm -r -f $tmp.*
+}
+
+# real QA test starts here
+_supported_fs generic
+_require_test
+_require_odirect
+_require_test_program fiemap-fault
+dst=$TEST_DIR/$seq/fiemap-fault
+
+mkdir -p $TEST_DIR/$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
+
+# success, all done
+status=$?
+exit
+
diff --git a/tests/generic/808.out b/tests/generic/808.out
new file mode 100644
index 00000000..88253428
--- /dev/null
+++ b/tests/generic/808.out
@@ -0,0 +1,2 @@
+QA output created by 808
+Silence is golden
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] generic/808: add a regression test for fiemap into an mmap range
2024-03-20 14:36 [PATCH v3] generic/808: add a regression test for fiemap into an mmap range Josef Bacik
@ 2024-03-20 14:52 ` Filipe Manana
2024-03-20 15:25 ` Darrick J. Wong
2024-03-21 4:20 ` Anand Jain
2 siblings, 0 replies; 5+ messages in thread
From: Filipe Manana @ 2024-03-20 14:52 UTC (permalink / raw)
To: Josef Bacik; +Cc: fstests, linux-btrfs, kernel-team
On Wed, Mar 20, 2024 at 2:36 PM Josef Bacik <josef@toxicpanda.com> 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>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Looks good to me now, thanks.
> ---
> v2->v3:
> - Add fiemap-fault to .gitignore
> - Added a _cleanup() helper
> - Just let the output of fiemap-fault go instead of using || _fail
> - Added the munmap
> - Moved $dst to $TEST_DIR/$seq
>
> .gitignore | 1 +
> src/Makefile | 2 +-
> src/fiemap-fault.c | 74 +++++++++++++++++++++++++++++++++++++++++++
> tests/generic/808 | 48 ++++++++++++++++++++++++++++
> tests/generic/808.out | 2 ++
> 5 files changed, 126 insertions(+), 1 deletion(-)
> create mode 100644 src/fiemap-fault.c
> create mode 100755 tests/generic/808
> create mode 100644 tests/generic/808.out
>
> diff --git a/.gitignore b/.gitignore
> index 3b160209..f0fb72bd 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -205,6 +205,7 @@ tags
> /src/vfs/mount-idmapped
> /src/log-writes/replay-log
> /src/perf/*.pyc
> +/src/filemap-fault
>
> # Symlinked files
> /tests/generic/035.out
> diff --git a/src/Makefile b/src/Makefile
> index e7442487..ab98a06f 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 t_snapshot_deleted_subvolume
> + uuid_ioctl t_snapshot_deleted_subvolume 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..73260068
> --- /dev/null
> +++ b/src/fiemap-fault.c
> @@ -0,0 +1,74 @@
> +// 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", errno);
> + for (i = 0; i < fiemap->fm_mapped_extents; i++)
> + last = fiemap->fm_extents[i].fe_logical +
> + fiemap->fm_extents[i].fe_length;
> + }
> +
> + munmap(buf, sz);
> + close(fd);
> + return 0;
> +}
> diff --git a/tests/generic/808 b/tests/generic/808
> new file mode 100755
> index 00000000..36015f35
> --- /dev/null
> +++ b/tests/generic/808
> @@ -0,0 +1,48 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 Meta Platforms, Inc. All Rights Reserved.
> +#
> +# FS QA Test 808
> +#
> +# 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 b0ad381fa769 \
> + "btrfs: fix deadlock with fiemap and extent locking"
> +
> +_cleanup()
> +{
> + rm -f $dst
> + cd /
> + rm -r -f $tmp.*
> +}
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_test
> +_require_odirect
> +_require_test_program fiemap-fault
> +dst=$TEST_DIR/$seq/fiemap-fault
> +
> +mkdir -p $TEST_DIR/$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
> +
> +# success, all done
> +status=$?
> +exit
> +
> diff --git a/tests/generic/808.out b/tests/generic/808.out
> new file mode 100644
> index 00000000..88253428
> --- /dev/null
> +++ b/tests/generic/808.out
> @@ -0,0 +1,2 @@
> +QA output created by 808
> +Silence is golden
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] generic/808: add a regression test for fiemap into an mmap range
2024-03-20 14:36 [PATCH v3] generic/808: add a regression test for fiemap into an mmap range Josef Bacik
2024-03-20 14:52 ` Filipe Manana
@ 2024-03-20 15:25 ` Darrick J. Wong
2024-03-20 15:37 ` Josef Bacik
2024-03-21 4:20 ` Anand Jain
2 siblings, 1 reply; 5+ messages in thread
From: Darrick J. Wong @ 2024-03-20 15:25 UTC (permalink / raw)
To: Josef Bacik; +Cc: fstests, linux-btrfs, kernel-team
On Wed, Mar 20, 2024 at 10:36:42AM -0400, 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>
> ---
> v2->v3:
> - Add fiemap-fault to .gitignore
> - Added a _cleanup() helper
> - Just let the output of fiemap-fault go instead of using || _fail
> - Added the munmap
> - Moved $dst to $TEST_DIR/$seq
>
> .gitignore | 1 +
> src/Makefile | 2 +-
> src/fiemap-fault.c | 74 +++++++++++++++++++++++++++++++++++++++++++
> tests/generic/808 | 48 ++++++++++++++++++++++++++++
> tests/generic/808.out | 2 ++
> 5 files changed, 126 insertions(+), 1 deletion(-)
> create mode 100644 src/fiemap-fault.c
> create mode 100755 tests/generic/808
> create mode 100644 tests/generic/808.out
>
> diff --git a/.gitignore b/.gitignore
> index 3b160209..f0fb72bd 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -205,6 +205,7 @@ tags
> /src/vfs/mount-idmapped
> /src/log-writes/replay-log
> /src/perf/*.pyc
> +/src/filemap-fault
>
> # Symlinked files
> /tests/generic/035.out
> diff --git a/src/Makefile b/src/Makefile
> index e7442487..ab98a06f 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 t_snapshot_deleted_subvolume
> + uuid_ioctl t_snapshot_deleted_subvolume 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..73260068
> --- /dev/null
> +++ b/src/fiemap-fault.c
> @@ -0,0 +1,74 @@
> +// 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", errno);
> + for (i = 0; i < fiemap->fm_mapped_extents; i++)
> + last = fiemap->fm_extents[i].fe_logical +
> + fiemap->fm_extents[i].fe_length;
> + }
> +
> + munmap(buf, sz);
> + close(fd);
> + return 0;
> +}
> diff --git a/tests/generic/808 b/tests/generic/808
> new file mode 100755
> index 00000000..36015f35
> --- /dev/null
> +++ b/tests/generic/808
> @@ -0,0 +1,48 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 Meta Platforms, Inc. All Rights Reserved.
> +#
> +# FS QA Test 808
> +#
> +# 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 b0ad381fa769 \
> + "btrfs: fix deadlock with fiemap and extent locking"
> +
> +_cleanup()
> +{
> + rm -f $dst
> + cd /
> + rm -r -f $tmp.*
> +}
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_test
> +_require_odirect
> +_require_test_program fiemap-fault
> +dst=$TEST_DIR/$seq/fiemap-fault
> +
> +mkdir -p $TEST_DIR/$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
I don't know if there's a specific reason that this does directio writes
at alternating offsets other than forcing allocations, but usually we do:
$XFS_IO_PROG -f -c "pwrite -q 0 409600" $dst
$src/punch-alternating $dst
to generate a file with a bunch of extent records. Also, since this is
a generic test that wants to create a file with sparse holes, it really
ought to be querying the file's allocation unit size:
blksz=$(_get_file_block_size $TEST_DIR)
$XFS_IO_PROG -f -c "pwrite -q 0 $((blksz * 100))" $dst
--D
> +
> +$here/src/fiemap-fault $dst
> +
> +# success, all done
> +status=$?
> +exit
> +
> diff --git a/tests/generic/808.out b/tests/generic/808.out
> new file mode 100644
> index 00000000..88253428
> --- /dev/null
> +++ b/tests/generic/808.out
> @@ -0,0 +1,2 @@
> +QA output created by 808
> +Silence is golden
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] generic/808: add a regression test for fiemap into an mmap range
2024-03-20 15:25 ` Darrick J. Wong
@ 2024-03-20 15:37 ` Josef Bacik
0 siblings, 0 replies; 5+ messages in thread
From: Josef Bacik @ 2024-03-20 15:37 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: fstests, linux-btrfs, kernel-team
On Wed, Mar 20, 2024 at 08:25:31AM -0700, Darrick J. Wong wrote:
> On Wed, Mar 20, 2024 at 10:36:42AM -0400, 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>
> > ---
> > v2->v3:
> > - Add fiemap-fault to .gitignore
> > - Added a _cleanup() helper
> > - Just let the output of fiemap-fault go instead of using || _fail
> > - Added the munmap
> > - Moved $dst to $TEST_DIR/$seq
> >
> > .gitignore | 1 +
> > src/Makefile | 2 +-
> > src/fiemap-fault.c | 74 +++++++++++++++++++++++++++++++++++++++++++
> > tests/generic/808 | 48 ++++++++++++++++++++++++++++
> > tests/generic/808.out | 2 ++
> > 5 files changed, 126 insertions(+), 1 deletion(-)
> > create mode 100644 src/fiemap-fault.c
> > create mode 100755 tests/generic/808
> > create mode 100644 tests/generic/808.out
> >
> > diff --git a/.gitignore b/.gitignore
> > index 3b160209..f0fb72bd 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -205,6 +205,7 @@ tags
> > /src/vfs/mount-idmapped
> > /src/log-writes/replay-log
> > /src/perf/*.pyc
> > +/src/filemap-fault
> >
> > # Symlinked files
> > /tests/generic/035.out
> > diff --git a/src/Makefile b/src/Makefile
> > index e7442487..ab98a06f 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 t_snapshot_deleted_subvolume
> > + uuid_ioctl t_snapshot_deleted_subvolume 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..73260068
> > --- /dev/null
> > +++ b/src/fiemap-fault.c
> > @@ -0,0 +1,74 @@
> > +// 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", errno);
> > + for (i = 0; i < fiemap->fm_mapped_extents; i++)
> > + last = fiemap->fm_extents[i].fe_logical +
> > + fiemap->fm_extents[i].fe_length;
> > + }
> > +
> > + munmap(buf, sz);
> > + close(fd);
> > + return 0;
> > +}
> > diff --git a/tests/generic/808 b/tests/generic/808
> > new file mode 100755
> > index 00000000..36015f35
> > --- /dev/null
> > +++ b/tests/generic/808
> > @@ -0,0 +1,48 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2024 Meta Platforms, Inc. All Rights Reserved.
> > +#
> > +# FS QA Test 808
> > +#
> > +# 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 b0ad381fa769 \
> > + "btrfs: fix deadlock with fiemap and extent locking"
> > +
> > +_cleanup()
> > +{
> > + rm -f $dst
> > + cd /
> > + rm -r -f $tmp.*
> > +}
> > +
> > +# real QA test starts here
> > +_supported_fs generic
> > +_require_test
> > +_require_odirect
> > +_require_test_program fiemap-fault
> > +dst=$TEST_DIR/$seq/fiemap-fault
> > +
> > +mkdir -p $TEST_DIR/$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
>
> I don't know if there's a specific reason that this does directio writes
> at alternating offsets other than forcing allocations, but usually we do:
>
> $XFS_IO_PROG -f -c "pwrite -q 0 409600" $dst
> $src/punch-alternating $dst
>
> to generate a file with a bunch of extent records. Also, since this is
> a generic test that wants to create a file with sparse holes, it really
> ought to be querying the file's allocation unit size:
>
> blksz=$(_get_file_block_size $TEST_DIR)
> $XFS_IO_PROG -f -c "pwrite -q 0 $((blksz * 100))" $dst
Ok I can do that instead, you're correct, all I want is a bunch of extents, and
for btrfs at least doing alternating directio writes to get that. Thanks,
Josef
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] generic/808: add a regression test for fiemap into an mmap range
2024-03-20 14:36 [PATCH v3] generic/808: add a regression test for fiemap into an mmap range Josef Bacik
2024-03-20 14:52 ` Filipe Manana
2024-03-20 15:25 ` Darrick J. Wong
@ 2024-03-21 4:20 ` Anand Jain
2 siblings, 0 replies; 5+ messages in thread
From: Anand Jain @ 2024-03-21 4:20 UTC (permalink / raw)
To: Josef Bacik, fstests, linux-btrfs, kernel-team
On 3/20/24 20:06, 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>
> ---
> v2->v3:
> - Add fiemap-fault to .gitignore
> - Added a _cleanup() helper
> - Just let the output of fiemap-fault go instead of using || _fail
> - Added the munmap
> - Moved $dst to $TEST_DIR/$seq
looks good.
Reviewed-by: Anand Jain <anand.jain@oracle.com>
A minor whitespace error below, maybe Zorro can handle it when applied.
<snap>
> diff --git a/tests/generic/808 b/tests/generic/808
> new file mode 100755
> index 00000000..36015f35
> --- /dev/null
> +++ b/tests/generic/808
> @@ -0,0 +1,48 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 Meta Platforms, Inc. All Rights Reserved.
> +#
> +# FS QA Test 808
> +#
> +# 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 b0ad381fa769 \
> + "btrfs: fix deadlock with fiemap and extent locking"
> +
> +_cleanup()
> +{
> + rm -f $dst
> + cd /
> + rm -r -f $tmp.*
> +}
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_test
> +_require_odirect
> +_require_test_program fiemap-fault
> +dst=$TEST_DIR/$seq/fiemap-fault
> +
> +mkdir -p $TEST_DIR/$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
> +
> +# success, all done
> +status=$?
> +exit
> +
Applying: generic/808: add a regression test for fiemap into an mmap range
.git/rebase-apply/patch:177: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-03-21 4:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-20 14:36 [PATCH v3] generic/808: add a regression test for fiemap into an mmap range Josef Bacik
2024-03-20 14:52 ` Filipe Manana
2024-03-20 15:25 ` Darrick J. Wong
2024-03-20 15:37 ` Josef Bacik
2024-03-21 4:20 ` Anand Jain
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox