On Tue, Dec 11, 2018 at 04:03:15PM +0100, Johannes Thumshirn wrote:
> err holds the return value of either btrfs_del_root_ref() or
> btrfs_del_inode_ref() but it hasn't been checked since it's
> introduction with commit fe66a05a0679 (Btrfs: improve error handling
> for btrfs_insert_dir_item callers) in 2012.
>
> To quote David:
>
> "If the error handling in the error handling fails, there's not much left to
> do
> and the abort either happened earlier in the callees or is necessary here."
>
> So if one of btrfs_del_root_ref() or btrfs_del_inode_ref() failed, abort the
> transaction.
>
> Link: https://lore.kernel.org/linux-btrfs/[email protected]/
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> fs/btrfs/inode.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c5c27d5457cc..c9457d2580c0 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6311,6 +6311,7 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
> struct btrfs_root *root = parent_inode->root;
> u64 ino = btrfs_ino(inode);
> u64 parent_ino = btrfs_ino(parent_inode);
> + int err = 0;
>
> if (unlikely(ino == BTRFS_FIRST_FREE_OBJECTID)) {
> memcpy(&key, &inode->root->root_key, sizeof(key));
> @@ -6355,18 +6356,21 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
> fail_dir_item:
> if (unlikely(ino == BTRFS_FIRST_FREE_OBJECTID)) {
> u64 local_index;
> - int err;
> +
> err = btrfs_del_root_ref(trans, key.objectid,
> root->root_key.objectid, parent_ino,
> &local_index, name, name_len);
>
> } else if (add_backref) {
> u64 local_index;
> - int err;
>
> err = btrfs_del_inode_ref(trans, root, name, name_len,
> ino, parent_ino, &local_index);
> }
> +
> + if (err)
> + btrfs_abort_transaction(trans, err);
> +
> return ret;
> }
What I wanted is this:
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6360,12 +6360,16 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
root->root_key.objectid, parent_ino,
&local_index, name, name_len);
+ if (err)
+ btrrfs_abort_transaction(trans, err);
} else if (add_backref) {
u64 local_index;
int err;
err = btrfs_del_inode_ref(trans, root, name, name_len,
ino, parent_ino, &local_index);
+ if (err)
+ btrrfs_abort_transaction(trans, err);
}
return ret;
}
Ie. each error has its own transaction abort that are not merged to one point.
I believe I said this during the previous reviews.
