Re: [PATCH 1/2] btrfs: Convert defrag_prepare_one_page() to use a folio

[Matthew Wilcox <willy@infradead.org>]

On Sat, Dec 09, 2023 at 07:26:45AM +1030, Qu Wenruo wrote:
> >   again:
> > -	page = find_or_create_page(mapping, index, mask);
> > -	if (!page)
> > -		return ERR_PTR(-ENOMEM);
> > +	folio = __filemap_get_folio(mapping, index,
> > +			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, mask);
> 
> When I was (and still am) a newbie to the folio interfaces, the "__"
> prefix is driving me away to use it.
> 
> Mind to change it in the future? Like adding a new
> filemap_get_or_create_folio()?

I'm always open to better naming ideas.  We definitely have too many
inline functions that call pagecache_get_page():

find_get_page()
find_get_page_flags()
find_lock_page()
find_or_create_page()
grab_cache_page_nowait()

... and I don't think anyone could tell you when to use which one
without looking at the implementations or cribbing from another
filesystem.

So I've tried to keep it simple for the folio replacements and so
far we only have:

filemap_get_folio()
filemap_lock_folio()
filemap_grab_folio()

and honestly, I don't love filemap_grab_folio() and I'm regretting
its creation.

What I do like is the creation of FGP_WRITEBEGIN, but that doesn't
address your concern about the leading __.  Would you be happier if
we renamed __filemap_get_folio() to filemap_get_folio_flags()?

This would all be much better if C allowed you to specify default
arguments to functions :-P

> > -	ret = set_page_extent_mapped(page);
> > +	ret = set_page_extent_mapped(&folio->page);
> 
> With my recent patches, set_page_extent_mapped() is already using folio
> interfaces, I guess it's time to finally change it to accept a folio.

I'll add this as a prep patch:

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b5a2399ed420..99ae54ab004c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -909,18 +909,22 @@ static int attach_extent_buffer_page(struct extent_buffer *eb,
 
 int set_page_extent_mapped(struct page *page)
 {
-	struct folio *folio = page_folio(page);
+	return set_folio_extent_mapped(page_folio(page));
+}
+
+int set_folio_extent_mapped(struct folio *folio)
+{
 	struct btrfs_fs_info *fs_info;
 
-	ASSERT(page->mapping);
+	ASSERT(folio->mapping);
 
 	if (folio_test_private(folio))
 		return 0;
 
-	fs_info = btrfs_sb(page->mapping->host->i_sb);
+	fs_info = btrfs_sb(folio->mapping->host->i_sb);
 
-	if (btrfs_is_subpage(fs_info, page))
-		return btrfs_attach_subpage(fs_info, page, BTRFS_SUBPAGE_DATA);
+	if (btrfs_is_subpage(fs_info, &folio->page))
+		return btrfs_attach_subpage(fs_info, &folio->page, BTRFS_SUBPAGE_DATA);
 
 	folio_attach_private(folio, (void *)EXTENT_FOLIO_PRIVATE);
 	return 0;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index c73d53c22ec5..b6b31fb41bdf 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -202,6 +202,7 @@ int btree_write_cache_pages(struct address_space *mapping,
 void extent_readahead(struct readahead_control *rac);
 int extent_fiemap(struct btrfs_inode *inode, struct fiemap_extent_info *fieinfo,
 		  u64 start, u64 len);
+int set_folio_extent_mapped(struct folio *folio);
 int set_page_extent_mapped(struct page *page);
 void clear_page_extent_mapped(struct page *page);
 

> > -		if (page->mapping != mapping || !PagePrivate(page)) {
> > -			unlock_page(page);
> > -			put_page(page);
> > +		if (folio->mapping != mapping || !folio->private) {
> 
> This folio->private check is not the same as PagePrivate(page) IIRC.
> Isn't folio_test_private() more suitable here?

One of the projects I'm pursuing on the side is getting rid of PG_private.
There's really no need to burn a very precious page flag telling us
whether the filesystem has something stored in folio->private when we
could just look at folio->private instead.

So yes, generates different code, but the two should be the same.