[PATCH] btrfs: Introduce btrfs_search_backwards function

[PATCH] btrfs: Introduce btrfs_search_backwards function

[Marcos Paulo de Souza]

It's a common practice to start a search using offset (u64)-1, which is
the u64 maximum value, meaning that we want the search_slot function to
be set in the last item with the same objectid and type.

Once we are in this position, it's a matter to start a search backwards
by calling btrfs_previous_item, which will check if we'll need to go to
a previous leaf and other necessary checks, only to be sure that we are
in last offset of the same object and type. If the item is found,
convert the ondisk structure to the current endianness of the machine
running the code.

The new btrfs_search_backwards function does the all these procedures when
necessary, and can be used to avoid code duplication.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/ctree.c   | 23 +++++++++++++++++++++++
 fs/btrfs/ctree.h   |  6 ++++++
 fs/btrfs/ioctl.c   | 30 ++++++++----------------------
 fs/btrfs/super.c   | 26 ++++++--------------------
 fs/btrfs/volumes.c |  7 +------
 5 files changed, 44 insertions(+), 48 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 394fec1d3fd9..2991ee845813 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2100,6 +2100,29 @@ int btrfs_search_slot_for_read(struct btrfs_root *root,
 	return 0;
 }
 
+/*
+ * Execute search and call btrfs_previous_item to traverse backwards if the item
+ * was not found. If found, convert the stored item to the correct endianness.
+ *
+ * Return 0 if found, 1 if not found and < 0 if error.
+ */
+int btrfs_search_backwards(struct btrfs_root *root,
+				struct btrfs_key *key,
+				struct btrfs_key *found_key,
+				struct btrfs_path *path)
+{
+	int ret;
+
+	ret = btrfs_search_slot(NULL, root, key, path, 0, 0);
+	if (ret > 0)
+		ret = btrfs_previous_item(root, path, key->objectid, key->type);
+
+	if (ret == 0)
+		btrfs_item_key_to_cpu(path->nodes[0], found_key,
+					path->slots[0]);
+	return ret;
+}
+
 /*
  * 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 4a69aa604ac5..27aa2b959861 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2903,6 +2903,12 @@ int btrfs_next_leaf(struct btrfs_root *root, struct btrfs_path *path);
 int btrfs_prev_leaf(struct btrfs_root *root, struct btrfs_path *path);
 int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 			u64 time_seq);
+
+int btrfs_search_backwards(struct btrfs_root *root,
+				struct btrfs_key *key,
+				struct btrfs_key *found_key,
+				struct btrfs_path *path);
+
 static inline int btrfs_next_old_item(struct btrfs_root *root,
 				      struct btrfs_path *p, u64 time_seq)
 {
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0ba98e08a029..ac6c0530a941 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2382,23 +2382,16 @@ static noinline int btrfs_search_path_in_tree(struct btrfs_fs_info *info,
 	key.offset = (u64)-1;
 
 	while (1) {
-		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
+		ret = btrfs_search_backwards(root, &key, &key, path);
 		if (ret < 0)
 			goto out;
 		else if (ret > 0) {
-			ret = btrfs_previous_item(root, path, dirid,
-						  BTRFS_INODE_REF_KEY);
-			if (ret < 0)
-				goto out;
-			else if (ret > 0) {
-				ret = -ENOENT;
-				goto out;
-			}
+			ret = -ENOENT;
+			goto out;
 		}
 
 		l = path->nodes[0];
 		slot = path->slots[0];
-		btrfs_item_key_to_cpu(l, &key, slot);
 
 		iref = btrfs_item_ptr(l, slot, struct btrfs_inode_ref);
 		len = btrfs_inode_ref_name_len(l, iref);
@@ -2473,23 +2466,16 @@ static int btrfs_search_path_in_tree_user(struct inode *inode,
 		key.type = BTRFS_INODE_REF_KEY;
 		key.offset = (u64)-1;
 		while (1) {
-			ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-			if (ret < 0) {
+			ret = btrfs_search_backwards(root, &key, &key, path);
+			if (ret < 0)
+				goto out_put;
+			else if (ret > 0) {
+				ret = -ENOENT;
 				goto out_put;
-			} else if (ret > 0) {
-				ret = btrfs_previous_item(root, path, dirid,
-							  BTRFS_INODE_REF_KEY);
-				if (ret < 0) {
-					goto out_put;
-				} else if (ret > 0) {
-					ret = -ENOENT;
-					goto out_put;
-				}
 			}
 
 			leaf = path->nodes[0];
 			slot = path->slots[0];
-			btrfs_item_key_to_cpu(leaf, &key, slot);
 
 			iref = btrfs_item_ptr(leaf, slot, struct btrfs_inode_ref);
 			len = btrfs_inode_ref_name_len(leaf, iref);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index d07b18b2b250..2960219f7299 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1201,21 +1201,14 @@ char *btrfs_get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
 		key.type = BTRFS_ROOT_BACKREF_KEY;
 		key.offset = (u64)-1;
 
-		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
+		ret = btrfs_search_backwards(root, &key, &key, path);
 		if (ret < 0) {
 			goto err;
 		} else if (ret > 0) {
-			ret = btrfs_previous_item(root, path, subvol_objectid,
-						  BTRFS_ROOT_BACKREF_KEY);
-			if (ret < 0) {
-				goto err;
-			} else if (ret > 0) {
-				ret = -ENOENT;
-				goto err;
-			}
+			ret = -ENOENT;
+			goto err;
 		}
 
-		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
 		subvol_objectid = key.offset;
 
 		root_ref = btrfs_item_ptr(path->nodes[0], path->slots[0],
@@ -1248,21 +1241,14 @@ char *btrfs_get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
 			key.type = BTRFS_INODE_REF_KEY;
 			key.offset = (u64)-1;
 
-			ret = btrfs_search_slot(NULL, fs_root, &key, path, 0, 0);
+			ret = btrfs_search_backwards(fs_root, &key, &key, path);
 			if (ret < 0) {
 				goto err;
 			} else if (ret > 0) {
-				ret = btrfs_previous_item(fs_root, path, dirid,
-							  BTRFS_INODE_REF_KEY);
-				if (ret < 0) {
-					goto err;
-				} else if (ret > 0) {
-					ret = -ENOENT;
-					goto err;
-				}
+				ret = -ENOENT;
+				goto err;
 			}
 
-			btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
 			dirid = key.offset;
 
 			inode_ref = btrfs_item_ptr(path->nodes[0],
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d57ad0c9bb99..8d08e200b712 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1597,14 +1597,9 @@ static int find_free_dev_extent_start(struct btrfs_device *device,
 	key.offset = search_start;
 	key.type = BTRFS_DEV_EXTENT_KEY;
 
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
+	ret = btrfs_search_backwards(root, &key, &key, path);
 	if (ret < 0)
 		goto out;
-	if (ret > 0) {
-		ret = btrfs_previous_item(root, path, key.objectid, key.type);
-		if (ret < 0)
-			goto out;
-	}
 
 	while (1) {
 		l = path->nodes[0];
-- 
2.26.2

Re: [PATCH] btrfs: Introduce btrfs_search_backwards function

[David Sterba]

On Mon, Jul 26, 2021 at 11:03:17AM -0300, Marcos Paulo de Souza wrote:
> It's a common practice to start a search using offset (u64)-1, which is
> the u64 maximum value, meaning that we want the search_slot function to
> be set in the last item with the same objectid and type.
> 
> Once we are in this position, it's a matter to start a search backwards
> by calling btrfs_previous_item, which will check if we'll need to go to
> a previous leaf and other necessary checks, only to be sure that we are
> in last offset of the same object and type. If the item is found,
> convert the ondisk structure to the current endianness of the machine
> running the code.
> 
> The new btrfs_search_backwards function does the all these procedures when
> necessary, and can be used to avoid code duplication.
> 
> No functional changes.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  fs/btrfs/ctree.c   | 23 +++++++++++++++++++++++
>  fs/btrfs/ctree.h   |  6 ++++++
>  fs/btrfs/ioctl.c   | 30 ++++++++----------------------
>  fs/btrfs/super.c   | 26 ++++++--------------------
>  fs/btrfs/volumes.c |  7 +------
>  5 files changed, 44 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 394fec1d3fd9..2991ee845813 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2100,6 +2100,29 @@ int btrfs_search_slot_for_read(struct btrfs_root *root,
>  	return 0;
>  }
>  
> +/*
> + * Execute search and call btrfs_previous_item to traverse backwards if the item
> + * was not found. If found, convert the stored item to the correct endianness.
> + *
> + * Return 0 if found, 1 if not found and < 0 if error.
> + */
> +int btrfs_search_backwards(struct btrfs_root *root,
> +				struct btrfs_key *key,
> +				struct btrfs_key *found_key,

Is it necessary to have 2 keys? All calls pass the same one, so either
this should be just one or you have other patches that make use of two
distinct keys?

> -		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> +		ret = btrfs_search_backwards(root, &key, &key, path);

						   &key, &key

Re: [PATCH] btrfs: Introduce btrfs_search_backwards function

[Marcos Paulo de Souza]

On Wed, 2021-07-28 at 15:10 +0200, David Sterba wrote:
> On Mon, Jul 26, 2021 at 11:03:17AM -0300, Marcos Paulo de Souza
> wrote:
> > It's a common practice to start a search using offset (u64)-1,
> > which is
> > the u64 maximum value, meaning that we want the search_slot
> > function to
> > be set in the last item with the same objectid and type.
> > 
> > Once we are in this position, it's a matter to start a search
> > backwards
> > by calling btrfs_previous_item, which will check if we'll need to
> > go to
> > a previous leaf and other necessary checks, only to be sure that we
> > are
> > in last offset of the same object and type. If the item is found,
> > convert the ondisk structure to the current endianness of the
> > machine
> > running the code.
> > 
> > The new btrfs_search_backwards function does the all these
> > procedures when
> > necessary, and can be used to avoid code duplication.
> > 
> > No functional changes.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >  fs/btrfs/ctree.c   | 23 +++++++++++++++++++++++
> >  fs/btrfs/ctree.h   |  6 ++++++
> >  fs/btrfs/ioctl.c   | 30 ++++++++----------------------
> >  fs/btrfs/super.c   | 26 ++++++--------------------
> >  fs/btrfs/volumes.c |  7 +------
> >  5 files changed, 44 insertions(+), 48 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> > index 394fec1d3fd9..2991ee845813 100644
> > --- a/fs/btrfs/ctree.c
> > +++ b/fs/btrfs/ctree.c
> > @@ -2100,6 +2100,29 @@ int btrfs_search_slot_for_read(struct
> > btrfs_root *root,
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Execute search and call btrfs_previous_item to traverse
> > backwards if the item
> > + * was not found. If found, convert the stored item to the correct
> > endianness.
> > + *
> > + * Return 0 if found, 1 if not found and < 0 if error.
> > + */
> > +int btrfs_search_backwards(struct btrfs_root *root,
> > +				struct btrfs_key *key,
> > +				struct btrfs_key *found_key,
> 
> Is it necessary to have 2 keys? All calls pass the same one, so
> either
> this should be just one or you have other patches that make use of
> two
> distinct keys?

Yes, in these cases yes, but I can see sometimes other places in the
btrfs codebase where the key used convert to cpu is different from the
key used in the search path, so I wanted to make it flexible to future
users.

I believe that we can extent this in the future if needed, so
I'll send a v2 using only one key argument, and use the same argument
to convert to the correct endianness.

Thanks,
  Marcos

> 
> > -		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> > +		ret = btrfs_search_backwards(root, &key, &key, path);
> 
> 						   &key, &key

Re: [PATCH] btrfs: Introduce btrfs_search_backwards function

[David Sterba]

On Wed, Jul 28, 2021 at 01:41:30PM -0300, Marcos Paulo de Souza wrote:
> On Wed, 2021-07-28 at 15:10 +0200, David Sterba wrote:
> > On Mon, Jul 26, 2021 at 11:03:17AM -0300, Marcos Paulo de Souza
> > wrote:
> > > +/*
> > > + * Execute search and call btrfs_previous_item to traverse
> > > backwards if the item
> > > + * was not found. If found, convert the stored item to the correct
> > > endianness.
> > > + *
> > > + * Return 0 if found, 1 if not found and < 0 if error.
> > > + */
> > > +int btrfs_search_backwards(struct btrfs_root *root,
> > > +				struct btrfs_key *key,
> > > +				struct btrfs_key *found_key,
> > 
> > Is it necessary to have 2 keys? All calls pass the same one, so
> > either
> > this should be just one or you have other patches that make use of
> > two
> > distinct keys?
> 
> Yes, in these cases yes, but I can see sometimes other places in the
> btrfs codebase where the key used convert to cpu is different from the
> key used in the search path, so I wanted to make it flexible to future
> users.

We can do that once there's need for this flexibility.

> I believe that we can extent this in the future if needed, so
> I'll send a v2 using only one key argument, and use the same argument
> to convert to the correct endianness.

Thanks, you can also drop the notices about endianness because that's
assumed that all keys normally used are ind the CPU order, the disk keys
are explicitly called 'disk_key' as their use is limited.