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

[Matthew Wilcox <willy@infradead.org>]

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).
In short, filesystems never need to call folio_index(), nor page_index().
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!).

> > @@ -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!