[PATCH 0/7] btrfs: Use btrfs_find_item whenever possible

[PATCH 0/7] btrfs: Use btrfs_find_item whenever possible

[Marcos Paulo de Souza]

Searching for a key in btrfs is a very common task, so try to use
btrfs_find_item whenever possible to avoid boilerplate code, while making the
code simpler when possible.

Please review!

Thanks,
  Marcos

Marcos Paulo de Souza (7):
  btrfs: Reorder btrfs_find_item arguments
  btrfs: backref: Use btrfs_find_item in btrfs_find_one_extref
  btrfs: zoned: Use btrfs_find_item in calculate_emulated_zone_size
  btrfs: root-tree: Use btrfs_find_item in btrfs_find_orphan_roots
  btrfs: scrub: Use btrfs_find_item in scrub_enumerate_chunks
  btrfs: tree-log: Simplify log_new_ancestors
  btrfs: ioctl: Simplify btrfs_ioctl_get_subvol_info

 fs/btrfs/backref.c   | 73 +++++++++-----------------------------------
 fs/btrfs/ctree.c     |  2 +-
 fs/btrfs/ctree.h     |  2 +-
 fs/btrfs/ioctl.c     | 56 +++++++++++++--------------------
 fs/btrfs/root-tree.c | 32 +++++--------------
 fs/btrfs/scrub.c     | 52 +++++++++----------------------
 fs/btrfs/tree-log.c  | 40 ++++++++----------------
 fs/btrfs/zoned.c     | 21 ++++---------
 8 files changed, 79 insertions(+), 199 deletions(-)

-- 
2.31.1

[PATCH 1/7] btrfs: Reorder btrfs_find_item arguments

[Marcos Paulo de Souza]

It's more natural do use objectid, type and offset, in this order, when
dealing with btrfs keys.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/backref.c | 9 ++++-----
 fs/btrfs/ctree.c   | 2 +-
 fs/btrfs/ctree.h   | 2 +-
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index f735b8798ba1..9e92faaafa02 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
 				btrfs_tree_read_unlock(eb);
 			free_extent_buffer(eb);
 		}
-		ret = btrfs_find_item(fs_root, path, parent, 0,
-				BTRFS_INODE_REF_KEY, &found_key);
+		ret = btrfs_find_item(fs_root, path, parent, BTRFS_INODE_REF_KEY,
+					0, &found_key);
 		if (ret > 0)
 			ret = -ENOENT;
 		if (ret)
@@ -2063,9 +2063,8 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
 	struct btrfs_key found_key;
 
 	while (!ret) {
-		ret = btrfs_find_item(fs_root, path, inum,
-				parent ? parent + 1 : 0, BTRFS_INODE_REF_KEY,
-				&found_key);
+		ret = btrfs_find_item(fs_root, path, inum, BTRFS_INODE_REF_KEY,
+				parent ? parent + 1 : 0, &found_key);
 
 		if (ret < 0)
 			break;
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 84627cbd5b5b..c0002ec9c025 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1528,7 +1528,7 @@ setup_nodes_for_search(struct btrfs_trans_handle *trans,
 }
 
 int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
-		u64 iobjectid, u64 ioff, u8 key_type,
+		u64 iobjectid, u8 key_type, u64 ioff,
 		struct btrfs_key *found_key)
 {
 	int ret;
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a898257ad2b5..0a971e98f5f9 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2858,7 +2858,7 @@ int btrfs_duplicate_item(struct btrfs_trans_handle *trans,
 			 struct btrfs_path *path,
 			 const struct btrfs_key *new_key);
 int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
-		u64 inum, u64 ioff, u8 key_type, struct btrfs_key *found_key);
+		u64 inum, u8 key_type, u64 ioff, struct btrfs_key *found_key);
 int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		      const struct btrfs_key *key, struct btrfs_path *p,
 		      int ins_len, int cow);
-- 
2.31.1

Re: [PATCH 1/7] btrfs: Reorder btrfs_find_item arguments

[Qu Wenruo]

On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote:
> It's more natural do use objectid, type and offset, in this order, when
> dealing with btrfs keys.

I'm completely fine with this part.

>
> No functional changes.
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>   fs/btrfs/backref.c | 9 ++++-----
>   fs/btrfs/ctree.c   | 2 +-
>   fs/btrfs/ctree.h   | 2 +-
>   3 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index f735b8798ba1..9e92faaafa02 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
>   				btrfs_tree_read_unlock(eb);
>   			free_extent_buffer(eb);
>   		}
> -		ret = btrfs_find_item(fs_root, path, parent, 0,
> -				BTRFS_INODE_REF_KEY, &found_key);
> +		ret = btrfs_find_item(fs_root, path, parent, BTRFS_INODE_REF_KEY,
> +					0, &found_key);
>   		if (ret > 0)
>   			ret = -ENOENT;
>   		if (ret)
> @@ -2063,9 +2063,8 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
>   	struct btrfs_key found_key;
>
>   	while (!ret) {
> -		ret = btrfs_find_item(fs_root, path, inum,
> -				parent ? parent + 1 : 0, BTRFS_INODE_REF_KEY,
> -				&found_key);
> +		ret = btrfs_find_item(fs_root, path, inum, BTRFS_INODE_REF_KEY,
> +				parent ? parent + 1 : 0, &found_key);
>
>   		if (ret < 0)
>   			break;
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 84627cbd5b5b..c0002ec9c025 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1528,7 +1528,7 @@ setup_nodes_for_search(struct btrfs_trans_handle *trans,
>   }
>
>   int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
> -		u64 iobjectid, u64 ioff, u8 key_type,
> +		u64 iobjectid, u8 key_type, u64 ioff,
>   		struct btrfs_key *found_key)

But the @found_key part makes me wonder.

Is it really needed?

The caller has @path and return value. If we return 0, we know it's an
exact match, no need to grab the key.
If we return 1, caller can easily grab the key using @path (if they
really need).

So can we also remove @found_key parameter, and add some comment on the
function?

Thanks,
Qu

>   {
>   	int ret;
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index a898257ad2b5..0a971e98f5f9 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2858,7 +2858,7 @@ int btrfs_duplicate_item(struct btrfs_trans_handle *trans,
>   			 struct btrfs_path *path,
>   			 const struct btrfs_key *new_key);
>   int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
> -		u64 inum, u64 ioff, u8 key_type, struct btrfs_key *found_key);
> +		u64 inum, u8 key_type, u64 ioff, struct btrfs_key *found_key);
>   int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>   		      const struct btrfs_key *key, struct btrfs_path *p,
>   		      int ins_len, int cow);
>

Re: [PATCH 1/7] btrfs: Reorder btrfs_find_item arguments

[Marcos Paulo de Souza]

