Re: [PATCH v6] btrfs: test block group size class loading logic

[Boris Burkov <boris@bur.io>]

On Thu, Feb 16, 2023 at 10:49:52PM +0800, Zorro Lang wrote:
> On Thu, Feb 16, 2023 at 12:11:39PM +0000, Filipe Manana wrote:
> > On Wed, Feb 15, 2023 at 11:37 PM Boris Burkov <boris@bur.io> wrote:
> > >
> > > Add a new test which checks that size classes in freshly loaded block
> > > groups after a cycle mount match size classes before going down
> > >
> > > Depends on the kernel patch:
> > > btrfs: add size class stats to sysfs
> > >
> > > Signed-off-by: Boris Burkov <boris@bur.io>
> > > ---
> > > Changelog:
> > > v6:
> > > Actually include changes for v5 in the commit.
> > > v5:
> > > Instead of using _fixed_by_kernel_commit, use require_fs_sysfs to handle
> > > the needed sysfs file. The test is skipped on kernels without the file
> > > and runs correctly on new ones.
> > > v4:
> > > Fix dumb typo in _fixed_by_kernel_commit (left out leading underscore
> > > copy+pasting). Re-tested happy and sad case...
> > >
> > > v3:
> > > Re-add fixed_by_kernel_commit, but for the stats patch which is
> > > required, but not a fix in the strictest sense.
> > >
> > > v2:
> > > Drop the fixed_by_kernel_commit since the fix is not out past the btrfs
> > > development tree, so the fix is getting rolled in to the original broken
> > > commit. Modified the commit message to note the dependency on the new
> > > sysfs counters.
> > >
> > >
> > >  tests/btrfs/283     | 50 +++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/btrfs/283.out |  2 ++
> > >  2 files changed, 52 insertions(+)
> > >  create mode 100755 tests/btrfs/283
> > >  create mode 100644 tests/btrfs/283.out
> > >
> > > diff --git a/tests/btrfs/283 b/tests/btrfs/283
> > > new file mode 100755
> > > index 00000000..2c26b41e
> > > --- /dev/null
> > > +++ b/tests/btrfs/283
> > > @@ -0,0 +1,50 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2023 Meta Platforms, Inc.  All Rights Reserved.
> > > +#
> > > +# FS QA Test 283
> > > +#
> > > +# Test that mounting a btrfs filesystem properly loads block group size classes.
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick mount
> > 
> > I'm still curious why the 'mount' group, and I've asked for that before.
> > 
> > We aren't testing a new mount option, so it feels weird for me.
> 
> Agree, the "mount" group looks not make sense.

My mistake, sorry for overlooking this. My reasoning is that it tests
the behavior on a fresh mount rather than while the fs is live, but it
is not specific to mounting logic. Happy to remove it.

> 
> The btrfs/283 has been taken in v2023.02.05. So please make a rebase.

Done. Is there any way we can try to script this process for the future
with placeholders or something? It's kind of a drag to deal with
in general. I'm happy to send some proposal if it's something you'd be
interested in taking.

> 
> The "fixed_by_kernel_commit xxxx" part has been added and removed several times,
> does this case need it or not? Or maybe you want a _wants_kernel_commit (just
> ask for sure) ?

Thanks for checking. I personally don't think it needs it. The commit it
wants/needs is the commit that adds the sysfs file. I think that is
adequately skipped and documented by the _require_fs_sysfs that Filipe
suggested. If someone is really interested in this behavior, it is not
too hard to find the kernel commit from the sysfs file.

> 
> Others look good to me.
> 
> Thanks,
> Zorro
> 
> > 
> > Otherwise, it looks fine now. Thanks.
> > 
> > > +
> > > +sysfs_size_classes() {
> > > +       local uuid="$(findmnt -n -o UUID "$SCRATCH_MNT")"
> > > +       cat "/sys/fs/btrfs/$uuid/allocation/data/size_classes"
> > > +}
> > > +
> > > +_supported_fs btrfs
> > > +_require_scratch
> > > +_require_btrfs_fs_sysfs
> > > +_require_fs_sysfs allocation/data/size_classes
> > > +
> > > +f="$SCRATCH_MNT/f"
> > > +small=$((16 * 1024))
> > > +medium=$((1024 * 1024))
> > > +large=$((16 * 1024 * 1024))
> > > +
> > > +_scratch_mkfs >/dev/null
> > > +_scratch_mount
> > > +# Write files with extents in each size class
> > > +$XFS_IO_PROG -fc "pwrite -q 0 $small" $f.small
> > > +$XFS_IO_PROG -fc "pwrite -q 0 $medium" $f.medium
> > > +$XFS_IO_PROG -fc "pwrite -q 0 $large" $f.large
> > > +# Sync to force the extent allocation
> > > +sync
> > > +pre=$(sysfs_size_classes)
> > > +
> > > +# cycle mount to drop the block group cache
> > > +_scratch_cycle_mount
> > > +
> > > +# Another write causes us to actually load the block groups
> > > +$XFS_IO_PROG -fc "pwrite -q 0 $large" $f.large.2
> > > +sync
> > > +
> > > +post=$(sysfs_size_classes)
> > > +diff <(echo $pre) <(echo $post)
> > > +
> > > +echo "Silence is golden"
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/btrfs/283.out b/tests/btrfs/283.out
> > > new file mode 100644
> > > index 00000000..efb2c583
> > > --- /dev/null
> > > +++ b/tests/btrfs/283.out
> > > @@ -0,0 +1,2 @@
> > > +QA output created by 283
> > > +Silence is golden
> > > --
> > > 2.39.1
> > >
> > 
>