[PATCH RFC 0/4] dm-zoned: Metadata V2

[PATCH RFC 0/4] dm-zoned: Metadata V2

[Hannes Reinecke]

Hi all,

here's a patchset to add UUIDs and labels to the metadata,
increasing the metadata version number to 2.
This allows to identify the various devices, and also to
have a persistent device-mapper name based on the label.
To handle and generate these new values I've also send
a pull request to the upstream dm-zoned-tools git repository.

This patchset requires the previous patch to add status
and message callbacks.

As usual, comments and reviews are welcome.

Hannes Reinecke (4):
  dm-zoned: store zone id within the zone structure
  dm-zoned: use array for superblock zones
  dm-zoned: V2 metadata handling
  dm-zoned: allow for device size smaller than the capacity

 drivers/md/dm-zoned-metadata.c | 117 ++++++++++++++++++++++++++++++-----------
 drivers/md/dm-zoned-target.c   |   4 +-
 drivers/md/dm-zoned.h          |   3 ++
 3 files changed, 91 insertions(+), 33 deletions(-)

-- 
2.16.4

[PATCH 1/4] dm-zoned: store zone id within the zone structure

[Hannes Reinecke]

Instead of calculating the zone index by the offset within the
zone array store the index within the structure itself.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-zoned-metadata.c | 3 ++-
 drivers/md/dm-zoned.h          | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
