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

[PATCH v2] 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.

Instead of silently ignoring the return values, print a message so the user
knows what kind of error has encountered.

Link: https://lore.kernel.org/linux-btrfs/20181119141323.GC24115@twin.jikos.cz
Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---

Changes to v1:
* Only print an error message and let the callers abort the transaction (Dave)
---
 fs/btrfs/inode.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 9becf8543489..8ca2f82b35a3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6351,6 +6351,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;
 
 	if (unlikely(ino == BTRFS_FIRST_FREE_OBJECTID)) {
 		memcpy(&key, &inode->root->root_key, sizeof(key));
@@ -6395,17 +6396,25 @@ 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);
+		if (err)
+			btrfs_info(trans->fs_info,
+		   "failed to delete reference to %.*s, root %llu ref %llu",
+				   name_len, name, key.objectid,
+				   root->root_key.objectid);
 
 	} 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_info(trans->fs_info,
+	   "failed to delete reference to %.*s, inode %llu parent %llu",
+				   name_len, name, ino, parent_ino);
 	}
 	return ret;
 }
-- 
2.16.4

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

[David Sterba]

On Fri, Nov 23, 2018 at 09:42:43AM +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.
> 
> Instead of silently ignoring the return values, print a message so the user
> knows what kind of error has encountered.
> 
> Link: https://lore.kernel.org/linux-btrfs/20181119141323.GC24115@twin.jikos.cz
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
> 
> Changes to v1:
> * Only print an error message and let the callers abort the transaction (Dave)
> ---
>  fs/btrfs/inode.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 9becf8543489..8ca2f82b35a3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6351,6 +6351,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;
>  
>  	if (unlikely(ino == BTRFS_FIRST_FREE_OBJECTID)) {
>  		memcpy(&key, &inode->root->root_key, sizeof(key));
> @@ -6395,17 +6396,25 @@ 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);
> +		if (err)
> +			btrfs_info(trans->fs_info,
> +		   "failed to delete reference to %.*s, root %llu ref %llu",
> +				   name_len, name, key.objectid,
> +				   root->root_key.objectid);

I though a transaction abort would be here, as the state is not
consistent. Also I'm not sure what I as a user would get from such error
message after calling link(). 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.

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

[Johannes Thumshirn]

On 26/11/2018 19:37, David Sterba wrote:
> I though a transaction abort would be here, as the state is not
> consistent. Also I'm not sure what I as a user would get from such error
> message after calling link(). 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.

OK, now you've lost me. Isn't that what my previous patch did?

Byte,
	Johannes
-- 
Johannes Thumshirn                                        SUSE Labs
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

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

[David Sterba]

On Tue, Nov 27, 2018 at 09:56:43AM +0100, Johannes Thumshirn wrote:
> On 26/11/2018 19:37, David Sterba wrote:
> > I though a transaction abort would be here, as the state is not
> > consistent. Also I'm not sure what I as a user would get from such error
> > message after calling link(). 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.
> 
> OK, now you've lost me. Isn't that what my previous patch did?

Yes (call abort in fail_dir_item:) but it also merged all the abort
calls -- this is the part that I wanted to be updated.