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@xxxxxxxxxxxxxx>
With a .gitignore entry added for the fiemap-fault binary:
Revewed-by: David Disseldorp <ddiss@xxxxxxx>
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
