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

From: Nikolay Borisov <nborisov@suse.com>
To: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
	"linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH 01/10] btrfs: Always initialize btrfs_bio::tgtdev_map/raid_map pointers
Date: Fri, 3 Jul 2020 11:31:02 +0300	[thread overview]
Message-ID: <ac973720-9d3b-1824-e7de-16e15b364c9c@suse.com> (raw)
In-Reply-To: <SN4PR0401MB359800E3D7D379E9161318379B6D0@SN4PR0401MB3598.namprd04.prod.outlook.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.
> 
> 