Re: [PATCH 3/4] btrfs: handle ENOMEM from btrfs_insert_dir_item() without aborting
From: Qu Wenruo
Date: Fri Jul 17 2026 - 20:19:25 EST
在 2026/7/18 09:25, Jeff Layton 写道:
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@xxxxxxxxxx>
---
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) {
+ /*
+ * 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));
+ if (unlikely(ret))
+ btrfs_abort_transaction(trans, ret);
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.
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?
For btrfs_alloc_path(), you can just use on-stack memory for that.
That's the practice we utilize for btrfs-progs, and it's still acceptable for kernel, since that structure is only 112 bytes.
We should not switch all btrfs_path to on-stack ones, but for critical ones that memory allocation can lead to trans abort, I'd say it's definitely worth the extra on-stack memory usage.
Thanks,
Qu