Re: [PATCH] btrfs: enhance function extent_range_clear_dirty_for_io()

[Qu Wenruo <wqu@suse.com>]

在 2024/5/20 20:21, Filipe Manana 写道:
> On Mon, May 20, 2024 at 4:56 AM Qu Wenruo <wqu@suse.com> wrote:
[...]
>> - Make it subpage compatible
>>    Although currently compression only happens in a full page basis even
>>    for subpage routine, there is no harm to make it subpage compatible
>>    now.
> 
> The changes seem ok and reasonable to me.
> 
> However I think these are really 3 separate changes that should be in
> 3 different patches.
> It makes it easier to review and to revert in case there's a need to do so.
> 
> So I would make the move to inode.c first, and then the other changes.
> Or the move last in case we need to backport the other changes.

Sure, that indeed sounds better

[...]
>> +       if (missing_folio)
>> +               return -ENOENT;
> 
> Why not return the error from filemap_get_folio()? We could keep it
> and then return it after finishing the loop.
> Currently it can only return -ENOENT, according to the function's
> comment, but it would be better future proof and return whatever error
> it returns.

Sure, although we can only either keep the first or the last error.

Thanks,
Qu

> 
> Thanks.
> 
>> +       return 0;
>> +}
>> +
>>   /*
>>    * Work queue call back to started compression on a file and pages.
>>    *
>> @@ -931,7 +957,10 @@ static void compress_file_range(struct btrfs_work *work)
>>           * Otherwise applications with the file mmap'd can wander in and change
>>           * the page contents while we are compressing them.
>>           */
>> -       extent_range_clear_dirty_for_io(&inode->vfs_inode, start, end);
>> +       ret = extent_range_clear_dirty_for_io(&inode->vfs_inode, start, end);
>> +
>> +       /* We have locked all the involved pagse, shouldn't hit a missing page. */
>> +       ASSERT(ret == 0);
>>
>>          /*
>>           * We need to save i_size before now because it could change in between
>> --
>> 2.45.1
>>
>>