Re: [PATCH v2] fstests: add a regression test for fiemap into an mmap range

[Josef Bacik <josef@toxicpanda.com>]

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