On 2020/05/26 17:25, Hannes Reinecke wrote: > 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@xxxxxxx> >>> --- >>> 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. OK. Works for me. > > Cheers, > > Hannes > -- Damien Le Moal Western Digital Research -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel