Re: [PATCH 3/4] btrfs: autodefrag: only scan one inode once

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

On 2022/2/24 17:45, Qu Wenruo wrote:
>
>
> On 2022/2/24 14:59, Qu Wenruo wrote:
>>
>>
>> On 2022/2/23 23:53, David Sterba wrote:
>>> On Wed, Feb 23, 2022 at 07:42:05AM +0800, Qu Wenruo wrote:
>>>> On 2022/2/23 01:32, David Sterba wrote:
>>>>> On Sun, Feb 13, 2022 at 03:42:32PM +0800, Qu Wenruo wrote:
>>>>> @@ -295,39 +265,29 @@ static int __btrfs_run_defrag_inode(struct
>>>>> btrfs_fs_info *fs_info,
>>>>>            goto cleanup;
>>>>>        }
>>>>>
>>>>> +    if (cur >= i_size_read(inode)) {
>>>>> +        iput(inode);
>>>>> +        break;
>>>>
>>>> Would this even compile?
>>>> Break without a while loop?
>>>
>>> That was a typo, s/break/goto cleanup/.
>>>
>>>> To me, the open-coded while loop using goto is even worse.
>>>> I don't think just saving one indent is worthy.
>>>
>>> Well for backport purposes the fix should be minimal and not necessarily
>>> pretty. Indenting code produces a diff that replaces one blob with
>>> another blob, with additional changes and increases line count, which is
>>> one of the criteria for stable acceptance.
>>>
>>>> Where can I find the final version to do more testing/review?
>>>
>>> Now pushed to branch fix/autodefrag-io in my git repos, I've only
>>> updated changelogs.
>>
>> Checked the code, it looks fine to me, just one small question related
>> to the ret < 0 case.
>>
>> Unlike the refactored version, which can return < 0 even if we defragged
>> some sectors. (Since we have different members to record those info)
>>
>> If we have defragged any sector in btrfs_defrag_file(), but some other
>> problems happened later, we will get a return value > 0 in this version.
>>
>> It's a not a big deal, as we will skip to the last scanned position
>> anyway, and we even have the safenet to increase @cur even if
>> range.start doesn't get increased.
>>
>> For backport it's completely fine.
>>
>> Just want to make sure for the proper version, what's is the expected
>> behavior.
>> Exit as soon as any error hit, or continue defrag as much as possible?
>>
>>
>> And I'll rebase my btrfs_defrag_ctrl patchset upon your fixes.
>
> OK, during my rebasing, I found a bug in the rebased version of "btrfs:
> reduce extent threshold for autodefrag".
>
> It doesn't really pass defrag->extent_thresh into btrfs_defrag_file(),
> thus it's not working at all.

This is the fixed version of that patch, based on your branch:

https://github.com/adam900710/linux/commit/5759b9f0006d205019d2ba9220b52c58054f3758

And my branch autodefrag_fixes has rebased all patches (with a small
reordering) upon your branch.

With trace event and my local test case, it indeeds shows the new defrag
will only defrag uncompressed writes smaller than 64K.

I'll submit a new test case for it.

Thanks,
Qu
>
> Thanks,
> Qu
>>
>> Thanks,
>> Qu