[PATCH 00/10] A bunch of misc cleanups

[PATCH 00/10] A bunch of misc cleanups

[Nikolay Borisov]

Here's an assortment of little quality-of-life patches that I created while
looking into the raid56 code. They should bear no functional changes and have
tested them with xfstest and nothing fell over so should be rather low risk.

Patch 1 moves code in __btrfs_map_block, essentially assigning tgtdev_map/raid_map
closet to where space for them is allocated. This also neccesiated moving the
call to sort_parity_stripes. The end result is (hopefully) slightly easier to
follow __btrfs_map_block.

Next 5 patches cleanup minor things in raid56.c such as removing redundant checks,
making code interacting with bio_list more in line with what the rest of the
kernel is doing. Finally it's using some macros/functions instead of open-coding
them. Really just a bunch of low hanging fruit.

Final 4 patches gradually remove all labels in btrfs_submit_compressed_read.
Current failures can be handled "inline" so to speak, without the need for
extra labels. This likely will change once the BUG_ONs are removed but we are
not there yet.

Nikolay Borisov (10):
  btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers
  btrfs: raid56: Remove redundant check in rbio_add_io_page
  btrfs: raid56: Assign bio in while()
  btrfs: raid56: Remove out label in __raid56_parity_recover
  btrfs: raid56: Use in_range where applicable
  btrfs: raid56: Don't opencode swap()
  btrfs: Remove fail label in check_compressed_csum
  btrfs: Remove fail1 label in btrfs_submit_compressed_read
  btrfs: Remove fail2 label from btrfs_submit_compressed_read
  btrfs: Remove out label in btrfs_submit_compressed_read

 fs/btrfs/compression.c | 48 +++++++++++++-------------------
 fs/btrfs/raid56.c      | 63 ++++++++++--------------------------------
 fs/btrfs/volumes.c     | 34 ++++++++++-------------
 3 files changed, 49 insertions(+), 96 deletions(-)

--
2.17.1

[PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers

[Nikolay Borisov]

Since btrfs_bio always contains the extra space for the tgtdev_map and
raid_maps it's pointless to make the assignment iff specific conditions
are met. Instead, always assign the pointers to their correct value at
allocation time. To accommodate this change also move code a bit in
__btrfs_map_block so that btrfs_bio::stripes array is always initialized
before the raid_map, subsequently move the call to sort_parity_stripes
in the 'if' building the raid_map, retaining the old behavior.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index cb9883c7f8b7..d74d21af77fb 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5522,6 +5522,9 @@ static struct btrfs_bio *alloc_btrfs_bio(int total_stripes, int real_stripes)
 	atomic_set(&bbio->error, 0);
 	refcount_set(&bbio->refs, 1);
 
+	bbio->tgtdev_map = (int *)(bbio->stripes + total_stripes);
+	bbio->raid_map = (u64 *)(bbio->tgtdev_map + real_stripes);
+
 	return bbio;
 }
 
@@ -6144,8 +6147,16 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
 		ret = -ENOMEM;
 		goto out;
 	}
-	if (dev_replace_is_ongoing && dev_replace->tgtdev != NULL)
-		bbio->tgtdev_map = (int *)(bbio->stripes + num_alloc_stripes);
+
+	for (i = 0; i < num_stripes; i++) {
+		bbio->stripes[i].physical =
+			map->stripes[stripe_index].physical +
+			stripe_offset +
+			stripe_nr * map->stripe_len;
+		bbio->stripes[i].dev =
+			map->stripes[stripe_index].dev;
+		stripe_index++;
+	}
 
 	/* build raid_map */
 	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK && need_raid_map &&
@@ -6153,10 +6164,6 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
 		u64 tmp;
 		unsigned rot;
 
-		bbio->raid_map = (u64 *)((void *)bbio->stripes +
-				 sizeof(struct btrfs_bio_stripe) *
-				 num_alloc_stripes +
-				 sizeof(int) * tgtdev_indexes);
 
 		/* Work out the disk rotation on this stripe-set */
 		div_u64_rem(stripe_nr, num_stripes, &rot);
@@ -6171,25 +6178,14 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
 		if (map->type & BTRFS_BLOCK_GROUP_RAID6)
 			bbio->raid_map[(i+rot+1) % num_stripes] =
 				RAID6_Q_STRIPE;
-	}
-
 