On Thu, 2021-08-05 at 10:16 +0800, Qu Wenruo wrote:
> 
> On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote:
> > It's more natural do use objectid, type and offset, in this order,
> > when
> > dealing with btrfs keys.
> 
> I'm completely fine with this part.
> 
> > No functional changes.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >   fs/btrfs/backref.c | 9 ++++-----
> >   fs/btrfs/ctree.c   | 2 +-
> >   fs/btrfs/ctree.h   | 2 +-
> >   3 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> > index f735b8798ba1..9e92faaafa02 100644
> > --- a/fs/btrfs/backref.c
> > +++ b/fs/btrfs/backref.c
> > @@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root
> > *fs_root, struct btrfs_path *path,
> >   				btrfs_tree_read_unlock(eb);
> >   			free_extent_buffer(eb);
> >   		}
> > -		ret = btrfs_find_item(fs_root, path, parent, 0,
> > -				BTRFS_INODE_REF_KEY, &found_key);
> > +		ret = btrfs_find_item(fs_root, path, parent,
> > BTRFS_INODE_REF_KEY,
> > +					0, &found_key);
> >   		if (ret > 0)
> >   			ret = -ENOENT;
> >   		if (ret)
> > @@ -2063,9 +2063,8 @@ static int iterate_inode_refs(u64 inum,
> > struct btrfs_root *fs_root,
> >   	struct btrfs_key found_key;
> > 
> >   	while (!ret) {
> > -		ret = btrfs_find_item(fs_root, path, inum,
> > -				parent ? parent + 1 : 0,
> > BTRFS_INODE_REF_KEY,
> > -				&found_key);
> > +		ret = btrfs_find_item(fs_root, path, inum,
> > BTRFS_INODE_REF_KEY,
> > +				parent ? parent + 1 : 0, &found_key);
> > 
> >   		if (ret < 0)
> >   			break;
> > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> > index 84627cbd5b5b..c0002ec9c025 100644
> > --- a/fs/btrfs/ctree.c
> > +++ b/fs/btrfs/ctree.c
> > @@ -1528,7 +1528,7 @@ setup_nodes_for_search(struct
> > btrfs_trans_handle *trans,
> >   }
> > 
> >   int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path
> > *path,
> > -		u64 iobjectid, u64 ioff, u8 key_type,
> > +		u64 iobjectid, u8 key_type, u64 ioff,
> >   		struct btrfs_key *found_key)
> 
> But the @found_key part makes me wonder.
> 
> Is it really needed?
> 
> The caller has @path and return value. If we return 0, we know it's
> an
> exact match, no need to grab the key.
> If we return 1, caller can easily grab the key using @path (if they
> really need).
> 
> So can we also remove @found_key parameter, and add some comment on
> the
> function?

I believe that the function name is misleading. Maybe we can adjust it
to something like btrfs_find_item_offset, since it validates if the
found item has the same objectid and type of the searched key.

This is very common for a lot of the callers, which expect to receive
the same objectid and type, and each caller validate the offset as
required. Maybe we can add a comment and change the function name to
reflect all aspects of how it works. What do you think?

Thanks,
  Marcos

> 
> Thanks,
> Qu
> 
> >   {
> >   	int ret;
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index a898257ad2b5..0a971e98f5f9 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -2858,7 +2858,7 @@ int btrfs_duplicate_item(struct
> > btrfs_trans_handle *trans,
> >   			 struct btrfs_path *path,
> >   			 const struct btrfs_key *new_key);
> >   int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path
> > *path,
> > -		u64 inum, u64 ioff, u8 key_type, struct btrfs_key
> > *found_key);
> > +		u64 inum, u8 key_type, u64 ioff, struct btrfs_key
> > *found_key);
> >   int btrfs_search_slot(struct btrfs_trans_handle *trans, struct
> > btrfs_root *root,
> >   		      const struct btrfs_key *key, struct btrfs_path
> > *p,
> >   		      int ins_len, int cow);
> > 

Re: [PATCH 1/7] btrfs: Reorder btrfs_find_item arguments

[Qu Wenruo]

On 2021/8/6 上午1:28, Marcos Paulo de Souza wrote:
> On Thu, 2021-08-05 at 10:16 +0800, Qu Wenruo wrote:
>>
>> On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote:
>>> It's more natural do use objectid, type and offset, in this order,
>>> when
>>> dealing with btrfs keys.
>>
>> I'm completely fine with this part.
>>
>>> No functional changes.
>>>
>>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
>>> ---
>>>    fs/btrfs/backref.c | 9 ++++-----
>>>    fs/btrfs/ctree.c   | 2 +-
>>>    fs/btrfs/ctree.h   | 2 +-
>>>    3 files changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>>> index f735b8798ba1..9e92faaafa02 100644
>>> --- a/fs/btrfs/backref.c
>>> +++ b/fs/btrfs/backref.c
>>> @@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root
>>> *fs_root, struct btrfs_path *path,
>>>    				btrfs_tree_read_unlock(eb);
>>>    			free_extent_buffer(eb);
>>>    		}
>>> -		ret = btrfs_find_item(fs_root, path, parent, 0,
>>> -				BTRFS_INODE_REF_KEY, &found_key);
>>> +		ret = btrfs_find_item(fs_root, path, parent,
>>> BTRFS_INODE_REF_KEY,
>>> +					0, &found_key);
>>>    		if (ret > 0)
>>>    			ret = -ENOENT;
>>>    		if (ret)
>>> @@ -2063,9 +2063,8 @@ static int iterate_inode_refs(u64 inum,
>>> struct btrfs_root *fs_root,
>>>    	struct btrfs_key found_key;
>>>
>>>    	while (!ret) {
>>> -		ret = btrfs_find_item(fs_root, path, inum,
>>> -				parent ? parent + 1 : 0,
>>> BTRFS_INODE_REF_KEY,
>>> -				&found_key);
>>> +		ret = btrfs_find_item(fs_root, path, inum,
>>> BTRFS_INODE_REF_KEY,
>>> +				parent ? parent + 1 : 0, &found_key);
>>>
>>>    		if (ret < 0)
>>>    			break;
>>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>>> index 84627cbd5b5b..c0002ec9c025 100644
>>> --- a/fs/btrfs/ctree.c
>>> +++ b/fs/btrfs/ctree.c
>>> @@ -1528,7 +1528,7 @@ setup_nodes_for_search(struct
>>> btrfs_trans_handle *trans,
>>>    }
>>>
>>>    int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path
>>> *path,
>>> -		u64 iobjectid, u64 ioff, u8 key_type,
>>> +		u64 iobjectid, u8 key_type, u64 ioff,
>>>    		struct btrfs_key *found_key)
>>
>> But the @found_key part makes me wonder.
>>
>> Is it really needed?
>>
>> The caller has @path and return value. If we return 0, we know it's
>> an
>> exact match, no need to grab the key.
>> If we return 1, caller can easily grab the key using @path (if they
>> really need).
>>
>> So can we also remove @found_key parameter, and add some comment on
>> the
>> function?
>
> I believe that the function name is misleading. Maybe we can adjust it
> to something like btrfs_find_item_offset, since it validates if the
> found item has the same objectid and type of the searched key.

Then the parameter @offset makes no sense.

Since we have exact key, it's really intuitional to think we're
searching for a specific key, and under most cases we are.

>
> This is very common for a lot of the callers, which expect to receive
> the same objectid and type, and each caller validate the offset as
> required. Maybe we can add a comment and change the function name to
> reflect all aspects of how it works. What do you think?

For such case, the caller doesn't have the full key, but are looking for
a key to meet certain key *range* requirement.

I believe that needs a new interface.

IMHO, we need a generic way to search for a key range (or doing
iteration for a key range), and a subset of above interface to just
search for a specific key (thus with much simpler interface).

Thanks,
Qu

>
> Thanks,
>    Marcos
>
>>
>> Thanks,
>> Qu
>>
>>>    {
>>>    	int ret;
>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>> index a898257ad2b5..0a971e98f5f9 100644
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -2858,7 +2858,7 @@ int btrfs_duplicate_item(struct
>>> btrfs_trans_handle *trans,
>>>    			 struct btrfs_path *path,
>>>    			 const struct btrfs_key *new_key);
>>>    int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path
>>> *path,
>>> -		u64 inum, u64 ioff, u8 key_type, struct btrfs_key
>>> *found_key);
>>> +		u64 inum, u8 key_type, u64 ioff, struct btrfs_key
>>> *found_key);
>>>    int btrfs_search_slot(struct btrfs_trans_handle *trans, struct
>>> btrfs_root *root,
>>>    		      const struct btrfs_key *key, struct btrfs_path
>>> *p,
>>>    		      int ins_len, int cow);
>>>
>

Re: [PATCH 1/7] btrfs: Reorder btrfs_find_item arguments

[David Sterba]

On Wed, Aug 04, 2021 at 03:48:48PM -0300, Marcos Paulo de Souza wrote:
> It's more natural do use objectid, type and offset, in this order, when
> dealing with btrfs keys.
> 
> No functional changes.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  fs/btrfs/backref.c | 9 ++++-----
>  fs/btrfs/ctree.c   | 2 +-
>  fs/btrfs/ctree.h   | 2 +-
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index f735b8798ba1..9e92faaafa02 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -1691,8 +1691,8 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
>  				btrfs_tree_read_unlock(eb);
>  			free_extent_buffer(eb);
>  		}
> -		ret = btrfs_find_item(fs_root, path, parent, 0,
> -				BTRFS_INODE_REF_KEY, &found_key);
> +		ret = btrfs_find_item(fs_root, path, parent, BTRFS_INODE_REF_KEY,
> +					0, &found_key);

Have you considered using the found_key as both input and output
parameter? As input it stores the objectid/key/offset parameters and
with no errors it's set to the found key.

All callers of the function already have a key and you add another
variable just for the one that changes in the iterations (eg. offset).

[PATCH 2/7] btrfs: backref: Use btrfs_find_item in btrfs_find_one_extref

[Marcos Paulo de Souza]

btrfs_find_one_extref is using btrfs_search_slot and iterating over the
slots, but in reality it only desires to find an extref, since there is
a break without any condition at the end of the while clause.

The function can be dramatically simplified by using btrfs_find_item, which
calls the btrfs_search_slot, compares if the objectid and type found
are the same of those passed as search key, and calls
btrfs_item_key_to_cpu if no error was found.

