[PATCHv2 0/8] btrfs: Create macro to iterate slots

[PATCHv2 0/8] btrfs: Create macro to iterate slots

[Marcos Paulo de Souza]

There is a common pattern when search for a key in btrfs:

 * Call btrfs_search_slot
 * Endless loop
     * If the found slot is bigger than the current items in the leaf, check the
       next one
     * If still not found in the next leaf, return 1
     * Do something with the code
     * Increment current slot, and continue

This pattern can be improved by creating an iterator macro, similar to
those for_each_X already existing in the linux kernel. using this
approach means to reduce significantly boilerplate code, along making it
easier to newcomers to understand how to code works.

This patchset survived a complete fstest run.

Changes from v1:
 * Separate xattr changes from the macro introducing code (Johannes)

Changes from RFC:
 * Add documentation to btrfs_for_each_slot macro and btrfs_valid_slot function
   (David)
 * Add documentation about the ret variable used as a macro argument (David)
 * Match function argument from prototype and implementation (David)
 * Changed ({ }) block to only () in btrfs_for_each_slot macro (David)
 * Add more patches to show the code being reduced by using this approach
   (Nikolay)

Marcos Paulo de Souza (8):
  fs: btrfs: Introduce btrfs_for_each_slot
  btrfs: block-group: use btrfs_for_each_slot in find_first_block_group
  btrfs: dev-replace: Use btrfs_for_each_slot in
    mark_block_group_to_copy
  btrfs: dir-item: use btrfs_for_each_slot in
    btrfs_search_dir_index_item
  btrfs: inode: use btrfs_for_each_slot in btrfs_read_readdir
  btrfs: send: Use btrfs_for_each_slot macro
  btrfs: volumes: use btrfs_for_each_slot in btrfs_read_chunk_tree
  btrfs: xattr: Use btrfs_for_each_slot macro in btrfs_listxattr

 fs/btrfs/block-group.c |  33 +-----
 fs/btrfs/ctree.c       |  28 ++++++
 fs/btrfs/ctree.h       |  25 +++++
 fs/btrfs/dev-replace.c |  51 ++--------
 fs/btrfs/dir-item.c    |  27 +----
 fs/btrfs/inode.c       |  46 ++++-----
 fs/btrfs/send.c        | 222 +++++++++++------------------------------
 fs/btrfs/volumes.c     |  23 ++---
 fs/btrfs/xattr.c       |  40 +++-----
 9 files changed, 169 insertions(+), 326 deletions(-)

-- 
2.31.1

[PATCH 1/8] fs: btrfs: Introduce btrfs_for_each_slot

[Marcos Paulo de Souza]

There is a common pattern when search for a key in btrfs:

 * Call btrfs_search_slot
 * Endless loop
         * If the found slot is bigger than the current items in the leaf, check the
           next one
         * If still not found in the next leaf, return 1
         * Do something with the code
         * Increment current slot, and continue

This pattern can be improved by creating an iterator macro, similar to
those for_each_X already existing in the linux kernel. Using this
approach means to reduce significantly boilerplate code, along making it
easier to newcomers to understand how to code works.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/ctree.c | 28 ++++++++++++++++++++++++++++
 fs/btrfs/ctree.h | 25 +++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 84627cbd5b5b..b1aa6e3987d0 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2122,6 +2122,34 @@ int btrfs_search_backwards(struct btrfs_root *root, struct btrfs_key *key,
 	return ret;
 }
 
