| Message ID | 20211214133939.751395-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Refactor unlock_up | expand |
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
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 >
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
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.
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 &&
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(-)