Re: [dm-devel] [PATCH 03/12] dm-zoned: use on-stack superblock for tertiary devices
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
>>> ---
>>> 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@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 03/12] dm-zoned: use on-stack superblock for tertiary devices
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
---
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 ReineckeTeamlead Storage & Networking
h...@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
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 03/12] dm-zoned: use on-stack superblock for tertiary devices
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
---
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 intmblk_primary;
unsigned intsb_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(>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, >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,
Re: [dm-devel] [PATCH 03/12] dm-zoned: use on-stack superblock for tertiary devices
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
> ---
> 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 intmblk_primary;
> unsigned intsb_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(>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, >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,
[dm-devel] [PATCH 03/12] dm-zoned: use on-stack superblock for tertiary devices
Checking the teriary superblock just consists of validating UUIDs,
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.
Signed-off-by: Hannes Reinecke
---
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 intmblk_primary;
unsigned intsb_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(>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, >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, >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,
