Re: [PATCH][v2] btrfs/194: add a test for multi-subvolume fsyncing

[Eryu Guan <guaneryu@gmail.com>]

On Thu, Oct 03, 2019 at 12:12:36PM +0100, Filipe Manana wrote:
> On Thu, Oct 3, 2019 at 11:59 AM Filipe Manana <fdmanana@gmail.com> wrote:
> >
> > On Wed, Oct 2, 2019 at 7:44 PM Josef Bacik <josef@toxicpanda.com> wrote:
> > >
> > > I discovered a problem in btrfs where we'd end up pointing at a block we
> > > hadn't written out yet.  This is triggered by a race when two different
> > > files on two different subvolumes fsync.  This test exercises this path
> > > with dm-log-writes, and then replays the log at every FUA to verify the
> > > file system is still mountable and the log is replayable.
> > >
> > > This test is to verify the fix
> > >
> > > btrfs: fix incorrect updating of log root tree
> > >
> > > actually fixed the problem.
> > >
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> >
> > Reviewed-by: Filipe Manana <fdmanana@suse.com>
> >
> > It's working now.
> > Confirmed this triggers the bug, and after about 4 hours of this
> > running with the btrfs patch, it doesn't trigger the bug anymore.
> >
> > Thanks!
> >
> > > ---
> > > v1->v2:
> > > - added the patchname related to this test in the comments and changelog.
> > > - running fio makes it use 400mib of shared memory, so running 50 of them is
> > >   impossible on boxes that don't have hundreds of gib of RAM.  Fixed this to
> > >   just generate a fio config so we can run 1 fio instance with 50 threads which
> > >   makes it not OOM boxes with tiny amounts of RAM.
> > > - fixed some formatting things that Filipe pointed out.
> > >
> > >  tests/btrfs/194     | 111 ++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/btrfs/194.out |   2 +
> > >  tests/btrfs/group   |   1 +
> > >  3 files changed, 114 insertions(+)
> > >  create mode 100755 tests/btrfs/194
> > >  create mode 100644 tests/btrfs/194.out
> > >
> > > diff --git a/tests/btrfs/194 b/tests/btrfs/194
> > > new file mode 100755
> > > index 00000000..b98064e2
> > > --- /dev/null
> > > +++ b/tests/btrfs/194
> > > @@ -0,0 +1,111 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2019 Facebook.  All Rights Reserved.
> > > +#
> > > +# FS QA Test 194
> > > +#
> > > +# Test multi subvolume fsync to test a bug where we'd end up pointing at a block
> > > +# we haven't written.  This was fixed by the patch
> > > +#
> > > +# btrfs: fix incorrect updating of log root tree
> > > +#
> > > +# Will do log replay and check the filesystem.
> > > +#
> > > +seq=`basename $0`
> > > +seqres=$RESULT_DIR/$seq
> > > +echo "QA output created by $seq"
> > > +
> > > +here=`pwd`
> > > +tmp=/tmp/$$
> > > +fio_config=$tmp.fio
> > > +status=1       # failure is the default!
> > > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > > +
> > > +_cleanup()
> > > +{
> > > +       cd /
> > > +       _log_writes_cleanup &> /dev/null
> > > +       _dmthin_cleanup
> > > +       rm -f $tmp.*
> > > +}
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common/rc
> > > +. ./common/filter
> > > +. ./common/dmthin
> > > +. ./common/dmlogwrites
> > > +
> > > +# remove previous $seqres.full before test
> > > +rm -f $seqres.full
> > > +
> > > +# real QA test starts here
> > > +
> > > +# Modify as appropriate.
> > > +_supported_fs generic
> 
> Btw, only forgot about this.
> Should be: _supported_fs btrfs
> 
> Eryu can probably fix that at commit time.
> Thanks.

Sure. Thanks for the review!

Eryu