No functional changes.

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

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 9e92faaafa02..57b955c8a875 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1588,67 +1588,25 @@ int btrfs_find_one_extref(struct btrfs_root *root, u64 inode_objectid,
 			  struct btrfs_inode_extref **ret_extref,
 			  u64 *found_off)
 {
-	int ret, slot;
+	int ret;
 	struct btrfs_key key;
-	struct btrfs_key found_key;
 	struct btrfs_inode_extref *extref;
-	const struct extent_buffer *leaf;
 	unsigned long ptr;
 
-	key.objectid = inode_objectid;
-	key.type = BTRFS_INODE_EXTREF_KEY;
-	key.offset = start_off;
-
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
+	ret = btrfs_find_item(root, path, inode_objectid, BTRFS_INODE_EXTREF_KEY,
+			      start_off, &key);
 	if (ret < 0)
 		return ret;
+	else if (ret > 0)
+		return -ENOENT;
 
-	while (1) {
-		leaf = path->nodes[0];
-		slot = path->slots[0];
-		if (slot >= btrfs_header_nritems(leaf)) {
-			/*
-			 * If the item at offset is not found,
-			 * btrfs_search_slot will point us to the slot
-			 * where it should be inserted. In our case
-			 * that will be the slot directly before the
-			 * next INODE_REF_KEY_V2 item. In the case
-			 * that we're pointing to the last slot in a
-			 * leaf, we must move one leaf over.
-			 */
-			ret = btrfs_next_leaf(root, path);
-			if (ret) {
-				if (ret >= 1)
-					ret = -ENOENT;
-				break;
-			}
-			continue;
-		}
-
-		btrfs_item_key_to_cpu(leaf, &found_key, slot);
-
-		/*
-		 * Check that we're still looking at an extended ref key for
-		 * this particular objectid. If we have different
-		 * objectid or type then there are no more to be found
-		 * in the tree and we can exit.
-		 */
-		ret = -ENOENT;
-		if (found_key.objectid != inode_objectid)
-			break;
-		if (found_key.type != BTRFS_INODE_EXTREF_KEY)
-			break;
-
-		ret = 0;
-		ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
-		extref = (struct btrfs_inode_extref *)ptr;
-		*ret_extref = extref;
-		if (found_off)
-			*found_off = found_key.offset;
-		break;
-	}
+	ptr = btrfs_item_ptr_offset(path->nodes[0], path->slots[0]);
+	extref = (struct btrfs_inode_extref *)ptr;
+	*ret_extref = extref;
+	if (found_off)
+		*found_off = key.offset;
 
-	return ret;
+	return 0;
 }
 
 /*
-- 
2.31.1

Re: [PATCH 2/7] btrfs: backref: Use btrfs_find_item in btrfs_find_one_extref

[Qu Wenruo]

On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote:
> btrfs_find_one_extref is using btrfs_search_slot and iterating over the
> slots, but in reality it only desires to find an extref, since there is
> a break without any condition at the end of the while clause.
>
> The function can be dramatically simplified by using btrfs_find_item, which
> calls the btrfs_search_slot, compares if the objectid and type found
> are the same of those passed as search key, and calls
> btrfs_item_key_to_cpu if no error was found.
>
> No functional changes.
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

But a small nitpick inlined below.

> ---
>   fs/btrfs/backref.c | 64 ++++++++--------------------------------------
>   1 file changed, 11 insertions(+), 53 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 9e92faaafa02..57b955c8a875 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -1588,67 +1588,25 @@ int btrfs_find_one_extref(struct btrfs_root *root, u64 inode_objectid,
>   			  struct btrfs_inode_extref **ret_extref,
>   			  u64 *found_off)
>   {
> -	int ret, slot;
> +	int ret;
>   	struct btrfs_key key;
> -	struct btrfs_key found_key;
>   	struct btrfs_inode_extref *extref;
> -	const struct extent_buffer *leaf;
>   	unsigned long ptr;
>
> -	key.objectid = inode_objectid;
> -	key.type = BTRFS_INODE_EXTREF_KEY;
> -	key.offset = start_off;
> -
> -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> +	ret = btrfs_find_item(root, path, inode_objectid, BTRFS_INODE_EXTREF_KEY,
> +			      start_off, &key);
>   	if (ret < 0)
>   		return ret;
> +	else if (ret > 0)
> +		return -ENOENT;
>
> -	while (1) {
> -		leaf = path->nodes[0];
> -		slot = path->slots[0];
> -		if (slot >= btrfs_header_nritems(leaf)) {
> -			/*
> -			 * If the item at offset is not found,
> -			 * btrfs_search_slot will point us to the slot
> -			 * where it should be inserted. In our case
> -			 * that will be the slot directly before the
> -			 * next INODE_REF_KEY_V2 item. In the case
> -			 * that we're pointing to the last slot in a
> -			 * leaf, we must move one leaf over.
> -			 */
> -			ret = btrfs_next_leaf(root, path);
> -			if (ret) {
> -				if (ret >= 1)
> -					ret = -ENOENT;
> -				break;
> -			}
> -			continue;
> -		}
> -
> -		btrfs_item_key_to_cpu(leaf, &found_key, slot);
> -
> -		/*
> -		 * Check that we're still looking at an extended ref key for
> -		 * this particular objectid. If we have different
> -		 * objectid or type then there are no more to be found
> -		 * in the tree and we can exit.
> -		 */
> -		ret = -ENOENT;
> -		if (found_key.objectid != inode_objectid)
> -			break;
> -		if (found_key.type != BTRFS_INODE_EXTREF_KEY)
> -			break;
> -
> -		ret = 0;
> -		ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
> -		extref = (struct btrfs_inode_extref *)ptr;
> -		*ret_extref = extref;
> -		if (found_off)
> -			*found_off = found_key.offset;
> -		break;
> -	}
> +	ptr = btrfs_item_ptr_offset(path->nodes[0], path->slots[0]);

@ptr is only a temporary variable.

We can replace it with:

extref = btrfs_item_ptr(path->nodes[0], path->slots[0], struct
btrfs_inode_extref);

Thanks,
Qu
> +	extref = (struct btrfs_inode_extref *)ptr;
> +	*ret_extref = extref;
> +	if (found_off)
> +		*found_off = key.offset;
>
> -	return ret;
> +	return 0;
>   }
>
>   /*
>

[PATCH 3/7] btrfs: zoned: Use btrfs_find_item in calculate_emulated_zone_size

[Marcos Paulo de Souza]

calculate_emulated_zone_size can be simplified by using btrfs_find_item, which
executes btrfs_search_slot and calls btrfs_next_leaf if needed.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/zoned.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 47af1ab3bf12..d344baa26de0 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -230,29 +230,20 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info)
 	struct btrfs_key key;
 	struct extent_buffer *leaf;
 	struct btrfs_dev_extent *dext;
-	int ret = 0;
-
-	key.objectid = 1;
-	key.type = BTRFS_DEV_EXTENT_KEY;
-	key.offset = 0;
+	int ret;
 
 	path = btrfs_alloc_path();
 	if (!path)
 		return -ENOMEM;
 
-	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
+	ret = btrfs_find_item(root, path, 1, BTRFS_DEV_EXTENT_KEY, 0, &key);
 	if (ret < 0)
 		goto out;
 
-	if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
-		ret = btrfs_next_leaf(root, path);
-		if (ret < 0)
-			goto out;
-		/* No dev extents at all? Not good */
-		if (ret > 0) {
-			ret = -EUCLEAN;
-			goto out;
-		}
+	/* No dev extents at all? Not good */
+	else if (ret > 0) {
+		ret = -EUCLEAN;
+		goto out;
 	}
 
 	leaf = path->nodes[0];
-- 
2.31.1

Re: [PATCH 3/7] btrfs: zoned: Use btrfs_find_item in calculate_emulated_zone_size

[Qu Wenruo]

On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote:
> calculate_emulated_zone_size can be simplified by using btrfs_find_item, which
> executes btrfs_search_slot and calls btrfs_next_leaf if needed.
>
> No functional changes.
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

But since we're here, some unrelated comment inlined below.
> ---
>   fs/btrfs/zoned.c | 21 ++++++---------------
>   1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 47af1ab3bf12..d344baa26de0 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -230,29 +230,20 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info)
>   	struct btrfs_key key;
>   	struct extent_buffer *leaf;
>   	struct btrfs_dev_extent *dext;
> -	int ret = 0;
> -
> -	key.objectid = 1;

Not related to this patch itself, but this immediate number is not that
straightforward.

