[PATCH 0/3] Cleanup backref_in_log and its callers

[PATCH 0/3] Cleanup backref_in_log and its callers

[Nikolay Borisov]

This series cleans up backref_in_log and its callers. Patch 1 removes the 
opencoding of btrfs_find_name_in_backref which greatly simplifies backref_in_log
itself. 

Patch 2 properly propagates error values of the internal functions to
backref_in_log's callers and also fixes, where necessary, callers to cope with 
those ret values. 

Patch 3 continues in the spirit of the previous patch, in that it open codes 
name_in_log_ref so that the caller can properly handle backref_in_log retvals. 

Nikolay Borisov (3):
  btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log
  btrfs: Properly handle backref_in_log retval
  btrfs: Open-code name_in_log_ref in replay_one_name

 fs/btrfs/tree-log.c | 113 +++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 69 deletions(-)

-- 
2.17.1

[PATCH 1/3] btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/tree-log.c | 48 ++++++++++++---------------------------------
 1 file changed, 12 insertions(+), 36 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 77b6797fcac3..369046a754b0 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -945,54 +945,30 @@ static noinline int backref_in_log(struct btrfs_root *log,
 				   const char *name, int namelen)
 {
 	struct btrfs_path *path;
-	struct btrfs_inode_ref *ref;
-	unsigned long ptr;
-	unsigned long ptr_end;
-	unsigned long name_ptr;
-	int found_name_len;
-	int item_size;
 	int ret;
-	int match = 0;
 
 	path = btrfs_alloc_path();
 	if (!path)
 		return -ENOMEM;
 
 	ret = btrfs_search_slot(NULL, log, key, path, 0, 0);
-	if (ret != 0)
-		goto out;
-
-	ptr = btrfs_item_ptr_offset(path->nodes[0], path->slots[0]);
-
-	if (key->type == BTRFS_INODE_EXTREF_KEY) {
-		if (btrfs_find_name_in_ext_backref(path->nodes[0],
-						   path->slots[0],
-						   ref_objectid,
-						   name, namelen))
-			match = 1;
-
+	if (ret != 0) {
+		ret = 0;
 		goto out;
 	}
 
-	item_size = btrfs_item_size_nr(path->nodes[0], path->slots[0]);
-	ptr_end = ptr + item_size;
-	while (ptr < ptr_end) {
-		ref = (struct btrfs_inode_ref *)ptr;
-		found_name_len = btrfs_inode_ref_name_len(path->nodes[0], ref);
-		if (found_name_len == namelen) {
-			name_ptr = (unsigned long)(ref + 1);
-			ret = memcmp_extent_buffer(path->nodes[0], name,
-						   name_ptr, namelen);
-			if (ret == 0) {
-				match = 1;
-				goto out;
-			}
-		}
-		ptr = (unsigned long)(ref + 1) + found_name_len;
-	}
+	if (key->type == BTRFS_INODE_EXTREF_KEY)
+		ret = !!btrfs_find_name_in_ext_backref(path->nodes[0],
+						       path->slots[0],
+						       ref_objectid,
+						       name, namelen);
+	else
+		ret = !!btrfs_find_name_in_backref(path->nodes[0],
+						   path->slots[0],
+						   name, namelen);
 out:
 	btrfs_free_path(path);
-	return match;
+	return ret;
 }
 
 static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
-- 
2.17.1

[PATCH 2/3] btrfs: Properly handle backref_in_log retval

[Nikolay Borisov]

This function can return a negative error value if btrfs_search_slot
errors for whatever reason or if btrfs_alloc_path runs out of memory.
This is currently problemattic because backref_in_log is treated by its
callers as if it returns boolean.

Fix this by adding proper error handling in callers. That also enables
the function to return the direct error code from btrfs_search_slot.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/tree-log.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 369046a754b0..ca294ff463de 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -952,10 +952,8 @@ static noinline int backref_in_log(struct btrfs_root *log,
 		return -ENOMEM;
 
 	ret = btrfs_search_slot(NULL, log, key, path, 0, 0);
-	if (ret != 0) {
-		ret = 0;
+	if (ret != 0)
 		goto out;
-	}
 
 	if (key->type == BTRFS_INODE_EXTREF_KEY)
 		ret = !!btrfs_find_name_in_ext_backref(path->nodes[0],
@@ -1026,10 +1024,13 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 					   (unsigned long)(victim_ref + 1),
 					   victim_name_len);
 
-			if (!backref_in_log(log_root, &search_key,
-					    parent_objectid,
-					    victim_name,
-					    victim_name_len)) {
+			ret = backref_in_log(log_root, &search_key,
+					     parent_objectid, victim_name,
+					     victim_name_len);
+			if (ret < 0) {
+				kfree(victim_name);
+				return ret;
+			} else if (!ret) {
 				inc_nlink(&inode->vfs_inode);
 				btrfs_release_path(path);
 
@@ -1091,10 +1092,12 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 			search_key.offset = btrfs_extref_hash(parent_objectid,
 							      victim_name,
 							      victim_name_len);
-			ret = 0;
-			if (!backref_in_log(log_root, &search_key,
-					    parent_objectid, victim_name,
-					    victim_name_len)) {
+			ret = backref_in_log(log_root, &search_key,
+					     parent_objectid, victim_name,
+					     victim_name_len);
+			if (ret < 0) {
+				return ret;
+			} else if (!ret) {
 				ret = -ENOENT;
 				victim_parent = read_one_inode(root,
 						parent_objectid);
@@ -1869,16 +1872,19 @@ static bool name_in_log_ref(struct btrfs_root *log_root,
 			    const u64 dirid, const u64 ino)
 {
 	struct btrfs_key search_key;
+	int ret;
 
 	search_key.objectid = ino;
 	search_key.type = BTRFS_INODE_REF_KEY;
 	search_key.offset = dirid;
-	if (backref_in_log(log_root, &search_key, dirid, name, name_len))
+	ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
+	if (ret == 1)
 		return true;
 
 	search_key.type = BTRFS_INODE_EXTREF_KEY;
 	search_key.offset = btrfs_extref_hash(dirid, name, name_len);
-	if (backref_in_log(log_root, &search_key, dirid, name, name_len))
+	ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
+	if (ret == 1)
 		return true;
 
 	return false;
-- 
2.17.1

[PATCH 3/3] btrfs: Open-code name_in_log_ref in replay_one_name

[Nikolay Borisov]

That function adds unnecessary indirection between backref_in_log and
the caller. Furthermore it also "downgrades" backref_in_log's return
value to a boolean, when in fact it could very well be an error.

Rectify the situation by simply opencoding name_in_log_ref in
replay_one_name and properly handling possible return codes from
backref_in_log.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/tree-log.c | 51 +++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index ca294ff463de..822dd0486f7c 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1863,33 +1863,6 @@ static noinline int insert_one_name(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-/*
- * Return true if an inode reference exists in the log for the given name,
- * inode and parent inode.
- */
-static bool name_in_log_ref(struct btrfs_root *log_root,
-			    const char *name, const int name_len,
-			    const u64 dirid, const u64 ino)
-{
-	struct btrfs_key search_key;
-	int ret;
-
-	search_key.objectid = ino;
-	search_key.type = BTRFS_INODE_REF_KEY;
-	search_key.offset = dirid;
-	ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
-	if (ret == 1)
-		return true;
-
-	search_key.type = BTRFS_INODE_EXTREF_KEY;
-	search_key.offset = btrfs_extref_hash(dirid, name, name_len);
-	ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
-	if (ret == 1)
-		return true;
-
-	return false;
-}
-
 /*
  * take a single entry in a log directory item and replay it into
  * the subvolume.
@@ -2006,8 +1979,28 @@ static noinline int replay_one_name(struct btrfs_trans_handle *trans,
 	return ret;
 
 insert:
-	if (name_in_log_ref(root->log_root, name, name_len,
-			    key->objectid, log_key.objectid)) {
+	/* Ensure the to-be-added name is not already in the subvol tree */
+	found_key.objectid = log_key.objectid;
+	found_key.type = BTRFS_INODE_REF_KEY;
+	found_key.offset = key->objectid;
+	ret = backref_in_log(root->log_root, &found_key, 0, name, name_len);
+	if (ret < 0)
+	        goto out;
+	else if (ret) {
+	        /* The dentry will be added later. */
+	        ret = 0;
+	        update_size = false;
+	        goto out;
+	}
+
+	found_key.objectid = log_key.objectid;
+	found_key.type = BTRFS_INODE_EXTREF_KEY;
+	found_key.offset = key->objectid;
+	ret = backref_in_log(root->log_root, &found_key, key->objectid, name,
+			     name_len);
+	if (ret < 0) {
+		goto out;
+	} else if (ret) {
 		/* The dentry will be added later. */
 		ret = 0;
 		update_size = false;
-- 
2.17.1

Re: [PATCH 2/3] btrfs: Properly handle backref_in_log retval

[David Sterba]

On Fri, Aug 30, 2019 at 05:44:48PM +0300, Nikolay Borisov wrote:
> This function can return a negative error value if btrfs_search_slot
> errors for whatever reason or if btrfs_alloc_path runs out of memory.
> This is currently problemattic because backref_in_log is treated by its
> callers as if it returns boolean.
> 
> Fix this by adding proper error handling in callers. That also enables
> the function to return the direct error code from btrfs_search_slot.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/tree-log.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 369046a754b0..ca294ff463de 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -952,10 +952,8 @@ static noinline int backref_in_log(struct btrfs_root *log,
>  		return -ENOMEM;
>  
>  	ret = btrfs_search_slot(NULL, log, key, path, 0, 0);
> -	if (ret != 0) {
> -		ret = 0;
> +	if (ret != 0)
>  		goto out;
> -	}

Is this correct? Before: nonzero -> zero, after the nonzero value is
returned from the function. The return value is checked by the callers
but this obscures the fact that it comes from btrfs_search_slot and is
not just 0 or < 0.

Because search slot returns 1 in case it does not find the key, which
is translated to ENOENT in some contexts. Here it would be "false", as
the reference is not found. So logically the return code of search slot
and find backref is inverted.

Due to this inverted logic I'd rather avoid propagating the semantics of
search slot error value and always make it obvious that it follows the
natural expectations after reading the code like:

	ret = backref_in_log()
	if (ret < 0)
		return ...;
	if (ret == 0)
		/* no, backref is not in log */
	if (ret > 0)
		/* yes, backref is in the log */

>  	if (key->type == BTRFS_INODE_EXTREF_KEY)
>  		ret = !!btrfs_find_name_in_ext_backref(path->nodes[0],
> @@ -1026,10 +1024,13 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
>  					   (unsigned long)(victim_ref + 1),
>  					   victim_name_len);
>  
> -			if (!backref_in_log(log_root, &search_key,
> -					    parent_objectid,
> -					    victim_name,
> -					    victim_name_len)) {
> +			ret = backref_in_log(log_root, &search_key,
> +					     parent_objectid, victim_name,
> +					     victim_name_len);
> +			if (ret < 0) {
> +				kfree(victim_name);
> +				return ret;
> +			} else if (!ret) {
>  				inc_nlink(&inode->vfs_inode);
>  				btrfs_release_path(path);
>  
> @@ -1091,10 +1092,12 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
>  			search_key.offset = btrfs_extref_hash(parent_objectid,
>  							      victim_name,
>  							      victim_name_len);
> -			ret = 0;
> -			if (!backref_in_log(log_root, &search_key,
> -					    parent_objectid, victim_name,
> -					    victim_name_len)) {
> +			ret = backref_in_log(log_root, &search_key,
> +					     parent_objectid, victim_name,
> +					     victim_name_len);
> +			if (ret < 0) {
> +				return ret;
> +			} else if (!ret) {
>  				ret = -ENOENT;
>  				victim_parent = read_one_inode(root,
>  						parent_objectid);
> @@ -1869,16 +1872,19 @@ static bool name_in_log_ref(struct btrfs_root *log_root,
>  			    const u64 dirid, const u64 ino)
>  {
>  	struct btrfs_key search_key;
> +	int ret;
>  
>  	search_key.objectid = ino;
>  	search_key.type = BTRFS_INODE_REF_KEY;
>  	search_key.offset = dirid;
> -	if (backref_in_log(log_root, &search_key, dirid, name, name_len))
> +	ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
> +	if (ret == 1)
>  		return true;
>  
>  	search_key.type = BTRFS_INODE_EXTREF_KEY;
>  	search_key.offset = btrfs_extref_hash(dirid, name, name_len);
> -	if (backref_in_log(log_root, &search_key, dirid, name, name_len))
> +	ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
> +	if (ret == 1)
>  		return true;

The error handling in the code snippets above seems to follow the
exptected logic but I don't see that it's correct due to the search slot
value inversion.

Re: [PATCH 1/3] btrfs: Don't opencode btrfs_find_name_in_backref in backref_in_log

[David Sterba]

On Fri, Aug 30, 2019 at 05:44:47PM +0300, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>
I wrote a short changelog.

[PATCH v2] btrfs: Properly handle backref_in_log retval

[Nikolay Borisov]

This function can return a negative error value if btrfs_search_slot
errors for whatever reason or if btrfs_alloc_path runs out of memory.
This is currently problemattic because backref_in_log is treated by its
callers as if it returns boolean.

Fix this by adding proper error handling in callers. That also enables
the function to return the direct error code from btrfs_search_slot.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

v2: 
 * Return 0 from backref_in_log in case btrfs_search_slot returns 1 (meaning it 
 didn't find appropriate item in the btree). 

 fs/btrfs/tree-log.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 7cac09a6f007..7332f7a00790 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -952,7 +952,9 @@ static noinline int backref_in_log(struct btrfs_root *log,
 		return -ENOMEM;
 
 	ret = btrfs_search_slot(NULL, log, key, path, 0, 0);
-	if (ret != 0) {
+	if (ret < 0) {
+		goto out;
+	} else if (ret == 1) {
 		ret = 0;
 		goto out;
 	}
@@ -1026,10 +1028,13 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 					   (unsigned long)(victim_ref + 1),
 					   victim_name_len);
 
-			if (!backref_in_log(log_root, &search_key,
-					    parent_objectid,
-					    victim_name,
-					    victim_name_len)) {
+			ret = backref_in_log(log_root, &search_key,
+					     parent_objectid, victim_name,
+					     victim_name_len);
+			if (ret < 0) {
+				kfree(victim_name);
+				return ret;
+			} else if (!ret) {
 				inc_nlink(&inode->vfs_inode);
 				btrfs_release_path(path);
 
@@ -1091,10 +1096,12 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 			search_key.offset = btrfs_extref_hash(parent_objectid,
 							      victim_name,
 							      victim_name_len);
-			ret = 0;
-			if (!backref_in_log(log_root, &search_key,
-					    parent_objectid, victim_name,
-					    victim_name_len)) {
+			ret = backref_in_log(log_root, &search_key,
+					     parent_objectid, victim_name,
+					     victim_name_len);
+			if (ret < 0) {
+				return ret;
+			} else if (!ret) {
 				ret = -ENOENT;
 				victim_parent = read_one_inode(root,
 						parent_objectid);
@@ -1869,16 +1876,19 @@ static bool name_in_log_ref(struct btrfs_root *log_root,
 			    const u64 dirid, const u64 ino)
 {
 	struct btrfs_key search_key;
+	int ret;
 
 	search_key.objectid = ino;
 	search_key.type = BTRFS_INODE_REF_KEY;
 	search_key.offset = dirid;
-	if (backref_in_log(log_root, &search_key, dirid, name, name_len))
+	ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
+	if (ret == 1)
 		return true;
 
 	search_key.type = BTRFS_INODE_EXTREF_KEY;
 	search_key.offset = btrfs_extref_hash(dirid, name, name_len);
-	if (backref_in_log(log_root, &search_key, dirid, name, name_len))
+	ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
+	if (ret == 1)
 		return true;
 
 	return false;
-- 
2.17.1

Re: [PATCH v2] btrfs: Properly handle backref_in_log retval

[Filipe Manana]

On Thu, Sep 26, 2019 at 10:27 AM Nikolay Borisov <nborisov@suse.com> wrote:
>
> This function can return a negative error value if btrfs_search_slot
> errors for whatever reason or if btrfs_alloc_path runs out of memory.
> This is currently problemattic because backref_in_log is treated by its
> callers as if it returns boolean.
>
> Fix this by adding proper error handling in callers. That also enables
> the function to return the direct error code from btrfs_search_slot.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>
> v2:
>  * Return 0 from backref_in_log in case btrfs_search_slot returns 1 (meaning it
>  didn't find appropriate item in the btree).
>
>  fs/btrfs/tree-log.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 7cac09a6f007..7332f7a00790 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -952,7 +952,9 @@ static noinline int backref_in_log(struct btrfs_root *log,
>                 return -ENOMEM;
>
>         ret = btrfs_search_slot(NULL, log, key, path, 0, 0);
> -       if (ret != 0) {
> +       if (ret < 0) {
> +               goto out;
> +       } else if (ret == 1) {
>                 ret = 0;
>                 goto out;
>         }
> @@ -1026,10 +1028,13 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
>                                            (unsigned long)(victim_ref + 1),
>                                            victim_name_len);
>
> -                       if (!backref_in_log(log_root, &search_key,
> -                                           parent_objectid,
> -                                           victim_name,
> -                                           victim_name_len)) {
> +                       ret = backref_in_log(log_root, &search_key,
> +                                            parent_objectid, victim_name,
> +                                            victim_name_len);
> +                       if (ret < 0) {
> +                               kfree(victim_name);
> +                               return ret;
> +                       } else if (!ret) {
>                                 inc_nlink(&inode->vfs_inode);
>                                 btrfs_release_path(path);
>
> @@ -1091,10 +1096,12 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
>                         search_key.offset = btrfs_extref_hash(parent_objectid,
>                                                               victim_name,
>                                                               victim_name_len);
> -                       ret = 0;
> -                       if (!backref_in_log(log_root, &search_key,
> -                                           parent_objectid, victim_name,
> -                                           victim_name_len)) {
> +                       ret = backref_in_log(log_root, &search_key,
> +                                            parent_objectid, victim_name,
> +                                            victim_name_len);
> +                       if (ret < 0) {
> +                               return ret;
> +                       } else if (!ret) {
>                                 ret = -ENOENT;
>                                 victim_parent = read_one_inode(root,
>                                                 parent_objectid);
> @@ -1869,16 +1876,19 @@ static bool name_in_log_ref(struct btrfs_root *log_root,
>                             const u64 dirid, const u64 ino)
>  {
>         struct btrfs_key search_key;
> +       int ret;
>
>         search_key.objectid = ino;
>         search_key.type = BTRFS_INODE_REF_KEY;
>         search_key.offset = dirid;
> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
> +       if (ret == 1)
>                 return true;
>
>         search_key.type = BTRFS_INODE_EXTREF_KEY;
>         search_key.offset = btrfs_extref_hash(dirid, name, name_len);
> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
> +       if (ret == 1)
>                 return true;

This function also needs to be able to return errors and its caller
check for errors.

Thanks.

>
>         return false;
> --
> 2.17.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] btrfs: Properly handle backref_in_log retval

[Nikolay Borisov]

On 26.09.19 г. 12:31 ч., Filipe Manana wrote:
> On Thu, Sep 26, 2019 at 10:27 AM Nikolay Borisov <nborisov@suse.com> wrote:
>>
>> This function can return a negative error value if btrfs_search_slot
>> errors for whatever reason or if btrfs_alloc_path runs out of memory.
>> This is currently problemattic because backref_in_log is treated by its
>> callers as if it returns boolean.
>>
>> Fix this by adding proper error handling in callers. That also enables
>> the function to return the direct error code from btrfs_search_slot.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>
>> v2:
>>  * Return 0 from backref_in_log in case btrfs_search_slot returns 1 (meaning it
>>  didn't find appropriate item in the btree).
>>
>>  fs/btrfs/tree-log.c | 32 +++++++++++++++++++++-----------
>>  1 file changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>> index 7cac09a6f007..7332f7a00790 100644
>> --- a/fs/btrfs/tree-log.c
>> +++ b/fs/btrfs/tree-log.c
>> @@ -952,7 +952,9 @@ static noinline int backref_in_log(struct btrfs_root *log,
>>                 return -ENOMEM;
>>
>>         ret = btrfs_search_slot(NULL, log, key, path, 0, 0);
>> -       if (ret != 0) {
>> +       if (ret < 0) {
>> +               goto out;
>> +       } else if (ret == 1) {
>>                 ret = 0;
>>                 goto out;
>>         }
>> @@ -1026,10 +1028,13 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
>>                                            (unsigned long)(victim_ref + 1),
>>                                            victim_name_len);
>>
>> -                       if (!backref_in_log(log_root, &search_key,
>> -                                           parent_objectid,
>> -                                           victim_name,
>> -                                           victim_name_len)) {
>> +                       ret = backref_in_log(log_root, &search_key,
>> +                                            parent_objectid, victim_name,
>> +                                            victim_name_len);
>> +                       if (ret < 0) {
>> +                               kfree(victim_name);
>> +                               return ret;
>> +                       } else if (!ret) {
>>                                 inc_nlink(&inode->vfs_inode);
>>                                 btrfs_release_path(path);
>>
>> @@ -1091,10 +1096,12 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
>>                         search_key.offset = btrfs_extref_hash(parent_objectid,
>>                                                               victim_name,
>>                                                               victim_name_len);
>> -                       ret = 0;
>> -                       if (!backref_in_log(log_root, &search_key,
>> -                                           parent_objectid, victim_name,
>> -                                           victim_name_len)) {
>> +                       ret = backref_in_log(log_root, &search_key,
>> +                                            parent_objectid, victim_name,
>> +                                            victim_name_len);
>> +                       if (ret < 0) {
>> +                               return ret;
>> +                       } else if (!ret) {
>>                                 ret = -ENOENT;
>>                                 victim_parent = read_one_inode(root,
>>                                                 parent_objectid);
>> @@ -1869,16 +1876,19 @@ static bool name_in_log_ref(struct btrfs_root *log_root,
>>                             const u64 dirid, const u64 ino)
>>  {
>>         struct btrfs_key search_key;
>> +       int ret;
>>
>>         search_key.objectid = ino;
>>         search_key.type = BTRFS_INODE_REF_KEY;
>>         search_key.offset = dirid;
>> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
>> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
>> +       if (ret == 1)
>>                 return true;
>>
>>         search_key.type = BTRFS_INODE_EXTREF_KEY;
>>         search_key.offset = btrfs_extref_hash(dirid, name, name_len);
>> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
>> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
>> +       if (ret == 1)
>>                 return true;
> 
> This function also needs to be able to return errors and its caller
> check for errors.

Yes but this is for a follow up patch. The current patch does not make
the code any more broken than it currently is.

> 
> Thanks.
> 
>>
>>         return false;
>> --
>> 2.17.1
>>
> 
> 

Re: [PATCH v2] btrfs: Properly handle backref_in_log retval

[David Sterba]

On Thu, Sep 26, 2019 at 01:39:58PM +0300, Nikolay Borisov wrote:
> >> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
> >> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
> >> +       if (ret == 1)
> >>                 return true;
> > 
> > This function also needs to be able to return errors and its caller
> > check for errors.
> 
> Yes but this is for a follow up patch. The current patch does not make
> the code any more broken than it currently is.

I'm going to merge the patches, please send the followup patch soon, so
we don't forget about adding the proper error handling. Thanks.

Re: [PATCH v2] btrfs: Properly handle backref_in_log retval

[David Sterba]

On Thu, Oct 03, 2019 at 02:55:59PM +0200, David Sterba wrote:
> On Thu, Sep 26, 2019 at 01:39:58PM +0300, Nikolay Borisov wrote:
> > >> -       if (backref_in_log(log_root, &search_key, dirid, name, name_len))
> > >> +       ret = backref_in_log(log_root, &search_key, dirid, name, name_len);
> > >> +       if (ret == 1)
> > >>                 return true;
> > > 
> > > This function also needs to be able to return errors and its caller
> > > check for errors.
> > 
> > Yes but this is for a follow up patch. The current patch does not make
> > the code any more broken than it currently is.
> 
> I'm going to merge the patches, please send the followup patch soon, so
> we don't forget about adding the proper error handling. Thanks.

Never mind, the patch 3/3 inlines the function and the error handling is
integrated to the caller.