[PATCH 0/3] 3 small cleanups for find_first_block_group

[PATCH 0/3] 3 small cleanups for find_first_block_group

[Johannes Thumshirn]

While trying to learn the Block Group code I've found some cleanup
possibilities for find_first_block_group().

Here's a proposal to make $ffbg a bit more easier to read by untangling the
gotos and if statements.

The patch set is based on misc-next from May 26 morning with 
HEAD 3f4a266717ed ("btrfs: split btrfs_direct_IO to read and write part")
and xfstests showed no regressions to the base misc-next in my test setup.

Johannes Thumshirn (3):
  btrfs: remove pointless out label in find_first_block_group
  btrfs: get mapping tree directly from fsinfo in find_first_block_group
  btrfs: factor out reading of bg from find_frist_block_group

 fs/btrfs/block-group.c | 103 +++++++++++++++++++++--------------------
 1 file changed, 53 insertions(+), 50 deletions(-)

-- 
2.24.1

[PATCH 1/3] btrfs: remove pointless out label in find_first_block_group

[Johannes Thumshirn]

The 'out' label in find_first_block_group() does not do anything in terms
of cleanup.

It is better to directly return 'ret' instead of jumping to out to not
confuse readers. Additionally there is no need to initialize ret with 0.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/block-group.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 176e8a292fd1..c5acd89c864c 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1527,7 +1527,7 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info,
 				  struct btrfs_key *key)
 {
 	struct btrfs_root *root = fs_info->extent_root;
-	int ret = 0;
+	int ret;
 	struct btrfs_key found_key;
 	struct extent_buffer *leaf;
 	struct btrfs_block_group_item bg;
@@ -1536,7 +1536,7 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info,
 
 	ret = btrfs_search_slot(NULL, root, key, path, 0, 0);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	while (1) {
 		slot = path->slots[0];
@@ -1546,7 +1546,7 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info,
 			if (ret == 0)
 				continue;
 			if (ret < 0)
-				goto out;
+				return ret;
 			break;
 		}
 		btrfs_item_key_to_cpu(leaf, &found_key, slot);
@@ -1594,11 +1594,11 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info,
 				}
 			}
 			free_extent_map(em);
-			goto out;
+			return ret;
 		}
 		path->slots[0]++;
 	}
-out:
+
 	return ret;
 }
 
-- 
2.24.1

[PATCH 2/3] btrfs: get mapping tree directly from fsinfo in find_first_block_group

[Johannes Thumshirn]

We already have an fs_info in our function parameters, there's no need to
do the maths again and get fs_info from the extent_root just to get the
mapping_tree.

Instead directly grab the mapping_tree from fs_info.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/block-group.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index c5acd89c864c..c4462e4c8413 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1556,7 +1556,7 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info,
 			struct extent_map_tree *em_tree;
 			struct extent_map *em;
 
-			em_tree = &root->fs_info->mapping_tree;
+			em_tree = &fs_info->mapping_tree;
 			read_lock(&em_tree->lock);
 			em = lookup_extent_mapping(em_tree, found_key.objectid,
 						   found_key.offset);
-- 
2.24.1

[PATCH 3/3] btrfs: factor out reading of bg from find_frist_block_group

[Johannes Thumshirn]

When find_first_block_group() finds a block group item in the extent-tree,
it does a lookup of the object in the extent mapping tree and does further
checks on the item.

Factor out this step from find_first_block_group() so we can further
simplify the code.

As a bonus we even get a slight decrease in size:
$ ./scripts/bloat-o-meter btrfs_old.ko btrfs.ko
add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-2503 (-2503)
Function                                     old     new   delta
btrfs_read_block_groups.cold                 462     337    -125
btrfs_read_block_groups                     4787    2409   -2378
Total: Before=2369371, After=2366868, chg -0.11%

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/block-group.c | 95 ++++++++++++++++++++++--------------------
 1 file changed, 49 insertions(+), 46 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index c4462e4c8413..3d9e0ee1d1be 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1522,6 +1522,52 @@ void btrfs_mark_bg_unused(struct btrfs_block_group *bg)
 	spin_unlock(&fs_info->unused_bgs_lock);
 }
 