In fact for DEV_EXTENT_KEY, the objectid means device number.

For current zoned device support IIRC we only support single device,
thus it's fixed to 1.

It would be better to have some extra comment/ASSERT() for it.


> -	key.type = BTRFS_DEV_EXTENT_KEY;
> -	key.offset = 0;

Normally for DEV_EXTENT_KEY, the offset is the dev physical offset,
which normally is not 0 (as we reserve 0~1M for each device)

So this is a special zoned on-disk format?

Thanks,
Qu

> +	int ret;
>
>   	path = btrfs_alloc_path();
>   	if (!path)
>   		return -ENOMEM;
>
> -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> +	ret = btrfs_find_item(root, path, 1, BTRFS_DEV_EXTENT_KEY, 0, &key);
>   	if (ret < 0)
>   		goto out;
>
> -	if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
> -		ret = btrfs_next_leaf(root, path);
> -		if (ret < 0)
> -			goto out;
> -		/* No dev extents at all? Not good */
> -		if (ret > 0) {
> -			ret = -EUCLEAN;
> -			goto out;
> -		}
> +	/* No dev extents at all? Not good */
> +	else if (ret > 0) {
> +		ret = -EUCLEAN;
> +		goto out;
>   	}
>
>   	leaf = path->nodes[0];
>

Re: [PATCH 3/7] btrfs: zoned: Use btrfs_find_item in calculate_emulated_zone_size

[Naohiro Aota]

On Thu, Aug 05, 2021 at 02:39:09PM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote:
> > calculate_emulated_zone_size can be simplified by using btrfs_find_item, which
> > executes btrfs_search_slot and calls btrfs_next_leaf if needed.
> > 
> > No functional changes.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> But since we're here, some unrelated comment inlined below.
> > ---
> >   fs/btrfs/zoned.c | 21 ++++++---------------
> >   1 file changed, 6 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> > index 47af1ab3bf12..d344baa26de0 100644
> > --- a/fs/btrfs/zoned.c
> > +++ b/fs/btrfs/zoned.c
> > @@ -230,29 +230,20 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info)
> >   	struct btrfs_key key;
> >   	struct extent_buffer *leaf;
> >   	struct btrfs_dev_extent *dext;
> > -	int ret = 0;
> > -
> > -	key.objectid = 1;
> 
> Not related to this patch itself, but this immediate number is not that
> straightforward.
> 
> In fact for DEV_EXTENT_KEY, the objectid means device number.
> 
> For current zoned device support IIRC we only support single device,
> thus it's fixed to 1.

To be precise, we can have multiple devices, but only support single
profile.

> It would be better to have some extra comment/ASSERT() for it.
> 
> 
> > -	key.type = BTRFS_DEV_EXTENT_KEY;
> > -	key.offset = 0;
> 
> Normally for DEV_EXTENT_KEY, the offset is the dev physical offset,
> which normally is not 0 (as we reserve 0~1M for each device)
> 
> So this is a special zoned on-disk format?

We also reserve the first two zones on a disk for superblock log
zones, so on typical SMR drive, 0-512M is reserved.

We can use any DEV_EXTENT item here to determine the emulated zone
size. So, it's trying to find the first one.

> Thanks,
> Qu
> 
> > +	int ret;
> > 
> >   	path = btrfs_alloc_path();
> >   	if (!path)
> >   		return -ENOMEM;
> > 
> > -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> > +	ret = btrfs_find_item(root, path, 1, BTRFS_DEV_EXTENT_KEY, 0, &key);
> >   	if (ret < 0)
> >   		goto out;
> > 
> > -	if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
> > -		ret = btrfs_next_leaf(root, path);
> > -		if (ret < 0)
> > -			goto out;
> > -		/* No dev extents at all? Not good */
> > -		if (ret > 0) {
> > -			ret = -EUCLEAN;
> > -			goto out;
> > -		}
> > +	/* No dev extents at all? Not good */
> > +	else if (ret > 0) {
> > +		ret = -EUCLEAN;
> > +		goto out;
> >   	}
> > 
> >   	leaf = path->nodes[0];
> > 

Re: [PATCH 3/7] btrfs: zoned: Use btrfs_find_item in calculate_emulated_zone_size

[Qu Wenruo]

On 2021/8/6 下午1:52, Naohiro Aota wrote:
> On Thu, Aug 05, 2021 at 02:39:09PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote:
>>> calculate_emulated_zone_size can be simplified by using btrfs_find_item, which
>>> executes btrfs_search_slot and calls btrfs_next_leaf if needed.
>>>
>>> No functional changes.
>>>
>>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> But since we're here, some unrelated comment inlined below.
>>> ---
>>>    fs/btrfs/zoned.c | 21 ++++++---------------
>>>    1 file changed, 6 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
>>> index 47af1ab3bf12..d344baa26de0 100644
>>> --- a/fs/btrfs/zoned.c
>>> +++ b/fs/btrfs/zoned.c
>>> @@ -230,29 +230,20 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info)
>>>    	struct btrfs_key key;
>>>    	struct extent_buffer *leaf;
>>>    	struct btrfs_dev_extent *dext;
>>> -	int ret = 0;
>>> -
>>> -	key.objectid = 1;
>>
>> Not related to this patch itself, but this immediate number is not that
>> straightforward.
>>
>> In fact for DEV_EXTENT_KEY, the objectid means device number.
>>
>> For current zoned device support IIRC we only support single device,
>> thus it's fixed to 1.
>
> To be precise, we can have multiple devices, but only support single
> profile.

So it means we could have cases where we only have devid 2 (device add,
then remove the original device).

The original code is fine, it's just searching the for the first device
extent.

But the new code is no longer doing the same thing.

My reviewed-by tag is wrong now....

>
>> It would be better to have some extra comment/ASSERT() for it.
>>
>>
>>> -	key.type = BTRFS_DEV_EXTENT_KEY;
>>> -	key.offset = 0;
>>
>> Normally for DEV_EXTENT_KEY, the offset is the dev physical offset,
>> which normally is not 0 (as we reserve 0~1M for each device)
>>
>> So this is a special zoned on-disk format?
>
> We also reserve the first two zones on a disk for superblock log
> zones, so on typical SMR drive, 0-512M is reserved.
>
> We can use any DEV_EXTENT item here to determine the emulated zone
> size. So, it's trying to find the first one.

Then the patch is changing the behavior.

Now it can't handle cases where we don't have devid 1.

And since we're search for the first DEV_EXTENT_KEY, I don't believe
btrfs_find_item() is the proper function here to call.

Please drop my reviewed-by tag.

Thanks,
Qu

>
>> Thanks,
>> Qu
>>
>>> +	int ret;
>>>
>>>    	path = btrfs_alloc_path();
>>>    	if (!path)
>>>    		return -ENOMEM;
>>>
>>> -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
>>> +	ret = btrfs_find_item(root, path, 1, BTRFS_DEV_EXTENT_KEY, 0, &key);
>>>    	if (ret < 0)
>>>    		goto out;
>>>
>>> -	if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
>>> -		ret = btrfs_next_leaf(root, path);
>>> -		if (ret < 0)
>>> -			goto out;
>>> -		/* No dev extents at all? Not good */
>>> -		if (ret > 0) {
>>> -			ret = -EUCLEAN;
>>> -			goto out;
>>> -		}
>>> +	/* No dev extents at all? Not good */
>>> +	else if (ret > 0) {
>>> +		ret = -EUCLEAN;
>>> +		goto out;
>>>    	}
>>>
>>>    	leaf = path->nodes[0];

Re: [PATCH 3/7] btrfs: zoned: Use btrfs_find_item in calculate_emulated_zone_size

[Naohiro Aota]

On Wed, Aug 04, 2021 at 03:48:50PM -0300, Marcos Paulo de Souza wrote:
> calculate_emulated_zone_size can be simplified by using btrfs_find_item, which
> executes btrfs_search_slot and calls btrfs_next_leaf if needed.
> 
> No functional changes.
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  fs/btrfs/zoned.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 47af1ab3bf12..d344baa26de0 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -230,29 +230,20 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info)
>  	struct btrfs_key key;
>  	struct extent_buffer *leaf;
>  	struct btrfs_dev_extent *dext;
> -	int ret = 0;
> -
> -	key.objectid = 1;
> -	key.type = BTRFS_DEV_EXTENT_KEY;
> -	key.offset = 0;
> +	int ret;
>  
>  	path = btrfs_alloc_path();
>  	if (!path)
>  		return -ENOMEM;
>  
> -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> +	ret = btrfs_find_item(root, path, 1, BTRFS_DEV_EXTENT_KEY, 0, &key);
>  	if (ret < 0)
>  		goto out;
>  
> -	if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
> -		ret = btrfs_next_leaf(root, path);
> -		if (ret < 0)
> -			goto out;
> -		/* No dev extents at all? Not good */
> -		if (ret > 0) {
> -			ret = -EUCLEAN;
> -			goto out;
> -		}
> +	/* No dev extents at all? Not good */
> +	else if (ret > 0) {
> +		ret = -EUCLEAN;
> +		goto out;
>  	}

