* [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
@ 2020-07-02 13:46 ` Nikolay Borisov
2020-07-02 14:04 ` Johannes Thumshirn
2020-07-02 13:46 ` [PATCH 02/10] btrfs: raid56: Remove redundant check in rbio_add_io_page Nikolay Borisov
` (9 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 13:46 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers
2020-07-02 13:46 ` [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers Nikolay Borisov
@ 2020-07-02 14:04 ` Johannes Thumshirn
2020-07-03 8:31 ` Nikolay Borisov
0 siblings, 1 reply; 27+ messages in thread
From: Johannes Thumshirn @ 2020-07-02 14:04 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers
2020-07-02 14:04 ` Johannes Thumshirn
@ 2020-07-03 8:31 ` Nikolay Borisov
2020-07-03 15:57 ` David Sterba
0 siblings, 1 reply; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-03 8:31 UTC (permalink / raw)
To: Johannes Thumshirn, linux-btrfs@vger.kernel.org
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.
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers
2020-07-03 8:31 ` Nikolay Borisov
@ 2020-07-03 15:57 ` David Sterba
2020-07-06 6:38 ` Johannes Thumshirn
0 siblings, 1 reply; 27+ messages in thread
From: David Sterba @ 2020-07-03 15:57 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: Johannes Thumshirn, linux-btrfs@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers
2020-07-03 15:57 ` David Sterba
@ 2020-07-06 6:38 ` Johannes Thumshirn
0 siblings, 0 replies; 27+ messages in thread
From: Johannes Thumshirn @ 2020-07-06 6:38 UTC (permalink / raw)
To: dsterba@suse.cz, Nikolay Borisov; +Cc: linux-btrfs@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 02/10] btrfs: raid56: Remove redundant check in rbio_add_io_page
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
2020-07-02 13:46 ` [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers Nikolay Borisov
@ 2020-07-02 13:46 ` Nikolay Borisov
2020-07-02 14:12 ` Johannes Thumshirn
2020-07-02 13:46 ` [PATCH 03/10] btrfs: raid56: Assign bio in while() Nikolay Borisov
` (8 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 13:46 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 02/10] btrfs: raid56: Remove redundant check in rbio_add_io_page
2020-07-02 13:46 ` [PATCH 02/10] btrfs: raid56: Remove redundant check in rbio_add_io_page Nikolay Borisov
@ 2020-07-02 14:12 ` Johannes Thumshirn
0 siblings, 0 replies; 27+ messages in thread
From: Johannes Thumshirn @ 2020-07-02 14:12 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs@vger.kernel.org
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);
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 03/10] btrfs: raid56: Assign bio in while()
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
2020-07-02 13:46 ` [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers Nikolay Borisov
2020-07-02 13:46 ` [PATCH 02/10] btrfs: raid56: Remove redundant check in rbio_add_io_page Nikolay Borisov
@ 2020-07-02 13:46 ` Nikolay Borisov
2020-07-02 14:14 ` Johannes Thumshirn
2020-07-02 13:46 ` [PATCH 04/10] btrfs: raid56: Remove out label in __raid56_parity_recover Nikolay Borisov
` (7 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 13:46 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 04/10] btrfs: raid56: Remove out label in __raid56_parity_recover
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
` (2 preceding siblings ...)
2020-07-02 13:46 ` [PATCH 03/10] btrfs: raid56: Assign bio in while() Nikolay Borisov
@ 2020-07-02 13:46 ` Nikolay Borisov
2020-07-02 14:02 ` David Sterba
2020-07-02 13:46 ` [PATCH 05/10] btrfs: raid56: Use in_range where applicable Nikolay Borisov
` (6 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 13:46 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 04/10] btrfs: raid56: Remove out label in __raid56_parity_recover
2020-07-02 13:46 ` [PATCH 04/10] btrfs: raid56: Remove out label in __raid56_parity_recover Nikolay Borisov
@ 2020-07-02 14:02 ` David Sterba
2020-07-02 14:51 ` Nikolay Borisov
0 siblings, 1 reply; 27+ messages in thread
From: David Sterba @ 2020-07-02 14:02 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 04/10] btrfs: raid56: Remove out label in __raid56_parity_recover
2020-07-02 14:02 ` David Sterba
@ 2020-07-02 14:51 ` Nikolay Borisov
0 siblings, 0 replies; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 14:51 UTC (permalink / raw)
To: dsterba, linux-btrfs
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 05/10] btrfs: raid56: Use in_range where applicable
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
` (3 preceding siblings ...)
2020-07-02 13:46 ` [PATCH 04/10] btrfs: raid56: Remove out label in __raid56_parity_recover Nikolay Borisov
@ 2020-07-02 13:46 ` Nikolay Borisov
2020-07-02 14:19 ` Johannes Thumshirn
2020-07-03 15:45 ` David Sterba
2020-07-02 13:46 ` [PATCH 06/10] btrfs: raid56: Don't opencode swap() Nikolay Borisov
` (5 subsequent siblings)
10 siblings, 2 replies; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 13:46 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 05/10] btrfs: raid56: Use in_range where applicable
2020-07-02 13:46 ` [PATCH 05/10] btrfs: raid56: Use in_range where applicable Nikolay Borisov
@ 2020-07-02 14:19 ` Johannes Thumshirn
2020-07-03 15:45 ` David Sterba
1 sibling, 0 replies; 27+ messages in thread
From: Johannes Thumshirn @ 2020-07-02 14:19 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs@vger.kernel.org
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 05/10] btrfs: raid56: Use in_range where applicable
2020-07-02 13:46 ` [PATCH 05/10] btrfs: raid56: Use in_range where applicable Nikolay Borisov
2020-07-02 14:19 ` Johannes Thumshirn
@ 2020-07-03 15:45 ` David Sterba
1 sibling, 0 replies; 27+ messages in thread
From: David Sterba @ 2020-07-03 15:45 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 06/10] btrfs: raid56: Don't opencode swap()
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
` (4 preceding siblings ...)
2020-07-02 13:46 ` [PATCH 05/10] btrfs: raid56: Use in_range where applicable Nikolay Borisov
@ 2020-07-02 13:46 ` Nikolay Borisov
2020-07-02 14:20 ` Johannes Thumshirn
2020-07-02 13:46 ` [PATCH 07/10] btrfs: Remove fail label in check_compressed_csum Nikolay Borisov
` (4 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 13:46 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 07/10] btrfs: Remove fail label in check_compressed_csum
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
` (5 preceding siblings ...)
2020-07-02 13:46 ` [PATCH 06/10] btrfs: raid56: Don't opencode swap() Nikolay Borisov
@ 2020-07-02 13:46 ` Nikolay Borisov
2020-07-02 14:10 ` David Sterba
2020-07-02 13:46 ` [PATCH 08/10] btrfs: Remove fail1 label in btrfs_submit_compressed_read Nikolay Borisov
` (3 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 13:46 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 07/10] btrfs: Remove fail label in check_compressed_csum
2020-07-02 13:46 ` [PATCH 07/10] btrfs: Remove fail label in check_compressed_csum Nikolay Borisov
@ 2020-07-02 14:10 ` David Sterba
0 siblings, 0 replies; 27+ messages in thread
From: David Sterba @ 2020-07-02 14:10 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 08/10] btrfs: Remove fail1 label in btrfs_submit_compressed_read
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
` (6 preceding siblings ...)
2020-07-02 13:46 ` [PATCH 07/10] btrfs: Remove fail label in check_compressed_csum Nikolay Borisov
@ 2020-07-02 13:46 ` Nikolay Borisov
2020-07-02 14:03 ` David Sterba
2020-07-02 13:46 ` [PATCH 09/10] btrfs: Remove fail2 label from btrfs_submit_compressed_read Nikolay Borisov
` (2 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 13:46 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 08/10] btrfs: Remove fail1 label in btrfs_submit_compressed_read
2020-07-02 13:46 ` [PATCH 08/10] btrfs: Remove fail1 label in btrfs_submit_compressed_read Nikolay Borisov
@ 2020-07-02 14:03 ` David Sterba
0 siblings, 0 replies; 27+ messages in thread
From: David Sterba @ 2020-07-02 14:03 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 09/10] btrfs: Remove fail2 label from btrfs_submit_compressed_read
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
` (7 preceding siblings ...)
2020-07-02 13:46 ` [PATCH 08/10] btrfs: Remove fail1 label in btrfs_submit_compressed_read Nikolay Borisov
@ 2020-07-02 13:46 ` Nikolay Borisov
2020-07-02 14:14 ` David Sterba
2020-07-02 13:46 ` [PATCH 10/10] btrfs: Remove out label in btrfs_submit_compressed_read Nikolay Borisov
2020-07-03 16:21 ` [PATCH 00/10] A bunch of misc cleanups David Sterba
10 siblings, 1 reply; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 13:46 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 09/10] btrfs: Remove fail2 label from btrfs_submit_compressed_read
2020-07-02 13:46 ` [PATCH 09/10] btrfs: Remove fail2 label from btrfs_submit_compressed_read Nikolay Borisov
@ 2020-07-02 14:14 ` David Sterba
0 siblings, 0 replies; 27+ messages in thread
From: David Sterba @ 2020-07-02 14:14 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 10/10] btrfs: Remove out label in btrfs_submit_compressed_read
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
` (8 preceding siblings ...)
2020-07-02 13:46 ` [PATCH 09/10] btrfs: Remove fail2 label from btrfs_submit_compressed_read Nikolay Borisov
@ 2020-07-02 13:46 ` Nikolay Borisov
2020-07-02 14:23 ` Johannes Thumshirn
2020-07-03 16:21 ` [PATCH 00/10] A bunch of misc cleanups David Sterba
10 siblings, 1 reply; 27+ messages in thread
From: Nikolay Borisov @ 2020-07-02 13:46 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 10/10] btrfs: Remove out label in btrfs_submit_compressed_read
2020-07-02 13:46 ` [PATCH 10/10] btrfs: Remove out label in btrfs_submit_compressed_read Nikolay Borisov
@ 2020-07-02 14:23 ` Johannes Thumshirn
0 siblings, 0 replies; 27+ messages in thread
From: Johannes Thumshirn @ 2020-07-02 14:23 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs@vger.kernel.org
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 00/10] A bunch of misc cleanups
2020-07-02 13:46 [PATCH 00/10] A bunch of misc cleanups Nikolay Borisov
` (9 preceding siblings ...)
2020-07-02 13:46 ` [PATCH 10/10] btrfs: Remove out label in btrfs_submit_compressed_read Nikolay Borisov
@ 2020-07-03 16:21 ` David Sterba
10 siblings, 0 replies; 27+ messages in thread
From: David Sterba @ 2020-07-03 16:21 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 27+ messages in thread