Re: [PATCH 1/3] btrfs: remove unnecessary early exits in delalloc folio lock and unlock

[Qu Wenruo <wqu@suse.com>]

在 2025/4/5 02:34, Filipe Manana 写道:
> On Fri, Apr 4, 2025 at 2:48 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> Inside function unlock_delalloc_folio() and lock_delalloc_folios(), we
> 
> function -> functions
> 
>> have the following early exist:
> 
> exist -> exit
> 
>>
>>          if (index == locked_folio->index && end_index == index)
>>                  return;
>>
>> This allows us to exist early if the range are inside the same locked
> 
> exist -> exit
> the range are inside -> the range is inside
> 
>> folio.
>>
>> But even without the above early check, the existing code can handle it
>> well, as both __process_folios_contig() and lock_delalloc_folios() will
>> skip any folio page lock/unlock if it's on the locked folio.
>>
>> Just remove those unnecessary early exits.
> 
> It looks good to me from a functional point of view.
> 
> But without this early exits there's a bit of work done, from function
> calls to initializing and releasing folio batches, calling
> filemap_get_folios_contig()  which
> will search the mapping's xarray and always grab one folio and add it
> to the batch, etc.
> 
> It's not uncommon to do IO on a range not spanning more than one
> folio, especially when supporting large folios, which will be more
> likely.
> I understand the goal here is to remove some code, but this code is
> cheaper compared to all that unnecessary overhead.
> 
> Have you considered that?

Yes, but the main reason here is the usage of (folio->index == index) check.

With  large folios, such check is no longer reliable anyway, and 
initially I thought to just change it to folio_contains(), but it turns 
out that is not really needed either.

So that's the main reason to get rid this of these early exits.

Thanks,
Qu

> 
> Thanks.
> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/extent_io.c | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 8b29eeac3884..013268f70621 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -224,12 +224,7 @@ static noinline void unlock_delalloc_folio(const struct inode *inode,
>>                                             const struct folio *locked_folio,
>>                                             u64 start, u64 end)
>>   {
>> -       unsigned long index = start >> PAGE_SHIFT;
>> -       unsigned long end_index = end >> PAGE_SHIFT;
>> -
>>          ASSERT(locked_folio);
>> -       if (index == locked_folio->index && end_index == index)
>> -               return;
>>
>>          __process_folios_contig(inode->i_mapping, locked_folio, start, end,
>>                                  PAGE_UNLOCK);
>> @@ -246,9 +241,6 @@ static noinline int lock_delalloc_folios(struct inode *inode,
>>          u64 processed_end = start;
>>          struct folio_batch fbatch;
>>
>> -       if (index == locked_folio->index && index == end_index)
>> -               return 0;
>> -
>>          folio_batch_init(&fbatch);
>>          while (index <= end_index) {
>>                  unsigned int found_folios, i;
>> --
>> 2.49.0
>>
>>