As I wrote in a reply to Qu, we want to find the minimal DEV_EXTENT
item here (somewhat like btrfs_verify_dev_extents()).
btrfs_find_item() returns 1 if the objectid is different. So, it cause
-EUCLEAN when a device with devid = 1 is removed.

>  
>  	leaf = path->nodes[0];
> -- 
> 2.31.1
> 

[PATCH 4/7] btrfs: root-tree: Use btrfs_find_item in btrfs_find_orphan_roots

[Marcos Paulo de Souza]

Prefer btrfs_find_item instead of btrfs_search_slot, since it calls
btrfs_next_leaf if needed and checks if the item found has the same
objectid and type passed in the search key.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/root-tree.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 702dc5441f03..4bb0ad192a2f 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -207,10 +207,10 @@ int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_root *tree_root = fs_info->tree_root;
-	struct extent_buffer *leaf;
 	struct btrfs_path *path;
 	struct btrfs_key key;
 	struct btrfs_root *root;
+	u64 offset = 0;
 	int err = 0;
 	int ret;
 
@@ -218,38 +218,22 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
 	if (!path)
 		return -ENOMEM;
 
-	key.objectid = BTRFS_ORPHAN_OBJECTID;
-	key.type = BTRFS_ORPHAN_ITEM_KEY;
-	key.offset = 0;
-
 	while (1) {
 		u64 root_objectid;
 
-		ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
+		ret = btrfs_find_item(tree_root, path, BTRFS_ORPHAN_OBJECTID,
+				BTRFS_ORPHAN_ITEM_KEY, offset, &key);
+
+		btrfs_release_path(path);
 		if (ret < 0) {
 			err = ret;
 			break;
-		}
-
-		leaf = path->nodes[0];
-		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
-			ret = btrfs_next_leaf(tree_root, path);
-			if (ret < 0)
-				err = ret;
-			if (ret != 0)
-				break;
-			leaf = path->nodes[0];
-		}
-
-		btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
-		btrfs_release_path(path);
-
-		if (key.objectid != BTRFS_ORPHAN_OBJECTID ||
-		    key.type != BTRFS_ORPHAN_ITEM_KEY)
+		} else if (ret > 0) {
 			break;
+		}
 
 		root_objectid = key.offset;
-		key.offset++;
+		offset++;
 
 		root = btrfs_get_fs_root(fs_info, root_objectid, false);
 		err = PTR_ERR_OR_ZERO(root);
-- 
2.31.1

Re: [PATCH 4/7] btrfs: root-tree: Use btrfs_find_item in btrfs_find_orphan_roots

[Qu Wenruo]

On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote:
> Prefer btrfs_find_item instead of btrfs_search_slot, since it calls
> btrfs_next_leaf if needed and checks if the item found has the same
> objectid and type passed in the search key.

This is fine, but IMHO btrfs_find_item() would be preferred to be
utilized when we know exactly the search key.

For orphan iteration, I guess we would really prefer something like
"btrfs_iterate_key_range()" or things like that?

Yeah, using btrfs_find_item() here is no harm, but it's also not that
typical to me.

Or you're going to introduce specific helper for this usage later?
Then I prefer to modify this call site when the iteration interface is
introduced.

Thanks,
Qu
>
> No functional changes.
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>   fs/btrfs/root-tree.c | 32 ++++++++------------------------
>   1 file changed, 8 insertions(+), 24 deletions(-)
>
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index 702dc5441f03..4bb0ad192a2f 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -207,10 +207,10 @@ int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>   int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>   {
>   	struct btrfs_root *tree_root = fs_info->tree_root;
> -	struct extent_buffer *leaf;
>   	struct btrfs_path *path;
>   	struct btrfs_key key;
>   	struct btrfs_root *root;
> +	u64 offset = 0;
>   	int err = 0;
>   	int ret;
>
> @@ -218,38 +218,22 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info)
>   	if (!path)
>   		return -ENOMEM;
>
> -	key.objectid = BTRFS_ORPHAN_OBJECTID;
> -	key.type = BTRFS_ORPHAN_ITEM_KEY;
> -	key.offset = 0;
> -
>   	while (1) {
>   		u64 root_objectid;
>
> -		ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
> +		ret = btrfs_find_item(tree_root, path, BTRFS_ORPHAN_OBJECTID,
> +				BTRFS_ORPHAN_ITEM_KEY, offset, &key);
> +
> +		btrfs_release_path(path);
>   		if (ret < 0) {
>   			err = ret;
>   			break;
> -		}
> -
> -		leaf = path->nodes[0];
> -		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
> -			ret = btrfs_next_leaf(tree_root, path);
> -			if (ret < 0)
> -				err = ret;
> -			if (ret != 0)
> -				break;
> -			leaf = path->nodes[0];
> -		}
> -
> -		btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
> -		btrfs_release_path(path);
> -
> -		if (key.objectid != BTRFS_ORPHAN_OBJECTID ||
> -		    key.type != BTRFS_ORPHAN_ITEM_KEY)
> +		} else if (ret > 0) {
>   			break;
> +		}
>
>   		root_objectid = key.offset;
> -		key.offset++;
> +		offset++;
>
>   		root = btrfs_get_fs_root(fs_info, root_objectid, false);
>   		err = PTR_ERR_OR_ZERO(root);
>

[PATCH 5/7] btrfs: scrub: Use btrfs_find_item in scrub_enumerate_chunks

[Marcos Paulo de Souza]

Prefer btrfs_find_item instead of btrfs_search_slot, since it calls
btrfs_next_leaf if needed and checks if the item found has the same
objectid and type passed in the search key.

As result, we can remove one btrfs_key from this function.

No functional changes.

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

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 088641ba7a8e..008eeb502267 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3657,11 +3657,10 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 	struct btrfs_root *root = fs_info->dev_root;
 	u64 length;
 	u64 chunk_offset;
+	u64 offset = 0;
 	int ret = 0;
 	int ro_set;
-	int slot;
 	struct extent_buffer *l;
-	struct btrfs_key key;
 	struct btrfs_key found_key;
 	struct btrfs_block_group *cache;
 	struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
@@ -3674,47 +3673,24 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 	path->search_commit_root = 1;
 	path->skip_locking = 1;
 
-	key.objectid = scrub_dev->devid;
-	key.offset = 0ull;
-	key.type = BTRFS_DEV_EXTENT_KEY;
-
 	while (1) {
-		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-		if (ret < 0)
-			break;
-		if (ret > 0) {
-			if (path->slots[0] >=
-			    btrfs_header_nritems(path->nodes[0])) {
-				ret = btrfs_next_leaf(root, path);
-				if (ret < 0)
-					break;
-				if (ret > 0) {
-					ret = 0;
-					break;
-				}
-			} else {
-				ret = 0;
-			}
-		}
-
-		l = path->nodes[0];
-		slot = path->slots[0];
-
-		btrfs_item_key_to_cpu(l, &found_key, slot);
-
-		if (found_key.objectid != scrub_dev->devid)
+		ret = btrfs_find_item(root, path, scrub_dev->devid,
+				BTRFS_DEV_EXTENT_KEY, offset, &found_key);
+		if (ret < 0) {
 			break;
-
-		if (found_key.type != BTRFS_DEV_EXTENT_KEY)
-			break;
-
-		if (found_key.offset >= end)
+		} else if (ret > 0) {
+			/* Reset error if not found. */
+			ret = 0;
 			break;
+		}
 
-		if (found_key.offset < key.offset)
+		if (found_key.offset >= end ||
+		    found_key.offset < offset)
 			break;
 
-		dev_extent = btrfs_item_ptr(l, slot, struct btrfs_dev_extent);
+		l = path->nodes[0];
+		dev_extent = btrfs_item_ptr(l, path->slots[0],
+						struct btrfs_dev_extent);
 		length = btrfs_dev_extent_length(l, dev_extent);
 
 		if (found_key.offset + length <= start)
@@ -3938,7 +3914,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 			break;
 		}
 skip:
