Re: [PATCH v2 2/5] btrfs: split btrfs_insert_delayed_dir_index() into prealloc and commit phases

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2026-08-06 at 08:04 +0930, Qu Wenruo wrote:
> 
> 在 2026/8/5 21:24, Jeff Layton 写道:
> > On Wed, 2026-08-05 at 08:44 +0930, 'Qu Wenruo' via Kernel Team wrote:
> > > 
> > > 在 2026/8/5 01:14, Jeff Layton 写道:
> > > > Split btrfs_insert_delayed_dir_index() into three functions using a new
> > > > btrfs_dir_index_prealloc struct to bundle the pre-allocated resources:
> > > > 
> > > > - btrfs_prealloc_delayed_dir_index(): performs the two GFP_NOFS
> > > >     allocations (delayed node + delayed item) that can fail with -ENOMEM.
> > > > - btrfs_insert_delayed_dir_index_prealloc(): populates the item data,
> > > >     inserts into the rb-tree, and reserves metadata space. Cannot fail
> > > >     with -ENOMEM since all allocations were done in the prealloc step.
> > > > - btrfs_free_delayed_dir_index_prealloc(): frees pre-allocated
> > > >     resources when the caller's btree insertion fails.
> > > > 
> > > > The original btrfs_insert_delayed_dir_index() is refactored into a thin
> > > > wrapper that calls the prealloc and commit functions.
> > > > 
> > > > This split allows callers to move the fallible memory allocations before
> > > > the point of no return (the DIR_ITEM btree insertion), so that -ENOMEM
> > > > can be returned cleanly without aborting the transaction.
> > > > 
> > > > Assisted-by: LLM
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >    fs/btrfs/delayed-inode.c | 111 +++++++++++++++++++++++++++++++++++++----------
> > > >    fs/btrfs/delayed-inode.h |  17 ++++++++
> > > >    2 files changed, 104 insertions(+), 24 deletions(-)
> > > > 
> > > > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> > > > index db2ffab0941a..95d2dca80444 100644
> > > > --- a/fs/btrfs/delayed-inode.c
> > > > +++ b/fs/btrfs/delayed-inode.c
> > > > @@ -6,6 +6,7 @@
> > > >    
> > > >    #include <linux/slab.h>
> > > >    #include <linux/iversion.h>
> > > > +#include <linux/error-injection.h>
> > > >    #include "ctree.h"
> > > >    #include "fs.h"
> > > >    #include "messages.h"
> > > > @@ -1469,35 +1470,74 @@ static void btrfs_release_dir_index_item_space(struct btrfs_trans_handle *trans)
> > > >    	trans->bytes_reserved -= bytes;
> > > >    }
> > > >    
> > > > -/* Will return 0, -ENOMEM or -EEXIST (index number collision, unexpected). */
> > > > -int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
> > > > -				   const char *name, int name_len,
> > > > -				   struct btrfs_inode *dir,
> > > > -				   const struct btrfs_disk_key *disk_key, u8 flags,
> > > > -				   u64 index)
> > > > +/*
> > > > + * Pre-allocate a delayed node and delayed item for a dir index insertion.
> > > > + * Call this before modifying the btree so that ENOMEM can be returned
> > > > + * before any on-disk state has changed.
> > > > + *
> > > > + * Returns 0 on success, -ENOMEM on allocation failure.
> > > > + */
> > > > +int btrfs_prealloc_delayed_dir_index(struct btrfs_inode *dir,
> > > > +				     const char *name, int name_len,
> > > > +				     struct btrfs_dir_index_prealloc *prealloc)
> > > 
> > > We can directly return a btrfs_dir_index_prealloc pointer, which reduce
> > > the parameter list.
> > > 
> > 
> > ...but then we'd have to allocate it separately. It's currently
> > allocated on the stack by the caller. I think we ought to keep it this
> > way, but I'm willing to be convinced otherwise.
> 
> In the end, there are only two callsites using on-stack memory for 
> prealloc structure, one in btrfs_rename() and one in 
> btrfs_insert_dir_item().
> 
> So it would be not that hard to convert to heap memory based solution.
> 
> 
> One thing I found a little weird is related to the lifespan management 
> of btrfs_dir_index_prealloc.
> 
> We have several call sites doing things like:
> 
> 	struct btrfs_dir_index_prealloc prealloc = { };
> 
> 	ret = btrfs_prealloc_delayed_dir_index();
> 
> 	ret = btrfs_add_link(..., prealloc.item ? prealloc : NULL);
> 	prealloc.item = NULL;
> 
> Which looks a little weird to me, as we're using item to indicate if the 
> structure is properly allocated, not the structure itself.
> 
> If we go regular heap memory allocation, it would be more 
> straightforward and more common:
> 
> 	struct btrfs_dir_index_prealloc *prealloc = NULL;
> 
> 	prealloc = btrfs_prealloc_delayed_dir_index();
> 
> 	ret = btrfs_add_link(..., prealloc);
> 	prealloc = NULL;
> 
> Feel free to ignore this one, it's mostly preference.
> 

It's a good point. It is more natural to be able to just look and say
"NULL pointer means no prealloc".

OTOH, the entire reason we're doing this is to deal with low-memory
situations. Do we want to add another heap allocation into this
codepath?

Either way, I'll think about it.

Thanks again for the review!
--
Jeff Layton <jlayton@kernel.org>