Re: [PATCH] btrfs: Convert add_ra_bio_pages() to use a folio

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

在 2024/2/28 08:05, Matthew Wilcox 写道:
> On Fri, Jan 26, 2024 at 06:56:29AM +0000, Matthew Wilcox (Oracle) wrote:
>> Allocate order-0 folios instead of pages.  Saves twelve hidden calls
>> to compound_head().
>
> Ping?  This is one of the few remaining callers of
> add_to_page_cache_lru() and I'd like to get rid of it soon.

Not an expert thus I can only do some basic reviews here.

>
>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> ---
>>   fs/btrfs/compression.c | 58 ++++++++++++++++++++----------------------
>>   1 file changed, 28 insertions(+), 30 deletions(-)
>>
>> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
>> index 68345f73d429..517f9bc58749 100644
>> --- a/fs/btrfs/compression.c
>> +++ b/fs/btrfs/compression.c
>> @@ -421,7 +421,6 @@ static noinline int add_ra_bio_pages(struct inode *inode,
>>   	u64 cur = cb->orig_bbio->file_offset + orig_bio->bi_iter.bi_size;
>>   	u64 isize = i_size_read(inode);
>>   	int ret;
>> -	struct page *page;
>>   	struct extent_map *em;
>>   	struct address_space *mapping = inode->i_mapping;
>>   	struct extent_map_tree *em_tree;
>> @@ -447,6 +446,7 @@ static noinline int add_ra_bio_pages(struct inode *inode,
>>   	end_index = (i_size_read(inode) - 1) >> PAGE_SHIFT;
>>
>>   	while (cur < compressed_end) {
>> +		struct folio *folio;
>>   		u64 page_end;
>>   		u64 pg_index = cur >> PAGE_SHIFT;
>>   		u32 add_size;
>> @@ -454,8 +454,12 @@ static noinline int add_ra_bio_pages(struct inode *inode,
>>   		if (pg_index > end_index)
>>   			break;
>>
>> -		page = xa_load(&mapping->i_pages, pg_index);
>> -		if (page && !xa_is_value(page)) {
>> +		folio = xa_load(&mapping->i_pages, pg_index);

Not familiar with the xa_load usage, does this mean for order-0 pages,
folio and page pointers are interchangeable?

And what if we go higher order folios in the futuer? Would that still be
interchangeable?

[...]
>>   		}
>>   		free_extent_map(em);
>>
>> -		if (page->index == end_index) {
>> -			size_t zero_offset = offset_in_page(isize);
>> -
>> -			if (zero_offset) {
>> -				int zeros;
>> -				zeros = PAGE_SIZE - zero_offset;
>> -				memzero_page(page, zero_offset, zeros);
>> -			}
>> -		}
>> +		if (folio->index == end_index)
>> +			folio_zero_segment(folio, offset_in_page(isize),
>> +					folio_size(folio));

This doesn't sound correct to me. If @isize is page aligned, e.g. 4K,
and we're the first folio of the page cache, this would zero the first
folio, meanwhile the old code would do nothing.

Thanks,
Qu

>>
>>   		add_size = min(em->start + em->len, page_end + 1) - cur;
>> -		ret = bio_add_page(orig_bio, page, add_size, offset_in_page(cur));
>> +		ret = bio_add_folio(orig_bio, folio, add_size, offset_in_page(cur));
>>   		if (ret != add_size) {
>>   			unlock_extent(tree, cur, page_end, NULL);
>> -			unlock_page(page);
>> -			put_page(page);
>> +			folio_unlock(folio);
>> +			folio_put(folio);
>>   			break;
>>   		}
>>   		/*
>> @@ -541,9 +539,9 @@ static noinline int add_ra_bio_pages(struct inode *inode,
>>   		 * subpage::readers and to unlock the page.
>>   		 */
>>   		if (fs_info->sectorsize < PAGE_SIZE)
>> -			btrfs_subpage_start_reader(fs_info, page_folio(page),
>> +			btrfs_subpage_start_reader(fs_info, folio,
>>   						   cur, add_size);
>> -		put_page(page);
>> +		folio_put(folio);
>>   		cur += add_size;
>>   	}
>>   	return 0;
>> --
>> 2.43.0
>>