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

[Nikolay Borisov <nborisov@suse.com>]

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.
> 
>