[PATCH 0/4] Small coding style cleanups

[PATCH 0/4] Small coding style cleanups

[Johannes Thumshirn]

Here are some minor coding style cleanups which I think are neat as they make
some functions a bit easier to read.

None of these patches is really needed though.

The patches have no regressions with regrads to the basline
btrfs-devel/misc-next on an xfstests -g auto run.

A gitweb preview can be found at
https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=btrfs-raid-cleanups

Note I did rebase the branch because one patch did not have a changelog.

Johannes Thumshirn (4):
  btrfs: reduce indentation in lock_stripe_add
  btrfs: remove pointless local variable in lock_stripe_add()
  btrfs: reduce indentation in btrfs_may_alloc_data_chunk
  btrfs: remove pointless indentation in btrfs_read_sys_array()

 fs/btrfs/raid56.c  |  95 ++++++++++++++++++++++-----------------------
 fs/btrfs/volumes.c | 112 +++++++++++++++++++++++++++--------------------------
 2 files changed, 103 insertions(+), 104 deletions(-)

-- 
2.16.4

[PATCH 1/4] btrfs: reduce indentation in lock_stripe_add

[Johannes Thumshirn]

In lock_stripe_add() we're traversing the stripe hash list and check if
the current list element's raid_map equals is equal to the raid bio's
raid_map. If both are equal we continue processing.

