[PATCH 0/3] Cleanup btrfs_find_name_in* functions

[PATCH 0/3] Cleanup btrfs_find_name_in* functions

[Nikolay Borisov]

This series cleans up the calling convention of the 2 function used to search 
the INODE_REF items for the presence of particular name. Namely, I switch the 2
functions to directly return a struct btrfs_inode_(ext)?ref. This reduces the 
number of parameters when calling the functions. 

While doing the aforementioned cleanup I stumbled upon backref_in_log which was 
opencoding btrfs_find_name_in_backref hence I just refactored the function. 

This series survived xfstest. 

Nikolay Borisov (3):
  btrfs: Make btrfs_find_name_in_backref return btrfs_inode_ref struct
  btrfs: Make btrfs_find_name_in_ext_backref return struct
    btrfs_inode_extref
  btrfs: Use btrfs_find_name_in_backref in backref_in_log

 fs/btrfs/ctree.h      | 15 +++++-----
 fs/btrfs/inode-item.c | 62 ++++++++++++++++++-----------------------
 fs/btrfs/tree-log.c   | 65 +++++++++++++------------------------------
 3 files changed, 54 insertions(+), 88 deletions(-)

-- 
2.17.1

[PATCH 1/3] btrfs: Make btrfs_find_name_in_backref return btrfs_inode_ref struct

[Nikolay Borisov]

btrfs_find_name_in_backref returns either 0/1 depending on whether it
found a backref for the given name. If it returns true then the actual
inode_ref struct is returned in one of its parameters. That's pointless,
instead refactor the function such that it returns either a pointer
to the btrfs_inode_ref or NULL it it didn't find anything. This
streamlines the function calling convention.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h      |  6 +++---
 fs/btrfs/inode-item.c | 29 ++++++++++++++---------------
 fs/btrfs/tree-log.c   |  4 ++--
 3 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 6052f7403032..11312aeb6ff6 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2860,9 +2860,9 @@ btrfs_lookup_inode_extref(struct btrfs_trans_handle *trans,
 			  u64 inode_objectid, u64 ref_objectid, int ins_len,
 			  int cow);
 