-	for (i = 0; i < num_stripes; i++) {
-		bbio->stripes[i].physical =
-			map->stripes[stripe_index].physical +
-			stripe_offset +
-			stripe_nr * map->stripe_len;
-		bbio->stripes[i].dev =
-			map->stripes[stripe_index].dev;
-		stripe_index++;
+		sort_parity_stripes(bbio, num_stripes);
 	}
 
+
 	if (need_full_stripe(op))
 		max_errors = btrfs_chunk_max_errors(map);
 
-	if (bbio->raid_map)
-		sort_parity_stripes(bbio, num_stripes);
-
 	if (dev_replace_is_ongoing && dev_replace->tgtdev != NULL &&
 	    need_full_stripe(op)) {
 		handle_ops_on_dev_replace(op, &bbio, dev_replace, &num_stripes,
-- 
2.17.1

[PATCH 02/10] btrfs: raid56: Remove redundant check in rbio_add_io_page

[Nikolay Borisov]

The merging logic is always executed if the current stripe's device
is not missing. So there's no point in duplicating the check. Simply
remove it, while at it reduce the scope of the 'last_end' variable.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/raid56.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 4efd9ed1a30e..f21bab45b7ce 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1083,7 +1083,6 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
 			    unsigned long bio_max_len)
 {
 	struct bio *last = bio_list->tail;
-	u64 last_end = 0;
 	int ret;
 	struct bio *bio;
 	struct btrfs_bio_stripe *stripe;
@@ -1098,15 +1097,14 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
 
 	/* see if we can add this page onto our existing bio */
 	if (last) {
-		last_end = (u64)last->bi_iter.bi_sector << 9;
+		u64 last_end = (u64)last->bi_iter.bi_sector << 9;
 		last_end += last->bi_iter.bi_size;
 
 		/*
 		 * we can't merge these if they are from different
 		 * devices or if they are not contiguous
 		 */
-		if (last_end == disk_start && stripe->dev->bdev &&
-		    !last->bi_status &&
+		if (last_end == disk_start && !last->bi_status &&
 		    last->bi_disk == stripe->dev->bdev->bd_disk &&
 		    last->bi_partno == stripe->dev->bdev->bd_partno) {
 			ret = bio_add_page(last, page, PAGE_SIZE, 0);
-- 
2.17.1

[PATCH 03/10] btrfs: raid56: Assign bio in while()

[Nikolay Borisov]

Unify the style in the file such that return value of bio_list_pop
is assigned directly in the while loop. This is in line with the rest
of the kernel.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/raid56.c | 30 +++++-------------------------
 1 file changed, 5 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index f21bab45b7ce..a7ae4d8a47ce 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1324,11 +1324,7 @@ static noinline void finish_rmw(struct btrfs_raid_bio *rbio)
 	atomic_set(&rbio->stripes_pending, bio_list_size(&bio_list));
 	BUG_ON(atomic_read(&rbio->stripes_pending) == 0);
 
-	while (1) {
-		bio = bio_list_pop(&bio_list);
-		if (!bio)
-			break;
-
+	while ((bio = bio_list_pop(&bio_list))) {
 		bio->bi_private = rbio;
 		bio->bi_end_io = raid_write_end_io;
 		bio->bi_opf = REQ_OP_WRITE;
@@ -1566,11 +1562,7 @@ static int raid56_rmw_stripe(struct btrfs_raid_bio *rbio)
 	 * not to touch it after that
 	 */
 	atomic_set(&rbio->stripes_pending, bios_to_read);
-	while (1) {
-		bio = bio_list_pop(&bio_list);
-		if (!bio)
-			break;
-
+	while ((bio = bio_list_pop(&bio_list))) {
 		bio->bi_private = rbio;
 		bio->bi_end_io = raid_rmw_end_io;
 		bio->bi_opf = REQ_OP_READ;
@@ -2112,11 +2104,7 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
 	 * not to touch it after that
 	 */
 	atomic_set(&rbio->stripes_pending, bios_to_read);
-	while (1) {
-		bio = bio_list_pop(&bio_list);
-		if (!bio)
-			break;
-
+	while ((bio = bio_list_pop(&bio_list))) {
 		bio->bi_private = rbio;
 		bio->bi_end_io = raid_recover_end_io;
 		bio->bi_opf = REQ_OP_READ;
@@ -2481,11 +2469,7 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
 
 	atomic_set(&rbio->stripes_pending, nr_data);
 
-	while (1) {
-		bio = bio_list_pop(&bio_list);
-		if (!bio)
-			break;
-
+	while ((bio = bio_list_pop(&bio_list))) {
 		bio->bi_private = rbio;
 		bio->bi_end_io = raid_write_end_io;
 		bio->bi_opf = REQ_OP_WRITE;
@@ -2663,11 +2647,7 @@ static void raid56_parity_scrub_stripe(struct btrfs_raid_bio *rbio)
 	 * not to touch it after that
 	 */
 	atomic_set(&rbio->stripes_pending, bios_to_read);
-	while (1) {
-		bio = bio_list_pop(&bio_list);
-		if (!bio)
-			break;
-
+	while ((bio = bio_list_pop(&bio_list))) {
 		bio->bi_private = rbio;
 		bio->bi_end_io = raid56_parity_scrub_end_io;
 		bio->bi_opf = REQ_OP_READ;
-- 
2.17.1

[PATCH 04/10] btrfs: raid56: Remove out label in __raid56_parity_recover

[Nikolay Borisov]

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index a7ae4d8a47ce..d9415a22617b 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2093,7 +2093,7 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
 		 */
 		if (atomic_read(&rbio->error) <= rbio->bbio->max_errors) {
 			__raid_recover_end_io(rbio);
-			goto out;
+			return 0;
 		} else {
 			goto cleanup;
 		}
@@ -2113,7 +2113,7 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
 
 		submit_bio(bio);
 	}
-out:
+
 	return 0;
 
 cleanup:
-- 
2.17.1

[PATCH 05/10] btrfs: raid56: Use in_range where applicable

[Nikolay Borisov]

While at it use the opportunity to simplify find_logical_bio_stripe by
reducing the scope of 'stripe_start' variable and squash the
sector-to-bytes conversion on one line.

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

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index d9415a22617b..d89dd3030bba 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1349,7 +1349,6 @@ static int find_bio_stripe(struct btrfs_raid_bio *rbio,
 			   struct bio *bio)
 {
 	u64 physical = bio->bi_iter.bi_sector;
-	u64 stripe_start;
 	int i;
 	struct btrfs_bio_stripe *stripe;
 
@@ -1357,9 +1356,7 @@ static int find_bio_stripe(struct btrfs_raid_bio *rbio,
 
 	for (i = 0; i < rbio->bbio->num_stripes; i++) {
 		stripe = &rbio->bbio->stripes[i];
-		stripe_start = stripe->physical;
-		if (physical >= stripe_start &&
-		    physical < stripe_start + rbio->stripe_len &&
+		if (in_range(physical, stripe->physical, rbio->stripe_len) &&
 		    stripe->dev->bdev &&
 		    bio->bi_disk == stripe->dev->bdev->bd_disk &&
 		    bio->bi_partno == stripe->dev->bdev->bd_partno) {
@@ -1377,18 +1374,13 @@ static int find_bio_stripe(struct btrfs_raid_bio *rbio,
 static int find_logical_bio_stripe(struct btrfs_raid_bio *rbio,
 				   struct bio *bio)
 {
-	u64 logical = bio->bi_iter.bi_sector;
-	u64 stripe_start;
+	u64 logical = bio->bi_iter.bi_sector << 9;
 	int i;
 
-	logical <<= 9;
-
 	for (i = 0; i < rbio->nr_data; i++) {
-		stripe_start = rbio->bbio->raid_map[i];
-		if (logical >= stripe_start &&
-		    logical < stripe_start + rbio->stripe_len) {
+		u64 stripe_start = rbio->bbio->raid_map[i];
+		if (in_range(logical, stripe_start, rbio->stripe_len))
 			return i;
-		}
 	}
 	return -1;
 }
-- 
2.17.1

[PATCH 06/10] btrfs: raid56: Don't opencode swap()

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/raid56.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index d89dd3030bba..e9173b6853c1 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1861,11 +1861,8 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 			}
 
 			/* make sure our ps and qs are in order */
-			if (faila > failb) {
-				int tmp = failb;
-				failb = faila;
-				faila = tmp;
-			}
+			if (faila > failb)
+				swap(faila, failb);
 
 			/* if the q stripe is failed, do a pstripe reconstruction
 			 * from the xors.
-- 
2.17.1

[PATCH 07/10] btrfs: Remove fail label in check_compressed_csum

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/compression.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 2f30bf4127f8..48ceab7be0fe 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -178,7 +178,6 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
 	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
 	const u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
-	int ret;
 	struct page *page;
 	unsigned long i;
 	char *kaddr;
@@ -204,15 +203,12 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
 			if (btrfs_io_bio(bio)->dev)
 				btrfs_dev_stat_inc_and_print(btrfs_io_bio(bio)->dev,
 					BTRFS_DEV_STAT_CORRUPTION_ERRS);
-			ret = -EIO;
-			goto fail;
+			return -EIO;
 		}
 		cb_sum += csum_size;
 
 	}
-	ret = 0;
-fail:
-	return ret;
+	return 0;
 }
 
 /* when we finish reading compressed pages from the disk, we
-- 
2.17.1

[PATCH 08/10] btrfs: Remove fail1 label in btrfs_submit_compressed_read

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/compression.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 48ceab7be0fe..2033ce17e5c6 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -703,8 +703,10 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	nr_pages = DIV_ROUND_UP(compressed_len, PAGE_SIZE);
 	cb->compressed_pages = kcalloc(nr_pages, sizeof(struct page *),
 				       GFP_NOFS);
-	if (!cb->compressed_pages)
-		goto fail1;
+	if (!cb->compressed_pages) {
+		kfree(cb);
+		return BLK_STS_RESOURCE;
+	}
 
 	for (pg_index = 0; pg_index < nr_pages; pg_index++) {
 		cb->compressed_pages[pg_index] = alloc_page(GFP_NOFS |
@@ -806,7 +808,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	}
 
 	kfree(cb->compressed_pages);
-fail1:
 	kfree(cb);
 out:
 	free_extent_map(em);
-- 
2.17.1

[PATCH 09/10] btrfs: Remove fail2 label from btrfs_submit_compressed_read

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/compression.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 2033ce17e5c6..c28ee9fcd15d 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -661,8 +661,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	u64 em_len;
 	u64 em_start;
 	struct extent_map *em;
-	blk_status_t ret = BLK_STS_RESOURCE;
-	int faili = 0;
+	blk_status_t ret;
 	const u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
 	u8 *sums;
 
@@ -712,12 +711,16 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 		cb->compressed_pages[pg_index] = alloc_page(GFP_NOFS |
 							      __GFP_HIGHMEM);
 		if (!cb->compressed_pages[pg_index]) {
-			faili = pg_index - 1;
-			ret = BLK_STS_RESOURCE;
-			goto fail2;
+			int i;
+
+			for (i = 0; i < pg_index; i++)
+				__free_page(cb->compressed_pages[i]);
+
+			kfree(cb->compressed_pages);
+			kfree(cb);
+			return BLK_STS_RESOURCE;
 		}
 	}
-	faili = nr_pages - 1;
 	cb->nr_pages = nr_pages;
 
 	add_ra_bio_pages(inode, em_start + em_len, cb);
@@ -801,14 +804,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 
 	return 0;
 
-fail2:
-	while (faili >= 0) {
-		__free_page(cb->compressed_pages[faili]);
-		faili--;
-	}
-
-	kfree(cb->compressed_pages);
-	kfree(cb);
 out:
 	free_extent_map(em);
 	return ret;
-- 
2.17.1

[PATCH 10/10] btrfs: Remove out label in btrfs_submit_compressed_read

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/compression.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index c28ee9fcd15d..f9a9ec51a1ec 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -678,8 +678,10 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 
 	compressed_len = em->block_len;
 	cb = kmalloc(compressed_bio_size(fs_info, compressed_len), GFP_NOFS);
-	if (!cb)
-		goto out;
+	if (!cb) {
+		free_extent_map(em);
+		return BLK_STS_RESOURCE;
+	}
 
 	refcount_set(&cb->pending_bios, 0);
 	cb->errors = 0;
@@ -803,10 +805,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	}
 
 	return 0;
-
-out:
-	free_extent_map(em);
-	return ret;
 }
 
 /*
-- 
2.17.1

Re: [PATCH 04/10] btrfs: raid56: Remove out label in __raid56_parity_recover

[David Sterba]

On Thu, Jul 02, 2020 at 04:46:44PM +0300, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/raid56.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index a7ae4d8a47ce..d9415a22617b 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2093,7 +2093,7 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
>  		 */
>  		if (atomic_read(&rbio->error) <= rbio->bbio->max_errors) {
>  			__raid_recover_end_io(rbio);
> -			goto out;
> +			return 0;

No please, when there are labels that do cleanup like the one in the
context, 'return's make it harder to follow.

>  		} else {
>  			goto cleanup;
>  		}
> @@ -2113,7 +2113,7 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
>  
>  		submit_bio(bio);
>  	}
> -out:
> +
>  	return 0;
>  
>  cleanup:
> -- 
> 2.17.1

Re: [PATCH 08/10] btrfs: Remove fail1 label in btrfs_submit_compressed_read

[David Sterba]

On Thu, Jul 02, 2020 at 04:46:48PM +0300, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/compression.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 48ceab7be0fe..2033ce17e5c6 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -703,8 +703,10 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	nr_pages = DIV_ROUND_UP(compressed_len, PAGE_SIZE);
>  	cb->compressed_pages = kcalloc(nr_pages, sizeof(struct page *),
>  				       GFP_NOFS);
> -	if (!cb->compressed_pages)
> -		goto fail1;
> +	if (!cb->compressed_pages) {
> +		kfree(cb);
> +		return BLK_STS_RESOURCE;

No please, that's exactly why there is th exit block so we don't have to
repeat the cleanup functions.

> +	}
>  
>  	for (pg_index = 0; pg_index < nr_pages; pg_index++) {
>  		cb->compressed_pages[pg_index] = alloc_page(GFP_NOFS |
> @@ -806,7 +808,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	}
>  
>  	kfree(cb->compressed_pages);
> -fail1:
>  	kfree(cb);
>  out:
>  	free_extent_map(em);
> -- 
> 2.17.1

Re: [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers

[Johannes Thumshirn]

On 02/07/2020 15:47, Nikolay Borisov wrote:
[...]
> -		bbio->raid_map = (u64 *)((void *)bbio->stripes +
> -				 sizeof(struct btrfs_bio_stripe) *
> -				 num_alloc_stripes +
> -				 sizeof(int) * tgtdev_indexes);

That one took me a while to be convinced it is correct.

>  
>  		/* Work out the disk rotation on this stripe-set */
>  		div_u64_rem(stripe_nr, num_stripes, &rot);
> @@ -6171,25 +6178,14 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
>  		if (map->type & BTRFS_BLOCK_GROUP_RAID6)
>  			bbio->raid_map[(i+rot+1) % num_stripes] =
>  				RAID6_Q_STRIPE;
> -	}
> -
>  
> -	for (i = 0; i < num_stripes; i++) {
> -		bbio->stripes[i].physical =
> -			map->stripes[stripe_index].physical +
> -			stripe_offset +
> -			stripe_nr * map->stripe_len;
> -		bbio->stripes[i].dev =
> -			map->stripes[stripe_index].dev;
> -		stripe_index++;
> +		sort_parity_stripes(bbio, num_stripes);
>  	}
>  
> +

Stray newline.

Re: [PATCH 07/10] btrfs: Remove fail label in check_compressed_csum

[David Sterba]

On Thu, Jul 02, 2020 at 04:46:47PM +0300, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/compression.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 2f30bf4127f8..48ceab7be0fe 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -178,7 +178,6 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>  	SHASH_DESC_ON_STACK(shash, fs_info->csum_shash);
>  	const u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
> -	int ret;
>  	struct page *page;
>  	unsigned long i;
>  	char *kaddr;
> @@ -204,15 +203,12 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
>  			if (btrfs_io_bio(bio)->dev)
>  				btrfs_dev_stat_inc_and_print(btrfs_io_bio(bio)->dev,
>  					BTRFS_DEV_STAT_CORRUPTION_ERRS);
> -			ret = -EIO;
> -			goto fail;
> +			return -EIO;

Ok, that one looks obvious.

Re: [PATCH 02/10] btrfs: raid56: Remove redundant check in rbio_add_io_page

[Johannes Thumshirn]

On 02/07/2020 15:46, Nikolay Borisov wrote:
> The merging logic is always executed if the current stripe's device
> is not missing. So there's no point in duplicating the check. Simply
> remove it, while at it reduce the scope of the 'last_end' variable.
> 

Maybe add "If the current stripe's device is missing we fail the stripe early on"

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


> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/raid56.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 4efd9ed1a30e..f21bab45b7ce 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1083,7 +1083,6 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
>  			    unsigned long bio_max_len)
>  {
>  	struct bio *last = bio_list->tail;
> -	u64 last_end = 0;
>  	int ret;
>  	struct bio *bio;
>  	struct btrfs_bio_stripe *stripe;
> @@ -1098,15 +1097,14 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
>  
>  	/* see if we can add this page onto our existing bio */
>  	if (last) {
> -		last_end = (u64)last->bi_iter.bi_sector << 9;
> +		u64 last_end = (u64)last->bi_iter.bi_sector << 9;
>  		last_end += last->bi_iter.bi_size;
>  
>  		/*
>  		 * we can't merge these if they are from different
>  		 * devices or if they are not contiguous
>  		 */
> -		if (last_end == disk_start && stripe->dev->bdev &&
> -		    !last->bi_status &&
> +		if (last_end == disk_start && !last->bi_status &&
>  		    last->bi_disk == stripe->dev->bdev->bd_disk &&
>  		    last->bi_partno == stripe->dev->bdev->bd_partno) {
>  			ret = bio_add_page(last, page, PAGE_SIZE, 0);
> 

Re: [PATCH 09/10] btrfs: Remove fail2 label from btrfs_submit_compressed_read

[David Sterba]

On Thu, Jul 02, 2020 at 04:46:49PM +0300, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/compression.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 2033ce17e5c6..c28ee9fcd15d 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -661,8 +661,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	u64 em_len;
>  	u64 em_start;
>  	struct extent_map *em;
> -	blk_status_t ret = BLK_STS_RESOURCE;
> -	int faili = 0;
> +	blk_status_t ret;
>  	const u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
>  	u8 *sums;
>  
> @@ -712,12 +711,16 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  		cb->compressed_pages[pg_index] = alloc_page(GFP_NOFS |
>  							      __GFP_HIGHMEM);
>  		if (!cb->compressed_pages[pg_index]) {
> -			faili = pg_index - 1;
> -			ret = BLK_STS_RESOURCE;
> -			goto fail2;
> +			int i;
> +
> +			for (i = 0; i < pg_index; i++)
> +				__free_page(cb->compressed_pages[i]);
> +
> +			kfree(cb->compressed_pages);
> +			kfree(cb);
> +			return BLK_STS_RESOURCE;

No, that's pushing the error cleanup code to the main code path.

>  		}
>  	}
> -	faili = nr_pages - 1;
>  	cb->nr_pages = nr_pages;
>  
>  	add_ra_bio_pages(inode, em_start + em_len, cb);
> @@ -801,14 +804,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  
>  	return 0;
>  
> -fail2:
> -	while (faili >= 0) {
> -		__free_page(cb->compressed_pages[faili]);
> -		faili--;
> -	}
> -
> -	kfree(cb->compressed_pages);
> -	kfree(cb);
>  out:
>  	free_extent_map(em);
>  	return ret;

If there's anything to fix in this function, it's the labels and
variable naming. 'faili' should be eg. last_index, fail1 -> out_free_cb,
fail2 -> out_free_pages, and out -> out_free_map.

Re: [PATCH 03/10] btrfs: raid56: Assign bio in while()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 05/10] btrfs: raid56: Use in_range where applicable

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 06/10] btrfs: raid56: Don't opencode swap()

[Johannes Thumshirn]

Easy enough,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 10/10] btrfs: Remove out label in btrfs_submit_compressed_read

[Johannes Thumshirn]

On 02/07/2020 15:47, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/compression.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index c28ee9fcd15d..f9a9ec51a1ec 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -678,8 +678,10 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  
>  	compressed_len = em->block_len;
>  	cb = kmalloc(compressed_bio_size(fs_info, compressed_len), GFP_NOFS);
> -	if (!cb)
> -		goto out;
> +	if (!cb) {
> +		free_extent_map(em);
> +		return BLK_STS_RESOURCE;
> +	}


Agree with David here, please don't do cleanups here, keep it at the out label

Re: [PATCH 04/10] btrfs: raid56: Remove out label in __raid56_parity_recover

[Nikolay Borisov]

On 2.07.20 г. 17:02 ч., David Sterba wrote:
> On Thu, Jul 02, 2020 at 04:46:44PM +0300, Nikolay Borisov wrote:
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/raid56.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
>> index a7ae4d8a47ce..d9415a22617b 100644
>> --- a/fs/btrfs/raid56.c
>> +++ b/fs/btrfs/raid56.c
>> @@ -2093,7 +2093,7 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
>>  		 */
>>  		if (atomic_read(&rbio->error) <= rbio->bbio->max_errors) {
>>  			__raid_recover_end_io(rbio);
>> -			goto out;
>> +			return 0;
> 
> No please, when there are labels that do cleanup like the one in the
> context, 'return's make it harder to follow.
> 

But I'm not touching the cleanup hand of the if, rather the one which
simply returns 0. SO why jmp + ret when we can straight ret ?


>>  		} else {
>>  			goto cleanup;
>>  		}
>> @@ -2113,7 +2113,7 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
>>  
>>  		submit_bio(bio);
>>  	}
>> -out:
>> +
>>  	return 0;
>>  
>>  cleanup:
>> -- 
>> 2.17.1
> 

Re: [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers

[Nikolay Borisov]

On 2.07.20 г. 17:04 ч., Johannes Thumshirn wrote:
> On 02/07/2020 15:47, Nikolay Borisov wrote:
> [...]
>> -		bbio->raid_map = (u64 *)((void *)bbio->stripes +
>> -				 sizeof(struct btrfs_bio_stripe) *
>> -				 num_alloc_stripes +
>> -				 sizeof(int) * tgtdev_indexes);
> 
> That one took me a while to be convinced it is correct.

There are 2 aspects to this:

1. I think the original code is harder to grasp because the calculations
 for initializing raid_map/tgtdev ponters are apart from the initial
allocation of memory. Having them predicated on 2 separate checks
doesn't help that either... So by moving the initialisation in
alloc_btrfs_bio puts everything together.

2. The second is that tgtdev/raid_maps are now always initialized
despite sometimes they might be equal i.e __btrfs_map_block_for_discard
calls alloc_btrfs_bio with tgtdev = 0 but their usage should be
predicated on external checks i.e. just because those pointers are
non-null doesn't mean they are valid per-se. And actually while taking
another look at __btrfs_map_block I saw a discrepancy:

Original code initialised tgtdev_map if the following check is true:
if (dev_replace_is_ongoing && dev_replace->tgtdev != NULL)

However, further down tgtdev_map is only used if the following check is
true:
if (dev_replace_is_ongoing && dev_replace->tgtdev != NULL &&
need_full_stripe(op))

e.g. the additional need_full_stripe(op) predicate is there.


> 
>>  
>>  		/* Work out the disk rotation on this stripe-set */
>>  		div_u64_rem(stripe_nr, num_stripes, &rot);
>> @@ -6171,25 +6178,14 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
>>  		if (map->type & BTRFS_BLOCK_GROUP_RAID6)
>>  			bbio->raid_map[(i+rot+1) % num_stripes] =
>>  				RAID6_Q_STRIPE;
>> -	}
>> -
>>  
>> -	for (i = 0; i < num_stripes; i++) {
>> -		bbio->stripes[i].physical =
>> -			map->stripes[stripe_index].physical +
>> -			stripe_offset +
>> -			stripe_nr * map->stripe_len;
>> -		bbio->stripes[i].dev =
>> -			map->stripes[stripe_index].dev;
>> -		stripe_index++;
>> +		sort_parity_stripes(bbio, num_stripes);
>>  	}
>>  
>> +
> 
> Stray newline.
> 
> 

Re: [PATCH 05/10] btrfs: raid56: Use in_range where applicable

[David Sterba]

On Thu, Jul 02, 2020 at 04:46:45PM +0300, Nikolay Borisov wrote:
> While at it use the opportunity to simplify find_logical_bio_stripe by
> reducing the scope of 'stripe_start' variable and squash the
> sector-to-bytes conversion on one line.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/raid56.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index d9415a22617b..d89dd3030bba 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1349,7 +1349,6 @@ static int find_bio_stripe(struct btrfs_raid_bio *rbio,
>  			   struct bio *bio)
>  {
>  	u64 physical = bio->bi_iter.bi_sector;
> -	u64 stripe_start;
>  	int i;
>  	struct btrfs_bio_stripe *stripe;
>  
> @@ -1357,9 +1356,7 @@ static int find_bio_stripe(struct btrfs_raid_bio *rbio,
>  
>  	for (i = 0; i < rbio->bbio->num_stripes; i++) {
>  		stripe = &rbio->bbio->stripes[i];
> -		stripe_start = stripe->physical;
> -		if (physical >= stripe_start &&
> -		    physical < stripe_start + rbio->stripe_len &&
> +		if (in_range(physical, stripe->physical, rbio->stripe_len) &&
>  		    stripe->dev->bdev &&
>  		    bio->bi_disk == stripe->dev->bdev->bd_disk &&
>  		    bio->bi_partno == stripe->dev->bdev->bd_partno) {
> @@ -1377,18 +1374,13 @@ static int find_bio_stripe(struct btrfs_raid_bio *rbio,
>  static int find_logical_bio_stripe(struct btrfs_raid_bio *rbio,
>  				   struct bio *bio)
>  {
> -	u64 logical = bio->bi_iter.bi_sector;
> -	u64 stripe_start;
> +	u64 logical = bio->bi_iter.bi_sector << 9;

FYI, if bi_iter::bi_sector is shifted left or right, it needs the u64
cast because the type sector_t is not always 64bit for some reasons.
We've had bugs with the conversion on 32bit arches, for consistency it
needs to be there.

>  	int i;
>  
> -	logical <<= 9;
> -
>  	for (i = 0; i < rbio->nr_data; i++) {
> -		stripe_start = rbio->bbio->raid_map[i];
> -		if (logical >= stripe_start &&
> -		    logical < stripe_start + rbio->stripe_len) {
> +		u64 stripe_start = rbio->bbio->raid_map[i];
> +		if (in_range(logical, stripe_start, rbio->stripe_len))
>  			return i;
> -		}
>  	}
>  	return -1;
>  }
> -- 
> 2.17.1

Re: [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers

[David Sterba]

On Fri, Jul 03, 2020 at 11:31:02AM +0300, Nikolay Borisov wrote:
> 
> 
> On 2.07.20 г. 17:04 ч., Johannes Thumshirn wrote:
> > On 02/07/2020 15:47, Nikolay Borisov wrote:
> > [...]
> >> -		bbio->raid_map = (u64 *)((void *)bbio->stripes +
> >> -				 sizeof(struct btrfs_bio_stripe) *
> >> -				 num_alloc_stripes +
> >> -				 sizeof(int) * tgtdev_indexes);
> > 
> > That one took me a while to be convinced it is correct.
> 
> There are 2 aspects to this:
> 
> 1. I think the original code is harder to grasp ...

Agreed, the rework is much better. Though understanding that's an
equivalent change is tough. I'll update the changelog with the
explanation.

Re: [PATCH 00/10] A bunch of misc cleanups

[David Sterba]

On Thu, Jul 02, 2020 at 04:46:40PM +0300, Nikolay Borisov wrote:
> Here's an assortment of little quality-of-life patches that I created while
> looking into the raid56 code. They should bear no functional changes and have
> tested them with xfstest and nothing fell over so should be rather low risk.
> 
> Patch 1 moves code in __btrfs_map_block, essentially assigning tgtdev_map/raid_map
> closet to where space for them is allocated. This also neccesiated moving the
> call to sort_parity_stripes. The end result is (hopefully) slightly easier to
> follow __btrfs_map_block.
> 
> Next 5 patches cleanup minor things in raid56.c such as removing redundant checks,
> making code interacting with bio_list more in line with what the rest of the
> kernel is doing. Finally it's using some macros/functions instead of open-coding
> them. Really just a bunch of low hanging fruit.
> 
> Final 4 patches gradually remove all labels in btrfs_submit_compressed_read.
> Current failures can be handled "inline" so to speak, without the need for
> extra labels. This likely will change once the BUG_ONs are removed but we are
> not there yet.
> 
> Nikolay Borisov (10):
>   btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers
>   btrfs: raid56: Remove redundant check in rbio_add_io_page
>   btrfs: raid56: Assign bio in while()
>   btrfs: raid56: Remove out label in __raid56_parity_recover
>   btrfs: raid56: Use in_range where applicable
>   btrfs: raid56: Don't opencode swap()
>   btrfs: Remove fail label in check_compressed_csum
>   btrfs: Remove fail1 label in btrfs_submit_compressed_read
>   btrfs: Remove fail2 label from btrfs_submit_compressed_read
>   btrfs: Remove out label in btrfs_submit_compressed_read

Except patches 4, 8, 9, 10, series merged to misc-next. We can have a
look at the label/return cleanups next week, the other cleanups are good
so I don't want to stall this patchset. Thanks.

Re: [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers

[Johannes Thumshirn]

On 03/07/2020 17:58, David Sterba wrote:
> On Fri, Jul 03, 2020 at 11:31:02AM +0300, Nikolay Borisov wrote:
>>
>>
>> On 2.07.20 г. 17:04 ч., Johannes Thumshirn wrote:
>>> On 02/07/2020 15:47, Nikolay Borisov wrote:
>>> [...]
>>>> -		bbio->raid_map = (u64 *)((void *)bbio->stripes +
>>>> -				 sizeof(struct btrfs_bio_stripe) *
>>>> -				 num_alloc_stripes +
>>>> -				 sizeof(int) * tgtdev_indexes);
>>>
>>> That one took me a while to be convinced it is correct.
>>
>> There are 2 aspects to this:
>>
>> 1. I think the original code is harder to grasp ...
> 
> Agreed, the rework is much better. Though understanding that's an
> equivalent change is tough. I'll update the changelog with the
> explanation.
> 

This was my point, I'm sorry if this didn't come along correctly.