+/* Search for a valid slot for the given path.
+ * @root: The root node of the tree.
+ * @key: Will contain a valid item if found.
+ * @path: The start point to validate the slot.
+ *
+ * Return 0 if the item is valid, 1 if not found and < 0 if error.
+ */
+int btrfs_valid_slot(struct btrfs_root *root, struct btrfs_key *key,
+		     struct btrfs_path *path)
+{
+	while (1) {
+		int ret;
+		const int slot = path->slots[0];
+		const struct extent_buffer *leaf = path->nodes[0];
+
+		if (slot >= btrfs_header_nritems(leaf)) {
+			ret = btrfs_next_leaf(root, path);
+			if (ret)
+				return ret;
+			continue;
+		}
+		btrfs_item_key_to_cpu(leaf, key, slot);
+		break;
+	}
+
+	return 0;
+}
+
 /*
  * adjust the pointers going up the tree, starting at level
  * making sure the right key of each node is points to 'key'.
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index f07c82fafa04..1e3c4a7741ca 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2912,6 +2912,31 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 int btrfs_search_backwards(struct btrfs_root *root, struct btrfs_key *key,
 			   struct btrfs_path *path);
 
+int btrfs_valid_slot(struct btrfs_root *root, struct btrfs_key *key,
+		     struct btrfs_path *path);
+
+/* Search in @root for a given @key, and store the slot found in @found_key.
+ * @root: The root node of the tree.
+ * @key: The key we are looking for.
+ * @found_key: Will hold the found item.
+ * @path: Holds the current slot/leaf.
+ * @iter_ret: Contains the returned value from btrfs_search_slot and
+ *            btrfs_valid_slot, whatever is executed later.
+ *
+ * The iter_ret is an output variable that will contain the result of the
+ * btrfs_search_slot if it returns an error, or the value returned from
+ * btrfs_valid_slot otherwise. The return value can be 0 if the something was
+ * found, 1 if there weren't bigger leaves, and <0 if error.
+ */
+#define btrfs_for_each_slot(root, key, found_key, path, iter_ret)		\
+	for (iter_ret = btrfs_search_slot(NULL, root, key, path, 0, 0);		\
+		(								\
+			iter_ret >= 0 &&					\
+			(iter_ret = btrfs_valid_slot(root, found_key, path)) == 0 \
+		);								  \
+		path->slots[0]++						  \
+	)
+
 static inline int btrfs_next_old_item(struct btrfs_root *root,
 				      struct btrfs_path *p, u64 time_seq)
 {
-- 
2.31.1

Re: [PATCH 1/8] fs: btrfs: Introduce btrfs_for_each_slot

[Nikolay Borisov]

On 26.08.21 г. 19:40, Marcos Paulo de Souza wrote:
> There is a common pattern when search for a key in btrfs:
> 
>  * Call btrfs_search_slot
>  * Endless loop
>          * If the found slot is bigger than the current items in the leaf, check the
>            next one
>          * If still not found in the next leaf, return 1
>          * Do something with the code
>          * Increment current slot, and continue
> 
> This pattern can be improved by creating an iterator macro, similar to
> those for_each_X already existing in the linux kernel. Using this
> approach means to reduce significantly boilerplate code, along making it
> easier to newcomers to understand how to code works.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  fs/btrfs/ctree.c | 28 ++++++++++++++++++++++++++++
>  fs/btrfs/ctree.h | 25 +++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 84627cbd5b5b..b1aa6e3987d0 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2122,6 +2122,34 @@ int btrfs_search_backwards(struct btrfs_root *root, struct btrfs_key *key,
>  	return ret;
>  }
>  
> +/* Search for a valid slot for the given path.
> + * @root: The root node of the tree.
> + * @key: Will contain a valid item if found.
> + * @path: The start point to validate the slot.
> + *
> + * Return 0 if the item is valid, 1 if not found and < 0 if error.
> + */
> +int btrfs_valid_slot(struct btrfs_root *root, struct btrfs_key *key,

nit: The name of this function is a bit misleading since it's not really
a predicate, more like a function that returns the value and if it can't
return the current value pointed to by path it gets the next leaf. I
guess a more apt name would be "btrfs_get_next_valid_item" or some such.

> +		     struct btrfs_path *path)
> +{
> +	while (1) {
> +		int ret;
> +		const int slot = path->slots[0];
> +		const struct extent_buffer *leaf = path->nodes[0];
> +
> +		if (slot >= btrfs_header_nritems(leaf)) {
> +			ret = btrfs_next_leaf(root, path);
> +			if (ret)
> +				return ret;
> +			continue;
> +		}
> +		btrfs_item_key_to_cpu(leaf, key, slot);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * adjust the pointers going up the tree, starting at level
>   * making sure the right key of each node is points to 'key'.
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index f07c82fafa04..1e3c4a7741ca 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2912,6 +2912,31 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>  int btrfs_search_backwards(struct btrfs_root *root, struct btrfs_key *key,
>  			   struct btrfs_path *path);
>  
> +int btrfs_valid_slot(struct btrfs_root *root, struct btrfs_key *key,
> +		     struct btrfs_path *path);
> +
> +/* Search in @root for a given @key, and store the slot found in @found_key.
> + * @root: The root node of the tree.
> + * @key: The key we are looking for.
> + * @found_key: Will hold the found item.
> + * @path: Holds the current slot/leaf.
> + * @iter_ret: Contains the returned value from btrfs_search_slot and
> + *            btrfs_valid_slot, whatever is executed later.
> + *
> + * The iter_ret is an output variable that will contain the result of the
> + * btrfs_search_slot if it returns an error, or the value returned from
> + * btrfs_valid_slot otherwise. The return value can be 0 if the something was
> + * found, 1 if there weren't bigger leaves, and <0 if error.
> + */
> +#define btrfs_for_each_slot(root, key, found_key, path, iter_ret)		\
> +	for (iter_ret = btrfs_search_slot(NULL, root, key, path, 0, 0);		\
> +		(								\
> +			iter_ret >= 0 &&					\
> +			(iter_ret = btrfs_valid_slot(root, found_key, path)) == 0 \
> +		);								  \
> +		path->slots[0]++						  \
> +	)
> +
>  static inline int btrfs_next_old_item(struct btrfs_root *root,
>  				      struct btrfs_path *p, u64 time_seq)
>  {
> 

Re: [PATCH 1/8] fs: btrfs: Introduce btrfs_for_each_slot

[David Sterba]

On Mon, Aug 30, 2021 at 03:37:03PM +0300, Nikolay Borisov wrote:
> On 26.08.21 г. 19:40, Marcos Paulo de Souza wrote:
> > +/* Search for a valid slot for the given path.
> > + * @root: The root node of the tree.
> > + * @key: Will contain a valid item if found.
> > + * @path: The start point to validate the slot.
> > + *
> > + * Return 0 if the item is valid, 1 if not found and < 0 if error.
> > + */
> > +int btrfs_valid_slot(struct btrfs_root *root, struct btrfs_key *key,
> 
> nit: The name of this function is a bit misleading since it's not really
> a predicate, more like a function that returns the value and if it can't
> return the current value pointed to by path it gets the next leaf. I
> guess a more apt name would be "btrfs_get_next_valid_item" or some such.

Yeah the function name is confusing, like a predicate. The suggested
name sounds good to me.

Re: [PATCH 1/8] fs: btrfs: Introduce btrfs_for_each_slot

[David Sterba]

On Thu, Aug 26, 2021 at 01:40:47PM -0300, Marcos Paulo de Souza wrote:
> There is a common pattern when search for a key in btrfs:
> 
>  * Call btrfs_search_slot
>  * Endless loop
>          * If the found slot is bigger than the current items in the leaf, check the
>            next one
>          * If still not found in the next leaf, return 1
>          * Do something with the code
>          * Increment current slot, and continue
> 
> This pattern can be improved by creating an iterator macro, similar to
> those for_each_X already existing in the linux kernel. Using this
> approach means to reduce significantly boilerplate code, along making it
> easier to newcomers to understand how to code works.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  fs/btrfs/ctree.c | 28 ++++++++++++++++++++++++++++
>  fs/btrfs/ctree.h | 25 +++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 84627cbd5b5b..b1aa6e3987d0 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2122,6 +2122,34 @@ int btrfs_search_backwards(struct btrfs_root *root, struct btrfs_key *key,
>  	return ret;
>  }
>  
> +/* Search for a valid slot for the given path.
> + * @root: The root node of the tree.
> + * @key: Will contain a valid item if found.
> + * @path: The start point to validate the slot.
> + *
> + * Return 0 if the item is valid, 1 if not found and < 0 if error.
> + */

Please fix the comment formatting like

/*
 * Search for a valid slot ...
 *
 * @root:  root node of the tre
 * @key:   ...
 *
 * Return 0 if ...
 */

> +int btrfs_valid_slot(struct btrfs_root *root, struct btrfs_key *key,
> +		     struct btrfs_path *path)
> +{
> +	while (1) {
> +		int ret;
> +		const int slot = path->slots[0];
> +		const struct extent_buffer *leaf = path->nodes[0];
> +
> +		if (slot >= btrfs_header_nritems(leaf)) {
> +			ret = btrfs_next_leaf(root, path);
> +			if (ret)
> +				return ret;
> +			continue;
> +		}
> +		btrfs_item_key_to_cpu(leaf, key, slot);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * adjust the pointers going up the tree, starting at level
>   * making sure the right key of each node is points to 'key'.
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index f07c82fafa04..1e3c4a7741ca 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2912,6 +2912,31 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>  int btrfs_search_backwards(struct btrfs_root *root, struct btrfs_key *key,
>  			   struct btrfs_path *path);
>  
> +int btrfs_valid_slot(struct btrfs_root *root, struct btrfs_key *key,
> +		     struct btrfs_path *path);
> +
> +/* Search in @root for a given @key, and store the slot found in @found_key.
> + * @root: The root node of the tree.
> + * @key: The key we are looking for.
> + * @found_key: Will hold the found item.
> + * @path: Holds the current slot/leaf.
> + * @iter_ret: Contains the returned value from btrfs_search_slot and
> + *            btrfs_valid_slot, whatever is executed later.
> + *
> + * The iter_ret is an output variable that will contain the result of the
> + * btrfs_search_slot if it returns an error, or the value returned from
> + * btrfs_valid_slot otherwise. The return value can be 0 if the something was
> + * found, 1 if there weren't bigger leaves, and <0 if error.
> + */

Same.

> +#define btrfs_for_each_slot(root, key, found_key, path, iter_ret)		\
> +	for (iter_ret = btrfs_search_slot(NULL, root, key, path, 0, 0);		\
> +		(								\
> +			iter_ret >= 0 &&					\
> +			(iter_ret = btrfs_valid_slot(root, found_key, path)) == 0 \
> +		);								  \

The innter ( ) and indentation is redundant and looks confusing, like
it's another statement block.

> +		path->slots[0]++						  \
> +	)
> +
>  static inline int btrfs_next_old_item(struct btrfs_root *root,
>  				      struct btrfs_path *p, u64 time_seq)
>  {
> -- 
> 2.31.1

[PATCH 2/8] btrfs: block-group: use btrfs_for_each_slot in find_first_block_group

[Marcos Paulo de Souza]

The function can be simplified by using the iterator like macro.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/block-group.c | 33 +++++----------------------------
 1 file changed, 5 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 1f8b06afbd03..0d999a3bfc84 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1654,38 +1654,15 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info,
 				  struct btrfs_path *path,
 				  struct btrfs_key *key)
 {
-	struct btrfs_root *root = fs_info->extent_root;
-	int ret;
 	struct btrfs_key found_key;
-	struct extent_buffer *leaf;
-	int slot;
-
-	ret = btrfs_search_slot(NULL, root, key, path, 0, 0);
-	if (ret < 0)
-		return ret;
-
-	while (1) {
-		slot = path->slots[0];
-		leaf = path->nodes[0];
-		if (slot >= btrfs_header_nritems(leaf)) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret == 0)
-				continue;
-			if (ret < 0)
-				goto out;
-			break;
-		}
-		btrfs_item_key_to_cpu(leaf, &found_key, slot);
+	int ret;
 
+	btrfs_for_each_slot(fs_info->extent_root, key, &found_key, path, ret) {
 		if (found_key.objectid >= key->objectid &&
-		    found_key.type == BTRFS_BLOCK_GROUP_ITEM_KEY) {
-			ret = read_bg_from_eb(fs_info, &found_key, path);
-			break;
-		}
-
-		path->slots[0]++;
+		    found_key.type == BTRFS_BLOCK_GROUP_ITEM_KEY)
+			return read_bg_from_eb(fs_info, &found_key, path);
 	}
-out:
+
 	return ret;
 }
 
-- 
2.31.1

[PATCH 3/8] btrfs: dev-replace: Use btrfs_for_each_slot in mark_block_group_to_copy

[Marcos Paulo de Souza]

The function can be simplified by using the macro.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/dev-replace.c | 51 +++++++++---------------------------------
 1 file changed, 11 insertions(+), 40 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index d029be40ea6f..6124f2e8e9f1 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -523,63 +523,34 @@ static int mark_block_group_to_copy(struct btrfs_fs_info *fs_info,
 	key.type = BTRFS_DEV_EXTENT_KEY;
 	key.offset = 0;
 
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0)
-		goto free_path;
-	if (ret > 0) {
-		if (path->slots[0] >=
-		    btrfs_header_nritems(path->nodes[0])) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0)
-				goto free_path;
-			if (ret > 0) {
-				ret = 0;
-				goto free_path;
-			}
-		} else {
-			ret = 0;
-		}
-	}
-
-	while (1) {
+	btrfs_for_each_slot(root, &key, &found_key, path, ret) {
 		struct extent_buffer *leaf = path->nodes[0];
-		int slot = path->slots[0];
-
-		btrfs_item_key_to_cpu(leaf, &found_key, slot);
-
-		if (found_key.objectid != src_dev->devid)
-			break;
 
-		if (found_key.type != BTRFS_DEV_EXTENT_KEY)
+		if (found_key.objectid != src_dev->devid ||
+		    found_key.type != BTRFS_DEV_EXTENT_KEY ||
+		    found_key.offset < key.offset)
 			break;
 
-		if (found_key.offset < key.offset)
-			break;
-
-		dev_extent = btrfs_item_ptr(leaf, slot, struct btrfs_dev_extent);
+		dev_extent = btrfs_item_ptr(leaf, path->slots[0],
+				struct btrfs_dev_extent);
 
 		chunk_offset = btrfs_dev_extent_chunk_offset(leaf, dev_extent);
 
 		cache = btrfs_lookup_block_group(fs_info, chunk_offset);
 		if (!cache)
-			goto skip;
+			continue;
 
 		spin_lock(&cache->lock);
 		cache->to_copy = 1;
 		spin_unlock(&cache->lock);
 
 		btrfs_put_block_group(cache);
-
-skip:
-		ret = btrfs_next_item(root, path);
-		if (ret != 0) {
-			if (ret > 0)
-				ret = 0;
-			break;
-		}
 	}
 
-free_path:
+	/* Reset error if the key wasn't found. */
+	if (ret > 0)
+		ret = 0;
+
 	btrfs_free_path(path);
 unlock:
 	mutex_unlock(&fs_info->chunk_mutex);
-- 
2.31.1

[PATCH 4/8] btrfs: dir-item: use btrfs_for_each_slot in btrfs_search_dir_index_item

[Marcos Paulo de Souza]

The function can be simplified by using the iterator like macro.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/dir-item.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index f1274d5c3805..10560663dae1 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -301,36 +301,15 @@ btrfs_search_dir_index_item(struct btrfs_root *root,
 			    struct btrfs_path *path, u64 dirid,
 			    const char *name, int name_len)
 {
-	struct extent_buffer *leaf;
 	struct btrfs_dir_item *di;
 	struct btrfs_key key;
-	u32 nritems;
 	int ret;
 
 	key.objectid = dirid;
 	key.type = BTRFS_DIR_INDEX_KEY;
 	key.offset = 0;
 
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0)
-		return ERR_PTR(ret);
-
-	leaf = path->nodes[0];
-	nritems = btrfs_header_nritems(leaf);
-
-	while (1) {
-		if (path->slots[0] >= nritems) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0)
-				return ERR_PTR(ret);
-			if (ret > 0)
-				break;
-			leaf = path->nodes[0];
-			nritems = btrfs_header_nritems(leaf);
-			continue;
-		}
-
-		btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
+	btrfs_for_each_slot(root, &key, &key, path, ret) {
 		if (key.objectid != dirid || key.type != BTRFS_DIR_INDEX_KEY)
 			break;
 
@@ -338,9 +317,9 @@ btrfs_search_dir_index_item(struct btrfs_root *root,
 					       name, name_len);
 		if (di)
 			return di;
-
-		path->slots[0]++;
 	}
+	if (ret < 0)
+		return ERR_PTR(ret);
 	return NULL;
 }
 
-- 
2.31.1

[PATCH 5/8] btrfs: inode: use btrfs_for_each_slot in btrfs_read_readdir

[Marcos Paulo de Souza]

The function can be simplified by using the macro.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/inode.c | 46 +++++++++++++++++-----------------------------
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2aa9646bce56..12bee0107015 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6109,8 +6109,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 	struct list_head ins_list;
 	struct list_head del_list;
 	int ret;
-	struct extent_buffer *leaf;
-	int slot;
+	int iter_ret;
 	char *name_ptr;
 	int name_len;
 	int entries = 0;
@@ -6137,35 +6136,19 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 	key.offset = ctx->pos;
 	key.objectid = btrfs_ino(BTRFS_I(inode));
 
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0)
-		goto err;
-
-	while (1) {
+	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
 		struct dir_entry *entry;
+		struct extent_buffer *leaf = path->nodes[0];
 
-		leaf = path->nodes[0];
-		slot = path->slots[0];
-		if (slot >= btrfs_header_nritems(leaf)) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0)
-				goto err;
-			else if (ret > 0)
-				break;
-			continue;
-		}
+		if (found_key.objectid != key.objectid ||
+		    found_key.type != BTRFS_DIR_INDEX_KEY)
+			break;
 