-int btrfs_find_name_in_backref(struct extent_buffer *leaf, int slot,
-			       const char *name,
-			       int name_len, struct btrfs_inode_ref **ref_ret);
+struct btrfs_inode_ref *btrfs_find_name_in_backref(struct extent_buffer *leaf,
+						   int slot, const char *name,
+						   int name_len);
 int btrfs_find_name_in_ext_backref(struct extent_buffer *leaf, int slot,
 				   u64 ref_objectid, const char *name,
 				   int name_len,
diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index 30d62ef918b9..6f43bedd3700 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -8,9 +8,9 @@
 #include "transaction.h"
 #include "print-tree.h"
 
-int btrfs_find_name_in_backref(struct extent_buffer *leaf, int slot,
-			       const char *name,
-			       int name_len, struct btrfs_inode_ref **ref_ret)
+struct btrfs_inode_ref *btrfs_find_name_in_backref(struct extent_buffer *leaf,
+						   int slot, const char *name,
+						   int name_len)
 {
 	struct btrfs_inode_ref *ref;
 	unsigned long ptr;
@@ -28,13 +28,10 @@ int btrfs_find_name_in_backref(struct extent_buffer *leaf, int slot,
 		cur_offset += len + sizeof(*ref);
 		if (len != name_len)
 			continue;
-		if (memcmp_extent_buffer(leaf, name, name_ptr, name_len) == 0) {
-			if (ref_ret)
-				*ref_ret = ref;
-			return 1;
-		}
+		if (memcmp_extent_buffer(leaf, name, name_ptr, name_len) == 0)
+			return ref;
 	}
-	return 0;
+	return NULL;
 }
 
 int btrfs_find_name_in_ext_backref(struct extent_buffer *leaf, int slot,
@@ -213,8 +210,10 @@ int btrfs_del_inode_ref(struct btrfs_trans_handle *trans,
 	} else if (ret < 0) {
 		goto out;
 	}
-	if (!btrfs_find_name_in_backref(path->nodes[0], path->slots[0],
-					name, name_len, &ref)) {
+
+	ref = btrfs_find_name_in_backref(path->nodes[0], path->slots[0], name,
+					 name_len);
+	if (!ref) {
 		ret = -ENOENT;
 		search_ext_refs = 1;
 		goto out;
@@ -341,9 +340,9 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans,
 				      ins_len);
 	if (ret == -EEXIST) {
 		u32 old_size;
-
-		if (btrfs_find_name_in_backref(path->nodes[0], path->slots[0],
-					       name, name_len, &ref))
+		ref = btrfs_find_name_in_backref(path->nodes[0], path->slots[0],
+					       name, name_len);
+		if (ref)
 			goto out;
 
 		old_size = btrfs_item_size_nr(path->nodes[0], path->slots[0]);
@@ -359,7 +358,7 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans,
 		if (ret == -EOVERFLOW) {
 			if (btrfs_find_name_in_backref(path->nodes[0],
 						       path->slots[0],
-						       name, name_len, &ref))
+						       name, name_len))
 				ret = -EEXIST;
 			else
 				ret = -EMLINK;
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 19a4b9dc669f..12151eb81976 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1271,7 +1271,7 @@ static int unlink_old_inode_refs(struct btrfs_trans_handle *trans,
 							     namelen, NULL);
 		else
 			ret = btrfs_find_name_in_backref(log_eb, log_slot, name,
-							 namelen, NULL);
+							 namelen) != NULL;
 
 		if (!ret) {
 			struct inode *dir;
@@ -1338,7 +1338,7 @@ static int btrfs_inode_ref_exists(struct inode *inode, struct inode *dir,
 						     name, namelen, NULL);
 	else
 		ret = btrfs_find_name_in_backref(path->nodes[0], path->slots[0],
-						 name, namelen, NULL);
+						 name, namelen) != NULL;
 
 out:
 	btrfs_free_path(path);
-- 
2.17.1

Re: [PATCH 1/3] btrfs: Make btrfs_find_name_in_backref return btrfs_inode_ref struct

[David Sterba]

On Tue, Aug 27, 2019 at 02:46:28PM +0300, Nikolay Borisov wrote:
>  			ret = btrfs_find_name_in_backref(log_eb, log_slot, name,
> -							 namelen, NULL);
> +							 namelen) != NULL;

Isn't there a less confusing and obscure way to do that?

Re: [PATCH 1/3] btrfs: Make btrfs_find_name_in_backref return btrfs_inode_ref struct

[David Sterba]

On Tue, Aug 27, 2019 at 04:13:41PM +0200, David Sterba wrote:
> On Tue, Aug 27, 2019 at 02:46:28PM +0300, Nikolay Borisov wrote:
> >  			ret = btrfs_find_name_in_backref(log_eb, log_slot, name,
> > -							 namelen, NULL);
> > +							 namelen) != NULL;
> 
> Isn't there a less confusing and obscure way to do that?

As patch 3/3 does, ret = !!btrfs_..., I'll switch to it where needed.

[PATCH 2/3] btrfs: Make btrfs_find_name_in_ext_backref return struct btrfs_inode_extref

[Nikolay Borisov]