+static int read_bg_from_eb(struct btrfs_fs_info *fs_info, struct btrfs_key *key,
+			   struct extent_buffer *leaf, int slot)
+{
+	struct extent_map_tree *em_tree;
+	struct extent_map *em;
+	struct btrfs_block_group_item bg;
+	u64 flags;
+	int ret = 0;
+
+	em_tree = &fs_info->mapping_tree;
+	read_lock(&em_tree->lock);
+	em = lookup_extent_mapping(em_tree, key->objectid, key->offset);
+	read_unlock(&em_tree->lock);
+	if (!em) {
+		btrfs_err(fs_info,
+			  "logical %llu len %llu found bg but no related chunk",
+			  key->objectid, key->offset);
+		return -ENOENT;
+	}
+
+	if (em->start != key->objectid || em->len != key->offset) {
+		btrfs_err(fs_info,
+			  "block group %llu len %llu mismatch with chunk %llu len %llu",
+			  key->objectid, key->offset, em->start, em->len);
+		ret = -EUCLEAN;
+		goto out_free_em;
+	}
+
+	read_extent_buffer(leaf, &bg, btrfs_item_ptr_offset(leaf, slot),
+			   sizeof(bg));
+	flags = btrfs_stack_block_group_flags(&bg) &
+		BTRFS_BLOCK_GROUP_TYPE_MASK;
+
+	if (flags != (em->map_lookup->type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
+		btrfs_err(fs_info,
+			  "block group %llu len %llu type flags 0x%llx mismatch with chunk type flags 0x%llx",
+			  key->objectid, key->offset, flags,
+			  (BTRFS_BLOCK_GROUP_TYPE_MASK & em->map_lookup->type));
+		ret = -EUCLEAN;
+	}
+
+out_free_em:
+	free_extent_map(em);
+	return ret;
+}
+
 static int find_first_block_group(struct btrfs_fs_info *fs_info,
 				  struct btrfs_path *path,
 				  struct btrfs_key *key)
@@ -1530,8 +1576,6 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info,
 	int ret;
 	struct btrfs_key found_key;
 	struct extent_buffer *leaf;
-	struct btrfs_block_group_item bg;
-	u64 flags;
 	int slot;
 
 	ret = btrfs_search_slot(NULL, root, key, path, 0, 0);
@@ -1552,50 +1596,9 @@ static int find_first_block_group(struct btrfs_fs_info *fs_info,
 		btrfs_item_key_to_cpu(leaf, &found_key, slot);
 
 		if (found_key.objectid >= key->objectid &&
-		    found_key.type == BTRFS_BLOCK_GROUP_ITEM_KEY) {
-			struct extent_map_tree *em_tree;
-			struct extent_map *em;
-
-			em_tree = &fs_info->mapping_tree;
-			read_lock(&em_tree->lock);
-			em = lookup_extent_mapping(em_tree, found_key.objectid,
-						   found_key.offset);
-			read_unlock(&em_tree->lock);
-			if (!em) {
-				btrfs_err(fs_info,
-			"logical %llu len %llu found bg but no related chunk",
-					  found_key.objectid, found_key.offset);
-				ret = -ENOENT;
-			} else if (em->start != found_key.objectid ||
-				   em->len != found_key.offset) {
-				btrfs_err(fs_info,
-		"block group %llu len %llu mismatch with chunk %llu len %llu",
-					  found_key.objectid, found_key.offset,
-					  em->start, em->len);
-				ret = -EUCLEAN;
-			} else {
-				read_extent_buffer(leaf, &bg,
-					btrfs_item_ptr_offset(leaf, slot),
-					sizeof(bg));
-				flags = btrfs_stack_block_group_flags(&bg) &
-					BTRFS_BLOCK_GROUP_TYPE_MASK;
-
-				if (flags != (em->map_lookup->type &
-					      BTRFS_BLOCK_GROUP_TYPE_MASK)) {
-					btrfs_err(fs_info,
-"block group %llu len %llu type flags 0x%llx mismatch with chunk type flags 0x%llx",
-						found_key.objectid,
-						found_key.offset, flags,
-						(BTRFS_BLOCK_GROUP_TYPE_MASK &
-						 em->map_lookup->type));
-					ret = -EUCLEAN;
-				} else {
-					ret = 0;
-				}
-			}
-			free_extent_map(em);
-			return ret;
-		}
+		    found_key.type == BTRFS_BLOCK_GROUP_ITEM_KEY)
+			return read_bg_from_eb(fs_info, &found_key, leaf, slot);
+
 		path->slots[0]++;
 	}
 
-- 
2.24.1

Re: [PATCH 1/3] btrfs: remove pointless out label in find_first_block_group

[Nikolay Borisov]

On 26.05.20 г. 17:21 ч., Johannes Thumshirn wrote:
> The 'out' label in find_first_block_group() does not do anything in terms
> of cleanup.
> 
> It is better to directly return 'ret' instead of jumping to out to not
> confuse readers. Additionally there is no need to initialize ret with 0.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

I personally prefer returning fast aka the way you've done it but dunno
if David is a fan of this. In any case:


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