-		btrfs_item_key_to_cpu(leaf, &found_key, slot);
+		if (found_key.offset < ctx->pos ||
+		    btrfs_should_delete_dir_index(&del_list, found_key.offset))
+			continue;
 
-		if (found_key.objectid != key.objectid)
-			break;
-		if (found_key.type != BTRFS_DIR_INDEX_KEY)
-			break;
-		if (found_key.offset < ctx->pos)
-			goto next;
-		if (btrfs_should_delete_dir_index(&del_list, found_key.offset))
-			goto next;
-		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
+		di = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_dir_item);
 		name_len = btrfs_dir_name_len(leaf, di);
 		if ((total_len + sizeof(struct dir_entry) + name_len) >=
 		    PAGE_SIZE) {
@@ -6192,9 +6175,14 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 		entries++;
 		addr += sizeof(struct dir_entry) + name_len;
 		total_len += sizeof(struct dir_entry) + name_len;
-next:
-		path->slots[0]++;
 	}
+
+	/* Error found while searching. */
+	if (iter_ret < 0) {
+		ret = iter_ret;
+		goto err;
+	}
+
 	btrfs_release_path(path);
 
 	ret = btrfs_filldir(private->filldir_buf, entries, ctx);
-- 
2.31.1

Re: [PATCH 5/8] btrfs: inode: use btrfs_for_each_slot in btrfs_read_readdir

