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

[Filipe Manana <fdmanana@gmail.com>]

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.

> > +_supported_os Linux
> > +
> > +# Use thin device as replay device, which requires $SCRATCH_DEV
> > +_require_scratch_nocheck
> > +# and we need extra device as log device
> > +_require_log_writes
> > +_require_dm_target thin-pool
> > +
> > +cat >$fio_config <<EOF
> > +[global]
> > +readwrite=write
> > +fallocate=none
> > +bs=4k
> > +fsync=1
> > +size=128k
> > +EOF
> > +
> > +for i in $(seq 0 49); do
> > +       echo "[foo$i]" >> $fio_config
> > +       echo "filename=$SCRATCH_MNT/$i/file" >> $fio_config
> > +done
> > +
> > +_require_fio $fio_config
> > +
> > +cat $fio_config >> $seqres.full
> > +
> > +# Use a thin device to provide deterministic discard behavior. Discards are used
> > +# by the log replay tool for fast zeroing to prevent out-of-order replay issues.
> > +_test_unmount
> > +_dmthin_init $devsize $devsize $csize $lowspace
> > +_log_writes_init $DMTHIN_VOL_DEV
> > +_log_writes_mkfs >> $seqres.full 2>&1
> > +_log_writes_mark mkfs
> > +
> > +_log_writes_mount
> > +
> > +# First create all the subvolumes
> > +for i in $(seq 0 49); do
> > +       $BTRFS_UTIL_PROG subvolume create "$SCRATCH_MNT/$i" > /dev/null
> > +done
> > +
> > +$FIO_PROG $fio_config > /dev/null 2>&1
> > +_log_writes_unmount
> > +
> > +_log_writes_remove
> > +prev=$(_log_writes_mark_to_entry_number mkfs)
> > +[ -z "$prev" ] && _fail "failed to locate entry mark 'mkfs'"
> > +cur=$(_log_writes_find_next_fua $prev)
> > +[ -z "$cur" ] && _fail "failed to locate next FUA write"
> > +
> > +while [ ! -z "$cur" ]; do
> > +       _log_writes_replay_log_range $cur $DMTHIN_VOL_DEV >> $seqres.full
> > +
> > +       # We need to mount the fs because btrfsck won't bother checking the log.
> > +       _dmthin_mount
> > +       _dmthin_check_fs
> > +
> > +       prev=$cur
> > +       cur=$(_log_writes_find_next_fua $(($cur + 1)))
> > +       [ -z "$cur" ] && break
> > +done
> > +
> > +echo "Silence is golden"
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/btrfs/194.out b/tests/btrfs/194.out
> > new file mode 100644
> > index 00000000..7bfd50ff
> > --- /dev/null
> > +++ b/tests/btrfs/194.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 194
> > +Silence is golden
> > diff --git a/tests/btrfs/group b/tests/btrfs/group
> > index b92cb12c..0d0e1bba 100644
> > --- a/tests/btrfs/group
> > +++ b/tests/btrfs/group
> > @@ -196,3 +196,4 @@
> >  191 auto quick send dedupe
> >  192 auto replay snapshot stress
> >  193 auto quick qgroup enospc limit
> > +194 auto metadata log volume
> > --
> > 2.21.0
> >
>
>
> --
> Filipe David Manana,
>
> “Whether you think you can, or you think you can't — you're right.”



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”