Re: [PATCH 1/5] btrfs: factor init_extent_buffer from __alloc_extent_buffer

On Thu, Jun 25, 2026 at 04:26:56PM +0200, Johannes Thumshirn wrote:
> On 6/24/26 12:35 AM, Boris Burkov wrote:
> > In preparation for preallocating extent_buffer data, factor eb
> > initialization away from specifically allocating it. This allows us to
> > allocate the eb, bfs, folios, etc. together in the main search_slot code
> > paths, but still share initialization code with the dummy/test/clone
> > allocation paths.
> > 
> > Signed-off-by: Boris Burkov <[email protected]>
> > ---
> >   fs/btrfs/extent_io.c | 20 ++++++++++++--------
> >   1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 0edd532174fa..fa4cc8bcd1af 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -3054,12 +3054,9 @@ void btrfs_uninhibit_all_eb_writeback(struct btrfs_trans_handle *trans)
> >   	xa_destroy(&trans->writeback_inhibited_ebs);
> >   }
> > -static struct extent_buffer *__alloc_extent_buffer(struct btrfs_fs_info *fs_info,
> > -						   u64 start)
> > +static void init_extent_buffer(struct btrfs_fs_info *fs_info,
> > +			       struct extent_buffer *eb, u64 start)
> >   {
> > -	struct extent_buffer *eb = NULL;
> > -
> > -	eb = kmem_cache_zalloc(extent_buffer_cache, GFP_NOFS|__GFP_NOFAIL);
> >   	eb->start = start;
> >   	eb->len = fs_info->nodesize;
> >   	eb->fs_info = fs_info;
> > @@ -3072,7 +3069,15 @@ static struct extent_buffer *__alloc_extent_buffer(struct btrfs_fs_info *fs_info
> >   	refcount_set(&eb->refs, 1);
> >   	ASSERT(eb->len <= BTRFS_MAX_METADATA_BLOCKSIZE);
> > +}
> > +
> > +static struct extent_buffer *__alloc_extent_buffer(struct btrfs_fs_info *fs_info,
> > +						   u64 start)
> > +{
> > +	struct extent_buffer *eb;
> > +	eb = kmem_cache_zalloc(extent_buffer_cache, GFP_NOFS | __GFP_NOFAIL);
> > +	init_extent_buffer(fs_info, eb, start);
> >   	return eb;
> >   }
> > @@ -3476,9 +3481,8 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
> >   	if (eb)
> >   		return eb;
> > -	eb = __alloc_extent_buffer(fs_info, start);
> > -	if (!eb)
> > -		return ERR_PTR(-ENOMEM);
> > +	eb = kmem_cache_zalloc(extent_buffer_cache, GFP_NOFS | __GFP_NOFAIL);
> > +	init_extent_buffer(fs_info, eb, start);
> >   	/*
> >   	 * The reloc trees are just snapshots, so we need them to appear to be
> 
> This looks wrong to me. Not the split out code, but how it is used in
> alloc_extent_buffer(). Either you keep calling __alloc_extent_buffer() in
> alloc_extent_buffer() OR don't call init_extent_buffer() in
> __alloc_extent_buffer(). I see this is an intermediate step (and I haven't
> looked at the rest of the series yet) but it looks wrong.
> 

The idea is that you eventually (optionally) have the allocation happen
elsewhere, so alloc_extent_buffer should not call __alloc_extent_buffer
any more. That should be clearer in the second patch, I think.

I left __alloc_extent_buffer for btrfs_clone_extent_buffer() and
alloc_dummy_extent_buffer(). Would you prefer me to delete
__alloc_extent_buffer and open code it in those two callers?

Thanks,
Boris