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

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

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.

Thanks,
Qu