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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/1/28 10:56, Ritesh Harjani wrote:
> 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.

This is in fact discussed before, I used to include the fixes in the
test description, but later move them into the commit message.

In the long run, the test should and would all pass, thus there is
really no need to bother mentioning it.

For the guys who really need to bother the test failure, aka QA testers
or some developers in the future causing some regression, they will
check the full commit messages anyway.

And the fixes tag has its own problems, like at the time of fstests
merging, the fixes may not yet being merged into mainline, or the title
may change.

Thus mentioning something volatile in the test description can be a
little confusing, and hiding it into the commit message may be preferred.

Thanks,
Qu

>
> 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
>