-		key.offset = found_key.offset + length;
+		offset = found_key.offset + length;
 		btrfs_release_path(path);
 	}
 
-- 
2.31.1

[PATCH 6/7] btrfs: tree-log: Simplify log_new_ancestors

[Marcos Paulo de Souza]

The search_key variable was being used only as argument of
btrfs_search_slot. This can be simplified by calling btrfs_find_item,
which also handles the next leaf condition as well.

No functional changes.

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

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 567adc3de11a..22417cd32347 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -5929,31 +5929,30 @@ static int btrfs_log_all_parents(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+/*
+ * Iterate over the given and all it's parent directories, logging them if
+ * needed.
+ *
+ * Return 0 if we reach the toplevel directory, or < 0 if error.
+ */
 static int log_new_ancestors(struct btrfs_trans_handle *trans,
 			     struct btrfs_root *root,
 			     struct btrfs_path *path,
 			     struct btrfs_log_ctx *ctx)
 {
+	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_key found_key;
+	u64 ino;
 
 	btrfs_item_key_to_cpu(path->nodes[0], &found_key, path->slots[0]);
 
 	while (true) {
-		struct btrfs_fs_info *fs_info = root->fs_info;
-		struct extent_buffer *leaf = path->nodes[0];
-		int slot = path->slots[0];
-		struct btrfs_key search_key;
 		struct inode *inode;
-		u64 ino;
 		int ret = 0;
 
 		btrfs_release_path(path);
 
 		ino = found_key.offset;
-
-		search_key.objectid = found_key.offset;
-		search_key.type = BTRFS_INODE_ITEM_KEY;
-		search_key.offset = 0;
 		inode = btrfs_iget(fs_info->sb, ino, root);
 		if (IS_ERR(inode))
 			return PTR_ERR(inode);
@@ -5966,29 +5965,14 @@ static int log_new_ancestors(struct btrfs_trans_handle *trans,
 		if (ret)
 			return ret;
 
-		if (search_key.objectid == BTRFS_FIRST_FREE_OBJECTID)
+		if (ino == BTRFS_FIRST_FREE_OBJECTID)
 			break;
 
-		search_key.type = BTRFS_INODE_REF_KEY;
-		ret = btrfs_search_slot(NULL, root, &search_key, path, 0, 0);
+		ret = btrfs_find_item(root, path, ino, BTRFS_INODE_REF_KEY, 0,
+					&found_key);
 		if (ret < 0)
 			return ret;
-
-		leaf = path->nodes[0];
-		slot = path->slots[0];
-		if (slot >= btrfs_header_nritems(leaf)) {
-			ret = btrfs_next_leaf(root, path);
-			if (ret < 0)
-				return ret;
-			else if (ret > 0)
-				return -ENOENT;
-			leaf = path->nodes[0];
-			slot = path->slots[0];
-		}
-
-		btrfs_item_key_to_cpu(leaf, &found_key, slot);
-		if (found_key.objectid != search_key.objectid ||
-		    found_key.type != BTRFS_INODE_REF_KEY)
+		else if (ret > 0)
 			return -ENOENT;
 	}
 	return 0;
-- 
2.31.1

Re: [PATCH 6/7] btrfs: tree-log: Simplify log_new_ancestors

[Filipe Manana]

On Wed, Aug 4, 2021 at 10:07 PM Marcos Paulo de Souza
<mpdesouza@suse.com> wrote:
>
> The search_key variable was being used only as argument of
> btrfs_search_slot. This can be simplified by calling btrfs_find_item,
> which also handles the next leaf condition as well.
>
> No functional changes.
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  fs/btrfs/tree-log.c | 40 ++++++++++++----------------------------
>  1 file changed, 12 insertions(+), 28 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 567adc3de11a..22417cd32347 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -5929,31 +5929,30 @@ static int btrfs_log_all_parents(struct btrfs_trans_handle *trans,
>         return ret;
>  }
>
> +/*
> + * Iterate over the given and all it's parent directories, logging them if
> + * needed.
> + *
> + * Return 0 if we reach the toplevel directory, or < 0 if error.
> + */
>  static int log_new_ancestors(struct btrfs_trans_handle *trans,
>                              struct btrfs_root *root,
>                              struct btrfs_path *path,
>                              struct btrfs_log_ctx *ctx)
>  {
> +       struct btrfs_fs_info *fs_info = root->fs_info;
>         struct btrfs_key found_key;
> +       u64 ino;
>
>         btrfs_item_key_to_cpu(path->nodes[0], &found_key, path->slots[0]);
>
>         while (true) {
> -               struct btrfs_fs_info *fs_info = root->fs_info;
> -               struct extent_buffer *leaf = path->nodes[0];
> -               int slot = path->slots[0];
> -               struct btrfs_key search_key;
>                 struct inode *inode;
> -               u64 ino;

Why are the 'ino' and 'fs_info' declarations moved to the outer scope?
They are only used inside the while loop's scope, so I don't see a
reason to move them.

Thanks.

>                 int ret = 0;
>
>                 btrfs_release_path(path);
>
>                 ino = found_key.offset;
> -
> -               search_key.objectid = found_key.offset;
> -               search_key.type = BTRFS_INODE_ITEM_KEY;
> -               search_key.offset = 0;
>                 inode = btrfs_iget(fs_info->sb, ino, root);
>                 if (IS_ERR(inode))
>                         return PTR_ERR(inode);
> @@ -5966,29 +5965,14 @@ static int log_new_ancestors(struct btrfs_trans_handle *trans,
>                 if (ret)
>                         return ret;
>
> -               if (search_key.objectid == BTRFS_FIRST_FREE_OBJECTID)
> +               if (ino == BTRFS_FIRST_FREE_OBJECTID)
>                         break;
>
> -               search_key.type = BTRFS_INODE_REF_KEY;
> -               ret = btrfs_search_slot(NULL, root, &search_key, path, 0, 0);
> +               ret = btrfs_find_item(root, path, ino, BTRFS_INODE_REF_KEY, 0,
> +                                       &found_key);
>                 if (ret < 0)
>                         return ret;
> -
> -               leaf = path->nodes[0];
> -               slot = path->slots[0];
> -               if (slot >= btrfs_header_nritems(leaf)) {
> -                       ret = btrfs_next_leaf(root, path);
> -                       if (ret < 0)
> -                               return ret;
> -                       else if (ret > 0)
> -                               return -ENOENT;
> -                       leaf = path->nodes[0];
> -                       slot = path->slots[0];
> -               }
> -
> -               btrfs_item_key_to_cpu(leaf, &found_key, slot);
> -               if (found_key.objectid != search_key.objectid ||
> -                   found_key.type != BTRFS_INODE_REF_KEY)
> +               else if (ret > 0)
>                         return -ENOENT;
>         }
>         return 0;
> --
> 2.31.1
>


-- 
Filipe David Manana,

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

Re: [PATCH 6/7] btrfs: tree-log: Simplify log_new_ancestors

[Marcos Paulo de Souza]

On Thu, 2021-08-05 at 10:00 +0100, Filipe Manana wrote:
> On Wed, Aug 4, 2021 at 10:07 PM Marcos Paulo de Souza
> <mpdesouza@suse.com> wrote:
> > The search_key variable was being used only as argument of
> > btrfs_search_slot. This can be simplified by calling
> > btrfs_find_item,
> > which also handles the next leaf condition as well.
> > 
> > No functional changes.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >  fs/btrfs/tree-log.c | 40 ++++++++++++----------------------------
> >  1 file changed, 12 insertions(+), 28 deletions(-)
> > 
> > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> > index 567adc3de11a..22417cd32347 100644
> > --- a/fs/btrfs/tree-log.c
> > +++ b/fs/btrfs/tree-log.c
> > @@ -5929,31 +5929,30 @@ static int btrfs_log_all_parents(struct
> > btrfs_trans_handle *trans,
> >         return ret;
> >  }
> > 
> > +/*
> > + * Iterate over the given and all it's parent directories, logging
> > them if
> > + * needed.
> > + *
> > + * Return 0 if we reach the toplevel directory, or < 0 if error.
> > + */
> >  static int log_new_ancestors(struct btrfs_trans_handle *trans,
> >                              struct btrfs_root *root,
> >                              struct btrfs_path *path,
> >                              struct btrfs_log_ctx *ctx)
> >  {
> > +       struct btrfs_fs_info *fs_info = root->fs_info;
> >         struct btrfs_key found_key;
> > +       u64 ino;
> > 
> >         btrfs_item_key_to_cpu(path->nodes[0], &found_key, path-
> > >slots[0]);
> > 
> >         while (true) {
> > -               struct btrfs_fs_info *fs_info = root->fs_info;
> > -               struct extent_buffer *leaf = path->nodes[0];
> > -               int slot = path->slots[0];
> > -               struct btrfs_key search_key;
> >                 struct inode *inode;
> > -               u64 ino;
> 
> Why are the 'ino' and 'fs_info' declarations moved to the outer
> scope?
> They are only used inside the while loop's scope, so I don't see a
> reason to move them.

You're right, I will keep them in place for v2. Thanks!

> 
> Thanks.
> 
> >                 int ret = 0;
> > 
> >                 btrfs_release_path(path);
> > 
> >                 ino = found_key.offset;
> > -
> > -               search_key.objectid = found_key.offset;
> > -               search_key.type = BTRFS_INODE_ITEM_KEY;
> > -               search_key.offset = 0;
> >                 inode = btrfs_iget(fs_info->sb, ino, root);
> >                 if (IS_ERR(inode))
> >                         return PTR_ERR(inode);
> > @@ -5966,29 +5965,14 @@ static int log_new_ancestors(struct
> > btrfs_trans_handle *trans,
> >                 if (ret)
> >                         return ret;
> > 
> > -               if (search_key.objectid ==
> > BTRFS_FIRST_FREE_OBJECTID)
> > +               if (ino == BTRFS_FIRST_FREE_OBJECTID)
> >                         break;
> > 
> > -               search_key.type = BTRFS_INODE_REF_KEY;
> > -               ret = btrfs_search_slot(NULL, root, &search_key,
> > path, 0, 0);
> > +               ret = btrfs_find_item(root, path, ino,
> > BTRFS_INODE_REF_KEY, 0,
> > +                                       &found_key);
> >                 if (ret < 0)
> >                         return ret;
> > -
> > -               leaf = path->nodes[0];
> > -               slot = path->slots[0];
> > -               if (slot >= btrfs_header_nritems(leaf)) {
> > -                       ret = btrfs_next_leaf(root, path);
> > -                       if (ret < 0)
> > -                               return ret;
> > -                       else if (ret > 0)
> > -                               return -ENOENT;
> > -                       leaf = path->nodes[0];
> > -                       slot = path->slots[0];
> > -               }
> > -
> > -               btrfs_item_key_to_cpu(leaf, &found_key, slot);
> > -               if (found_key.objectid != search_key.objectid ||
> > -                   found_key.type != BTRFS_INODE_REF_KEY)
> > +               else if (ret > 0)
> >                         return -ENOENT;
> >         }
> >         return 0;
> > --
> > 2.31.1
> > 
> 
> 

[PATCH 7/7] btrfs: ioctl: Simplify btrfs_ioctl_get_subvol_info

[Marcos Paulo de Souza]

By using btrfs_find_item we can simplify the code. Also, remove the
-ENOENT error condition, since it'll never hit. If find_item returns 0,
it means it found the desired objectid and type, so it won't reach the -ENOENT
condition.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/ioctl.c | 56 +++++++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 34 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d09eaa83b5d2..2c57bea16c92 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2685,6 +2685,7 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 	unsigned long item_off;
 	unsigned long item_len;
 	struct inode *inode;
+	u64 treeid;
 	int slot;
 	int ret = 0;
 
@@ -2702,15 +2703,15 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 	fs_info = BTRFS_I(inode)->root->fs_info;
 
 	/* Get root_item of inode's subvolume */
-	key.objectid = BTRFS_I(inode)->root->root_key.objectid;
-	root = btrfs_get_fs_root(fs_info, key.objectid, true);
+	treeid = BTRFS_I(inode)->root->root_key.objectid;
+	root = btrfs_get_fs_root(fs_info, treeid, true);
 	if (IS_ERR(root)) {
 		ret = PTR_ERR(root);
 		goto out_free;
 	}
 	root_item = &root->root_item;
 
-	subvol_info->treeid = key.objectid;
+	subvol_info->treeid = treeid;
 
 	subvol_info->generation = btrfs_root_generation(root_item);
 	subvol_info->flags = btrfs_root_flags(root_item);
@@ -2737,44 +2738,31 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 	subvol_info->rtime.sec = btrfs_stack_timespec_sec(&root_item->rtime);
 	subvol_info->rtime.nsec = btrfs_stack_timespec_nsec(&root_item->rtime);
 
-	if (key.objectid != BTRFS_FS_TREE_OBJECTID) {
+	if (treeid != BTRFS_FS_TREE_OBJECTID) {
 		/* Search root tree for ROOT_BACKREF of this subvolume */
-		key.type = BTRFS_ROOT_BACKREF_KEY;
-		key.offset = 0;
-		ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
+		ret = btrfs_find_item(fs_info->tree_root, path, treeid,
+					BTRFS_ROOT_BACKREF_KEY, 0, &key);
 		if (ret < 0) {
 			goto out;
-		} else if (path->slots[0] >=
-			   btrfs_header_nritems(path->nodes[0])) {
-			ret = btrfs_next_leaf(fs_info->tree_root, path);
-			if (ret < 0) {
-				goto out;
-			} else if (ret > 0) {
-				ret = -EUCLEAN;
-				goto out;
-			}
+		} else if (ret > 0) {
+			ret = -EUCLEAN;
+			goto out;
 		}
 
 		leaf = path->nodes[0];
 		slot = path->slots[0];
-		btrfs_item_key_to_cpu(leaf, &key, slot);
-		if (key.objectid == subvol_info->treeid &&
-		    key.type == BTRFS_ROOT_BACKREF_KEY) {
-			subvol_info->parent_id = key.offset;
-
-			rref = btrfs_item_ptr(leaf, slot, struct btrfs_root_ref);
-			subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
-
-			item_off = btrfs_item_ptr_offset(leaf, slot)
-					+ sizeof(struct btrfs_root_ref);
-			item_len = btrfs_item_size_nr(leaf, slot)
-					- sizeof(struct btrfs_root_ref);
-			read_extent_buffer(leaf, subvol_info->name,
-					   item_off, item_len);
-		} else {
-			ret = -ENOENT;
-			goto out;
-		}
+
+		subvol_info->parent_id = key.offset;
+
+		rref = btrfs_item_ptr(leaf, slot, struct btrfs_root_ref);
+		subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
+
+		item_off = btrfs_item_ptr_offset(leaf, slot)
+				+ sizeof(struct btrfs_root_ref);
+		item_len = btrfs_item_size_nr(leaf, slot)
+				- sizeof(struct btrfs_root_ref);
+		read_extent_buffer(leaf, subvol_info->name,
+				   item_off, item_len);
 	}
 
 	if (copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
-- 
2.31.1

Re: [PATCH 7/7] btrfs: ioctl: Simplify btrfs_ioctl_get_subvol_info

[Anand Jain]

On 05/08/2021 02:48, Marcos Paulo de Souza wrote:
> By using btrfs_find_item we can simplify the code.

  Yep. I like the idea.

> Also, remove the
> -ENOENT error condition, since it'll never hit. If find_item returns 0,
> it means it found the desired objectid and type, so it won't reach the -ENOENT
> condition.
> 
> No functional changes.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

  Looks good to me.

  Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> ---
>   fs/btrfs/ioctl.c | 56 +++++++++++++++++++-----------------------------
>   1 file changed, 22 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index d09eaa83b5d2..2c57bea16c92 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2685,6 +2685,7 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   	unsigned long item_off;
>   	unsigned long item_len;
>   	struct inode *inode;
> +	u64 treeid;
>   	int slot;
>   	int ret = 0;
>   
> @@ -2702,15 +2703,15 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   	fs_info = BTRFS_I(inode)->root->fs_info;
>   
>   	/* Get root_item of inode's subvolume */
> -	key.objectid = BTRFS_I(inode)->root->root_key.objectid;
> -	root = btrfs_get_fs_root(fs_info, key.objectid, true);
> +	treeid = BTRFS_I(inode)->root->root_key.objectid;
> +	root = btrfs_get_fs_root(fs_info, treeid, true);
>   	if (IS_ERR(root)) {
>   		ret = PTR_ERR(root);
>   		goto out_free;
>   	}
>   	root_item = &root->root_item;
>   
> -	subvol_info->treeid = key.objectid;
> +	subvol_info->treeid = treeid;
>   
>   	subvol_info->generation = btrfs_root_generation(root_item);
>   	subvol_info->flags = btrfs_root_flags(root_item);
> @@ -2737,44 +2738,31 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   	subvol_info->rtime.sec = btrfs_stack_timespec_sec(&root_item->rtime);
>   	subvol_info->rtime.nsec = btrfs_stack_timespec_nsec(&root_item->rtime);
>   
> -	if (key.objectid != BTRFS_FS_TREE_OBJECTID) {
> +	if (treeid != BTRFS_FS_TREE_OBJECTID) {
>   		/* Search root tree for ROOT_BACKREF of this subvolume */
> -		key.type = BTRFS_ROOT_BACKREF_KEY;
> -		key.offset = 0;
> -		ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
> +		ret = btrfs_find_item(fs_info->tree_root, path, treeid,
> +					BTRFS_ROOT_BACKREF_KEY, 0, &key);
>   		if (ret < 0) {
>   			goto out;
> -		} else if (path->slots[0] >=
> -			   btrfs_header_nritems(path->nodes[0])) {
> -			ret = btrfs_next_leaf(fs_info->tree_root, path);
> -			if (ret < 0) {
> -				goto out;
> -			} else if (ret > 0) {
> -				ret = -EUCLEAN;
> -				goto out;
> -			}
> +		} else if (ret > 0) {
> +			ret = -EUCLEAN;
> +			goto out;
>   		}
>   
>   		leaf = path->nodes[0];
>   		slot = path->slots[0];
> -		btrfs_item_key_to_cpu(leaf, &key, slot);
> -		if (key.objectid == subvol_info->treeid &&
> -		    key.type == BTRFS_ROOT_BACKREF_KEY) {
> -			subvol_info->parent_id = key.offset;
> -
> -			rref = btrfs_item_ptr(leaf, slot, struct btrfs_root_ref);
> -			subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
> -
> -			item_off = btrfs_item_ptr_offset(leaf, slot)
> -					+ sizeof(struct btrfs_root_ref);
> -			item_len = btrfs_item_size_nr(leaf, slot)
> -					- sizeof(struct btrfs_root_ref);
> -			read_extent_buffer(leaf, subvol_info->name,
> -					   item_off, item_len);
> -		} else {
> -			ret = -ENOENT;
> -			goto out;
> -		}
> +
> +		subvol_info->parent_id = key.offset;
> +
> +		rref = btrfs_item_ptr(leaf, slot, struct btrfs_root_ref);
> +		subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
> +
> +		item_off = btrfs_item_ptr_offset(leaf, slot)
> +				+ sizeof(struct btrfs_root_ref);
> +		item_len = btrfs_item_size_nr(leaf, slot)
> +				- sizeof(struct btrfs_root_ref);
> +		read_extent_buffer(leaf, subvol_info->name,
> +				   item_off, item_len);
>   	}
>   
>   	if (copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
> 

Re: [PATCH 7/7] btrfs: ioctl: Simplify btrfs_ioctl_get_subvol_info

[Marcos Paulo de Souza]

On Thu, 2021-08-05 at 20:13 +0800, Anand Jain wrote:
> On 05/08/2021 02:48, Marcos Paulo de Souza wrote:
> > By using btrfs_find_item we can simplify the code.
> 
>   Yep. I like the idea.
> 
> > Also, remove the
> > -ENOENT error condition, since it'll never hit. If find_item
> > returns 0,
> > it means it found the desired objectid and type, so it won't reach
> > the -ENOENT
> > condition.
> > 
> > No functional changes.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
>   Looks good to me.
> 
>   Reviewed-by: Anand Jain <anand.jain@oracle.com>

Sorry, I believe that I did a mistake here.

See bellow.

> 
> Thanks, Anand
> 
> > ---
> >   fs/btrfs/ioctl.c | 56 +++++++++++++++++++----------------------
> > -------
> >   1 file changed, 22 insertions(+), 34 deletions(-)
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index d09eaa83b5d2..2c57bea16c92 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -2685,6 +2685,7 @@ static int btrfs_ioctl_get_subvol_info(struct
> > file *file, void __user *argp)
> >   	unsigned long item_off;
> >   	unsigned long item_len;
> >   	struct inode *inode;
> > +	u64 treeid;
> >   	int slot;
> >   	int ret = 0;
> >   
> > @@ -2702,15 +2703,15 @@ static int
> > btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
> >   	fs_info = BTRFS_I(inode)->root->fs_info;
> >   
> >   	/* Get root_item of inode's subvolume */
> > -	key.objectid = BTRFS_I(inode)->root->root_key.objectid;
> > -	root = btrfs_get_fs_root(fs_info, key.objectid, true);
> > +	treeid = BTRFS_I(inode)->root->root_key.objectid;
> > +	root = btrfs_get_fs_root(fs_info, treeid, true);
> >   	if (IS_ERR(root)) {
> >   		ret = PTR_ERR(root);
> >   		goto out_free;
> >   	}
> >   	root_item = &root->root_item;
> >   
> > -	subvol_info->treeid = key.objectid;
> > +	subvol_info->treeid = treeid;
> >   
> >   	subvol_info->generation = btrfs_root_generation(root_item);
> >   	subvol_info->flags = btrfs_root_flags(root_item);
> > @@ -2737,44 +2738,31 @@ static int
> > btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
> >   	subvol_info->rtime.sec = btrfs_stack_timespec_sec(&root_item-
> > >rtime);
> >   	subvol_info->rtime.nsec = btrfs_stack_timespec_nsec(&root_item-
> > >rtime);
> >   
> > -	if (key.objectid != BTRFS_FS_TREE_OBJECTID) {
> > +	if (treeid != BTRFS_FS_TREE_OBJECTID) {
> >   		/* Search root tree for ROOT_BACKREF of this subvolume
> > */
> > -		key.type = BTRFS_ROOT_BACKREF_KEY;
> > -		key.offset = 0;
> > -		ret = btrfs_search_slot(NULL, fs_info->tree_root, &key,
> > path, 0, 0);
> > +		ret = btrfs_find_item(fs_info->tree_root, path, treeid,
> > +					BTRFS_ROOT_BACKREF_KEY, 0,
> > &key);
> >   		if (ret < 0) {
> >   			goto out;
> > -		} else if (path->slots[0] >=
> > -			   btrfs_header_nritems(path->nodes[0])) {
> > -			ret = btrfs_next_leaf(fs_info->tree_root,
> > path);
> > -			if (ret < 0) {
> > -				goto out;
> > -			} else if (ret > 0) {
> > -				ret = -EUCLEAN;
> > -				goto out;
> > -			}

btrfs_next_leaf returns > 0 if it there aren't any other leaf. So, in
this case, it can find another leaf.

> > +		} else if (ret > 0) {
> > +			ret = -EUCLEAN;
> > +			goto out;
> >   		}

This is wrong, since btrfs_find_item can return 1 is next_leaf returned
something different OR if the objectid or type aren't the same.

In this case, the -ENOENT path can be reached, since there can be more
leaves but with different objectid and type.

In this case, with the following change, we would report such situation
wrongly as -EUCLEAN.

I'll rework this patch in the v2 of this patchset.

> >   
> >   		leaf = path->nodes[0];
> >   		slot = path->slots[0];
> > -		btrfs_item_key_to_cpu(leaf, &key, slot);
> > -		if (key.objectid == subvol_info->treeid &&
> > -		    key.type == BTRFS_ROOT_BACKREF_KEY) {
> > -			subvol_info->parent_id = key.offset;
> > -
> > -			rref = btrfs_item_ptr(leaf, slot, struct
> > btrfs_root_ref);
> > -			subvol_info->dirid = btrfs_root_ref_dirid(leaf,
> > rref);
> > -
> > -			item_off = btrfs_item_ptr_offset(leaf, slot)
> > -					+ sizeof(struct
> > btrfs_root_ref);
> > -			item_len = btrfs_item_size_nr(leaf, slot)
> > -					- sizeof(struct
> > btrfs_root_ref);
> > -			read_extent_buffer(leaf, subvol_info->name,
> > -					   item_off, item_len);
> > -		} else {
> > -			ret = -ENOENT;
> > -			goto out;
> > -		}
> > +
> > +		subvol_info->parent_id = key.offset;
> > +
> > +		rref = btrfs_item_ptr(leaf, slot, struct
> > btrfs_root_ref);
> > +		subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
> > +
> > +		item_off = btrfs_item_ptr_offset(leaf, slot)
> > +				+ sizeof(struct btrfs_root_ref);
> > +		item_len = btrfs_item_size_nr(leaf, slot)
> > +				- sizeof(struct btrfs_root_ref);
> > +		read_extent_buffer(leaf, subvol_info->name,
> > +				   item_off, item_len);
> >   	}
> >   
> >   	if (copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
> >