[PATCH v2 0/3] 3 small cleanups for find_first_block_group

[PATCH v2 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.

Changes to v1:
- Pass btrfs_path instead of leaf & slot to read_bg_from_eb (Nikolay)
- Don't comment about the size change (Nikolay)
- Add Nikolay's Reviewed-by's

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 | 108 ++++++++++++++++++++++-------------------
 1 file changed, 58 insertions(+), 50 deletions(-)

-- 
2.24.1

[PATCH v2 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>
Reviewed-by: Nikolay Borisov <nborisov@suse.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

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

[David Sterba]

On Wed, May 27, 2020 at 05:12:25PM +0900, 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.

https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions

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

[Johannes Thumshirn]

On 27/05/2020 15:07, David Sterba wrote:
> On Wed, May 27, 2020 at 05:12:25PM +0900, 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.
> 
> https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions
> 

To cite the Link you posted:

"The goto statement comes in handy when a function exits from multiple 
locations and some common work such as cleanup has to be done. If there 
is no cleanup needed then just return directly."

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

[David Sterba]

On Wed, May 27, 2020 at 01:45:47PM +0000, Johannes Thumshirn wrote:
> On 27/05/2020 15:07, David Sterba wrote:
> > On Wed, May 27, 2020 at 05:12:25PM +0900, 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.
> > 
> > https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions
> > 
> 
> To cite the Link you posted:
> 
> "The goto statement comes in handy when a function exits from multiple 
> locations and some common work such as cleanup has to be done. If there 
> is no cleanup needed then just return directly."

The preference in btrfs code has been to avoid returns from the middle
of while loops. The kernel coding style does not cover everything, we've
settled on some style in btrfs and that's what I'm arguing for and
fixing up in many patches so it's consistent.

[PATCH v2 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>
Reviewed-by: Nikolay Borisov <nborisov@suse.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 v2 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.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/block-group.c | 100 ++++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 46 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index c4462e4c8413..c2cbdbca4d47 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1522,6 +1522,57 @@ 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 btrfs_path *path)
+{
+	struct extent_map_tree *em_tree;
+	struct extent_map *em;
+	struct btrfs_block_group_item bg;
+	struct extent_buffer *leaf;
+	int slot;
+	u64 flags;
+	int ret = 0;
+
+	slot = path->slots[0];
+	leaf = path->nodes[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 +1581,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 +1601,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, path);
+
 		path->slots[0]++;
 	}
 
-- 
2.24.1

Re: [PATCH v2 0/3] 3 small cleanups for find_first_block_group

[Johannes Thumshirn]

On 27/05/2020 10:12, Johannes Thumshirn wrote:
> 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.
> 
> Changes to v1:
> - Pass btrfs_path instead of leaf & slot to read_bg_from_eb (Nikolay)
> - Don't comment about the size change (Nikolay)
> - Add Nikolay's Reviewed-by's
> 
> 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 | 108 ++++++++++++++++++++++-------------------
>  1 file changed, 58 insertions(+), 50 deletions(-)
> 

David, any comment if you will consider this series? Patch 2/3 actually makes 
the function more complainant to the kernel's coding style.

Re: [PATCH v2 0/3] 3 small cleanups for find_first_block_group

[David Sterba]

On Fri, May 29, 2020 at 02:25:32PM +0000, Johannes Thumshirn wrote:
> On 27/05/2020 10:12, Johannes Thumshirn wrote:
> > 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.
> > 
> > Changes to v1:
> > - Pass btrfs_path instead of leaf & slot to read_bg_from_eb (Nikolay)
> > - Don't comment about the size change (Nikolay)
> > - Add Nikolay's Reviewed-by's
> > 
> > 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 | 108 ++++++++++++++++++++++-------------------
> >  1 file changed, 58 insertions(+), 50 deletions(-)
> 
> David, any comment if you will consider this series? Patch 2/3 actually makes 
> the function more complainant to the kernel's coding style.

2/3 and 3/3 are good cleanups, please resend, thanks.