Re: [PATCH v2 5/5] btrfs: prepare btrfs_punch_hole_lock_range() for large data folios

[Qu Wenruo <wqu@suse.com>]

在 2025/3/31 22:20, Filipe Manana 写道:
> On Sat, Mar 29, 2025 at 9:20 AM Qu Wenruo <wqu@suse.com> wrote:
[...]
>> +       found_folios = filemap_get_folios(inode->i_mapping, &tmp, end_index,
>> +                                         &fbatch);
>> +       for (int i = 0; i < found_folios; i++) {
>> +               struct folio *folio = fbatch.folios[i];
>> +
>> +               /* A large folio begins before the start. Not a target. */
>> +               if (folio->index < start_index)
>> +                       continue;
> 
> We passed start_index (via tmp) to filemap_get_folios(). Isn't the
> function supposed to return folios only at an index >= start_index?
> It's what the documentation says and the implementation seems to
> behave that way too.

Not exactly, comments of filemap_get_folios_tag() shows exactly this:

  * The first folio may start before @start; if it does, it will contain
  * @start.  The final folio may extend beyond @end; if it does, it will
  * contain @end.

This is exactly what we need to support large folios.

> 
> Removing that we could also use start_index to pass to
> filemap_get_folios(), making it non-const, and remove the tmp
> variable.

The reason I use @tmp is, filemap_get_folios() can update the @tmp to 
the next folio's start index, which makes us unable to use the original 
start_index to compare.

Thanks,
Qu

> 
> Either way it looks good and that's a minor thing:
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Thanks.
> 
>> +               /* A large folio extends beyond the end. Not a target. */
>> +               if (folio->index + folio_nr_pages(folio) > end_index)
>> +                       continue;
>> +               /* A folio doesn't cover the head/tail index. Found a target. */
>> +               ret = true;
>> +               break;
>> +       }
>> +       folio_batch_release(&fbatch);
>> +       return ret;
>> +}
>> +
>> +static void btrfs_punch_hole_lock_range(struct inode *inode,
>> +                                       const u64 lockstart,
>> +                                       const u64 lockend,
>> +                                       struct extent_state **cached_state)
>> +{
>>          while (1) {
>>                  truncate_pagecache_range(inode, lockstart, lockend);
>>
>>                  lock_extent(&BTRFS_I(inode)->io_tree, lockstart, lockend,
>>                              cached_state);
>> -               /* The same page or adjacent pages. */
>> -               if (page_lockend <= page_lockstart)
>> -                       break;
>>                  /*
>>                   * We can't have ordered extents in the range, nor dirty/writeback
>>                   * pages, because we have locked the inode's VFS lock in exclusive
>> @@ -2195,8 +2243,7 @@ static void btrfs_punch_hole_lock_range(struct inode *inode,
>>                   * locking the range check if we have pages in the range, and if
>>                   * we do, unlock the range and retry.
>>                   */
>> -               if (!filemap_range_has_page(inode->i_mapping, page_lockstart,
>> -                                           page_lockend - 1))
>> +               if (!check_range_has_page(inode, lockstart, lockend))
>>                          break;
>>
>>                  unlock_extent(&BTRFS_I(inode)->io_tree, lockstart, lockend,
>> --
>> 2.49.0
>>
>>\