Re: [PATCH] btrfs: add test case to verify that btrfs won't waste IO/CPU to defrag compressed extents already at their max size

[Ritesh Harjani <riteshh@linux.ibm.com>]

On 22/01/28 06:20AM, Qu Wenruo wrote:
>
>
> On 2022/1/27 23:38, Ritesh Harjani wrote:
> > On 22/01/27 01:53PM, Qu Wenruo wrote:
> > > There is a long existing bug in btrfs defrag code that it will always
> > > try to defrag compressed extents, even they are already at max capacity.
> > >
> > > This will not reduce the number of extents, but only waste IO/CPU.
> > >
> > > The kernel fix is titled:
> > >
> > >    btrfs: defrag: don't defrag extents which is already at its max capacity
> > >
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > ---
> > >   tests/btrfs/257     | 79 +++++++++++++++++++++++++++++++++++++++++++++
> > >   tests/btrfs/257.out |  2 ++
> > >   2 files changed, 81 insertions(+)
> > >   create mode 100755 tests/btrfs/257
> > >   create mode 100644 tests/btrfs/257.out
> > >
> > > diff --git a/tests/btrfs/257 b/tests/btrfs/257
> > > new file mode 100755
> > > index 00000000..326687dc
> > > --- /dev/null
> > > +++ b/tests/btrfs/257
> > > @@ -0,0 +1,79 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
> > > +#
> > > +# FS QA Test 257
> > > +#
> > > +# Make sure btrfs defrag ioctl won't defrag compressed extents which are already
> > > +# at their max capacity.
> >
> > Haven't really looked into this fstest. But it is a good practice to add the
> > commit id and the title here for others to easily refer kernel commit.
>
> Isn't that already in the commit message?

Yes, that's true. And thanks for adding that.
I generally found mentioning commit-id and commit-title
in the description section of the test too to be lot more helpful.

For e.g. tests/btrfs/232

# FS QA Test 232
#
# Test that performing io and exhausting qgroup limit won't deadlock. This
# exercises issues fixed by the following kernel commits:
#
# 4f6a49de64fd ("btrfs: unlock extents in btrfs_zero_range in case of quota
# reservation errors")
# 4d14c5cde5c2 ("btrfs: don't flush from btrfs_delayed_inode_reserve_metadata")

Though I don't think it is mandatory, but as I said, it is generally helpful
for anyone to refer to commit directly / title directly from here if it has
a commit-id (might be it's just me :))

Thanks!
-ritesh