[dm-devel] [PATCH 03/12] dm-zoned: use on-stack superblock for tertiary devices

[Hannes Reinecke]

On 5/25/20 4:09 AM, Damien Le Moal wrote:
> On 2020/05/23 0:39, Hannes Reinecke wrote:
>> Checking the teriary superblock just consists of validating UUIDs,
> 
> s/teriary/tertiary
> 
>> crcs, and the generation number; it doesn't have contents which
>> would be required during the actual operation.
>> So we should use an on-stack superblock and avoid having to store
>> it together with the 'real' superblocks.
> 
> ...a temoprary in-memory superblock allocation...
> 
> The entire structure should not be on stack... see below.
> 
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>> ---
>>   drivers/md/dm-zoned-metadata.c | 98 +++++++++++++++++++++++-------------------
>>   1 file changed, 53 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
>> index 3da6702bb1ae..b70a988fa771 100644
>> --- a/drivers/md/dm-zoned-metadata.c
>> +++ b/drivers/md/dm-zoned-metadata.c
[ .. ]
>> @@ -1326,18 +1327,32 @@ static int dmz_load_sb(struct dmz_metadata *zmd)
>>   		      "Using super block %u (gen %llu)",
>>   		      zmd->mblk_primary, zmd->sb_gen);
>>   
>> -	if ((zmd->sb_version > 1) && zmd->sb[2].zone) {
>> -		zmd->sb[2].block = dmz_start_block(zmd, zmd->sb[2].zone);
>> -		zmd->sb[2].dev = dmz_zone_to_dev(zmd, zmd->sb[2].zone);
>> -		ret = dmz_get_sb(zmd, 2);
>> -		if (ret) {
>> -			dmz_dev_err(zmd->sb[2].dev,
>> -				    "Read tertiary super block failed");
>> -			return ret;
>> +	if (zmd->sb_version > 1) {
>> +		int i;
>> +
>> +		for (i = 1; i < zmd->nr_devs; i++) {
>> +			struct dmz_sb sb;
> 
> I would rather have dmz_get_sb() allocate this struct than have it on stack...
> It is not big, but still. To be symetric, we can add dmz_put_sb() for freeing it.
> 
While I do agree to not having it on the stack, having dmz_get_sb() 
returning the structure would require (yet another) overhaul of the
main metadata structure which currently has the primary and secondary
superblocks embedded.
And I would argue to keep it that way, as the primary and secondary 
superblocks are essential to the actual operation. So allocating them 
separately would mean yet another indirection to get to them.
At the same time, any tertiary superblock is just used for validation
during startup, and not referenced anywhere afterwards.
So using kzalloc() here and freeing them after checking is fine.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer