[PATCH v3] btrfs: improve error handling of btrfs_add_link()

[PATCH v3] btrfs: improve error handling of btrfs_add_link()

[Johannes Thumshirn]

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/20181126183759.GJ2842@twin.jikos.cz/
Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 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;
 }
 
-- 
2.16.4

Re: [PATCH v3] btrfs: improve error handling of btrfs_add_link()

[David Sterba]

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/20181126183759.GJ2842@twin.jikos.cz/
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  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.

Re: [PATCH v3] btrfs: improve error handling of btrfs_add_link()

[Johannes Thumshirn]

On 12/12/2018 13:52, David Sterba wrote:
> 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.

Ah OK, I misunderstood you there. But now that I see it it makes sense.

Thanks,
	Johannes
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850