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 <[email protected]>
---
  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
@@ -174,7 +174,7 @@ struct dmz_metadata {
        /* Zone information array */
        struct xarray           zones;
- struct dmz_sb sb[3];
+       struct dmz_sb           sb[2];
        unsigned int            mblk_primary;
        unsigned int            sb_version;
        u64                     sb_gen;
@@ -995,10 +995,11 @@ int dmz_flush_metadata(struct dmz_metadata *zmd)
  /*
   * Check super block.
   */
-static int dmz_check_sb(struct dmz_metadata *zmd, unsigned int set)
+static int dmz_check_sb(struct dmz_metadata *zmd, struct dmz_sb *dsb,
+                       bool tertiary)
  {
-       struct dmz_super *sb = zmd->sb[set].sb;
-       struct dmz_dev *dev = zmd->sb[set].dev;
+       struct dmz_super *sb = dsb->sb;
+       struct dmz_dev *dev = dsb->dev;
        unsigned int nr_meta_zones, nr_data_zones;
        u32 crc, stored_crc;
        u64 gen;
@@ -1015,7 +1016,7 @@ static int dmz_check_sb(struct dmz_metadata *zmd, 
unsigned int set)
                            DMZ_META_VER, zmd->sb_version);
                return -EINVAL;
        }
-       if ((zmd->sb_version < 1) && (set == 2)) {
+       if ((zmd->sb_version < 1) && tertiary) {
                dmz_dev_err(dev, "Tertiary superblocks are not supported");
                return -EINVAL;
        }
@@ -1059,7 +1060,7 @@ static int dmz_check_sb(struct dmz_metadata *zmd, 
unsigned int set)
                        return -ENXIO;
                }
- if (set == 2) {
+               if (tertiary) {
                        /*
                         * Generation number should be 0, but it doesn't
                         * really matter if it isn't.
@@ -1108,13 +1109,13 @@ static int dmz_check_sb(struct dmz_metadata *zmd, 
unsigned int set)
  /*
   * Read the first or second super block from disk.
   */
-static int dmz_read_sb(struct dmz_metadata *zmd, unsigned int set)
+static int dmz_read_sb(struct dmz_metadata *zmd, struct dmz_sb *sb, int set)
  {
        DMDEBUG("(%s): read superblock set %d dev %s block %llu",
                zmd->devname, set, zmd->sb[set].dev->name,
                zmd->sb[set].block);
-       return dmz_rdwr_block(zmd->sb[set].dev, REQ_OP_READ,
-                             zmd->sb[set].block, zmd->sb[set].mblk->page);
+       return dmz_rdwr_block(sb->dev, REQ_OP_READ,
+                             sb->block, sb->mblk->page);
  }
/*
@@ -1142,7 +1143,7 @@ static int dmz_lookup_secondary_sb(struct dmz_metadata 
*zmd)
        zmd->sb[1].zone = xa_load(&zmd->zones, zone_id + 1);
        zmd->sb[1].dev = dmz_zone_to_dev(zmd, zmd->sb[1].zone);
        for (i = 1; i < zmd->nr_rnd_zones; i++) {
-               if (dmz_read_sb(zmd, 1) != 0)
+               if (dmz_read_sb(zmd, &zmd->sb[1], 1) != 0)
                        break;
                if (le32_to_cpu(zmd->sb[1].sb->magic) == DMZ_MAGIC)
                        return 0;
@@ -1160,9 +1161,9 @@ static int dmz_lookup_secondary_sb(struct dmz_metadata 
*zmd)
  }
/*
- * Read the first or second super block from disk.
+ * Read a super block from disk.
   */
-static int dmz_get_sb(struct dmz_metadata *zmd, unsigned int set)
+static int dmz_get_sb(struct dmz_metadata *zmd, struct dmz_sb *sb, int set)
  {
        struct dmz_mblock *mblk;
        int ret;
@@ -1172,14 +1173,14 @@ static int dmz_get_sb(struct dmz_metadata *zmd, 
unsigned int set)
        if (!mblk)
                return -ENOMEM;
- zmd->sb[set].mblk = mblk;
-       zmd->sb[set].sb = mblk->data;
+       sb->mblk = mblk;
+       sb->sb = mblk->data;
/* Read super block */
-       ret = dmz_read_sb(zmd, set);
+       ret = dmz_read_sb(zmd, sb, set);
        if (ret) {
                dmz_free_mblock(zmd, mblk);
-               zmd->sb[set].mblk = NULL;
+               sb->mblk = NULL;
                return ret;
        }
@@ -1253,13 +1254,13 @@ static int dmz_load_sb(struct dmz_metadata *zmd)
        /* Read and check the primary super block */
        zmd->sb[0].block = dmz_start_block(zmd, zmd->sb[0].zone);
        zmd->sb[0].dev = dmz_zone_to_dev(zmd, zmd->sb[0].zone);
-       ret = dmz_get_sb(zmd, 0);
+       ret = dmz_get_sb(zmd, &zmd->sb[0], 0);
        if (ret) {
                dmz_dev_err(zmd->sb[0].dev, "Read primary super block failed");
                return ret;
        }
- ret = dmz_check_sb(zmd, 0);
+       ret = dmz_check_sb(zmd, &zmd->sb[0], false);
/* Read and check secondary super block */
        if (ret == 0) {
@@ -1272,7 +1273,7 @@ static int dmz_load_sb(struct dmz_metadata *zmd)
                }
                zmd->sb[1].block = dmz_start_block(zmd, zmd->sb[1].zone);
                zmd->sb[1].dev = dmz_zone_to_dev(zmd, zmd->sb[1].zone);
-               ret = dmz_get_sb(zmd, 1);
+               ret = dmz_get_sb(zmd, &zmd->sb[1], 1);
        } else
                ret = dmz_lookup_secondary_sb(zmd);
@@ -1281,7 +1282,7 @@ static int dmz_load_sb(struct dmz_metadata *zmd)
                return ret;
        }
- ret = dmz_check_sb(zmd, 1);
+       ret = dmz_check_sb(zmd, &zmd->sb[1], false);
        if (ret == 0)
                sb_good[1] = true;
@@ -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.


Okay, no big deal.
I'll convert it to allocate one.

Cheers,

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


--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Reply via email to