Re: [PATCH 2/2] btrfs: Use a folio array throughout the defrag process

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

On 2023/12/9 07:54, Matthew Wilcox wrote:
> On Sat, Dec 09, 2023 at 07:44:34AM +1030, Qu Wenruo wrote:
>>> @@ -875,7 +875,7 @@ static struct page *defrag_prepare_one_page(struct btrfs_inode *inode, pgoff_t i
>>>    	folio = __filemap_get_folio(mapping, index,
>>>    			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
>>>    	if (IS_ERR(folio))
>>> -		return &folio->page;
>>
>> What's the proper way to grab the first page of a folio?
>
> It depends why you're doing it.
>
> If you're just doing it temporarily to convert back from folio to page
> so you can make progress on the conversion, use &folio->page.  That
> is a "bad code smell", but acceptable while you're doing the conversion.
> It's a good signal "there is more work here to be done".
>
>> For now, I'm going folio_page() and it's the same wrapper using folio->page,
>> but I'm wondering would there be any recommendation for it.
>
> folio_page() is good when you actually need a particular page.  eg you're
> going to call kmap().  When we get to finally divorcing folio from page,
> folio_page() will still work, and &folio->page won't.
>
>>> @@ -1172,7 +1172,7 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
>>>    	const u64 len = target->len;
>>>    	unsigned long last_index = (start + len - 1) >> PAGE_SHIFT;
>>>    	unsigned long start_index = start >> PAGE_SHIFT;
>>> -	unsigned long first_index = page_index(pages[0]);
>>> +	unsigned long first_index = folios[0]->index;
>>
>> The same for index, there is a folio_index() wrapper.
>>
>> So should we go the wrapper or would the wrapper be gone in the future?
>
> folio_index() has a specific purpose (just like page_index() did, but it
> seems like a lot of filesystem people never read the kernel-doc on it).

Indeed I should check the code, it says very clearly:

  * If you know
  * the page is definitely in the page cache, you can look at the folio's
  * index directly.


> In short, filesystems never need to call folio_index(), nor page_index().

Yep, as when fs got the page they are mostly already mapped.

> You can always just use folio->index unless you're in the direct-io path
> (and you shouldn't be looking at folio_index() then either!).

And DIO pages are never mapped thus never call that.

>
>>> @@ -1189,8 +1189,8 @@ static int defrag_one_locked_target(struct btrfs_inode *inode,
>>>    	/* Update the page status */
>>>    	for (i = start_index - first_index; i <= last_index - first_index; i++) {
>>> -		ClearPageChecked(pages[i]);
>>> -		btrfs_page_clamp_set_dirty(fs_info, pages[i], start, len);
>>> +		folio_clear_checked(folios[i]);
>>> +		btrfs_page_clamp_set_dirty(fs_info, &folios[i]->page, start, len);
>>
>> After my patch "2/3 btrfs: migrate subpage code to folio interfaces",
>> btrfs_page_*() helpers accept folio parameter directly, thus this may lead
>> to a conflicts.
>
> I built against linux-next 20231207; I thought all your folio changes
> had landed there?  If you want to adopt these patches and land them
> after yours, I am more than OK with that!

No big deal, David and I can definitely handle the conflicts, no need to
bother you.

Thanks a lot for the lesson on the folio interface best practice!
Qu

>