Re: [PATCH 2/3] btrfs: get mapping tree directly from fsinfo in find_first_block_group

[Nikolay Borisov]

On 26.05.20 г. 17:21 ч., Johannes Thumshirn wrote:
> We already have an fs_info in our function parameters, there's no need to
> do the maths again and get fs_info from the extent_root just to get the
> mapping_tree.
> 
> Instead directly grab the mapping_tree from fs_info.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

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

Re: [PATCH 3/3] btrfs: factor out reading of bg from find_frist_block_group

[Nikolay Borisov]

On 26.05.20 г. 17:21 ч., Johannes Thumshirn wrote:
> When find_first_block_group() finds a block group item in the extent-tree,
> it does a lookup of the object in the extent mapping tree and does further
> checks on the item.
> 
> Factor out this step from find_first_block_group() so we can further
> simplify the code.
> 
> As a bonus we even get a slight decrease in size:
> $ ./scripts/bloat-o-meter btrfs_old.ko btrfs.ko
> add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-2503 (-2503)
> Function                                     old     new   delta
> btrfs_read_block_groups.cold                 462     337    -125
> btrfs_read_block_groups                     4787    2409   -2378
> Total: Before=2369371, After=2366868, chg -0.11%

Always be careful of such results since this is likely due to less
inlining. The bulk of the size overhead is likely in the new function
and now those have been replaced by a 'call' and a function prologue
etc. So this doesn't always mean better performance is all I'm trying to
say ;)

> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

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

Although look down for one minor discussion point.

> ---
>  fs/btrfs/block-group.c | 95 ++++++++++++++++++++++--------------------
>  1 file changed, 49 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index c4462e4c8413..3d9e0ee1d1be 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1522,6 +1522,52 @@ void btrfs_mark_bg_unused(struct btrfs_block_group *bg)
>  	spin_unlock(&fs_info->unused_bgs_lock);
>  }
>  
> +static int read_bg_from_eb(struct btrfs_fs_info *fs_info, struct btrfs_key *key,
> +			   struct extent_buffer *leaf, int slot)

nit: I wonder if instead of passing leaf/slot it'll be better to pass
btrfs_path, since that function always refers to nodes/slots [0]. My gut
feeling is this provides better interface abstraction, but this is
effectively a private, function so I might be overthinking it.

<snip>

Re: [PATCH 3/3] btrfs: factor out reading of bg from find_frist_block_group

[Johannes Thumshirn]

On 26/05/2020 17:33, Nikolay Borisov wrote:
[...]
> Always be careful of such results since this is likely due to less
> inlining. The bulk of the size overhead is likely in the new function
> and now those have been replaced by a 'call' and a function prologue
> etc. So this doesn't always mean better performance is all I'm trying to
> say ;)

Yep I know, I've just included it as a small "bonus" but my point was the 
increased readability.


>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> Although look down for one minor discussion point.

[...]

>> +static int read_bg_from_eb(struct btrfs_fs_info *fs_info, struct btrfs_key *key,
>> +			   struct extent_buffer *leaf, int slot)
> 
> nit: I wonder if instead of passing leaf/slot it'll be better to pass
> btrfs_path, since that function always refers to nodes/slots [0]. My gut
> feeling is this provides better interface abstraction, but this is
> effectively a private, function so I might be overthinking it.

Agreed, I'll add it in a v2.

Re: [PATCH 1/3] btrfs: remove pointless out label in find_first_block_group

[David Sterba]

On Tue, May 26, 2020 at 06:24:49PM +0300, Nikolay Borisov wrote:
> 
> 
> On 26.05.20 г. 17:21 ч., Johannes Thumshirn wrote:
> > The 'out' label in find_first_block_group() does not do anything in terms
> > of cleanup.
> > 
> > It is better to directly return 'ret' instead of jumping to out to not
> > confuse readers. Additionally there is no need to initialize ret with 0.
> > 
> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> I personally prefer returning fast aka the way you've done it but dunno
> if David is a fan of this. In any case:

I'm not and the pattern that should be used is a mix of both. The first
part is for the 'obvious' cases:

Early exit from function can use return, eg. when a feature is not
enabled, when there's no cleanup needed, when the return is inside an if
and is not nested

For the rest it's recommended to use the goto and single return as it
may be a big chunk of code with a lot of nesting and a return somewhere
in the middle reads harder.

In example of find_first_block_group the first 'goto out' after
btrfs_search_slot would be a candidate to convert to return, but the
rest is inside a while loop so goto is preferred.

It is also important to keep one style and switch to it eventually and I
think that the goto + single return is quite common nowadays, exceptions
exist in the old code.