Re: [PATCH v2 1/3] btrfs: introduce a read path dedicated extent lock helper

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

在 2025/2/25 23:30, David Sterba 写道:
> On Wed, Feb 12, 2025 at 01:22:45PM +1030, Qu Wenruo wrote:
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
[...]
>> +	folio = filemap_get_folio(binode->vfs_inode.i_mapping,
>> +				  cur >> PAGE_SHIFT);
>
> Should this be folio_shift?

This is the biggest trap!

The filemap_* helpers are always using page index, no matter the folio size.

The filemap can be considered as a super large array of folio pointers.
(Implenmented by xarray)

For the current folio size == page size case, it's straight forward, if
there is a pointer then there is a cached folio for that index.

For larger folios, the overall idea is not changed, just we can have a
larger folio covering multiple slots, no longer one folio one slot.

So when doing the search we should always use PAGE_SHIFT.

And that why I hope the MM guys can provide a fileoff based
filemap_get_folio_by_fileoff().

CC MM guys, will a dedicated helper reduce such confusion?
Or it's just making the currently very simple filemap_*() helpers too
complex?

Thanks,
Qu

[...]
>> +again:
>> +	lock_extent(&binode->io_tree, start, end, cached_state);
>> +	cur_pos = start;
>> +	while (cur_pos < end) {
>> +		ordered = btrfs_lookup_ordered_range(binode, cur_pos,
>> +						     end - cur_pos + 1);
>> +		/*
>> +		 * No ordered extents in the range, and we hold the
>> +		 * extent lock, no one can modify the extent maps
>> +		 * in the range, we're safe to return.
>> +		 */
>> +		if (!ordered)
>> +			break;
>> +
>> +		/* Check if we can skip waiting for the whole OE. */
>> +		if (can_skip_ordered_extent(binode, ordered, start, end)) {
>> +			cur_pos = min(ordered->file_offset + ordered->num_bytes,
>> +				      end + 1);
>> +			btrfs_put_ordered_extent(ordered);
>> +			continue;
>> +		}
>> +
>> +		/* Now wait for the OE to finish. */
>> +		unlock_extent(&binode->io_tree, start, end,
>> +			      cached_state);
>> +		btrfs_start_ordered_extent(ordered, start, end + 1 - start);
>> +		btrfs_put_ordered_extent(ordered);
>> +		/* We have unlocked the whole range, restart from the beginning. */
>> +		goto again;
>
> This is a bit wild, goto at the end of a while loop but I don't see a
> cleaner way without making complicated in another way.

I have fixed this in the one submitted the mail list, by introducing
another layer of while loop (in another function).

Thanks,
Qu