[PATCH] btrfs: Refactor unlock_up

[PATCH] btrfs: Refactor unlock_up

[Nikolay Borisov]

The purpose of this function is to unlock all nodes in a btrfs path
which are above 'lowest_unlock' and whose slot used is different than 0.
As such it used slightly awkward structure of 'if' as well as somewhat
cryptic "no_skip" control variable which denotes whether we should
check the current level of skipiability or no.

This patch does the following (cosmetic) refactorings:

* Renames 'no_skip' to 'check_skip' and makes it a boolean. This
variable controls whether we are below the lowest_unlock/skip_level
levels.

* Consolidates the 2 conditions which warrant checking whether the
current level should be skipped under 1 common if (check_skip) branch,
this increase indentation level but is not critical.

* Consolidates the 'skip_level < i && i >= lowest_unlock' and
'i >= lowest_unlock && i > skip_level' condition into a common branch
since those are identical.

* Eliminates the local extent_buffer variable as in this case it doesn't
bring anything to function readability.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 62066c034363..ab2ea0b2863c 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1348,33 +1348,34 @@ static noinline void unlock_up(struct btrfs_path *path, int level,
 {
 	int i;
 	int skip_level = level;
-	int no_skips = 0;
-	struct extent_buffer *t;
+	int check_skip = true;
 
 	for (i = level; i < BTRFS_MAX_LEVEL; i++) {
 		if (!path->nodes[i])
 			break;
 		if (!path->locks[i])
 			break;
-		if (!no_skips && path->slots[i] == 0) {
-			skip_level = i + 1;
-			continue;
-		}
-		if (!no_skips && path->keep_locks) {
-			u32 nritems;
-			t = path->nodes[i];
-			nritems = btrfs_header_nritems(t);
-			if (nritems < 1 || path->slots[i] >= nritems - 1) {
+
+		if (check_skip) {
+			if (path->slots[i] == 0) {
 				skip_level = i + 1;
 				continue;
 			}
+
+			if (path->keep_locks) {
+				u32 nritems;
+
+				nritems = btrfs_header_nritems(path->nodes[i]);
+				if (nritems < 1 || path->slots[i] >= nritems - 1) {
+					skip_level = i + 1;
+					continue;
+				}
+			}
 		}
-		if (skip_level < i && i >= lowest_unlock)
-			no_skips = 1;
 
-		t = path->nodes[i];
 		if (i >= lowest_unlock && i > skip_level) {
-			btrfs_tree_unlock_rw(t, path->locks[i]);
+			check_skip = false;
+			btrfs_tree_unlock_rw(path->nodes[i], path->locks[i]);
 			path->locks[i] = 0;
 			if (write_lock_level &&
 			    i > min_write_lock_level &&
-- 
2.25.1

Re: [PATCH] btrfs: Refactor unlock_up

[Josef Bacik]

On Tue, Dec 14, 2021 at 03:39:39PM +0200, Nikolay Borisov wrote:
> The purpose of this function is to unlock all nodes in a btrfs path
> which are above 'lowest_unlock' and whose slot used is different than 0.
> As such it used slightly awkward structure of 'if' as well as somewhat
> cryptic "no_skip" control variable which denotes whether we should
> check the current level of skipiability or no.
> 
> This patch does the following (cosmetic) refactorings:
> 
> * Renames 'no_skip' to 'check_skip' and makes it a boolean. This
> variable controls whether we are below the lowest_unlock/skip_level
> levels.
> 
> * Consolidates the 2 conditions which warrant checking whether the
> current level should be skipped under 1 common if (check_skip) branch,
> this increase indentation level but is not critical.
> 
> * Consolidates the 'skip_level < i && i >= lowest_unlock' and
> 'i >= lowest_unlock && i > skip_level' condition into a common branch
> since those are identical.
> 
> * Eliminates the local extent_buffer variable as in this case it doesn't
> bring anything to function readability.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

This was weirdly difficult to review in both diff and vimdiff, had to look at
the resulting code to see how it worked out.

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH] btrfs: Refactor unlock_up

[Nikolay Borisov]

On 14.12.21 г. 16:45, Josef Bacik wrote:
> On Tue, Dec 14, 2021 at 03:39:39PM +0200, Nikolay Borisov wrote:
>> The purpose of this function is to unlock all nodes in a btrfs path
>> which are above 'lowest_unlock' and whose slot used is different than 0.
>> As such it used slightly awkward structure of 'if' as well as somewhat
>> cryptic "no_skip" control variable which denotes whether we should
>> check the current level of skipiability or no.
>>
>> This patch does the following (cosmetic) refactorings:
>>
>> * Renames 'no_skip' to 'check_skip' and makes it a boolean. This
>> variable controls whether we are below the lowest_unlock/skip_level
>> levels.
>>
>> * Consolidates the 2 conditions which warrant checking whether the
>> current level should be skipped under 1 common if (check_skip) branch,
>> this increase indentation level but is not critical.
>>
>> * Consolidates the 'skip_level < i && i >= lowest_unlock' and
>> 'i >= lowest_unlock && i > skip_level' condition into a common branch
>> since those are identical.
>>
>> * Eliminates the local extent_buffer variable as in this case it doesn't
>> bring anything to function readability.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> This was weirdly difficult to review in both diff and vimdiff, had to look at
> the resulting code to see how it worked out.
> 
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Yeah, sorry about that but the function has a bunch of IF's ... I could
have probably broken it into 3 patches but each separate refactoring is
really small.

> 
> Thanks,
> 
> Josef
> 

Re: [PATCH] btrfs: Refactor unlock_up

[David Sterba]

On Tue, Dec 14, 2021 at 03:39:39PM +0200, Nikolay Borisov wrote:
> The purpose of this function is to unlock all nodes in a btrfs path
> which are above 'lowest_unlock' and whose slot used is different than 0.
> As such it used slightly awkward structure of 'if' as well as somewhat
> cryptic "no_skip" control variable which denotes whether we should
> check the current level of skipiability or no.
> 
> This patch does the following (cosmetic) refactorings:
> 
> * Renames 'no_skip' to 'check_skip' and makes it a boolean. This
> variable controls whether we are below the lowest_unlock/skip_level
> levels.
> 
> * Consolidates the 2 conditions which warrant checking whether the
> current level should be skipped under 1 common if (check_skip) branch,
> this increase indentation level but is not critical.
> 
> * Consolidates the 'skip_level < i && i >= lowest_unlock' and
> 'i >= lowest_unlock && i > skip_level' condition into a common branch
> since those are identical.
> 
> * Eliminates the local extent_buffer variable as in this case it doesn't
> bring anything to function readability.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ctree.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 62066c034363..ab2ea0b2863c 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1348,33 +1348,34 @@ static noinline void unlock_up(struct btrfs_path *path, int level,
>  {
>  	int i;
>  	int skip_level = level;
> -	int no_skips = 0;
> -	struct extent_buffer *t;
> +	int check_skip = true;

this should be bool, right

Re: [PATCH] btrfs: Refactor unlock_up

[David Sterba]

On Tue, Dec 14, 2021 at 03:39:39PM +0200, Nikolay Borisov wrote:
> The purpose of this function is to unlock all nodes in a btrfs path
> which are above 'lowest_unlock' and whose slot used is different than 0.
> As such it used slightly awkward structure of 'if' as well as somewhat
> cryptic "no_skip" control variable which denotes whether we should
> check the current level of skipiability or no.
> 
> This patch does the following (cosmetic) refactorings:
> 
> * Renames 'no_skip' to 'check_skip' and makes it a boolean. This
> variable controls whether we are below the lowest_unlock/skip_level
> levels.
> 
> * Consolidates the 2 conditions which warrant checking whether the
> current level should be skipped under 1 common if (check_skip) branch,
> this increase indentation level but is not critical.
> 
> * Consolidates the 'skip_level < i && i >= lowest_unlock' and
> 'i >= lowest_unlock && i > skip_level' condition into a common branch
> since those are identical.
> 
> * Eliminates the local extent_buffer variable as in this case it doesn't
> bring anything to function readability.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Much better, added to misc-next, thanks.