[01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers
diff mbox series

Message ID 20200702134650.16550-2-nborisov@suse.com
State New
Headers show
Series
  • A bunch of misc cleanups
Related show

Commit Message

Nikolay Borisov July 2, 2020, 1:46 p.m. UTC
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(-)

Comments

Johannes Thumshirn July 2, 2020, 2:04 p.m. UTC | #1
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.
Nikolay Borisov July 3, 2020, 8:31 a.m. UTC | #2
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.
> 
>
David Sterba July 3, 2020, 3:57 p.m. UTC | #3
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.
Johannes Thumshirn July 6, 2020, 6:38 a.m. UTC | #4
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.

Patch
diff mbox series

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,