Re: [PATCH] btrfs: refactor alloc_extent_buffer() to allocate-then-attach method

[Josef Bacik <josef@toxicpanda.com>]

On Thu, Nov 23, 2023 at 06:30:05AM +1030, Qu Wenruo wrote:
> 
> 
> On 2023/11/23 00:44, Josef Bacik wrote:
> > On Wed, Nov 22, 2023 at 10:05:04AM +1030, Qu Wenruo wrote:
> > > Currently alloc_extent_buffer() utilizes find_or_create_page() to
> > > allocate one page a time for an extent buffer.
> > > 
> > > This method has the following disadvantages:
> > > 
> > > - find_or_create_page() is the legacy way of allocating new pages
> > >    With the new folio infrastructure, find_or_create_page() is just
> > >    redirected to filemap_get_folio().
> > > 
> > > - Lacks the way to support higher order (order >= 1) folios
> > >    As we can not yet let filemap to give us a higher order folio (yet).
> > > 
> > > This patch would change the workflow by the following way:
> > > 
> > > 		Old		   |		new
> > > -----------------------------------+-------------------------------------
> > >                                     | ret = btrfs_alloc_page_array();
> > > for (i = 0; i < num_pages; i++) {  | for (i = 0; i < num_pages; i++) {
> > >      p = find_or_create_page();     |     ret = filemap_add_folio();
> > >      /* Attach page private */      |     /* Reuse page cache if needed */
> > >      /* Reused eb if needed */      |
> > > 				   |     /* Attach page private and
> > > 				   |        reuse eb if needed */
> > > 				   | }
> > > 
> > > By this we split the page allocation and private attaching into two
> > > parts, allowing future updates to each part more easily, and migrate to
> > > folio interfaces (especially for possible higher order folios).
> > > 
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > ---
> > >   fs/btrfs/extent_io.c | 173 +++++++++++++++++++++++++++++++------------
> > >   1 file changed, 126 insertions(+), 47 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > index 99cc16aed9d7..0ea65f248c15 100644
> > > --- a/fs/btrfs/extent_io.c
> > > +++ b/fs/btrfs/extent_io.c
> > > @@ -3084,6 +3084,14 @@ static bool page_range_has_eb(struct btrfs_fs_info *fs_info, struct page *page)
> > >   static void detach_extent_buffer_page(struct extent_buffer *eb, struct page *page)
> > >   {
> > >   	struct btrfs_fs_info *fs_info = eb->fs_info;
> > > +	/*
> > > +	 * We can no longer using page->mapping reliably, as some extent buffer
> > > +	 * may not have any page mapped to btree_inode yet.
> > > +	 * Furthermore we have to handle dummy ebs during selftests, where
> > > +	 * btree_inode is not yet initialized.
> > > +	 */
> > > +	struct address_space *mapping = fs_info->btree_inode ?
> > > +					fs_info->btree_inode->i_mapping : NULL;
> > 
> > I don't understand this, this should only happen if we managed to get
> > PagePrivate set on the page, and in that case page->mapping is definitely
> > reliable.  We shouldn't be getting in here with a page that hasn't actually been
> > attached to the extent buffer, and if we are that needs to be fixed, we don't
> > need to be dealing with that case in this way.  Thanks,
> 
> The problem is, with the new way of allocating pages first, then attach
> them to filemap, we can have a case where the extent buffer has 4 pages,
> but only the first page is attached to filemap of btree inode.
> 
> In that case, we still want to lock the btree inode private, but
> page->mapping is still NULL for the remaining 3 pages.
> 

In this case I think we just change the error handling at the end, as we already
have to do something special anyways, so we now would do

for (int i = 0; i < attached; i++) {
	unlock_page(eb->pages[i]);
	detach_extent_buffer_page(eb->pages[i]);
}

for (int i = 0; i < num_pages; i++) {
	if (!eb->pages[i])
		break;
	put_page(eb->pages[i]);
}

and then we can leave detach_extent_buffer_page on its own.  Thanks,

Josef