btrfs_find_name_in_ext_backref returns either 0/1 depending on whether it
found a backref for the given name. If it returns true then the actual
inode_ref struct is returned in one of its parameters. That's pointless,
instead refactor the function such that it returns either a pointer
to the btrfs_inode_extref or NULL it it didn't find anything. This
streamlines the function calling convention.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h      |  9 ++++-----
 fs/btrfs/inode-item.c | 33 +++++++++++++--------------------
 fs/btrfs/tree-log.c   |  6 +++---
 3 files changed, 20 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 11312aeb6ff6..8b9469df8e3f 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2863,11 +2863,10 @@ btrfs_lookup_inode_extref(struct btrfs_trans_handle *trans,
 struct btrfs_inode_ref *btrfs_find_name_in_backref(struct extent_buffer *leaf,
 						   int slot, const char *name,
 						   int name_len);
-int btrfs_find_name_in_ext_backref(struct extent_buffer *leaf, int slot,
-				   u64 ref_objectid, const char *name,
-				   int name_len,
-				   struct btrfs_inode_extref **extref_ret);
-
+struct btrfs_inode_extref *
+btrfs_find_name_in_ext_backref(struct extent_buffer *leaf, int slot,
+			       u64 ref_objectid, const char *name,
+			       int name_len);
 /* file-item.c */
 struct btrfs_dio_private;
 int btrfs_del_csums(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index 6f43bedd3700..d1ee56dde758 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -34,10 +34,9 @@ struct btrfs_inode_ref *btrfs_find_name_in_backref(struct extent_buffer *leaf,
 	return NULL;
 }
 
-int btrfs_find_name_in_ext_backref(struct extent_buffer *leaf, int slot,
-				   u64 ref_objectid,
-				   const char *name, int name_len,
-				   struct btrfs_inode_extref **extref_ret)
+struct btrfs_inode_extref *
+btrfs_find_name_in_ext_backref(struct extent_buffer *leaf, int slot,
+			       u64 ref_objectid, const char *name, int name_len)
 {
 	struct btrfs_inode_extref *extref;
 	unsigned long ptr;
@@ -62,15 +61,12 @@ int btrfs_find_name_in_ext_backref(struct extent_buffer *leaf, int slot,
 
 		if (ref_name_len == name_len &&
 		    btrfs_inode_extref_parent(leaf, extref) == ref_objectid &&
-		    (memcmp_extent_buffer(leaf, name, name_ptr, name_len) == 0)) {
-			if (extref_ret)
-				*extref_ret = extref;
-			return 1;
-		}
+		    (memcmp_extent_buffer(leaf, name, name_ptr, name_len) == 0))
+			return extref;
 
 		cur_offset += ref_name_len + sizeof(*extref);
 	}
-	return 0;
+	return NULL;
 }
 
 /* Returns NULL if no extref found */
@@ -84,7 +80,6 @@ btrfs_lookup_inode_extref(struct btrfs_trans_handle *trans,
 {
 	int ret;
 	struct btrfs_key key;
-	struct btrfs_inode_extref *extref;
 
 	key.objectid = inode_objectid;
 	key.type = BTRFS_INODE_EXTREF_KEY;
@@ -95,11 +90,9 @@ btrfs_lookup_inode_extref(struct btrfs_trans_handle *trans,
 		return ERR_PTR(ret);
 	if (ret > 0)
 		return NULL;
-	if (!btrfs_find_name_in_ext_backref(path->nodes[0], path->slots[0],
-					    ref_objectid, name, name_len,
-					    &extref))
-		return NULL;
-	return extref;
+	return btrfs_find_name_in_ext_backref(path->nodes[0], path->slots[0],
+					      ref_objectid, name, name_len);
+
 }
 
 static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
@@ -139,9 +132,9 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans,
 	 * This should always succeed so error here will make the FS
 	 * readonly.
 	 */
-	if (!btrfs_find_name_in_ext_backref(path->nodes[0], path->slots[0],
-					    ref_objectid,
-					    name, name_len, &extref)) {
+	extref = btrfs_find_name_in_ext_backref(path->nodes[0], path->slots[0],
+						ref_objectid, name, name_len);
+	if (!extref) {
 		btrfs_handle_fs_error(root->fs_info, -ENOENT, NULL);
 		ret = -EROFS;
 		goto out;
@@ -284,7 +277,7 @@ static int btrfs_insert_inode_extref(struct btrfs_trans_handle *trans,
 		if (btrfs_find_name_in_ext_backref(path->nodes[0],
 						   path->slots[0],
 						   ref_objectid,
-						   name, name_len, NULL))
+						   name, name_len))
 			goto out;
 
 		btrfs_extend_item(path, ins_len);
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 12151eb81976..7d45a4869bc9 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -967,7 +967,7 @@ static noinline int backref_in_log(struct btrfs_root *log,
 		if (btrfs_find_name_in_ext_backref(path->nodes[0],
 						   path->slots[0],
 						   ref_objectid,
-						   name, namelen, NULL))
+						   name, namelen))
 			match = 1;
 
 		goto out;
@@ -1268,7 +1268,7 @@ static int unlink_old_inode_refs(struct btrfs_trans_handle *trans,
 		if (key->type == BTRFS_INODE_EXTREF_KEY)
 			ret = btrfs_find_name_in_ext_backref(log_eb, log_slot,
 							     parent_id, name,
-							     namelen, NULL);
+							     namelen) != NULL;
 		else
 			ret = btrfs_find_name_in_backref(log_eb, log_slot, name,
 							 namelen) != NULL;
@@ -1335,7 +1335,7 @@ static int btrfs_inode_ref_exists(struct inode *inode, struct inode *dir,
 	if (key.type == BTRFS_INODE_EXTREF_KEY)
 		ret = btrfs_find_name_in_ext_backref(path->nodes[0],
 						     path->slots[0], parent_id,
-						     name, namelen, NULL);
+						     name, namelen) != NULL;
 	else
 		ret = btrfs_find_name_in_backref(path->nodes[0], path->slots[0],
 						 name, namelen) != NULL;
-- 
2.17.1

Re: [PATCH 2/3] btrfs: Make btrfs_find_name_in_ext_backref return struct btrfs_inode_extref

[David Sterba]

On Tue, Aug 27, 2019 at 02:46:29PM +0300, Nikolay Borisov wrote:
> btrfs_find_name_in_ext_backref returns either 0/1 depending on whether it
> found a backref for the given name. If it returns true then the actual
> inode_ref struct is returned in one of its parameters. That's pointless,
> instead refactor the function such that it returns either a pointer
> to the btrfs_inode_extref or NULL it it didn't find anything. This
> streamlines the function calling convention.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ctree.h      |  9 ++++-----
>  fs/btrfs/inode-item.c | 33 +++++++++++++--------------------
>  fs/btrfs/tree-log.c   |  6 +++---
>  3 files changed, 20 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 11312aeb6ff6..8b9469df8e3f 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2863,11 +2863,10 @@ btrfs_lookup_inode_extref(struct btrfs_trans_handle *trans,
>  struct btrfs_inode_ref *btrfs_find_name_in_backref(struct extent_buffer *leaf,
>  						   int slot, const char *name,
>  						   int name_len);
> -int btrfs_find_name_in_ext_backref(struct extent_buffer *leaf, int slot,
> -				   u64 ref_objectid, const char *name,
> -				   int name_len,
> -				   struct btrfs_inode_extref **extref_ret);
> -
> +struct btrfs_inode_extref *
> +btrfs_find_name_in_ext_backref(struct extent_buffer *leaf, int slot,
> +			       u64 ref_objectid, const char *name,
> +			       int name_len);

Please use the common style for function declarations/definitions with
type and name on one line.

[PATCH 3/3] btrfs: Use btrfs_find_name_in_backref in backref_in_log

[Nikolay Borisov]

By not opencoding btrfs_find_name_in_backref most of the local variables
can be removed, this in turn alleviates stack pressure. Additionally,
backref_in_log is only used as predicate so make it return bool.

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

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 7d45a4869bc9..070016e023b8 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -938,60 +938,35 @@ static noinline int inode_in_dir(struct btrfs_root *root,
  * want to delete valid links to a file from the subvolume if that
  * link is also in the log.
  */
-static noinline int backref_in_log(struct btrfs_root *log,
-				   struct btrfs_key *key,
-				   u64 ref_objectid,
-				   const char *name, int namelen)
+static noinline bool backref_in_log(struct btrfs_root *log,
+				    struct btrfs_key *key,
+				    u64 ref_objectid,
+				    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;
+	bool found = false;
 	int ret;
-	int match = 0;
 
 	path = btrfs_alloc_path();
 	if (!path)
-		return -ENOMEM;
+		return false;
 
 	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;
-
-		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)
+		found = !!btrfs_find_name_in_ext_backref(path->nodes[0],
+							 path->slots[0],
+							 ref_objectid,
+							 name, namelen);
+	else
+		found = !!btrfs_find_name_in_backref(path->nodes[0],
+						     path->slots[0],
+						     name, namelen);
 out:
 	btrfs_free_path(path);
-	return match;
+	return found;
 }
 
 static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
-- 
2.17.1

Re: [PATCH 3/3] btrfs: Use btrfs_find_name_in_backref in backref_in_log

[David Sterba]

On Tue, Aug 27, 2019 at 02:46:30PM +0300, Nikolay Borisov wrote:
> By not opencoding btrfs_find_name_in_backref most of the local variables
> can be removed, this in turn alleviates stack pressure. Additionally,
> backref_in_log is only used as predicate so make it return bool.

While it is used as bool, it returns error numbers so the callsites
should be updated to distinguish errors from true/false.

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/tree-log.c | 57 +++++++++++++--------------------------------
>  1 file changed, 16 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 7d45a4869bc9..070016e023b8 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -938,60 +938,35 @@ static noinline int inode_in_dir(struct btrfs_root *root,
>   * want to delete valid links to a file from the subvolume if that
>   * link is also in the log.
>   */
> -static noinline int backref_in_log(struct btrfs_root *log,
> -				   struct btrfs_key *key,
> -				   u64 ref_objectid,
> -				   const char *name, int namelen)
> +static noinline bool backref_in_log(struct btrfs_root *log,
> +				    struct btrfs_key *key,
> +				    u64 ref_objectid,
> +				    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;
> +	bool found = false;
>  	int ret;
> -	int match = 0;
>  
>  	path = btrfs_alloc_path();
>  	if (!path)
> -		return -ENOMEM;
> +		return false;

There would have to be a very good reasoning behind that but I doubt
it's correct.

>  	ret = btrfs_search_slot(NULL, log, key, path, 0, 0);
>  	if (ret != 0)

This should also check for errors, though it's not changed in your
patch.

So if you want to de-opencode btrfs_find_name_in_backref, then fine, but
the other changes should be separate and possibly updating the whole
callchain.

Re: [PATCH 0/3] Cleanup btrfs_find_name_in* functions

[David Sterba]

On Tue, Aug 27, 2019 at 02:46:27PM +0300, Nikolay Borisov wrote:
> This series cleans up the calling convention of the 2 function used to search 
> the INODE_REF items for the presence of particular name. Namely, I switch the 2
> functions to directly return a struct btrfs_inode_(ext)?ref. This reduces the 
> number of parameters when calling the functions. 
> 
> While doing the aforementioned cleanup I stumbled upon backref_in_log which was 
> opencoding btrfs_find_name_in_backref hence I just refactored the function. 
> 
> This series survived xfstest. 
> 
> Nikolay Borisov (3):
>   btrfs: Make btrfs_find_name_in_backref return btrfs_inode_ref struct
>   btrfs: Make btrfs_find_name_in_ext_backref return struct
>     btrfs_inode_extref
>   btrfs: Use btrfs_find_name_in_backref in backref_in_log

1 and 2 merged to misc-next, thanks.