[Nikolay Borisov]

On 26.08.21 г. 19:40, Marcos Paulo de Souza wrote:
> The function can be simplified by using the macro.
> 
> No functional changes.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  fs/btrfs/inode.c | 46 +++++++++++++++++-----------------------------
>  1 file changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 2aa9646bce56..12bee0107015 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6109,8 +6109,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  	struct list_head ins_list;
>  	struct list_head del_list;
>  	int ret;
> -	struct extent_buffer *leaf;
> -	int slot;
> +	int iter_ret;
>  	char *name_ptr;
>  	int name_len;
>  	int entries = 0;
> @@ -6137,35 +6136,19 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  	key.offset = ctx->pos;
>  	key.objectid = btrfs_ino(BTRFS_I(inode));
>  
> -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> -	if (ret < 0)
> -		goto err;
> -
> -	while (1) {
> +	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {

I don't think it's necessary to use iter_ret, instead you can use ret
directly. Because if either btrfs_search_slot return an error or
btrfs_valid_slot then ret would be set to the respective return value
and the body of the loop won't be executed at all, no?

>  		struct dir_entry *entry;
> +		struct extent_buffer *leaf = path->nodes[0];
>  
> -		leaf = path->nodes[0];
> -		slot = path->slots[0];
> -		if (slot >= btrfs_header_nritems(leaf)) {
> -			ret = btrfs_next_leaf(root, path);
> -			if (ret < 0)
> -				goto err;
> -			else if (ret > 0)
> -				break;
> -			continue;
> -		}
> +		if (found_key.objectid != key.objectid ||
> +		    found_key.type != BTRFS_DIR_INDEX_KEY)
> +			break;
>  
> -		btrfs_item_key_to_cpu(leaf, &found_key, slot);
> +		if (found_key.offset < ctx->pos ||
> +		    btrfs_should_delete_dir_index(&del_list, found_key.offset))
> +			continue;
>  
> -		if (found_key.objectid != key.objectid)
> -			break;
> -		if (found_key.type != BTRFS_DIR_INDEX_KEY)
> -			break;
> -		if (found_key.offset < ctx->pos)
> -			goto next;
> -		if (btrfs_should_delete_dir_index(&del_list, found_key.offset))
> -			goto next;
> -		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
> +		di = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_dir_item);
>  		name_len = btrfs_dir_name_len(leaf, di);
>  		if ((total_len + sizeof(struct dir_entry) + name_len) >=
>  		    PAGE_SIZE) {
> @@ -6192,9 +6175,14 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>  		entries++;
>  		addr += sizeof(struct dir_entry) + name_len;
>  		total_len += sizeof(struct dir_entry) + name_len;
> -next:
> -		path->slots[0]++;
>  	}
> +
> +	/* Error found while searching. */
> +	if (iter_ret < 0) {
> +		ret = iter_ret;
> +		goto err;
> +	}
> +
>  	btrfs_release_path(path);
>  
>  	ret = btrfs_filldir(private->filldir_buf, entries, ctx);
> 

Re: [PATCH 5/8] btrfs: inode: use btrfs_for_each_slot in btrfs_read_readdir

[David Sterba]

On Mon, Aug 30, 2021 at 04:05:28PM +0300, Nikolay Borisov wrote:
> > @@ -6137,35 +6136,19 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
> >  	key.offset = ctx->pos;
> >  	key.objectid = btrfs_ino(BTRFS_I(inode));
> >  
> > -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> > -	if (ret < 0)
> > -		goto err;
> > -
> > -	while (1) {
> > +	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
> 
> I don't think it's necessary to use iter_ret, instead you can use ret
> directly. Because if either btrfs_search_slot return an error or
> btrfs_valid_slot then ret would be set to the respective return value
> and the body of the loop won't be executed at all, no?

Yeah thre's no reason to add another variable in this case. As long as
the loop body does not use ret internally, then reusing ret is fine.

The point of having an explicit return value for the iterator is to be
able to read the reason of failure after the iterator scope ends, so it
can't be defined inside. We'd need to be careful to make sure that the
iterator 'ret' is never used inside the body so that could be also
useful to put to the documentation. I think a coccinelle script can be
also useful to catch such things.

Re: [PATCH 5/8] btrfs: inode: use btrfs_for_each_slot in btrfs_read_readdir

[Nikolay Borisov]

On 31.08.21 г. 14:10, David Sterba wrote:
> On Mon, Aug 30, 2021 at 04:05:28PM +0300, Nikolay Borisov wrote:
>>> @@ -6137,35 +6136,19 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>>>  	key.offset = ctx->pos;
>>>  	key.objectid = btrfs_ino(BTRFS_I(inode));
>>>  
>>> -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
>>> -	if (ret < 0)
>>> -		goto err;
>>> -
>>> -	while (1) {
>>> +	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
>>
>> I don't think it's necessary to use iter_ret, instead you can use ret
>> directly. Because if either btrfs_search_slot return an error or
>> btrfs_valid_slot then ret would be set to the respective return value
>> and the body of the loop won't be executed at all, no?
> 
> Yeah thre's no reason to add another variable in this case. As long as
> the loop body does not use ret internally, then reusing ret is fine.
> 
> The point of having an explicit return value for the iterator is to be
> able to read the reason of failure after the iterator scope ends, so it
> can't be defined inside. We'd need to be careful to make sure that the
> iterator 'ret' is never used inside the body so that could be also
> useful to put to the documentation. I think a coccinelle script can be
> also useful to catch such things.
> 

Actually even if 'ret' is used inside the loop body it's still fine.
That's because as soon as we call the btrfs_valid_item (aka iter) and it
returns an error we break from the loop.

[PATCH 6/8] btrfs: send: Use btrfs_for_each_slot macro

[Marcos Paulo de Souza]

This makes the code much simpler and smaller.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/send.c | 222 +++++++++++++-----------------------------------
 1 file changed, 59 insertions(+), 163 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index afdcbe7844e0..3e32886e171e 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -2647,6 +2647,7 @@ static int send_create_inode(struct send_ctx *sctx, u64 ino)
  */
 static int did_create_dir(struct send_ctx *sctx, u64 dir)
 {
+	int iter_ret;
 	int ret = 0;
 	struct btrfs_path *path = NULL;
 	struct btrfs_key key;
@@ -2654,43 +2655,23 @@ static int did_create_dir(struct send_ctx *sctx, u64 dir)
 	struct btrfs_key di_key;
 	struct extent_buffer *eb;
 	struct btrfs_dir_item *di;
-	int slot;
 
 	path = alloc_path_for_send();
-	if (!path) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!path)
+		return -ENOMEM;
 
 	key.objectid = dir;
 	key.type = BTRFS_DIR_INDEX_KEY;
 	key.offset = 0;
-	ret = btrfs_search_slot(NULL, sctx->send_root, &key, path, 0, 0);
-	if (ret < 0)
-		goto out;
-
-	while (1) {
-		eb = path->nodes[0];
-		slot = path->slots[0];
-		if (slot >= btrfs_header_nritems(eb)) {
-			ret = btrfs_next_leaf(sctx->send_root, path);
-			if (ret < 0) {
-				goto out;
-			} else if (ret > 0) {
-				ret = 0;
-				break;
-			}
-			continue;
-		}
-
-		btrfs_item_key_to_cpu(eb, &found_key, slot);
+	btrfs_for_each_slot(sctx->send_root, &key, &found_key, path, iter_ret) {
 		if (found_key.objectid != key.objectid ||
 		    found_key.type != key.type) {
 			ret = 0;
 			goto out;
 		}
 
-		di = btrfs_item_ptr(eb, slot, struct btrfs_dir_item);
+		eb = path->nodes[0];
+		di = btrfs_item_ptr(eb, path->slots[0], struct btrfs_dir_item);
 		btrfs_dir_item_key_to_cpu(eb, di, &di_key);
 
 		if (di_key.type != BTRFS_ROOT_ITEM_KEY &&
@@ -2698,10 +2679,12 @@ static int did_create_dir(struct send_ctx *sctx, u64 dir)
 			ret = 1;
 			goto out;
 		}
-
-		path->slots[0]++;
 	}
 
+	/* Only set ret if there was an error in the search. */
+	if (iter_ret < 0)
+		ret = iter_ret;
+
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -2905,6 +2888,7 @@ static void free_orphan_dir_info(struct send_ctx *sctx,
 static int can_rmdir(struct send_ctx *sctx, u64 dir, u64 dir_gen,
 		     u64 send_progress)
 {
+	int iter_ret;
 	int ret = 0;
 	struct btrfs_root *root = sctx->parent_root;
 	struct btrfs_path *path;
@@ -2932,23 +2916,9 @@ static int can_rmdir(struct send_ctx *sctx, u64 dir, u64 dir_gen,
 	if (odi)
 		key.offset = odi->last_dir_index_offset;
 
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0)
-		goto out;
-
-	while (1) {
+	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
 		struct waiting_dir_move *dm;
 
-		if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0)
-				goto out;
-			else if (ret > 0)
-				break;
-			continue;
-		}
-		btrfs_item_key_to_cpu(path->nodes[0], &found_key,
-				      path->slots[0]);
 		if (found_key.objectid != key.objectid ||
 		    found_key.type != key.type)
 			break;
@@ -2984,8 +2954,13 @@ static int can_rmdir(struct send_ctx *sctx, u64 dir, u64 dir_gen,
 			goto out;
 		}
 
-		path->slots[0]++;
 	}
+
+	if (iter_ret < 0) {
+		ret = iter_ret;
+		goto out;
+	}
+
 	free_orphan_dir_info(sctx, odi);
 
 	ret = 1;
@@ -3563,6 +3538,7 @@ static int is_ancestor(struct btrfs_root *root,
 		       struct fs_path *fs_path)
 {
 	bool free_fs_path = false;
+	int iter_ret = 0;
 	int ret = 0;
 	struct btrfs_path *path = NULL;
 	struct btrfs_key key;
@@ -3584,26 +3560,12 @@ static int is_ancestor(struct btrfs_root *root,
 	key.type = BTRFS_INODE_REF_KEY;
 	key.offset = 0;
 
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0)
-		goto out;
-
-	while (true) {
+	btrfs_for_each_slot(root, &key, &key, path, iter_ret) {
 		struct extent_buffer *leaf = path->nodes[0];
 		int slot = path->slots[0];
 		u32 cur_offset = 0;
 		u32 item_size;
 
-		if (slot >= btrfs_header_nritems(leaf)) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0)
-				goto out;
-			if (ret > 0)
-				break;
-			continue;
-		}
-
-		btrfs_item_key_to_cpu(leaf, &key, slot);
 		if (key.objectid != ino2)
 			break;
 		if (key.type != BTRFS_INODE_REF_KEY &&
@@ -3641,8 +3603,13 @@ static int is_ancestor(struct btrfs_root *root,
 			if (ret)
 				goto out;
 		}
-		path->slots[0]++;
 	}
+
+	if (iter_ret < 0) {
+		ret = iter_ret;
+		goto out;
+	}
+
 	ret = 0;
  out:
 	btrfs_free_path(path);
@@ -4524,13 +4491,12 @@ static int record_changed_ref(struct send_ctx *sctx)
 static int process_all_refs(struct send_ctx *sctx,
 			    enum btrfs_compare_tree_result cmd)
 {
-	int ret;
+	int iter_ret;
+	int ret = 0;
 	struct btrfs_root *root;
 	struct btrfs_path *path;
 	struct btrfs_key key;
 	struct btrfs_key found_key;
-	struct extent_buffer *eb;
-	int slot;
 	iterate_inode_ref_t cb;
 	int pending_move = 0;
 
@@ -4554,24 +4520,7 @@ static int process_all_refs(struct send_ctx *sctx,
 	key.objectid = sctx->cmp_key->objectid;
 	key.type = BTRFS_INODE_REF_KEY;
 	key.offset = 0;
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0)
-		goto out;
-
-	while (1) {
-		eb = path->nodes[0];
-		slot = path->slots[0];
-		if (slot >= btrfs_header_nritems(eb)) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0)
-				goto out;
-			else if (ret > 0)
-				break;
-			continue;
-		}
-
-		btrfs_item_key_to_cpu(eb, &found_key, slot);
-
+	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
 		if (found_key.objectid != key.objectid ||
 		    (found_key.type != BTRFS_INODE_REF_KEY &&
 		     found_key.type != BTRFS_INODE_EXTREF_KEY))
@@ -4580,9 +4529,13 @@ static int process_all_refs(struct send_ctx *sctx,
 		ret = iterate_inode_ref(root, path, &found_key, 0, cb, sctx);
 		if (ret < 0)
 			goto out;
+	}
 
-		path->slots[0]++;
+	if (iter_ret < 0) {
+		ret = iter_ret;
+		goto out;
 	}
+
 	btrfs_release_path(path);
 
 	/*
@@ -4847,13 +4800,12 @@ static int process_changed_xattr(struct send_ctx *sctx)
 
 static int process_all_new_xattrs(struct send_ctx *sctx)
 {
-	int ret;
+	int iter_ret;
+	int ret = 0;
 	struct btrfs_root *root;
 	struct btrfs_path *path;
 	struct btrfs_key key;
 	struct btrfs_key found_key;
-	struct extent_buffer *eb;
-	int slot;
 
 	path = alloc_path_for_send();
 	if (!path)
@@ -4864,39 +4816,18 @@ static int process_all_new_xattrs(struct send_ctx *sctx)
 	key.objectid = sctx->cmp_key->objectid;
 	key.type = BTRFS_XATTR_ITEM_KEY;
 	key.offset = 0;
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0)
-		goto out;
-
-	while (1) {
-		eb = path->nodes[0];
-		slot = path->slots[0];
-		if (slot >= btrfs_header_nritems(eb)) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0) {
-				goto out;
-			} else if (ret > 0) {
-				ret = 0;
-				break;
-			}
-			continue;
-		}
-
-		btrfs_item_key_to_cpu(eb, &found_key, slot);
+	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
 		if (found_key.objectid != key.objectid ||
-		    found_key.type != key.type) {
-			ret = 0;
-			goto out;
-		}
+		    found_key.type != key.type)
+			break;
 
 		ret = iterate_dir_item(root, path, __process_new_xattr, sctx);
 		if (ret < 0)
-			goto out;
-
-		path->slots[0]++;
+			break;
 	}
+	if (iter_ret < 0)
+		ret = iter_ret;
 
-out:
 	btrfs_free_path(path);
 	return ret;
 }
@@ -5938,13 +5869,12 @@ static int process_extent(struct send_ctx *sctx,
 
 static int process_all_extents(struct send_ctx *sctx)
 {
-	int ret;
+	int iter_ret;
+	int ret = 0;
 	struct btrfs_root *root;
 	struct btrfs_path *path;
 	struct btrfs_key key;
 	struct btrfs_key found_key;
-	struct extent_buffer *eb;
-	int slot;
 
 	root = sctx->send_root;
 	path = alloc_path_for_send();
@@ -5954,40 +5884,19 @@ static int process_all_extents(struct send_ctx *sctx)
 	key.objectid = sctx->cmp_key->objectid;
 	key.type = BTRFS_EXTENT_DATA_KEY;
 	key.offset = 0;
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0)
-		goto out;
-
-	while (1) {
-		eb = path->nodes[0];
-		slot = path->slots[0];
-
-		if (slot >= btrfs_header_nritems(eb)) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0) {
-				goto out;
-			} else if (ret > 0) {
-				ret = 0;
-				break;
-			}
-			continue;
-		}
-
-		btrfs_item_key_to_cpu(eb, &found_key, slot);
-
+	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
 		if (found_key.objectid != key.objectid ||
-		    found_key.type != key.type) {
-			ret = 0;
+		    found_key.type != key.type)
 			goto out;
-		}
 
 		ret = process_extent(sctx, path, &found_key);
 		if (ret < 0)
 			goto out;
-
-		path->slots[0]++;
 	}
 
+	if (iter_ret < 0)
+		ret = iter_ret;
+
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -6180,36 +6089,20 @@ static int btrfs_unlink_all_paths(struct send_ctx *sctx)
 	struct btrfs_path *path;
 	struct btrfs_key key;
 	struct parent_paths_ctx ctx;
-	int ret;
+	int iter_ret;
+	int ret = 0;
 
 	path = alloc_path_for_send();
 	if (!path)
 		return -ENOMEM;
 
-	key.objectid = sctx->cur_ino;
-	key.type = BTRFS_INODE_REF_KEY;
-	key.offset = 0;
-	ret = btrfs_search_slot(NULL, sctx->parent_root, &key, path, 0, 0);
-	if (ret < 0)
-		goto out;
-
 	ctx.refs = &deleted_refs;
 	ctx.sctx = sctx;
 
-	while (true) {
-		struct extent_buffer *eb = path->nodes[0];
-		int slot = path->slots[0];
-
-		if (slot >= btrfs_header_nritems(eb)) {
-			ret = btrfs_next_leaf(sctx->parent_root, path);
-			if (ret < 0)
-				goto out;
-			else if (ret > 0)
-				break;
-			continue;
-		}
-
-		btrfs_item_key_to_cpu(eb, &key, slot);
+	key.objectid = sctx->cur_ino;
+	key.type = BTRFS_INODE_REF_KEY;
+	key.offset = 0;
+	btrfs_for_each_slot(sctx->parent_root, &key, &key, path, iter_ret) {
 		if (key.objectid != sctx->cur_ino)
 			break;
 		if (key.type != BTRFS_INODE_REF_KEY &&
@@ -6220,8 +6113,11 @@ static int btrfs_unlink_all_paths(struct send_ctx *sctx)
 					record_parent_ref, &ctx);
 		if (ret < 0)
 			goto out;
+	}
 
-		path->slots[0]++;
+	if (iter_ret < 0) {
+		ret = iter_ret;
+		goto out;
 	}
 
 	while (!list_empty(&deleted_refs)) {
-- 
2.31.1

Re: [PATCH 6/8] btrfs: send: Use btrfs_for_each_slot macro

[Nikolay Borisov]

On 26.08.21 г. 19:40, Marcos Paulo de Souza wrote:
> This makes the code much simpler and smaller.
> 
> No functional changes.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  fs/btrfs/send.c | 222 +++++++++++++-----------------------------------
>  1 file changed, 59 insertions(+), 163 deletions(-)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index afdcbe7844e0..3e32886e171e 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -2647,6 +2647,7 @@ static int send_create_inode(struct send_ctx *sctx, u64 ino)
>   */
>  static int did_create_dir(struct send_ctx *sctx, u64 dir)
>  {
> +	int iter_ret;
>  	int ret = 0;
>  	struct btrfs_path *path = NULL;
>  	struct btrfs_key key;
> @@ -2654,43 +2655,23 @@ static int did_create_dir(struct send_ctx *sctx, u64 dir)
>  	struct btrfs_key di_key;
>  	struct extent_buffer *eb;
>  	struct btrfs_dir_item *di;
> -	int slot;
>  
>  	path = alloc_path_for_send();
> -	if (!path) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> +	if (!path)
> +		return -ENOMEM;
>  

This is irrelevant changes, albeit it's good. But please refrain from
mixing unrelated changes as it makes review a bit harder.

>  	key.objectid = dir;
>  	key.type = BTRFS_DIR_INDEX_KEY;
>  	key.offset = 0;
> -	ret = btrfs_search_slot(NULL, sctx->send_root, &key, path, 0, 0);
> -	if (ret < 0)
> -		goto out;
> -
> -	while (1) {
> -		eb = path->nodes[0];
> -		slot = path->slots[0];

In the original code we take the node/slot in 2 local variables for
convenience.
> -		if (slot >= btrfs_header_nritems(eb)) {
> -			ret = btrfs_next_leaf(sctx->send_root, path);
> -			if (ret < 0) {
> -				goto out;
> -			} else if (ret > 0) {
> -				ret = 0;
> -				break;
> -			}
> -			continue;
> -		}
> -
> -		btrfs_item_key_to_cpu(eb, &found_key, slot);
> +	btrfs_for_each_slot(sctx->send_root, &key, &found_key, path, iter_ret) {
>  		if (found_key.objectid != key.objectid ||
>  		    found_key.type != key.type) {

nit: The way the checks for termination are performed are somwhat
inconsistent. Here the original key is not touched hence its
.object/.type can be used when checking whether the found key is
appropriate, but in btrfs_unlink_all_paths you use 'key' as both an
input and an output and for the terminating condition you refer to the
original vales that the search key was set to. I don't have an opinion
which is better but I'd rather have a single style in the file.

>  			ret = 0;
>  			goto out;
>  		}
>  
> -		di = btrfs_item_ptr(eb, slot, struct btrfs_dir_item);
> +		eb = path->nodes[0];
> +		di = btrfs_item_ptr(eb, path->slots[0], struct btrfs_dir_item);

nit: In the refactored code you eliminated the 'slot' variable but
retained the eb. This seems to be a rather arbitrary choice. While not
wrong technically, it adds unnecessary diff hunks, which again make
review more involved that it could have been. What's more, in this
particular function actually retaining the 'eb' and 'slot definition and
making them local to the btrfs_for_each_slot loop would have resulted in
somewhat better code (albeit this might be subjective):

https://pastebin.com/avye4wZe

No need to resend but just something to keep in mind for future patches.


>  		btrfs_dir_item_key_to_cpu(eb, di, &di_key);
>  
>  		if (di_key.type != BTRFS_ROOT_ITEM_KEY &&

<snip>

> @@ -4864,39 +4816,18 @@ static int process_all_new_xattrs(struct send_ctx *sctx)
>  	key.objectid = sctx->cmp_key->objectid;
>  	key.type = BTRFS_XATTR_ITEM_KEY;
>  	key.offset = 0;
> -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> -	if (ret < 0)
> -		goto out;
> -
> -	while (1) {
> -		eb = path->nodes[0];
> -		slot = path->slots[0];
> -		if (slot >= btrfs_header_nritems(eb)) {
> -			ret = btrfs_next_leaf(root, path);
> -			if (ret < 0) {
> -				goto out;
> -			} else if (ret > 0) {
> -				ret = 0;
> -				break;
> -			}
> -			continue;
> -		}
> -
> -		btrfs_item_key_to_cpu(eb, &found_key, slot);
> +	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
>  		if (found_key.objectid != key.objectid ||
> -		    found_key.type != key.type) {
> -			ret = 0;

nit: You've removed this 'ret = 0', however AFAICS this is ok, because
iterate_dir_item returns either 0 or a negative error code so we can't
return from btrfs_for_each_slot with ret set to anything other than an
error code or 0.

> -			goto out;
> -		}
> +		    found_key.type != key.type)
> +			break;
>  
>  		ret = iterate_dir_item(root, path, __process_new_xattr, sctx);
>  		if (ret < 0)
> -			goto out;
> -
> -		path->slots[0]++;
> +			break;
>  	}
> +	if (iter_ret < 0)
> +		ret = iter_ret;
>  
> -out:
>  	btrfs_free_path(path);
>  	return ret;
>  }

<snip>

> @@ -5954,40 +5884,19 @@ static int process_all_extents(struct send_ctx *sctx)
>  	key.objectid = sctx->cmp_key->objectid;
>  	key.type = BTRFS_EXTENT_DATA_KEY;
>  	key.offset = 0;
> -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> -	if (ret < 0)
> -		goto out;
> -
> -	while (1) {
> -		eb = path->nodes[0];
> -		slot = path->slots[0];
> -
> -		if (slot >= btrfs_header_nritems(eb)) {
> -			ret = btrfs_next_leaf(root, path);
> -			if (ret < 0) {
> -				goto out;
> -			} else if (ret > 0) {
> -				ret = 0;
> -				break;
> -			}
> -			continue;
> -		}
> -
> -		btrfs_item_key_to_cpu(eb, &found_key, slot);
> -
> +	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
>  		if (found_key.objectid != key.objectid ||
> -		    found_key.type != key.type) {
> -			ret = 0;

However, here I think this is significant because process_extent can
return 1 via
process_extent
 find_extent_clone
   send_write_or_clone
     clone_range
      while (true) loop and the items are exhausted, we break with ret = 1


> +		    found_key.type != key.type)
>  			goto out;
> -		}
>  
>  		ret = process_extent(sctx, path, &found_key);
>  		if (ret < 0)
>  			goto out;
> -
> -		path->slots[0]++;
>  	}
>  
> +	if (iter_ret < 0)
> +		ret = iter_ret;
> +
>  out:
>  	btrfs_free_path(path);
>  	return ret;
> @@ -6180,36 +6089,20 @@ static int btrfs_unlink_all_paths(struct send_ctx *sctx)
>  	struct btrfs_path *path;
>  	struct btrfs_key key;
>  	struct parent_paths_ctx ctx;
> -	int ret;
> +	int iter_ret;
> +	int ret = 0;
>  
>  	path = alloc_path_for_send();
>  	if (!path)
>  		return -ENOMEM;
>  
> -	key.objectid = sctx->cur_ino;
> -	key.type = BTRFS_INODE_REF_KEY;
> -	key.offset = 0;
> -	ret = btrfs_search_slot(NULL, sctx->parent_root, &key, path, 0, 0);
> -	if (ret < 0)
> -		goto out;
> -
>  	ctx.refs = &deleted_refs;
>  	ctx.sctx = sctx;
>  
> -	while (true) {
> -		struct extent_buffer *eb = path->nodes[0];
> -		int slot = path->slots[0];
> -
> -		if (slot >= btrfs_header_nritems(eb)) {
> -			ret = btrfs_next_leaf(sctx->parent_root, path);
> -			if (ret < 0)
> -				goto out;
> -			else if (ret > 0)
> -				break;
> -			continue;
> -		}
> -
> -		btrfs_item_key_to_cpu(eb, &key, slot);
> +	key.objectid = sctx->cur_ino;
> +	key.type = BTRFS_INODE_REF_KEY;
> +	key.offset = 0;

Those 3 lines are moved needlessly after the 2 ctx assignments which
serve no purpose other than to increase the diff.

> +	btrfs_for_each_slot(sctx->parent_root, &key, &key, path, iter_ret) {
>  		if (key.objectid != sctx->cur_ino)
>  			break;
>  		if (key.type != BTRFS_INODE_REF_KEY &&
> @@ -6220,8 +6113,11 @@ static int btrfs_unlink_all_paths(struct send_ctx *sctx)
>  					record_parent_ref, &ctx);
>  		if (ret < 0)
>  			goto out;
> +	}
>  
> -		path->slots[0]++;
> +	if (iter_ret < 0) {
> +		ret = iter_ret;
> +		goto out;
>  	}
>  
>  	while (!list_empty(&deleted_refs)) {
> 

[PATCH 7/8] btrfs: volumes: use btrfs_for_each_slot in btrfs_read_chunk_tree

[Marcos Paulo de Souza]

The function can be simplified by using the iterator like macro.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/volumes.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d72e4e3e02b1..998838ac106c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7428,8 +7428,9 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info)
 	struct extent_buffer *leaf;
 	struct btrfs_key key;
 	struct btrfs_key found_key;
-	int ret;
 	int slot;
+	int iter_ret;
+	int ret = 0;
 	u64 total_dev = 0;
 	u64 last_ra_node = 0;
 
@@ -7460,22 +7461,12 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info)
 	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
 	key.offset = 0;
 	key.type = 0;
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0)
-		goto error;
-	while (1) {
+	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
 		struct extent_buffer *node;
 
 		leaf = path->nodes[0];
 		slot = path->slots[0];
-		if (slot >= btrfs_header_nritems(leaf)) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret == 0)
-				continue;
-			if (ret < 0)
-				goto error;
-			break;
-		}
+
 		/*
 		 * The nodes on level 1 are not locked but we don't need to do
 		 * that during mount time as nothing else can access the tree
@@ -7513,7 +7504,11 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info)
 			if (ret)
 				goto error;
 		}
-		path->slots[0]++;
+	}
+
+	if (iter_ret < 0) {
+		ret = iter_ret;
+		goto error;
 	}
 
 	/*
-- 
2.31.1

[PATCH 8/8] btrfs: xattr: Use btrfs_for_each_slot macro in btrfs_listxattr

[Marcos Paulo de Souza]

The function can be simplified by using the iterator like macro.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/xattr.c | 40 ++++++++++++----------------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 8a4514283a4b..f85febba1891 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -274,10 +274,12 @@ int btrfs_setxattr_trans(struct inode *inode, const char *name,
 ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
 	struct btrfs_key key;
+	struct btrfs_key found_key;
 	struct inode *inode = d_inode(dentry);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	struct btrfs_path *path;
 	int ret = 0;
+	int iter_ret = 0;
 	size_t total_size = 0, size_left = size;
 
 	/*
@@ -295,44 +297,22 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 	path->reada = READA_FORWARD;
 
 	/* search for our xattrs */
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0)
-		goto err;
-
-	while (1) {
+	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
 		struct extent_buffer *leaf;
 		int slot;
 		struct btrfs_dir_item *di;
-		struct btrfs_key found_key;
 		u32 item_size;
 		u32 cur;
 
 		leaf = path->nodes[0];
 		slot = path->slots[0];
 
-		/* this is where we start walking through the path */
-		if (slot >= btrfs_header_nritems(leaf)) {
-			/*
-			 * if we've reached the last slot in this leaf we need
-			 * to go to the next leaf and reset everything
-			 */
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0)
-				goto err;
-			else if (ret > 0)
-				break;
-			continue;
-		}
-
-		btrfs_item_key_to_cpu(leaf, &found_key, slot);
-
 		/* check to make sure this item is what we want */
-		if (found_key.objectid != key.objectid)
-			break;
-		if (found_key.type > BTRFS_XATTR_ITEM_KEY)
+		if (found_key.objectid != key.objectid ||
+		    found_key.type > BTRFS_XATTR_ITEM_KEY)
 			break;
 		if (found_key.type < BTRFS_XATTR_ITEM_KEY)
-			goto next_item;
+			continue;
 
 		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
 		item_size = btrfs_item_size_nr(leaf, slot);
@@ -365,9 +345,13 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 			cur += this_len;
 			di = (struct btrfs_dir_item *)((char *)di + this_len);
 		}
-next_item:
-		path->slots[0]++;
 	}
+
+	if (iter_ret < 0) {
+		ret = iter_ret;
+		goto err;
+	}
+
 	ret = total_size;
 
 err:
-- 
2.31.1

Re: [PATCHv2 0/8] btrfs: Create macro to iterate slots

[Marcos Paulo de Souza]

On Thu, 2021-08-26 at 13:40 -0300, Marcos Paulo de Souza wrote:
> There is a common pattern when search for a key in btrfs:
> 
>  * Call btrfs_search_slot
>  * Endless loop
>      * If the found slot is bigger than the current items in the
> leaf, check the
>        next one
>      * If still not found in the next leaf, return 1
>      * Do something with the code
>      * Increment current slot, and continue
> 
> This pattern can be improved by creating an iterator macro, similar
> to
> those for_each_X already existing in the linux kernel. using this
> approach means to reduce significantly boilerplate code, along making
> it
> easier to newcomers to understand how to code works.
> 
> This patchset survived a complete fstest run.

My bad, I only added v2 to the cover-letter, but the only change from
v1 is that now the xattr changes are in a separate patch.

> 
> Changes from v1:
>  * Separate xattr changes from the macro introducing code (Johannes)
> 
> Changes from RFC:
>  * Add documentation to btrfs_for_each_slot macro and
> btrfs_valid_slot function
>    (David)
>  * Add documentation about the ret variable used as a macro argument
> (David)
>  * Match function argument from prototype and implementation (David)
>  * Changed ({ }) block to only () in btrfs_for_each_slot macro
> (David)
>  * Add more patches to show the code being reduced by using this
> approach
>    (Nikolay)
> 
> Marcos Paulo de Souza (8):
>   fs: btrfs: Introduce btrfs_for_each_slot
>   btrfs: block-group: use btrfs_for_each_slot in
> find_first_block_group
>   btrfs: dev-replace: Use btrfs_for_each_slot in
>     mark_block_group_to_copy
>   btrfs: dir-item: use btrfs_for_each_slot in
>     btrfs_search_dir_index_item
>   btrfs: inode: use btrfs_for_each_slot in btrfs_read_readdir
>   btrfs: send: Use btrfs_for_each_slot macro
>   btrfs: volumes: use btrfs_for_each_slot in btrfs_read_chunk_tree
>   btrfs: xattr: Use btrfs_for_each_slot macro in btrfs_listxattr
> 
>  fs/btrfs/block-group.c |  33 +-----
>  fs/btrfs/ctree.c       |  28 ++++++
>  fs/btrfs/ctree.h       |  25 +++++
>  fs/btrfs/dev-replace.c |  51 ++--------
>  fs/btrfs/dir-item.c    |  27 +----
>  fs/btrfs/inode.c       |  46 ++++-----
>  fs/btrfs/send.c        | 222 +++++++++++--------------------------
> ----
>  fs/btrfs/volumes.c     |  23 ++---
>  fs/btrfs/xattr.c       |  40 +++-----
>  9 files changed, 169 insertions(+), 326 deletions(-)
> 

Re: [PATCHv2 0/8] btrfs: Create macro to iterate slots

[David Sterba]

On Thu, Aug 26, 2021 at 01:40:46PM -0300, Marcos Paulo de Souza wrote:
> Marcos Paulo de Souza (8):
>   fs: btrfs: Introduce btrfs_for_each_slot
>   btrfs: block-group: use btrfs_for_each_slot in find_first_block_group
>   btrfs: dev-replace: Use btrfs_for_each_slot in
>     mark_block_group_to_copy
>   btrfs: dir-item: use btrfs_for_each_slot in
>     btrfs_search_dir_index_item
>   btrfs: inode: use btrfs_for_each_slot in btrfs_read_readdir
>   btrfs: send: Use btrfs_for_each_slot macro
>   btrfs: volumes: use btrfs_for_each_slot in btrfs_read_chunk_tree
>   btrfs: xattr: Use btrfs_for_each_slot macro in btrfs_listxattr

There are few comments regarding patch organization. Please:

- drop the file prefixes from the subject (like "inode:", "send:")

- change "fs: btrfs:" to just "btrfs:" in the first patch

- unify the subjects that switch to the iterator to be
  "btrfs: use btrfs_for_each_slot in FUNCTION"

- do one patch per function, even if there are several in one file