Re: [PATCH 3/4] btrfs: handle ENOMEM from btrfs_insert_dir_item() without aborting

[Boris Burkov <boris@bur.io>]

On Fri, Jul 17, 2026 at 07:55:35PM -0400, Jeff Layton wrote:
> On Fri, 2026-07-17 at 16:04 -0700, Boris Burkov wrote:
> > On Fri, Jul 17, 2026 at 06:40:42PM -0400, Jeff Layton wrote:
> > > On Fri, 2026-07-17 at 13:18 -0700, Boris Burkov wrote:
> > > > On Fri, Jul 17, 2026 at 12:52:38PM -0400, Jeff Layton wrote:
> > > > > Now that btrfs_insert_dir_item() returns -ENOMEM before modifying the
> > > > > btree (thanks to delayed dir index pre-allocation), callers can handle
> > > > > ENOMEM gracefully instead of aborting the transaction.
> > > > > 
> > > > > In btrfs_add_link(), add -ENOMEM to the set of recoverable errors
> > > > > alongside -EEXIST and -EOVERFLOW. The fail_dir_item cleanup path
> > > > > unwinds the inode_ref/root_ref and returns the error to userspace.
> > > > > 
> > > > > In btrfs_create_new_inode(), when btrfs_add_link() fails with -ENOMEM,
> > > > > convert the newly-created inode into an orphan instead of aborting.
> > > > > This is done by clearing nlink and adding an orphan item, which ensures > > > > > btrfs_evict_inode() will delete the INODE_ITEM and INODE_REF, and
> > > > > crash-recovery will clean it up via orphan processing. If
> > > > > btrfs_orphan_add() itself fails, we fall back to aborting.
> > > > > 
> > > > > This turns a filesystem-killing transaction abort into a graceful
> > > > > -ENOMEM return to userspace for create(), mkdir(), mknod(), symlink(),
> > > > > and link() operations under memory pressure.
> > > > > 
> > > > > Assisted-by: LLM
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > ---
> > > > >  fs/btrfs/inode.c | 17 +++++++++++++++--
> > > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > > > > index b7b4e6177135..4d9947ae08f7 100644
> > > > > --- a/fs/btrfs/inode.c
> > > > > +++ b/fs/btrfs/inode.c
> > > > > @@ -6676,7 +6676,20 @@ int btrfs_create_new_inode(struct btrfs_trans_handle *trans,
> > > > >  	} else {
> > > > >  		ret = btrfs_add_link(trans, BTRFS_I(dir), BTRFS_I(inode), name,
> > > > >  				     false, BTRFS_I(inode)->dir_index);
> > > > > -		if (unlikely(ret)) {
> > > > > +		if (ret == -ENOMEM) {

So we are now handling ENOMEM from btrfs_add_link()

> > > > > +			/*
> > > > > +			 * The ENOMEM came before the DIR_ITEM was inserted,
> > > > > +			 * so the btree has our INODE_ITEM + INODE_REF but no
> > > > > +			 * directory entry. Convert this into an orphan so
> > > > > +			 * eviction (or crash-recovery) cleans up the inode.
> > > > > +			 */
> > > > > +			clear_nlink(inode);
> > > > > +			ret = btrfs_orphan_add(trans, BTRFS_I(inode));

And when we get ENOMEM, we call btrfs_orphan_add() isntead of
unilaterally aborting. I contend that this call is now likely to ENOMEM

> > > > > +			if (unlikely(ret))
> > > > > +				btrfs_abort_transaction(trans, ret);

And abort here.

> > > > 
> > > > I feel like the crux of this series to me is whether you have practical
> > > > conditions where the allocation of the delayed_node is failing, but the
> > > > allocations involved in btrfs_orphan_add() succeed. It allocates a
> > > > btrfs_path and has to walk the btree which might have to read the node
> > > > at every level which might need to allocate 16k extent buffers and
> > > > extent buffer objects and xarray storage for each one. For size
> > > > reference, on my build (maybe debug..?) a delayed_node is 552 bytes,
> > > > while a btrfs_path is 112 and an extent_buffer is 432. So they are
> > > > pretty similar in size (not to mention the 16k of node file backed
> > > > memory we are sort of likely to have to allocate if we are under
> > > > reclaim)
> > > > 
> > > > Were you able to reproduce this issue and help in practice or is this a
> > > > theoretical / structural improvement?
> > > > 
> > > 
> > > I didn't really try to reproduce this in earnest. We only see it in our
> > > fleet under heavy memory pressure, and even then at such low frequency,
> > > I doubt our chances of hitting this on anything other than a huge set
> > > of machines.
> > > 
> > > So, theoretical / structural, but we have record of filesystem aborts
> > > where the stack indicates that this would have prevented it. Userland
> > > would have gotten an -ENOMEM back but the fs wouldn't have aborted.
> > > 
> > 
> > My concern is not that we don't hit ENOMEM in btrfs_add_link(), since
> > like you said we can observe that in abort logs. I am worried that even
> > if we try to handle it gracefully, we will just ENOMEM in
> > btrfs_orphan_add() and abort anyway. That is why I was wanting to see
> > some more concrete evidence this actually helps to make it worth the
> > complexity.
> > 
> 
> In the case where we handle this gracefully, we won't hit that because
> it will have returned -ENOMEM before that point. But, you do have a
> good point that we could allocate these objects successfully, and then
> hit an error in btrfs_orphan_add() anyway.

I am still a bit confused. I made comments inline closer to the the code.
Sorry if I am being dense or missing the point!

> 
> One thought: It looks like the main allocation in that codepath is
> btrfs_alloc_path()? We could consider preallocating that too -- maybe
> stash it in a new pointer in btrfs_trans_handle?
> 
> Thanks! This is good food for thought.

Take a peek at https://lore.kernel.org/linux-btrfs/cover.1782249000.git.boris@bur.io/

which I need to RESEND :)

Thanks,
Boris

> 
> > > I see that there are some ALLOW_ERROR_INJECTION() calls in btrfs. We
> > > could wire some of these functions up with that, which would make this
> > > easier to test. I'll look into that in the meantime.
> > > 
> > > > With that said, all the prealloc wiring looks good to me in general, and
> > > > it seems to be a pretty clean win for the "name exists" case in the next
> > > > patch.
> > > > 
> > > > 
> > > 
> > > Thanks for the review!
> > > 
> > > > > +			ret = -ENOMEM;
> > > > > +			goto discard;
> > > > > +		} else if (unlikely(ret)) {
> > > > >  			btrfs_abort_transaction(trans, ret);
> > > > >  			goto discard;
> > > > >  		}
> > > > > @@ -6738,7 +6751,7 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
> > > > >  
> > > > >  	ret = btrfs_insert_dir_item(trans, name, parent_inode, &key,
> > > > >  				    btrfs_inode_type(inode), index, NULL);
> > > > > -	if (ret == -EEXIST || ret == -EOVERFLOW)
> > > > > +	if (ret == -EEXIST || ret == -EOVERFLOW || ret == -ENOMEM)
> > > > >  		goto fail_dir_item;
> > > > >  	else if (unlikely(ret)) {
> > > > >  		btrfs_abort_transaction(trans, ret);
> > > > > 
> > > > > -- 
> > > > > 2.55.0
> > > > > 
> > > 
> > > -- 
> > > Jeff Layton <jlayton@kernel.org>
> 
> -- 
> Jeff Layton <jlayton@kernel.org>