index c8787560fa9f..afce594067fb 100644
--- a/drivers/md/dm-zoned-metadata.c
+++ b/drivers/md/dm-zoned-metadata.c
@@ -189,7 +189,7 @@ struct dmz_metadata {
  */
 unsigned int dmz_id(struct dmz_metadata *zmd, struct dm_zone *zone)
 {
-	return ((unsigned int)(zone - zmd->zones));
+	return zone->id;
 }
 
 sector_t dmz_start_sect(struct dmz_metadata *zmd, struct dm_zone *zone)
@@ -1119,6 +1119,7 @@ static int dmz_init_zone(struct blk_zone *blkz, unsigned int idx, void *data)
 
 	INIT_LIST_HEAD(&zone->link);
 	atomic_set(&zone->refcount, 0);
+	zone->id = idx;
 	zone->chunk = DMZ_MAP_UNMAPPED;
 
 	switch (blkz->type) {
diff --git a/drivers/md/dm-zoned.h b/drivers/md/dm-zoned.h
index 884c0e586082..39d59898abbe 100644
--- a/drivers/md/dm-zoned.h
+++ b/drivers/md/dm-zoned.h
@@ -87,6 +87,9 @@ struct dm_zone {
 	/* Zone activation reference count */
 	atomic_t		refcount;
 
+	/* Zone id */
+	unsigned int		id;
+
 	/* Zone write pointer block (relative to the zone start block) */
 	unsigned int		wp_block;
 
-- 
2.16.4

[PATCH 2/4] dm-zoned: use array for superblock zones

[Hannes Reinecke]

Instead of storing just the first superblock zone and calculate
the secondary relative to that we should be using an array for
holding the superblock zones.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-zoned-metadata.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
index afce594067fb..dc1d17bc3bbb 100644
--- a/drivers/md/dm-zoned-metadata.c
+++ b/drivers/md/dm-zoned-metadata.c
@@ -124,6 +124,7 @@ struct dmz_sb {
 	sector_t		block;
 	struct dmz_mblock	*mblk;
 	struct dmz_super	*sb;
+	struct dm_zone		*zone;
 };
 
 /*
@@ -150,7 +151,6 @@ struct dmz_metadata {
 	/* Zone information array */
 	struct dm_zone		*zones;
 
-	struct dm_zone		*sb_zone;
 	struct dmz_sb		sb[2];
 	unsigned int		mblk_primary;
 	u64			sb_gen;
@@ -937,16 +937,20 @@ static int dmz_lookup_secondary_sb(struct dmz_metadata *zmd)
 
 	/* Bad first super block: search for the second one */
 	zmd->sb[1].block = zmd->sb[0].block + zone_nr_blocks;
+	zmd->sb[1].zone = zmd->sb[0].zone + 1;
 	for (i = 0; i < zmd->nr_rnd_zones - 1; i++) {
 		if (dmz_read_sb(zmd, 1) != 0)
 			break;
-		if (le32_to_cpu(zmd->sb[1].sb->magic) == DMZ_MAGIC)
+		if (le32_to_cpu(zmd->sb[1].sb->magic) == DMZ_MAGIC) {
+			zmd->sb[1].zone += i;
 			return 0;
+		}
 		zmd->sb[1].block += zone_nr_blocks;
 	}
 
 	dmz_free_mblock(zmd, mblk);
 	zmd->sb[1].mblk = NULL;
+	zmd->sb[1].zone = NULL;
 
 	return -EIO;
 }
@@ -990,11 +994,9 @@ static int dmz_recover_mblocks(struct dmz_metadata *zmd, unsigned int dst_set)
 	dmz_dev_warn(zmd->dev, "Metadata set %u invalid: recovering", dst_set);
 
 	if (dst_set == 0)
-		zmd->sb[0].block = dmz_start_block(zmd, zmd->sb_zone);
-	else {
-		zmd->sb[1].block = zmd->sb[0].block +
-			(zmd->nr_meta_zones << zmd->dev->zone_nr_blocks_shift);
-	}
+		zmd->sb[0].block = dmz_start_block(zmd, zmd->sb[0].zone);
+	else
+		zmd->sb[1].block = dmz_start_block(zmd, zmd->sb[1].zone);
 
 	page = alloc_page(GFP_NOIO);
 	if (!page)
@@ -1038,8 +1040,13 @@ static int dmz_load_sb(struct dmz_metadata *zmd)
 	u64 sb_gen[2] = {0, 0};
 	int ret;
 
+	if (!zmd->sb[0].zone) {
+		dmz_dev_err(zmd->dev, "Primary super block zone not set");
+		return -ENXIO;
+	}
+
 	/* Read and check the primary super block */
-	zmd->sb[0].block = dmz_start_block(zmd, zmd->sb_zone);
+	zmd->sb[0].block = dmz_start_block(zmd, zmd->sb[0].zone);
 	ret = dmz_get_sb(zmd, 0);
 	if (ret) {
 		dmz_dev_err(zmd->dev, "Read primary super block failed");
@@ -1051,8 +1058,9 @@ static int dmz_load_sb(struct dmz_metadata *zmd)
 	/* Read and check secondary super block */
 	if (ret == 0) {
 		sb_good[0] = true;
-		zmd->sb[1].block = zmd->sb[0].block +
-			(zmd->nr_meta_zones << zmd->dev->zone_nr_blocks_shift);
+		if (!zmd->sb[1].zone)
+			zmd->sb[1].zone = zmd->sb[0].zone + zmd->nr_meta_zones;
+		zmd->sb[1].block = dmz_start_block(zmd, zmd->sb[1].zone);
 		ret = dmz_get_sb(zmd, 1);
 	} else
 		ret = dmz_lookup_secondary_sb(zmd);
@@ -1147,9 +1155,9 @@ static int dmz_init_zone(struct blk_zone *blkz, unsigned int idx, void *data)
 		zmd->nr_useable_zones++;
 		if (dmz_is_rnd(zone)) {
 			zmd->nr_rnd_zones++;
-			if (!zmd->sb_zone) {
+			if (!zmd->sb[0].zone) {
 				/* Super block zone */
-				zmd->sb_zone = zone;
+				zmd->sb[0].zone = zone;
 			}
 		}
 	}
@@ -2420,7 +2428,7 @@ int dmz_ctr_metadata(struct dmz_dev *dev, struct dmz_metadata **metadata)
 		goto err;
 
 	/* Set metadata zones starting from sb_zone */
-	zid = dmz_id(zmd, zmd->sb_zone);
+	zid = dmz_id(zmd, zmd->sb[0].zone);
 	for (i = 0; i < zmd->nr_meta_zones << 1; i++) {
 		zone = dmz_get(zmd, zid + i);
 		if (!dmz_is_rnd(zone))
-- 
2.16.4

[PATCH 3/4] dm-zoned: V2 metadata handling

[Hannes Reinecke]

Add 'V2' metadata which includes UUIDs for the dmz set and for
the device itself.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-zoned-metadata.c | 80 +++++++++++++++++++++++++++++++++---------
 drivers/md/dm-zoned-target.c   |  2 +-
 2 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
index dc1d17bc3bbb..026f285fba33 100644
--- a/drivers/md/dm-zoned-metadata.c
+++ b/drivers/md/dm-zoned-metadata.c
@@ -16,7 +16,8 @@
 /*
  * Metadata version.
  */
-#define DMZ_META_VER	1
+#define DMZ_META_COMPAT_VER 1
+#define DMZ_META_VER	2
 
 /*
  * On-disk super block magic.
@@ -69,8 +70,17 @@ struct dmz_super {
 	/* Checksum */
 	__le32		crc;			/*  48 */
 
-	/* Padding to full 512B sector */
-	u8		reserved[464];		/* 512 */
+	/* DM-Zoned label */
+	u8		dmz_label[32];		/*  80 */
+
+	/* DM-Zoned UUID */
+	u8		dmz_uuid[16];		/*  96 */
+
+	/* Device UUID */
+	u8		dev_uuid[16];		/* 112 */
+
+	/* Padding to full 512B - CRC sector */
+	u8		reserved[400];		/* 512 */
 };
 
 /*
@@ -133,6 +143,10 @@ struct dmz_sb {
 struct dmz_metadata {
 	struct dmz_dev		*dev;
 
+	char			dmz_label[BDEVNAME_SIZE];
+	uuid_t			dmz_uuid;
+	uuid_t			dev_uuid;
+
 	sector_t		zone_bitmap_size;
 	unsigned int		zone_nr_bitmap_blocks;
 	unsigned int		zone_bits_per_mblk;
@@ -659,7 +673,14 @@ static int dmz_write_sb(struct dmz_metadata *zmd, unsigned int set)
 	int ret;
 
 	sb->magic = cpu_to_le32(DMZ_MAGIC);
-	sb->version = cpu_to_le32(DMZ_META_VER);
+
+	if (!uuid_is_null(&zmd->dmz_uuid)) {
+		sb->version = cpu_to_le32(DMZ_META_VER);
+		uuid_copy((uuid_t *)sb->dmz_uuid, &zmd->dmz_uuid);
+		memcpy(sb->dmz_label, zmd->dmz_label, BDEVNAME_SIZE);
+		uuid_copy((uuid_t *)sb->dev_uuid, &zmd->dev_uuid);
+	} else
+		sb->version = cpu_to_le32(DMZ_META_COMPAT_VER);
 
 	sb->gen = cpu_to_le64(sb_gen);
 
@@ -848,28 +869,29 @@ static int dmz_check_sb(struct dmz_metadata *zmd, struct dmz_super *sb)
 {
 	unsigned int nr_meta_zones, nr_data_zones;
 	struct dmz_dev *dev = zmd->dev;
-	u32 crc, stored_crc;
+	u32 crc, stored_crc, dmz_version;
 	u64 gen;
 
-	gen = le64_to_cpu(sb->gen);
-	stored_crc = le32_to_cpu(sb->crc);
-	sb->crc = 0;
-	crc = crc32_le(gen, (unsigned char *)sb, DMZ_BLOCK_SIZE);
-	if (crc != stored_crc) {
-		dmz_dev_err(dev, "Invalid checksum (needed 0x%08x, got 0x%08x)",
-			    crc, stored_crc);
-		return -ENXIO;
-	}
-
 	if (le32_to_cpu(sb->magic) != DMZ_MAGIC) {
 		dmz_dev_err(dev, "Invalid meta magic (needed 0x%08x, got 0x%08x)",
 			    DMZ_MAGIC, le32_to_cpu(sb->magic));
 		return -ENXIO;
 	}
 
-	if (le32_to_cpu(sb->version) != DMZ_META_VER) {
+	dmz_version = le32_to_cpu(sb->version);
+	if (dmz_version > DMZ_META_VER) {
 		dmz_dev_err(dev, "Invalid meta version (needed %d, got %d)",
-			    DMZ_META_VER, le32_to_cpu(sb->version));
+			    DMZ_META_VER, dmz_version);
+		return -ENXIO;
+	}
+
+	gen = le64_to_cpu(sb->gen);
+	stored_crc = le32_to_cpu(sb->crc);
+	sb->crc = 0;
+	crc = crc32_le(gen, (unsigned char *)sb, DMZ_BLOCK_SIZE);
+	if (crc != stored_crc) {
+		dmz_dev_err(dev, "Invalid checksum (needed 0x%08x, got 0x%08x)",
+			    crc, stored_crc);
 		return -ENXIO;
 	}
 
@@ -895,6 +917,21 @@ static int dmz_check_sb(struct dmz_metadata *zmd, struct dmz_super *sb)
 		return -ENXIO;
 	}
 
+	if (dmz_version == DMZ_META_VER) {
+		if (uuid_is_null((uuid_t *)sb->dmz_uuid)) {
+			dmz_dev_err(dev, "NULL DM-Zoned uuid");
+			return -ENXIO;
+		}
+		uuid_copy(&zmd->dmz_uuid, (uuid_t *)sb->dmz_uuid);
+		memcpy(zmd->dmz_label, sb->dmz_label, BDEVNAME_SIZE);
+		if (uuid_is_null((uuid_t *)sb->dev_uuid)) {
+			dmz_dev_err(dev, "NULL device uuid");
+			return -ENXIO;
+		}
+		uuid_copy(&zmd->dev_uuid, (uuid_t *)sb->dev_uuid);
+
+	}
+
 	/* OK */
 	zmd->nr_meta_blocks = le32_to_cpu(sb->nr_meta_blocks);
 	zmd->nr_reserved_seq = le32_to_cpu(sb->nr_reserved_seq);
@@ -2460,9 +2497,18 @@ int dmz_ctr_metadata(struct dmz_dev *dev, struct dmz_metadata **metadata)
 		goto err;
 	}
 
+	dmz_dev_info(dev, "DM-Zoned version %d",
+		     uuid_is_null(&zmd->dmz_uuid) ?
+		     DMZ_META_COMPAT_VER : DMZ_META_VER);
+	if (!uuid_is_null(&zmd->dmz_uuid))
+		dmz_dev_info(dev, "DM UUID %pUl", &zmd->dmz_uuid);
+	if (strlen(zmd->dmz_label))
+		dmz_dev_info(dev, "DM Label %s", zmd->dmz_label);
 	dmz_dev_info(dev, "Host-%s zoned block device",
 		     bdev_zoned_model(dev->bdev) == BLK_ZONED_HA ?
 		     "aware" : "managed");
+	if (!uuid_is_null(&zmd->dev_uuid))
+		dmz_dev_info(dev, "  uuid %pUl", &zmd->dev_uuid);
 	dmz_dev_info(dev, "  %llu 512-byte logical sectors",
 		     (u64)dev->capacity);
 	dmz_dev_info(dev, "  %u zones of %llu 512-byte logical sectors",
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index 44e30a7de8b9..7ec9dde24516 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -1008,7 +1008,7 @@ static int dmz_message(struct dm_target *ti, unsigned int argc, char **argv,
 
 static struct target_type dmz_type = {
 	.name		 = "zoned",
-	.version	 = {1, 1, 0},
+	.version	 = {1, 2, 0},
 	.features	 = DM_TARGET_SINGLETON | DM_TARGET_ZONED_HM,
 	.module		 = THIS_MODULE,
 	.ctr		 = dmz_ctr,
-- 
2.16.4

[PATCH 4/4] dm-zoned: allow for device size smaller than the capacity

[Hannes Reinecke]

dm-zoned requires several zones for metadata and chunk bitmaps,
so it cannot expose the entire capacity as the device size.
Originally the code would check for the capacity being equal to
the device size, which is arguably wrong.
So relax this check and increase the interface version number
to signal to userspace that it can set a smaller device size.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-zoned-target.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index 7ec9dde24516..89a825d1034e 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -715,7 +715,7 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
 	aligned_capacity = dev->capacity &
 				~((sector_t)blk_queue_zone_sectors(q) - 1);
 	if (ti->begin ||
-	    ((ti->len != dev->capacity) && (ti->len != aligned_capacity))) {
+	    ((ti->len > dev->capacity) && (ti->len > aligned_capacity))) {
 		ti->error = "Partial mapping not supported";
 		ret = -EINVAL;
 		goto err;
@@ -1008,7 +1008,7 @@ static int dmz_message(struct dm_target *ti, unsigned int argc, char **argv,
 
 static struct target_type dmz_type = {
 	.name		 = "zoned",
-	.version	 = {1, 2, 0},
+	.version	 = {1, 3, 0},
 	.features	 = DM_TARGET_SINGLETON | DM_TARGET_ZONED_HM,
 	.module		 = THIS_MODULE,
 	.ctr		 = dmz_ctr,
-- 
2.16.4

Re: [PATCH 4/4] dm-zoned: allow for device size smaller than the capacity

[Damien Le Moal]

On 2020/03/27 16:15, Hannes Reinecke wrote:
> dm-zoned requires several zones for metadata and chunk bitmaps,
> so it cannot expose the entire capacity as the device size.
> Originally the code would check for the capacity being equal to
> the device size, which is arguably wrong.
> So relax this check and increase the interface version number
> to signal to userspace that it can set a smaller device size.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/md/dm-zoned-target.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
> index 7ec9dde24516..89a825d1034e 100644
> --- a/drivers/md/dm-zoned-target.c
> +++ b/drivers/md/dm-zoned-target.c
> @@ -715,7 +715,7 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
>  	aligned_capacity = dev->capacity &
>  				~((sector_t)blk_queue_zone_sectors(q) - 1);
>  	if (ti->begin ||
> -	    ((ti->len != dev->capacity) && (ti->len != aligned_capacity))) {
> +	    ((ti->len > dev->capacity) && (ti->len > aligned_capacity))) {
>  		ti->error = "Partial mapping not supported";

The message is now wrong. Also, the condition can now be simplified to:

if ((ti->begin + ti->len) > aligned_capacity) {

Since aligned capacity is equal or smaller than dev capacity. And we have to
account for the potential non-zero begin.

>  		ret = -EINVAL;
>  		goto err;
> @@ -1008,7 +1008,7 @@ static int dmz_message(struct dm_target *ti, unsigned int argc, char **argv,
>  
>  static struct target_type dmz_type = {
>  	.name		 = "zoned",
> -	.version	 = {1, 2, 0},
> +	.version	 = {1, 3, 0},
>  	.features	 = DM_TARGET_SINGLETON | DM_TARGET_ZONED_HM,
>  	.module		 = THIS_MODULE,
>  	.ctr		 = dmz_ctr,
> 

I do not think this is nearly enough: dmz_init_zones() is still considering the
entire drive and does a zone report from 0 on the backend bdev. It should start
at ti->begin and up to ti->begin+ti->len, no ?

Furthermore, this introduce a change in the meaning of the zone ID. Since this
is set to the index of the zone in the report (patch 1), if the mapping is
partial and the zone report does not start at 0, then zone ID is not zone number
on the device anymore. So dmz_start_block() needs to be offset by ti->begin
otherwise IOs will go to the wrong zones.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/4] dm-zoned: use array for superblock zones

[Damien Le Moal]

On 2020/03/27 16:15, Hannes Reinecke wrote:
> Instead of storing just the first superblock zone and calculate
> the secondary relative to that we should be using an array for
> holding the superblock zones.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

> ---
>  drivers/md/dm-zoned-metadata.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
> index afce594067fb..dc1d17bc3bbb 100644
> --- a/drivers/md/dm-zoned-metadata.c
> +++ b/drivers/md/dm-zoned-metadata.c
> @@ -124,6 +124,7 @@ struct dmz_sb {
>  	sector_t		block;
>  	struct dmz_mblock	*mblk;
>  	struct dmz_super	*sb;
> +	struct dm_zone		*zone;
>  };
>  
>  /*
> @@ -150,7 +151,6 @@ struct dmz_metadata {
>  	/* Zone information array */
>  	struct dm_zone		*zones;
>  
> -	struct dm_zone		*sb_zone;
>  	struct dmz_sb		sb[2];
>  	unsigned int		mblk_primary;
>  	u64			sb_gen;
> @@ -937,16 +937,20 @@ static int dmz_lookup_secondary_sb(struct dmz_metadata *zmd)
>  
>  	/* Bad first super block: search for the second one */
>  	zmd->sb[1].block = zmd->sb[0].block + zone_nr_blocks;
> +	zmd->sb[1].zone = zmd->sb[0].zone + 1;
>  	for (i = 0; i < zmd->nr_rnd_zones - 1; i++) {
>  		if (dmz_read_sb(zmd, 1) != 0)
>  			break;
> -		if (le32_to_cpu(zmd->sb[1].sb->magic) == DMZ_MAGIC)
> +		if (le32_to_cpu(zmd->sb[1].sb->magic) == DMZ_MAGIC) {
> +			zmd->sb[1].zone += i;
>  			return 0;
> +		}
>  		zmd->sb[1].block += zone_nr_blocks;
>  	}
>  
>  	dmz_free_mblock(zmd, mblk);
>  	zmd->sb[1].mblk = NULL;
> +	zmd->sb[1].zone = NULL;
>  
>  	return -EIO;
>  }
> @@ -990,11 +994,9 @@ static int dmz_recover_mblocks(struct dmz_metadata *zmd, unsigned int dst_set)
>  	dmz_dev_warn(zmd->dev, "Metadata set %u invalid: recovering", dst_set);
>  
>  	if (dst_set == 0)
> -		zmd->sb[0].block = dmz_start_block(zmd, zmd->sb_zone);
> -	else {
> -		zmd->sb[1].block = zmd->sb[0].block +
> -			(zmd->nr_meta_zones << zmd->dev->zone_nr_blocks_shift);
> -	}
> +		zmd->sb[0].block = dmz_start_block(zmd, zmd->sb[0].zone);
> +	else
> +		zmd->sb[1].block = dmz_start_block(zmd, zmd->sb[1].zone);
>  
>  	page = alloc_page(GFP_NOIO);
>  	if (!page)
> @@ -1038,8 +1040,13 @@ static int dmz_load_sb(struct dmz_metadata *zmd)
>  	u64 sb_gen[2] = {0, 0};
>  	int ret;
>  
> +	if (!zmd->sb[0].zone) {
> +		dmz_dev_err(zmd->dev, "Primary super block zone not set");
> +		return -ENXIO;
> +	}
> +
>  	/* Read and check the primary super block */
> -	zmd->sb[0].block = dmz_start_block(zmd, zmd->sb_zone);
> +	zmd->sb[0].block = dmz_start_block(zmd, zmd->sb[0].zone);
>  	ret = dmz_get_sb(zmd, 0);
>  	if (ret) {
>  		dmz_dev_err(zmd->dev, "Read primary super block failed");
> @@ -1051,8 +1058,9 @@ static int dmz_load_sb(struct dmz_metadata *zmd)
>  	/* Read and check secondary super block */
>  	if (ret == 0) {
>  		sb_good[0] = true;
> -		zmd->sb[1].block = zmd->sb[0].block +
> -			(zmd->nr_meta_zones << zmd->dev->zone_nr_blocks_shift);
> +		if (!zmd->sb[1].zone)
> +			zmd->sb[1].zone = zmd->sb[0].zone + zmd->nr_meta_zones;
> +		zmd->sb[1].block = dmz_start_block(zmd, zmd->sb[1].zone);
>  		ret = dmz_get_sb(zmd, 1);
>  	} else
>  		ret = dmz_lookup_secondary_sb(zmd);
> @@ -1147,9 +1155,9 @@ static int dmz_init_zone(struct blk_zone *blkz, unsigned int idx, void *data)
>  		zmd->nr_useable_zones++;
>  		if (dmz_is_rnd(zone)) {
>  			zmd->nr_rnd_zones++;
> -			if (!zmd->sb_zone) {
> +			if (!zmd->sb[0].zone) {
>  				/* Super block zone */
> -				zmd->sb_zone = zone;
> +				zmd->sb[0].zone = zone;
>  			}
>  		}
>  	}
> @@ -2420,7 +2428,7 @@ int dmz_ctr_metadata(struct dmz_dev *dev, struct dmz_metadata **metadata)
>  		goto err;
>  
>  	/* Set metadata zones starting from sb_zone */
> -	zid = dmz_id(zmd, zmd->sb_zone);
> +	zid = dmz_id(zmd, zmd->sb[0].zone);
>  	for (i = 0; i < zmd->nr_meta_zones << 1; i++) {
>  		zone = dmz_get(zmd, zid + i);
>  		if (!dmz_is_rnd(zone))
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] dm-zoned: V2 metadata handling

[Damien Le Moal]

On 2020/03/27 16:15, Hannes Reinecke wrote:
> Add 'V2' metadata which includes UUIDs for the dmz set and for
> the device itself.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

> ---
>  drivers/md/dm-zoned-metadata.c | 80 +++++++++++++++++++++++++++++++++---------
>  drivers/md/dm-zoned-target.c   |  2 +-
>  2 files changed, 64 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
> index dc1d17bc3bbb..026f285fba33 100644
> --- a/drivers/md/dm-zoned-metadata.c
> +++ b/drivers/md/dm-zoned-metadata.c
> @@ -16,7 +16,8 @@
>  /*
>   * Metadata version.
>   */
> -#define DMZ_META_VER	1
> +#define DMZ_META_COMPAT_VER 1
> +#define DMZ_META_VER	2
>  
>  /*
>   * On-disk super block magic.
> @@ -69,8 +70,17 @@ struct dmz_super {
>  	/* Checksum */
>  	__le32		crc;			/*  48 */
>  
> -	/* Padding to full 512B sector */
> -	u8		reserved[464];		/* 512 */
> +	/* DM-Zoned label */
> +	u8		dmz_label[32];		/*  80 */
> +
> +	/* DM-Zoned UUID */
> +	u8		dmz_uuid[16];		/*  96 */
> +
> +	/* Device UUID */
> +	u8		dev_uuid[16];		/* 112 */
> +
> +	/* Padding to full 512B - CRC sector */
> +	u8		reserved[400];		/* 512 */
>  };
>  
>  /*
> @@ -133,6 +143,10 @@ struct dmz_sb {
>  struct dmz_metadata {
>  	struct dmz_dev		*dev;
>  
> +	char			dmz_label[BDEVNAME_SIZE];
> +	uuid_t			dmz_uuid;
> +	uuid_t			dev_uuid;
> +
>  	sector_t		zone_bitmap_size;
>  	unsigned int		zone_nr_bitmap_blocks;
>  	unsigned int		zone_bits_per_mblk;
> @@ -659,7 +673,14 @@ static int dmz_write_sb(struct dmz_metadata *zmd, unsigned int set)
>  	int ret;
>  
>  	sb->magic = cpu_to_le32(DMZ_MAGIC);
> -	sb->version = cpu_to_le32(DMZ_META_VER);
> +
> +	if (!uuid_is_null(&zmd->dmz_uuid)) {
> +		sb->version = cpu_to_le32(DMZ_META_VER);
> +		uuid_copy((uuid_t *)sb->dmz_uuid, &zmd->dmz_uuid);
> +		memcpy(sb->dmz_label, zmd->dmz_label, BDEVNAME_SIZE);
> +		uuid_copy((uuid_t *)sb->dev_uuid, &zmd->dev_uuid);
> +	} else
> +		sb->version = cpu_to_le32(DMZ_META_COMPAT_VER);
>  
>  	sb->gen = cpu_to_le64(sb_gen);
>  
> @@ -848,28 +869,29 @@ static int dmz_check_sb(struct dmz_metadata *zmd, struct dmz_super *sb)
>  {
>  	unsigned int nr_meta_zones, nr_data_zones;
>  	struct dmz_dev *dev = zmd->dev;
> -	u32 crc, stored_crc;
> +	u32 crc, stored_crc, dmz_version;
>  	u64 gen;
>  
> -	gen = le64_to_cpu(sb->gen);
> -	stored_crc = le32_to_cpu(sb->crc);
> -	sb->crc = 0;
> -	crc = crc32_le(gen, (unsigned char *)sb, DMZ_BLOCK_SIZE);
> -	if (crc != stored_crc) {
> -		dmz_dev_err(dev, "Invalid checksum (needed 0x%08x, got 0x%08x)",
> -			    crc, stored_crc);
> -		return -ENXIO;
> -	}
> -
>  	if (le32_to_cpu(sb->magic) != DMZ_MAGIC) {
>  		dmz_dev_err(dev, "Invalid meta magic (needed 0x%08x, got 0x%08x)",
>  			    DMZ_MAGIC, le32_to_cpu(sb->magic));
>  		return -ENXIO;
>  	}
>  
> -	if (le32_to_cpu(sb->version) != DMZ_META_VER) {
> +	dmz_version = le32_to_cpu(sb->version);
> +	if (dmz_version > DMZ_META_VER) {
>  		dmz_dev_err(dev, "Invalid meta version (needed %d, got %d)",
> -			    DMZ_META_VER, le32_to_cpu(sb->version));
> +			    DMZ_META_VER, dmz_version);
> +		return -ENXIO;
> +	}
> +
> +	gen = le64_to_cpu(sb->gen);
> +	stored_crc = le32_to_cpu(sb->crc);
> +	sb->crc = 0;
> +	crc = crc32_le(gen, (unsigned char *)sb, DMZ_BLOCK_SIZE);
> +	if (crc != stored_crc) {
> +		dmz_dev_err(dev, "Invalid checksum (needed 0x%08x, got 0x%08x)",
> +			    crc, stored_crc);
>  		return -ENXIO;
>  	}
>  
> @@ -895,6 +917,21 @@ static int dmz_check_sb(struct dmz_metadata *zmd, struct dmz_super *sb)
>  		return -ENXIO;
>  	}
>  
> +	if (dmz_version == DMZ_META_VER) {
> +		if (uuid_is_null((uuid_t *)sb->dmz_uuid)) {
> +			dmz_dev_err(dev, "NULL DM-Zoned uuid");
> +			return -ENXIO;
> +		}
> +		uuid_copy(&zmd->dmz_uuid, (uuid_t *)sb->dmz_uuid);
> +		memcpy(zmd->dmz_label, sb->dmz_label, BDEVNAME_SIZE);
> +		if (uuid_is_null((uuid_t *)sb->dev_uuid)) {
> +			dmz_dev_err(dev, "NULL device uuid");
> +			return -ENXIO;
> +		}
> +		uuid_copy(&zmd->dev_uuid, (uuid_t *)sb->dev_uuid);
> +
> +	}
> +
>  	/* OK */
>  	zmd->nr_meta_blocks = le32_to_cpu(sb->nr_meta_blocks);
>  	zmd->nr_reserved_seq = le32_to_cpu(sb->nr_reserved_seq);
> @@ -2460,9 +2497,18 @@ int dmz_ctr_metadata(struct dmz_dev *dev, struct dmz_metadata **metadata)
>  		goto err;
>  	}
>  
> +	dmz_dev_info(dev, "DM-Zoned version %d",
> +		     uuid_is_null(&zmd->dmz_uuid) ?
> +		     DMZ_META_COMPAT_VER : DMZ_META_VER);
> +	if (!uuid_is_null(&zmd->dmz_uuid))
> +		dmz_dev_info(dev, "DM UUID %pUl", &zmd->dmz_uuid);
> +	if (strlen(zmd->dmz_label))
> +		dmz_dev_info(dev, "DM Label %s", zmd->dmz_label);
>  	dmz_dev_info(dev, "Host-%s zoned block device",
>  		     bdev_zoned_model(dev->bdev) == BLK_ZONED_HA ?
>  		     "aware" : "managed");
> +	if (!uuid_is_null(&zmd->dev_uuid))
> +		dmz_dev_info(dev, "  uuid %pUl", &zmd->dev_uuid);
>  	dmz_dev_info(dev, "  %llu 512-byte logical sectors",
>  		     (u64)dev->capacity);
>  	dmz_dev_info(dev, "  %u zones of %llu 512-byte logical sectors",
> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
> index 44e30a7de8b9..7ec9dde24516 100644
> --- a/drivers/md/dm-zoned-target.c
> +++ b/drivers/md/dm-zoned-target.c
> @@ -1008,7 +1008,7 @@ static int dmz_message(struct dm_target *ti, unsigned int argc, char **argv,
>  
>  static struct target_type dmz_type = {
>  	.name		 = "zoned",
> -	.version	 = {1, 1, 0},
> +	.version	 = {1, 2, 0},
>  	.features	 = DM_TARGET_SINGLETON | DM_TARGET_ZONED_HM,
>  	.module		 = THIS_MODULE,
>  	.ctr		 = dmz_ctr,
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/4] dm-zoned: store zone id within the zone structure

[Damien Le Moal]

On 2020/03/27 16:15, Hannes Reinecke wrote:
> Instead of calculating the zone index by the offset within the
> zone array store the index within the structure itself.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/md/dm-zoned-metadata.c | 3 ++-
>  drivers/md/dm-zoned.h          | 3 +++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
> index c8787560fa9f..afce594067fb 100644
> --- a/drivers/md/dm-zoned-metadata.c
> +++ b/drivers/md/dm-zoned-metadata.c
> @@ -189,7 +189,7 @@ struct dmz_metadata {
>   */
>  unsigned int dmz_id(struct dmz_metadata *zmd, struct dm_zone *zone)
>  {
> -	return ((unsigned int)(zone - zmd->zones));
> +	return zone->id;
>  }
>  
>  sector_t dmz_start_sect(struct dmz_metadata *zmd, struct dm_zone *zone)
> @@ -1119,6 +1119,7 @@ static int dmz_init_zone(struct blk_zone *blkz, unsigned int idx, void *data)
>  
>  	INIT_LIST_HEAD(&zone->link);
>  	atomic_set(&zone->refcount, 0);
> +	zone->id = idx;

See my comment on patch 4. Allowing partial bdev mapping changes the meaning of
this ID: for a partial mapping, this is not equal to the zone number anymore. So
we should document that (in the struct declaration), and may be merge this patch
with patch 4 since they are related so closely ?

>  	zone->chunk = DMZ_MAP_UNMAPPED;
>  
>  	switch (blkz->type) {
> diff --git a/drivers/md/dm-zoned.h b/drivers/md/dm-zoned.h
> index 884c0e586082..39d59898abbe 100644
> --- a/drivers/md/dm-zoned.h
> +++ b/drivers/md/dm-zoned.h
> @@ -87,6 +87,9 @@ struct dm_zone {
>  	/* Zone activation reference count */
>  	atomic_t		refcount;
>  
> +	/* Zone id */
> +	unsigned int		id;
> +
>  	/* Zone write pointer block (relative to the zone start block) */
>  	unsigned int		wp_block;
>  
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] dm-zoned: allow for device size smaller than the capacity

[Hannes Reinecke]

On 3/31/20 2:49 AM, Damien Le Moal wrote:
> On 2020/03/27 16:15, Hannes Reinecke wrote:
>> dm-zoned requires several zones for metadata and chunk bitmaps,
>> so it cannot expose the entire capacity as the device size.
>> Originally the code would check for the capacity being equal to
>> the device size, which is arguably wrong.
>> So relax this check and increase the interface version number
>> to signal to userspace that it can set a smaller device size.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/md/dm-zoned-target.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
>> index 7ec9dde24516..89a825d1034e 100644
>> --- a/drivers/md/dm-zoned-target.c
>> +++ b/drivers/md/dm-zoned-target.c
>> @@ -715,7 +715,7 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
>>   	aligned_capacity = dev->capacity &
>>   				~((sector_t)blk_queue_zone_sectors(q) - 1);
>>   	if (ti->begin ||
>> -	    ((ti->len != dev->capacity) && (ti->len != aligned_capacity))) {
>> +	    ((ti->len > dev->capacity) && (ti->len > aligned_capacity))) {
>>   		ti->error = "Partial mapping not supported";
> 
> The message is now wrong. Also, the condition can now be simplified to:
> 
> if ((ti->begin + ti->len) > aligned_capacity) {
> 
> Since aligned capacity is equal or smaller than dev capacity. And we have to
> account for the potential non-zero begin.
> 
_Actually_ I would forbid for 'ti->begin' to be anything other than 0.
For a zoned device there is no point in allowing for partial handling at 
all.
Problem is that 'dev->capacity' is the capacity of the zoned block 
device, whereas 'ti-len' is the exported capacity of the device-mapper 
device, which is smaller than the device capacity by the number of 
metadata blocks/zones.

>>   		ret = -EINVAL;
>>   		goto err;
>> @@ -1008,7 +1008,7 @@ static int dmz_message(struct dm_target *ti, unsigned int argc, char **argv,
>>   
>>   static struct target_type dmz_type = {
>>   	.name		 = "zoned",
>> -	.version	 = {1, 2, 0},
>> +	.version	 = {1, 3, 0},
>>   	.features	 = DM_TARGET_SINGLETON | DM_TARGET_ZONED_HM,
>>   	.module		 = THIS_MODULE,
>>   	.ctr		 = dmz_ctr,
>>
> 
> I do not think this is nearly enough: dmz_init_zones() is still considering the
> entire drive and does a zone report from 0 on the backend bdev. It should start
> at ti->begin and up to ti->begin+ti->len, no ?
> 
Yes, and no. I want to disallow 'ti->begin' to be anything else than 0 
as ti->begin and ti->len are relative to exported device-mapper device, 
and we always want to have block 0 mapped :-)

And as such dmz_init_zones() needs to cover all zones, as this is 
relative to the zoned block device.

> Furthermore, this introduce a change in the meaning of the zone ID. Since this
> is set to the index of the zone in the report (patch 1), if the mapping is
> partial and the zone report does not start at 0, then zone ID is not zone number
> on the device anymore. So dmz_start_block() needs to be offset by ti->begin
> otherwise IOs will go to the wrong zones.
> 
As I said: We will never do a partial mapping.
What this patch does it to bring the device-mapper mapping in-line with 
the exported device size.

Originally we would export a device-mapper mapping for blocks up to the 
zone-device capacity. As the resulting device-mapper block device has a 
smaller capacity than the mapping would allow for those 'spare' blocks 
would never been used, thus the invalid mapping was never triggered.

What this patch does is to bring the device-mapper mapping in-line with 
the exported block device capacity, so that we don't have an invalid 
mapping. Nothing else has (and should) be changed.

Especially not the partial zoned device handling :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@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: [PATCH 1/4] dm-zoned: store zone id within the zone structure

[Hannes Reinecke]

On 3/31/20 2:57 AM, Damien Le Moal wrote:
> On 2020/03/27 16:15, Hannes Reinecke wrote:
>> Instead of calculating the zone index by the offset within the
>> zone array store the index within the structure itself.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/md/dm-zoned-metadata.c | 3 ++-
>>   drivers/md/dm-zoned.h          | 3 +++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
>> index c8787560fa9f..afce594067fb 100644
>> --- a/drivers/md/dm-zoned-metadata.c
>> +++ b/drivers/md/dm-zoned-metadata.c
>> @@ -189,7 +189,7 @@ struct dmz_metadata {
>>    */
>>   unsigned int dmz_id(struct dmz_metadata *zmd, struct dm_zone *zone)
>>   {
>> -	return ((unsigned int)(zone - zmd->zones));
>> +	return zone->id;
>>   }
>>   
>>   sector_t dmz_start_sect(struct dmz_metadata *zmd, struct dm_zone *zone)
>> @@ -1119,6 +1119,7 @@ static int dmz_init_zone(struct blk_zone *blkz, unsigned int idx, void *data)
>>   
>>   	INIT_LIST_HEAD(&zone->link);
>>   	atomic_set(&zone->refcount, 0);
>> +	zone->id = idx;
> 
> See my comment on patch 4. Allowing partial bdev mapping changes the meaning of
> this ID: for a partial mapping, this is not equal to the zone number anymore. So
> we should document that (in the struct declaration), and may be merge this patch
> with patch 4 since they are related so closely ?
> 
See my reply to patch 4. I do not allow for partial mapping, nor was it 
ever the intention to do so.
The zone numbering will not change.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@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: [PATCH 2/4] dm-zoned: use array for superblock zones

[Bob Liu]

On 3/27/20 3:14 PM, Hannes Reinecke wrote:
> Instead of storing just the first superblock zone and calculate
> the secondary relative to that we should be using an array for
> holding the superblock zones.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Reviewed-by: Bob Liu <bob.liu@oracle.com>

> ---
>  drivers/md/dm-zoned-metadata.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
> index afce594067fb..dc1d17bc3bbb 100644
> --- a/drivers/md/dm-zoned-metadata.c
> +++ b/drivers/md/dm-zoned-metadata.c
> @@ -124,6 +124,7 @@ struct dmz_sb {
>  	sector_t		block;
>  	struct dmz_mblock	*mblk;
>  	struct dmz_super	*sb;
> +	struct dm_zone		*zone;
>  };
>  
>  /*
> @@ -150,7 +151,6 @@ struct dmz_metadata {
>  	/* Zone information array */
>  	struct dm_zone		*zones;
>  
> -	struct dm_zone		*sb_zone;
>  	struct dmz_sb		sb[2];
>  	unsigned int		mblk_primary;
>  	u64			sb_gen;
> @@ -937,16 +937,20 @@ static int dmz_lookup_secondary_sb(struct dmz_metadata *zmd)
>  
>  	/* Bad first super block: search for the second one */
>  	zmd->sb[1].block = zmd->sb[0].block + zone_nr_blocks;
> +	zmd->sb[1].zone = zmd->sb[0].zone + 1;
>  	for (i = 0; i < zmd->nr_rnd_zones - 1; i++) {
>  		if (dmz_read_sb(zmd, 1) != 0)
>  			break;
> -		if (le32_to_cpu(zmd->sb[1].sb->magic) == DMZ_MAGIC)
> +		if (le32_to_cpu(zmd->sb[1].sb->magic) == DMZ_MAGIC) {
> +			zmd->sb[1].zone += i;
>  			return 0;
> +		}
>  		zmd->sb[1].block += zone_nr_blocks;
>  	}
>  
>  	dmz_free_mblock(zmd, mblk);
>  	zmd->sb[1].mblk = NULL;
> +	zmd->sb[1].zone = NULL;
>  
>  	return -EIO;
>  }
> @@ -990,11 +994,9 @@ static int dmz_recover_mblocks(struct dmz_metadata *zmd, unsigned int dst_set)
>  	dmz_dev_warn(zmd->dev, "Metadata set %u invalid: recovering", dst_set);
>  
>  	if (dst_set == 0)
> -		zmd->sb[0].block = dmz_start_block(zmd, zmd->sb_zone);
> -	else {
> -		zmd->sb[1].block = zmd->sb[0].block +
> -			(zmd->nr_meta_zones << zmd->dev->zone_nr_blocks_shift);
> -	}
> +		zmd->sb[0].block = dmz_start_block(zmd, zmd->sb[0].zone);
> +	else
> +		zmd->sb[1].block = dmz_start_block(zmd, zmd->sb[1].zone);
>  
>  	page = alloc_page(GFP_NOIO);
>  	if (!page)
> @@ -1038,8 +1040,13 @@ static int dmz_load_sb(struct dmz_metadata *zmd)
>  	u64 sb_gen[2] = {0, 0};
>  	int ret;
>  
> +	if (!zmd->sb[0].zone) {
> +		dmz_dev_err(zmd->dev, "Primary super block zone not set");
> +		return -ENXIO;
> +	}
> +
>  	/* Read and check the primary super block */
> -	zmd->sb[0].block = dmz_start_block(zmd, zmd->sb_zone);
> +	zmd->sb[0].block = dmz_start_block(zmd, zmd->sb[0].zone);
>  	ret = dmz_get_sb(zmd, 0);
>  	if (ret) {
>  		dmz_dev_err(zmd->dev, "Read primary super block failed");
> @@ -1051,8 +1058,9 @@ static int dmz_load_sb(struct dmz_metadata *zmd)
>  	/* Read and check secondary super block */
>  	if (ret == 0) {
>  		sb_good[0] = true;
> -		zmd->sb[1].block = zmd->sb[0].block +
> -			(zmd->nr_meta_zones << zmd->dev->zone_nr_blocks_shift);
> +		if (!zmd->sb[1].zone)
> +			zmd->sb[1].zone = zmd->sb[0].zone + zmd->nr_meta_zones;
> +		zmd->sb[1].block = dmz_start_block(zmd, zmd->sb[1].zone);
>  		ret = dmz_get_sb(zmd, 1);
>  	} else
>  		ret = dmz_lookup_secondary_sb(zmd);
> @@ -1147,9 +1155,9 @@ static int dmz_init_zone(struct blk_zone *blkz, unsigned int idx, void *data)
>  		zmd->nr_useable_zones++;
>  		if (dmz_is_rnd(zone)) {
>  			zmd->nr_rnd_zones++;
> -			if (!zmd->sb_zone) {
> +			if (!zmd->sb[0].zone) {
>  				/* Super block zone */
> -				zmd->sb_zone = zone;
> +				zmd->sb[0].zone = zone;
>  			}
>  		}
>  	}
> @@ -2420,7 +2428,7 @@ int dmz_ctr_metadata(struct dmz_dev *dev, struct dmz_metadata **metadata)
>  		goto err;
>  
>  	/* Set metadata zones starting from sb_zone */
> -	zid = dmz_id(zmd, zmd->sb_zone);
> +	zid = dmz_id(zmd, zmd->sb[0].zone);
>  	for (i = 0; i < zmd->nr_meta_zones << 1; i++) {
>  		zone = dmz_get(zmd, zid + i);
>  		if (!dmz_is_rnd(zone))
> 

Re: [PATCH 3/4] dm-zoned: V2 metadata handling

[Bob Liu]

On 3/27/20 3:14 PM, Hannes Reinecke wrote:
> Add 'V2' metadata which includes UUIDs for the dmz set and for
> the device itself.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Reviewed-by: Bob Liu <bob.liu@oracle.com>

> ---
>  drivers/md/dm-zoned-metadata.c | 80 +++++++++++++++++++++++++++++++++---------
>  drivers/md/dm-zoned-target.c   |  2 +-
>  2 files changed, 64 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
> index dc1d17bc3bbb..026f285fba33 100644
> --- a/drivers/md/dm-zoned-metadata.c
> +++ b/drivers/md/dm-zoned-metadata.c
> @@ -16,7 +16,8 @@
>  /*
>   * Metadata version.
>   */
> -#define DMZ_META_VER	1
> +#define DMZ_META_COMPAT_VER 1
> +#define DMZ_META_VER	2
>  
>  /*
>   * On-disk super block magic.
> @@ -69,8 +70,17 @@ struct dmz_super {
>  	/* Checksum */
>  	__le32		crc;			/*  48 */
>  
> -	/* Padding to full 512B sector */
> -	u8		reserved[464];		/* 512 */
> +	/* DM-Zoned label */
> +	u8		dmz_label[32];		/*  80 */
> +
> +	/* DM-Zoned UUID */
> +	u8		dmz_uuid[16];		/*  96 */
> +
> +	/* Device UUID */
> +	u8		dev_uuid[16];		/* 112 */
> +
> +	/* Padding to full 512B - CRC sector */
> +	u8		reserved[400];		/* 512 */
>  };
>  
>  /*
> @@ -133,6 +143,10 @@ struct dmz_sb {
>  struct dmz_metadata {
>  	struct dmz_dev		*dev;
>  
> +	char			dmz_label[BDEVNAME_SIZE];
> +	uuid_t			dmz_uuid;
> +	uuid_t			dev_uuid;
> +
>  	sector_t		zone_bitmap_size;
>  	unsigned int		zone_nr_bitmap_blocks;
>  	unsigned int		zone_bits_per_mblk;
> @@ -659,7 +673,14 @@ static int dmz_write_sb(struct dmz_metadata *zmd, unsigned int set)
>  	int ret;
>  
>  	sb->magic = cpu_to_le32(DMZ_MAGIC);
> -	sb->version = cpu_to_le32(DMZ_META_VER);
> +
> +	if (!uuid_is_null(&zmd->dmz_uuid)) {
> +		sb->version = cpu_to_le32(DMZ_META_VER);
> +		uuid_copy((uuid_t *)sb->dmz_uuid, &zmd->dmz_uuid);
> +		memcpy(sb->dmz_label, zmd->dmz_label, BDEVNAME_SIZE);
> +		uuid_copy((uuid_t *)sb->dev_uuid, &zmd->dev_uuid);
> +	} else
> +		sb->version = cpu_to_le32(DMZ_META_COMPAT_VER);
>  
>  	sb->gen = cpu_to_le64(sb_gen);
>  
> @@ -848,28 +869,29 @@ static int dmz_check_sb(struct dmz_metadata *zmd, struct dmz_super *sb)
>  {
>  	unsigned int nr_meta_zones, nr_data_zones;
>  	struct dmz_dev *dev = zmd->dev;
> -	u32 crc, stored_crc;
> +	u32 crc, stored_crc, dmz_version;
>  	u64 gen;
>  
> -	gen = le64_to_cpu(sb->gen);
> -	stored_crc = le32_to_cpu(sb->crc);
> -	sb->crc = 0;
> -	crc = crc32_le(gen, (unsigned char *)sb, DMZ_BLOCK_SIZE);
> -	if (crc != stored_crc) {
> -		dmz_dev_err(dev, "Invalid checksum (needed 0x%08x, got 0x%08x)",
> -			    crc, stored_crc);
> -		return -ENXIO;
> -	}
> -
>  	if (le32_to_cpu(sb->magic) != DMZ_MAGIC) {
>  		dmz_dev_err(dev, "Invalid meta magic (needed 0x%08x, got 0x%08x)",
>  			    DMZ_MAGIC, le32_to_cpu(sb->magic));
>  		return -ENXIO;
>  	}
>  
> -	if (le32_to_cpu(sb->version) != DMZ_META_VER) {
> +	dmz_version = le32_to_cpu(sb->version);
> +	if (dmz_version > DMZ_META_VER) {
>  		dmz_dev_err(dev, "Invalid meta version (needed %d, got %d)",
> -			    DMZ_META_VER, le32_to_cpu(sb->version));
> +			    DMZ_META_VER, dmz_version);
> +		return -ENXIO;
> +	}
> +
> +	gen = le64_to_cpu(sb->gen);
> +	stored_crc = le32_to_cpu(sb->crc);
> +	sb->crc = 0;
> +	crc = crc32_le(gen, (unsigned char *)sb, DMZ_BLOCK_SIZE);
> +	if (crc != stored_crc) {
> +		dmz_dev_err(dev, "Invalid checksum (needed 0x%08x, got 0x%08x)",
> +			    crc, stored_crc);
>  		return -ENXIO;
>  	}
>  
> @@ -895,6 +917,21 @@ static int dmz_check_sb(struct dmz_metadata *zmd, struct dmz_super *sb)
>  		return -ENXIO;
>  	}
>  
> +	if (dmz_version == DMZ_META_VER) {
> +		if (uuid_is_null((uuid_t *)sb->dmz_uuid)) {
> +			dmz_dev_err(dev, "NULL DM-Zoned uuid");
> +			return -ENXIO;
> +		}
> +		uuid_copy(&zmd->dmz_uuid, (uuid_t *)sb->dmz_uuid);
> +		memcpy(zmd->dmz_label, sb->dmz_label, BDEVNAME_SIZE);
> +		if (uuid_is_null((uuid_t *)sb->dev_uuid)) {
> +			dmz_dev_err(dev, "NULL device uuid");
> +			return -ENXIO;
> +		}
> +		uuid_copy(&zmd->dev_uuid, (uuid_t *)sb->dev_uuid);
> +
> +	}
> +
>  	/* OK */
>  	zmd->nr_meta_blocks = le32_to_cpu(sb->nr_meta_blocks);
>  	zmd->nr_reserved_seq = le32_to_cpu(sb->nr_reserved_seq);
> @@ -2460,9 +2497,18 @@ int dmz_ctr_metadata(struct dmz_dev *dev, struct dmz_metadata **metadata)
>  		goto err;
>  	}
>  
> +	dmz_dev_info(dev, "DM-Zoned version %d",
> +		     uuid_is_null(&zmd->dmz_uuid) ?
> +		     DMZ_META_COMPAT_VER : DMZ_META_VER);
> +	if (!uuid_is_null(&zmd->dmz_uuid))
> +		dmz_dev_info(dev, "DM UUID %pUl", &zmd->dmz_uuid);
> +	if (strlen(zmd->dmz_label))
> +		dmz_dev_info(dev, "DM Label %s", zmd->dmz_label);
>  	dmz_dev_info(dev, "Host-%s zoned block device",
>  		     bdev_zoned_model(dev->bdev) == BLK_ZONED_HA ?
>  		     "aware" : "managed");
> +	if (!uuid_is_null(&zmd->dev_uuid))
> +		dmz_dev_info(dev, "  uuid %pUl", &zmd->dev_uuid);
>  	dmz_dev_info(dev, "  %llu 512-byte logical sectors",
>  		     (u64)dev->capacity);
>  	dmz_dev_info(dev, "  %u zones of %llu 512-byte logical sectors",
> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
> index 44e30a7de8b9..7ec9dde24516 100644
> --- a/drivers/md/dm-zoned-target.c
> +++ b/drivers/md/dm-zoned-target.c
> @@ -1008,7 +1008,7 @@ static int dmz_message(struct dm_target *ti, unsigned int argc, char **argv,
>  
>  static struct target_type dmz_type = {
>  	.name		 = "zoned",
> -	.version	 = {1, 1, 0},
> +	.version	 = {1, 2, 0},
>  	.features	 = DM_TARGET_SINGLETON | DM_TARGET_ZONED_HM,
>  	.module		 = THIS_MODULE,
>  	.ctr		 = dmz_ctr,
> 

Re: [PATCH 4/4] dm-zoned: allow for device size smaller than the capacity

[Damien Le Moal]

On 2020/03/31 17:53, Hannes Reinecke wrote:
> On 3/31/20 2:49 AM, Damien Le Moal wrote:
>> On 2020/03/27 16:15, Hannes Reinecke wrote:
>>> dm-zoned requires several zones for metadata and chunk bitmaps,
>>> so it cannot expose the entire capacity as the device size.
>>> Originally the code would check for the capacity being equal to
>>> the device size, which is arguably wrong.
>>> So relax this check and increase the interface version number
>>> to signal to userspace that it can set a smaller device size.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>   drivers/md/dm-zoned-target.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
>>> index 7ec9dde24516..89a825d1034e 100644
>>> --- a/drivers/md/dm-zoned-target.c
>>> +++ b/drivers/md/dm-zoned-target.c
>>> @@ -715,7 +715,7 @@ static int dmz_get_zoned_device(struct dm_target *ti, char *path)
>>>   	aligned_capacity = dev->capacity &
>>>   				~((sector_t)blk_queue_zone_sectors(q) - 1);
>>>   	if (ti->begin ||
>>> -	    ((ti->len != dev->capacity) && (ti->len != aligned_capacity))) {
>>> +	    ((ti->len > dev->capacity) && (ti->len > aligned_capacity))) {
>>>   		ti->error = "Partial mapping not supported";
>>
>> The message is now wrong. Also, the condition can now be simplified to:
>>
>> if ((ti->begin + ti->len) > aligned_capacity) {
>>
>> Since aligned capacity is equal or smaller than dev capacity. And we have to
>> account for the potential non-zero begin.
>>
> _Actually_ I would forbid for 'ti->begin' to be anything other than 0.
> For a zoned device there is no point in allowing for partial handling at 
> all.
> Problem is that 'dev->capacity' is the capacity of the zoned block 
> device, whereas 'ti-len' is the exported capacity of the device-mapper 
> device, which is smaller than the device capacity by the number of 
> metadata blocks/zones.

OK. Got it. I misunderstood that. Sounds good to me.

> 
>>>   		ret = -EINVAL;
>>>   		goto err;
>>> @@ -1008,7 +1008,7 @@ static int dmz_message(struct dm_target *ti, unsigned int argc, char **argv,
>>>   
>>>   static struct target_type dmz_type = {
>>>   	.name		 = "zoned",
>>> -	.version	 = {1, 2, 0},
>>> +	.version	 = {1, 3, 0},
>>>   	.features	 = DM_TARGET_SINGLETON | DM_TARGET_ZONED_HM,
>>>   	.module		 = THIS_MODULE,
>>>   	.ctr		 = dmz_ctr,
>>>
>>
>> I do not think this is nearly enough: dmz_init_zones() is still considering the
>> entire drive and does a zone report from 0 on the backend bdev. It should start
>> at ti->begin and up to ti->begin+ti->len, no ?
>>
> Yes, and no. I want to disallow 'ti->begin' to be anything else than 0 
> as ti->begin and ti->len are relative to exported device-mapper device, 
> and we always want to have block 0 mapped :-)
> 
> And as such dmz_init_zones() needs to cover all zones, as this is 
> relative to the zoned block device.

Yes. Which is already the case since we need to see the metatdata and reserved
zones too. Makes sense.

> 
>> Furthermore, this introduce a change in the meaning of the zone ID. Since this
>> is set to the index of the zone in the report (patch 1), if the mapping is
>> partial and the zone report does not start at 0, then zone ID is not zone number
>> on the device anymore. So dmz_start_block() needs to be offset by ti->begin
>> otherwise IOs will go to the wrong zones.
>>
> As I said: We will never do a partial mapping.
> What this patch does it to bring the device-mapper mapping in-line with 
> the exported device size.

Got it !

> Originally we would export a device-mapper mapping for blocks up to the 
> zone-device capacity. As the resulting device-mapper block device has a 
> smaller capacity than the mapping would allow for those 'spare' blocks 
> would never been used, thus the invalid mapping was never triggered.
> 
> What this patch does is to bring the device-mapper mapping in-line with 
> the exported block device capacity, so that we don't have an invalid 
> mapping. Nothing else has (and should) be changed.
> 
> Especially not the partial zoned device handling :-)

OK. Thanks for doing that ! I  totally missed these points when I wrote the
mapper :)

Cheers.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] dm-zoned: V2 metadata handling

[John Dorminy]

[-- Attachment #1.1: Type: text/plain, Size: 468 bytes --]

I'm worried about hardcoding uuid members as u8[16].

May I ask why you're not using uuid_t to define it in the on-disk
structure? It would save the casting of the uuid members to (uuid_t *)
every time you use a uuid.h function.

Possibly it is customary to use only raw datatypes on disk rather than
opaque types like uuid_t, I'm not sure. But in that case, perhaps using the
named constant UUID_SIZE instead of 16 would make the meaning clearer?

Thanks!

[-- Attachment #1.2: Type: text/html, Size: 545 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/4] dm-zoned: V2 metadata handling

[Hannes Reinecke]

On 4/2/20 4:53 PM, John Dorminy wrote:
> I'm worried about hardcoding uuid members as u8[16].
> 
> May I ask why you're not using uuid_t to define it in the on-disk 
> structure? It would save the casting of the uuid members to (uuid_t *) 
> every time you use a uuid.h function.
> 
> Possibly it is customary to use only raw datatypes on disk rather than 
> opaque types like uuid_t, I'm not sure. But in that case, perhaps using 
> the named constant UUID_SIZE instead of 16 would make the meaning clearer?
> 
I prefer to use absolute types (like __u8) when describing the on-disk 
format.
When using opaque types like uuid_t there always is the risk that the 
internal representation will change, leading to an involuntary change of 
the on-disk format structure and subsequent compability issues.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@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: [PATCH 3/4] dm-zoned: V2 metadata handling

[John Dorminy]

[-- Attachment #1.1: Type: text/plain, Size: 1368 bytes --]

That does make sense. May I request, then, using UUID_SIZE instead of 16?
Perhaps with a compile-time assertion that UUID_SIZE has not change from 16?

On Thu, Apr 2, 2020 at 11:10 AM Hannes Reinecke <hare@suse.de> wrote:

> On 4/2/20 4:53 PM, John Dorminy wrote:
> > I'm worried about hardcoding uuid members as u8[16].
> >
> > May I ask why you're not using uuid_t to define it in the on-disk
> > structure? It would save the casting of the uuid members to (uuid_t *)
> > every time you use a uuid.h function.
> >
> > Possibly it is customary to use only raw datatypes on disk rather than
> > opaque types like uuid_t, I'm not sure. But in that case, perhaps using
> > the named constant UUID_SIZE instead of 16 would make the meaning
> clearer?
> >
> I prefer to use absolute types (like __u8) when describing the on-disk
> format.
> When using opaque types like uuid_t there always is the risk that the
> internal representation will change, leading to an involuntary change of
> the on-disk format structure and subsequent compability issues.
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke            Teamlead Storage & Networking
> hare@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
>
>

[-- Attachment #1.2: Type: text/html, Size: 1817 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/4] dm-zoned: V2 metadata handling

[John Dorminy]

[-- Attachment #1.1: Type: text/plain, Size: 2130 bytes --]

Actually, I take that back. If it is a worry that the internal
representation will change, it seems also unsafe to be casting the sixteen
bytes as a uuid_t when copying into them with uuid_copy(). If the internal
representation changed to add a new field, then there would be a buffer
overrun when using uuid_copy(). If the internal representation changed the
order of the bytes, uuid_copy() would propagate that to the on-disk
structure and yield compatibility issues.

Perhaps it would be safer to store the UUIDs on disk as a string. Or add to
uuid.[ch] a function to get the raw bytes in a fixed order suitable for
storing on disk?

On Thu, Apr 2, 2020 at 11:52 AM John Dorminy <jdorminy@redhat.com> wrote:

> That does make sense. May I request, then, using UUID_SIZE instead of 16?
> Perhaps with a compile-time assertion that UUID_SIZE has not change from 16?
>
> On Thu, Apr 2, 2020 at 11:10 AM Hannes Reinecke <hare@suse.de> wrote:
>
>> On 4/2/20 4:53 PM, John Dorminy wrote:
>> > I'm worried about hardcoding uuid members as u8[16].
>> >
>> > May I ask why you're not using uuid_t to define it in the on-disk
>> > structure? It would save the casting of the uuid members to (uuid_t *)
>> > every time you use a uuid.h function.
>> >
>> > Possibly it is customary to use only raw datatypes on disk rather than
>> > opaque types like uuid_t, I'm not sure. But in that case, perhaps using
>> > the named constant UUID_SIZE instead of 16 would make the meaning
>> clearer?
>> >
>> I prefer to use absolute types (like __u8) when describing the on-disk
>> format.
>> When using opaque types like uuid_t there always is the risk that the
>> internal representation will change, leading to an involuntary change of
>> the on-disk format structure and subsequent compability issues.
>>
>> Cheers,
>>
>> Hannes
>> --
>> Dr. Hannes Reinecke            Teamlead Storage & Networking
>> hare@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
>>
>>

[-- Attachment #1.2: Type: text/html, Size: 2852 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/4] dm-zoned: V2 metadata handling

[Damien Le Moal]

On 2020/04/03 5:01, John Dorminy wrote:
> Actually, I take that back. If it is a worry that the internal representation
> will change, it seems also unsafe to be casting the sixteen bytes as a uuid_t
> when copying into them with uuid_copy(). If the internal representation changed
> to add a new field, then there would be a buffer overrun when using uuid_copy().
> If the internal representation changed the order of the bytes, uuid_copy() would
> propagate that to the on-disk structure and yield compatibility issues.
> 
> Perhaps it would be safer to store the UUIDs on disk as a string. Or add to
> uuid.[ch] a function to get the raw bytes in a fixed order suitable for storing
> on disk?

Please have a look at file systems on-disk metadata definition and checks. There
are plenty of examples of how UUIDs are handled. For instance, XFS has in
fs/xfs/libxfs/xfs_format.h:

typedef struct xfs_dsb {
	...
	uuid_t          sb_uuid;        /* user-visible file system unique id */
	...
}

For the on-disk super block UUID and the same for the in-memory version.
And you can see code like:
memcpy(&to->sb_uuid, &from->sb_uuid, sizeof(to->sb_uuid));
uuid_copy(&to->sb_meta_uuid, &from->sb_uuid);

and checks such as:

BUILD_BUG_ON(sizeof(geo->uuid) != sizeof(sbp->sb_uuid));

for some UUID declared with:

unsigned char   uuid[16];       /* unique id of the filesystem  */
(struct xfs_fsop_geom_v1 in fs/xfs/libxfs/xfs_fs.h.

On the other hand, f2fs defines its on-disk and in-memory super block UUID as:

__u8 uuid[16];                  /* 128-bit uuid for volume */

And copies it with:

memcpy(&sb->s_uuid, raw_super->uuid, sizeof(raw_super->uuid));

The general pattern is:
* For UUIDs defined as uuid_t, use uuid_copy() or memcpy()
* For UUIDs defined as an array of bytes or char, use memcpy()
* Add BUILD_BUG_ON() checks when mixing both definitions.

There is no need to go as far as using a string for the UUID.

-- 
Damien Le Moal
Western Digital Research