diff mbox series

btrfs: do not BUG_ON on failure to get dir index for new snapshot

Message ID 5563d2c9f143485c62d3ab07446ed1f77f765a2c.1686670878.git.fdmanana@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: do not BUG_ON on failure to get dir index for new snapshot | expand

Commit Message

Filipe Manana June 13, 2023, 3:42 p.m. UTC 
From: Filipe Manana <fdmanana@suse.com>

During the transaction commit path, at create_pending_snapshot(), there
is no need to BUG_ON() in case we fail to get a dir index for the snapshot
in the parent directory. This should fail very rarely because the parent
inode should be loaded in memory already, with the respective delayed
inode created and the parent inode's index_cnt field already initialized.

However if it fails, it may be -ENOMEM like the comment at
create_pending_snapshot() says or any error returned by
btrfs_search_slot() through btrfs_set_inode_index_count(), which can be
pretty much anything such as -EIO or -EUCLEAN for example. So the comment
is not correct when it says it can only be -ENOMEM.

However doing a BUG_ON() here is overkill, since we can instead abort
the transaction and return the error. Note that any error returned by
create_pending_snapshot() will eventually result in a transaction
abort at cleanup_transaction(), called from btrfs_commit_transaction(),
but we can explicitly abort the transaction at this point instead so that
we get a stack trace to tell us that the call to btrfs_set_inode_index()
failed.

So just abort the transaction and return in case btrfs_set_inode_index()
returned an error at create_pending_snapshot().

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/transaction.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Johannes Thumshirn June 14, 2023, 9:55 a.m. UTC | #1 
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
David Sterba June 14, 2023, 9:40 p.m. UTC | #2 
On Tue, Jun 13, 2023 at 04:42:16PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> During the transaction commit path, at create_pending_snapshot(), there
> is no need to BUG_ON() in case we fail to get a dir index for the snapshot
> in the parent directory. This should fail very rarely because the parent
> inode should be loaded in memory already, with the respective delayed
> inode created and the parent inode's index_cnt field already initialized.
> 
> However if it fails, it may be -ENOMEM like the comment at
> create_pending_snapshot() says or any error returned by
> btrfs_search_slot() through btrfs_set_inode_index_count(), which can be
> pretty much anything such as -EIO or -EUCLEAN for example. So the comment
> is not correct when it says it can only be -ENOMEM.
> 
> However doing a BUG_ON() here is overkill, since we can instead abort
> the transaction and return the error. Note that any error returned by
> create_pending_snapshot() will eventually result in a transaction
> abort at cleanup_transaction(), called from btrfs_commit_transaction(),
> but we can explicitly abort the transaction at this point instead so that
> we get a stack trace to tell us that the call to btrfs_set_inode_index()
> failed.
> 
> So just abort the transaction and return in case btrfs_set_inode_index()
> returned an error at create_pending_snapshot().
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.
diff mbox series

Patch

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index fe0f00e717a8..cf306351b148 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1684,7 +1684,10 @@  static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 	 * insert the directory item
 	 */
 	ret = btrfs_set_inode_index(BTRFS_I(parent_inode), &index);
-	BUG_ON(ret); /* -ENOMEM */
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		goto fail;
+	}
 
 	/* check if there is a file/dir which has the same name. */
 	dir_item = btrfs_lookup_dir_item(NULL, parent_root, path,