If we'd check for inequality instead of equality we can reduce one level
of indentation.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/raid56.c | 90 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 47 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 8f47a85944eb..9e8a6c447e51 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -682,62 +682,58 @@ static noinline int lock_stripe_add(struct btrfs_raid_bio *rbio)
 
 	spin_lock_irqsave(&h->lock, flags);
 	list_for_each_entry(cur, &h->hash_list, hash_list) {
-		if (cur->bbio->raid_map[0] == rbio->bbio->raid_map[0]) {
-			spin_lock(&cur->bio_list_lock);
-
-			/* can we steal this cached rbio's pages? */
-			if (bio_list_empty(&cur->bio_list) &&
-			    list_empty(&cur->plug_list) &&
-			    test_bit(RBIO_CACHE_BIT, &cur->flags) &&
-			    !test_bit(RBIO_RMW_LOCKED_BIT, &cur->flags)) {
-				list_del_init(&cur->hash_list);
-				refcount_dec(&cur->refs);
-
-				steal_rbio(cur, rbio);
-				cache_drop = cur;
-				spin_unlock(&cur->bio_list_lock);
+		if (cur->bbio->raid_map[0] != rbio->bbio->raid_map[0])
+			continue;
 
-				goto lockit;
-			}
+		spin_lock(&cur->bio_list_lock);
 
-			/* can we merge into the lock owner? */
-			if (rbio_can_merge(cur, rbio)) {
-				merge_rbio(cur, rbio);
-				spin_unlock(&cur->bio_list_lock);
-				freeit = rbio;
-				ret = 1;
-				goto out;
-			}
+		/* can we steal this cached rbio's pages? */
+		if (bio_list_empty(&cur->bio_list) &&
+		    list_empty(&cur->plug_list) &&
+		    test_bit(RBIO_CACHE_BIT, &cur->flags) &&
+		    !test_bit(RBIO_RMW_LOCKED_BIT, &cur->flags)) {
+			list_del_init(&cur->hash_list);
+			refcount_dec(&cur->refs);
 
+			steal_rbio(cur, rbio);
+			cache_drop = cur;
+			spin_unlock(&cur->bio_list_lock);
 
-			/*
-			 * we couldn't merge with the running
-			 * rbio, see if we can merge with the
-			 * pending ones.  We don't have to
-			 * check for rmw_locked because there
-			 * is no way they are inside finish_rmw
-			 * right now
-			 */
-			list_for_each_entry(pending, &cur->plug_list,
-					    plug_list) {
-				if (rbio_can_merge(pending, rbio)) {
-					merge_rbio(pending, rbio);
-					spin_unlock(&cur->bio_list_lock);
-					freeit = rbio;
-					ret = 1;
-					goto out;
-				}
-			}
+			goto lockit;
+		}
 
-			/* no merging, put us on the tail of the plug list,
-			 * our rbio will be started with the currently
-			 * running rbio unlocks
-			 */
-			list_add_tail(&rbio->plug_list, &cur->plug_list);
+		/* can we merge into the lock owner? */
+		if (rbio_can_merge(cur, rbio)) {
+			merge_rbio(cur, rbio);
 			spin_unlock(&cur->bio_list_lock);
+			freeit = rbio;
 			ret = 1;
 			goto out;
 		}
+
+
+		/*
+		 * we couldn't merge with the running rbio, see if we can merge
+		 * with the pending ones.  We don't have to check for rmw_locked
+		 * because there is no way they are inside finish_rmw right now
+		 */
+		list_for_each_entry(pending, &cur->plug_list, plug_list) {
+			if (rbio_can_merge(pending, rbio)) {
+				merge_rbio(pending, rbio);
+				spin_unlock(&cur->bio_list_lock);
+				freeit = rbio;
+				ret = 1;
+				goto out;
+			}
+		}
+
+		/* no merging, put us on the tail of the plug list, our rbio
+		 * will be started with the currently running rbio unlocks
+		 */
+		list_add_tail(&rbio->plug_list, &cur->plug_list);
+		spin_unlock(&cur->bio_list_lock);
+		ret = 1;
+		goto out;
 	}
 lockit:
 	refcount_inc(&rbio->refs);
-- 
2.16.4

Re: [PATCH 1/4] btrfs: reduce indentation in lock_stripe_add

[Nikolay Borisov]

On 18.10.19 г. 12:58 ч., Johannes Thumshirn wrote:
> In lock_stripe_add() we're traversing the stripe hash list and check if
> the current list element's raid_map equals is equal to the raid bio's
> raid_map. If both are equal we continue processing.
> 
> If we'd check for inequality instead of equality we can reduce one level
> of indentation.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

After comparing before/after applying the patch I can say:

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

Re: [PATCH 1/4] btrfs: reduce indentation in lock_stripe_add

[David Sterba]

On Fri, Oct 18, 2019 at 11:58:20AM +0200, Johannes Thumshirn wrote:
> -			/* can we steal this cached rbio's pages? */

> +		/* can we steal this cached rbio's pages? */

> +		/* no merging, put us on the tail of the plug list, our rbio
> +		 * will be started with the currently running rbio unlocks
> +		 */

In patches that touch comments it's allowed if not encouraged to
reformat the comments to the preferred style. Ie. capital first letter,
aligned to 80, and not the ugl^Wnet code format. I'm fixing that in many
other patches anyway, no need to resend.

[PATCH 2/4] btrfs: remove pointless local variable in lock_stripe_add()

[Johannes Thumshirn]

In lock_stripe_add() we're caching the bucket for the stripe hash table
just for a single call to dereference the stripe hash.

If we just directly call rbio_bucket() we can safe the pointless local
variable.

Also move the dereferencing of the stripe hash outside of the variable
declaration block to not break over the 80 characters limit.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/raid56.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 9e8a6c447e51..530719ff8185 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -671,8 +671,7 @@ static struct page *rbio_qstripe_page(struct btrfs_raid_bio *rbio, int index)
  */
 static noinline int lock_stripe_add(struct btrfs_raid_bio *rbio)
 {
-	int bucket = rbio_bucket(rbio);
-	struct btrfs_stripe_hash *h = rbio->fs_info->stripe_hash_table->table + bucket;
+	struct btrfs_stripe_hash *h;
 	struct btrfs_raid_bio *cur;
 	struct btrfs_raid_bio *pending;
 	unsigned long flags;
@@ -680,6 +679,8 @@ static noinline int lock_stripe_add(struct btrfs_raid_bio *rbio)
 	struct btrfs_raid_bio *cache_drop = NULL;
 	int ret = 0;
 
+	h = rbio->fs_info->stripe_hash_table->table + rbio_bucket(rbio);
+
 	spin_lock_irqsave(&h->lock, flags);
 	list_for_each_entry(cur, &h->hash_list, hash_list) {
 		if (cur->bbio->raid_map[0] != rbio->bbio->raid_map[0])
-- 
2.16.4

[PATCH 3/4] btrfs: reduce indentation in btrfs_may_alloc_data_chunk

[Johannes Thumshirn]

In btrfs_may_alloc_data_chunk() we're checking if the chunk type is of
type BTRFS_BLOCK_GROUP_DATA and if it is we process it.

Instead of checking if the chunk type is a BTRFS_BLOCK_GROUP_DATA chunk we
can negate the check and bail out early if it isn't.

This makes the code a bit more readable.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/volumes.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bba74e3bd9d8..4c630356bb30 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2982,27 +2982,28 @@ static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
 	chunk_type = cache->flags;
 	btrfs_put_block_group(cache);
 
-	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
-		spin_lock(&fs_info->data_sinfo->lock);
-		bytes_used = fs_info->data_sinfo->bytes_used;
-		spin_unlock(&fs_info->data_sinfo->lock);
-
-		if (!bytes_used) {
-			struct btrfs_trans_handle *trans;
-			int ret;
-
-			trans =	btrfs_join_transaction(fs_info->tree_root);
-			if (IS_ERR(trans))
-				return PTR_ERR(trans);
-
-			ret = btrfs_force_chunk_alloc(trans,
-						      BTRFS_BLOCK_GROUP_DATA);
-			btrfs_end_transaction(trans);
-			if (ret < 0)
-				return ret;
-			return 1;
-		}
+	if (!(chunk_type & BTRFS_BLOCK_GROUP_DATA))
+		return 0;
+
+	spin_lock(&fs_info->data_sinfo->lock);
+	bytes_used = fs_info->data_sinfo->bytes_used;
+	spin_unlock(&fs_info->data_sinfo->lock);
+
+	if (!bytes_used) {
+		struct btrfs_trans_handle *trans;
+		int ret;
+
+		trans =	btrfs_join_transaction(fs_info->tree_root);
+		if (IS_ERR(trans))
+			return PTR_ERR(trans);
+
+		ret = btrfs_force_chunk_alloc(trans, BTRFS_BLOCK_GROUP_DATA);
+		btrfs_end_transaction(trans);
+		if (ret < 0)
+			return ret;
+		return 1;
 	}
+
 	return 0;
 }
 
-- 
2.16.4

[PATCH 4/4] btrfs: remove pointless indentation in btrfs_read_sys_array()

[Johannes Thumshirn]

Instead of checking if we've read a BTRFS_CHUNK_ITEM_KEY from disk and
then process it we could just bail out early if the read disk key wasn't a
BTRFS_CHUNK_ITEM_KEY.

This removes a level of indentation and makes the code nicer to read.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/volumes.c | 71 +++++++++++++++++++++++++++---------------------------
 1 file changed, 36 insertions(+), 35 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4c630356bb30..08a2d1f6ec41 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6805,48 +6805,49 @@ int btrfs_read_sys_array(struct btrfs_fs_info *fs_info)
 		sb_array_offset += len;
 		cur_offset += len;
 
-		if (key.type == BTRFS_CHUNK_ITEM_KEY) {
-			chunk = (struct btrfs_chunk *)sb_array_offset;
-			/*
-			 * At least one btrfs_chunk with one stripe must be
-			 * present, exact stripe count check comes afterwards
-			 */
-			len = btrfs_chunk_item_size(1);
-			if (cur_offset + len > array_size)
-				goto out_short_read;
-
-			num_stripes = btrfs_chunk_num_stripes(sb, chunk);
-			if (!num_stripes) {
-				btrfs_err(fs_info,
-					"invalid number of stripes %u in sys_array at offset %u",
-					num_stripes, cur_offset);
-				ret = -EIO;
-				break;
-			}
+		if (key.type != BTRFS_CHUNK_ITEM_KEY) {
+			btrfs_err(fs_info,
+			    "unexpected item type %u in sys_array at offset %u",
+				  (u32)key.type, cur_offset);
+			ret = -EIO;
+			break;
+		}
 
-			type = btrfs_chunk_type(sb, chunk);
-			if ((type & BTRFS_BLOCK_GROUP_SYSTEM) == 0) {
-				btrfs_err(fs_info,
-			    "invalid chunk type %llu in sys_array at offset %u",
-					type, cur_offset);
-				ret = -EIO;
-				break;
-			}
+		chunk = (struct btrfs_chunk *)sb_array_offset;
+		/*
+		 * At least one btrfs_chunk with one stripe must be
+		 * present, exact stripe count check comes afterwards
+		 */
+		len = btrfs_chunk_item_size(1);
+		if (cur_offset + len > array_size)
+			goto out_short_read;
 
-			len = btrfs_chunk_item_size(num_stripes);
-			if (cur_offset + len > array_size)
-				goto out_short_read;
+		num_stripes = btrfs_chunk_num_stripes(sb, chunk);
+		if (!num_stripes) {
+			btrfs_err(fs_info,
+				  "invalid number of stripes %u in sys_array at offset %u",
+				  num_stripes, cur_offset);
+			ret = -EIO;
+			break;
+		}
 
-			ret = read_one_chunk(&key, sb, chunk);
-			if (ret)
-				break;
-		} else {
+		type = btrfs_chunk_type(sb, chunk);
+		if ((type & BTRFS_BLOCK_GROUP_SYSTEM) == 0) {
 			btrfs_err(fs_info,
-			    "unexpected item type %u in sys_array at offset %u",
-				  (u32)key.type, cur_offset);
+				  "invalid chunk type %llu in sys_array at offset %u",
+				  type, cur_offset);
 			ret = -EIO;
 			break;
 		}
+
+		len = btrfs_chunk_item_size(num_stripes);
+		if (cur_offset + len > array_size)
+			goto out_short_read;
+
+		ret = read_one_chunk(&key, sb, chunk);
+		if (ret)
+			break;
+
 		array_ptr += len;
 		sb_array_offset += len;
 		cur_offset += len;
-- 
2.16.4

Re: [PATCH 0/4] Small coding style cleanups

[Nikolay Borisov]

On 18.10.19 г. 12:58 ч., Johannes Thumshirn wrote:
> Here are some minor coding style cleanups which I think are neat as they make
> some functions a bit easier to read.
> 
> None of these patches is really needed though.
> 
> The patches have no regressions with regrads to the basline
> btrfs-devel/misc-next on an xfstests -g auto run.
> 
> A gitweb preview can be found at
> https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=btrfs-raid-cleanups
> 
> Note I did rebase the branch because one patch did not have a changelog.
> 
> Johannes Thumshirn (4):
>   btrfs: reduce indentation in lock_stripe_add
>   btrfs: remove pointless local variable in lock_stripe_add()
>   btrfs: reduce indentation in btrfs_may_alloc_data_chunk
>   btrfs: remove pointless indentation in btrfs_read_sys_array()
> 
>  fs/btrfs/raid56.c  |  95 ++++++++++++++++++++++-----------------------
>  fs/btrfs/volumes.c | 112 +++++++++++++++++++++++++++--------------------------
>  2 files changed, 103 insertions(+), 104 deletions(-)
> 

Patches 2-4 LGTM you can add, :

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

Re: [PATCH 0/4] Small coding style cleanups

[Johannes Thumshirn]

On 18/10/2019 14:02, Nikolay Borisov wrote:
[...]

> Patches 2-4 LGTM you can add, :

What's wrong with 1/4?



-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 0/4] Small coding style cleanups

[Nikolay Borisov]

On 21.10.19 г. 11:06 ч., Johannes Thumshirn wrote:
> On 18/10/2019 14:02, Nikolay Borisov wrote:
> [...]
> 
>> Patches 2-4 LGTM you can add, :
> 
> What's wrong with 1/4?

Nothing per-se but it looked ugly and my brain refused to make sense of
it so I haven't really reviewed it :) .

> 
> 
> 

Re: [PATCH 0/4] Small coding style cleanups

[Johannes Thumshirn]

On 21/10/2019 14:32, Nikolay Borisov wrote:
> 
> 
> On 21.10.19 г. 11:06 ч., Johannes Thumshirn wrote:
>> On 18/10/2019 14:02, Nikolay Borisov wrote:
>> [...]
>>
>>> Patches 2-4 LGTM you can add, :
>>
>> What's wrong with 1/4?
> 
> Nothing per-se but it looked ugly and my brain refused to make sense of
> it so I haven't really reviewed it :) .

It will look less ugly after the patch is applied ;-)

https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/tree/fs/btrfs/raid56.c?h=btrfs-raid-cleanups&id=ad33a49606b6f31c7fe20aa089f5603ad3065bd5#n684

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 0/4] Small coding style cleanups

[David Sterba]

On Fri, Oct 18, 2019 at 11:58:19AM +0200, Johannes Thumshirn wrote:
> Here are some minor coding style cleanups which I think are neat as they make
> some functions a bit easier to read.
> 
> None of these patches is really needed though.
> 
> The patches have no regressions with regrads to the basline
> btrfs-devel/misc-next on an xfstests -g auto run.
> 
> A gitweb preview can be found at
> https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=btrfs-raid-cleanups
> 
> Note I did rebase the branch because one patch did not have a changelog.
> 
> Johannes Thumshirn (4):
>   btrfs: reduce indentation in lock_stripe_add
>   btrfs: remove pointless local variable in lock_stripe_add()
>   btrfs: reduce indentation in btrfs_may_alloc_data_chunk
>   btrfs: remove pointless indentation in btrfs_read_sys_array()

Added to misc-next, thanks.