* [PATCH v2 1/4] docs/qcow2: add the zoned format feature
2023-08-14 8:57 [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver Sam Li
@ 2023-08-14 8:57 ` Sam Li
2023-08-14 8:58 ` [PATCH v2 2/4] qcow2: add configurations for zoned format extension Sam Li
` (3 subsequent siblings)
4 siblings, 0 replies; 26+ messages in thread
From: Sam Li @ 2023-08-14 8:57 UTC (permalink / raw)
To: qemu-devel
Cc: hare, Hanna Reitz, dmitry.fomichev, qemu-block, Kevin Wolf,
Markus Armbruster, Eric Blake, stefanha, dlemoal, Sam Li
Add the specs for the zoned format feature of the qcow2 driver. If
the zoned_profile is set to `zbc`, then the qcow2 file can be taken
as zoned device and passed through by virtio-blk device to the guest.
If it's `zns`, then it can be passed through by virtio-blk device or
NVMe ZNS device as a ZNS drive.
Signed-off-by: Sam Li <faithilikerun@gmail.com>
---
docs/system/qemu-block-drivers.rst.inc | 39 ++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/docs/system/qemu-block-drivers.rst.inc b/docs/system/qemu-block-drivers.rst.inc
index 105cb9679c..2c1620668f 100644
--- a/docs/system/qemu-block-drivers.rst.inc
+++ b/docs/system/qemu-block-drivers.rst.inc
@@ -172,6 +172,45 @@ This section describes each format and the options that are supported for it.
filename`` to check if the NOCOW flag is set or not (Capital 'C' is
NOCOW flag).
+ .. option:: zoned_profile
+
+ The option configures the zoned format feature on the qcow2 driver. If
+ this is set to ``zbc``, then it follows the basics of ZBC/ZAC protocol.
+ If setting to ``zns``, then it follows NVMe ZNS protocol.
+
+ The virtio-blk device allows ``zbc`` and ``zns`` options to pass through
+ zoned devices. While NVMe ZNS device only allows ``zns`` option.
+
+ .. option:: zone_size
+
+ The size of a zone of the zoned device in bytes. The device is divided
+ into zones of this size with the exception of the last zone, which may
+ be smaller.
+
+ .. option:: zone_capacity
+
+ The initial capacity value for all zones. The capacity must be less than
+ or equal to zone size. If the last zone is smaller, then its capacity is
+ capped. The device follows the ZBC protocol tends to have the same size
+ as its zone.
+
+ .. option:: zone_nr_conv
+
+ The number of conventional zones of the zoned device.
+
+ .. option:: max_open_zones
+
+ The maximal allowed open zones.
+
+ .. option:: max_active_zones
+
+ The limit of the zones with implicit open, explicit open or closed state.
+
+ .. option:: max_append_sectors
+
+ The maximal sectors in 512B blocks that is allowed to append to zones
+ while writing.
+
.. program:: image-formats
.. option:: qed
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-14 8:57 [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver Sam Li
2023-08-14 8:57 ` [PATCH v2 1/4] docs/qcow2: add the zoned format feature Sam Li
@ 2023-08-14 8:58 ` Sam Li
2023-08-16 19:31 ` Stefan Hajnoczi
` (2 more replies)
2023-08-14 8:58 ` [PATCH v2 3/4] qcow2: add zoned emulation capability Sam Li
` (2 subsequent siblings)
4 siblings, 3 replies; 26+ messages in thread
From: Sam Li @ 2023-08-14 8:58 UTC (permalink / raw)
To: qemu-devel
Cc: hare, Hanna Reitz, dmitry.fomichev, qemu-block, Kevin Wolf,
Markus Armbruster, Eric Blake, stefanha, dlemoal, Sam Li
To configure the zoned format feature on the qcow2 driver, it
requires following arguments: the device size, zoned profile,
zoned model, zone size, zone capacity, number of conventional
zones, limits on zone resources (max append sectors, max open
zones, and max_active_zones). The zoned profile option is set
to zns when using the qcow2 file as a ZNS drive.
To create a qcow2 file with zoned format, use command like this:
$ qemu-img create -f qcow2 test.qcow2 -o size=768M -o
zone_size=64M -o zone_capacity=64M -o zone_nr_conv=0 -o
max_append_sectors=512 -o max_open_zones=0 -o max_active_zones=0
-o zoned_profile=zbc/zns
Signed-off-by: Sam Li <faithilikerun@gmail.com>
---
block/qcow2.c | 125 +++++++++++++++++++++++++++++++
block/qcow2.h | 21 ++++++
docs/interop/qcow2.txt | 24 ++++++
include/block/block-common.h | 5 ++
include/block/block_int-common.h | 16 ++++
qapi/block-core.json | 46 ++++++++----
6 files changed, 223 insertions(+), 14 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index c51388e99d..c1077c4a4a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -73,6 +73,7 @@ typedef struct {
#define QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
#define QCOW2_EXT_MAGIC_BITMAPS 0x23852875
#define QCOW2_EXT_MAGIC_DATA_FILE 0x44415441
+#define QCOW2_EXT_MAGIC_ZONED_FORMAT 0x7a6264
static int coroutine_fn
qcow2_co_preadv_compressed(BlockDriverState *bs,
@@ -210,6 +211,7 @@ qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
uint64_t offset;
int ret;
Qcow2BitmapHeaderExt bitmaps_ext;
+ Qcow2ZonedHeaderExtension zoned_ext;
if (need_update_header != NULL) {
*need_update_header = false;
@@ -431,6 +433,38 @@ qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
break;
}
+ case QCOW2_EXT_MAGIC_ZONED_FORMAT:
+ {
+ if (ext.len != sizeof(zoned_ext)) {
+ error_setg_errno(errp, -ret, "zoned_ext: "
+ "Invalid extension length");
+ return -EINVAL;
+ }
+ ret = bdrv_pread(bs->file, offset, ext.len, &zoned_ext, 0);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "zoned_ext: "
+ "Could not read ext header");
+ return ret;
+ }
+
+ zoned_ext.zone_size = be32_to_cpu(zoned_ext.zone_size);
+ zoned_ext.zone_capacity = be32_to_cpu(zoned_ext.zone_capacity);
+ zoned_ext.nr_zones = be32_to_cpu(zoned_ext.nr_zones);
+ zoned_ext.zone_nr_conv = be32_to_cpu(zoned_ext.zone_nr_conv);
+ zoned_ext.max_open_zones = be32_to_cpu(zoned_ext.max_open_zones);
+ zoned_ext.max_active_zones =
+ be32_to_cpu(zoned_ext.max_active_zones);
+ zoned_ext.max_append_sectors =
+ be32_to_cpu(zoned_ext.max_append_sectors);
+ s->zoned_header = zoned_ext;
+
+#ifdef DEBUG_EXT
+ printf("Qcow2: Got zoned format extension: "
+ "offset=%" PRIu32 "\n", offset);
+#endif
+ break;
+ }
+
default:
/* unknown magic - save it in case we need to rewrite the header */
/* If you add a new feature, make sure to also update the fast
@@ -3089,6 +3123,31 @@ int qcow2_update_header(BlockDriverState *bs)
buflen -= ret;
}
+ /* Zoned devices header extension */
+ if (s->zoned_header.zoned == BLK_Z_HM) {
+ Qcow2ZonedHeaderExtension zoned_header = {
+ .zoned_profile = s->zoned_header.zoned_profile,
+ .zoned = s->zoned_header.zoned,
+ .nr_zones = cpu_to_be32(s->zoned_header.nr_zones),
+ .zone_size = cpu_to_be32(s->zoned_header.zone_size),
+ .zone_capacity = cpu_to_be32(s->zoned_header.zone_capacity),
+ .zone_nr_conv = cpu_to_be32(s->zoned_header.zone_nr_conv),
+ .max_open_zones = cpu_to_be32(s->zoned_header.max_open_zones),
+ .max_active_zones =
+ cpu_to_be32(s->zoned_header.max_active_zones),
+ .max_append_sectors =
+ cpu_to_be32(s->zoned_header.max_append_sectors)
+ };
+ ret = header_ext_add(buf, QCOW2_EXT_MAGIC_ZONED_FORMAT,
+ &zoned_header, sizeof(zoned_header),
+ buflen);
+ if (ret < 0) {
+ goto fail;
+ }
+ buf += ret;
+ buflen -= ret;
+ }
+
/* Keep unknown header extensions */
QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
@@ -3773,6 +3832,23 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
s->image_data_file = g_strdup(data_bs->filename);
}
+ if (qcow2_opts->zoned_profile) {
+ BDRVQcow2State *s = blk_bs(blk)->opaque;
+ if (!strcmp(qcow2_opts->zoned_profile, "zbc")) {
+ s->zoned_header.zoned_profile = BLK_ZP_ZBC;
+ s->zoned_header.zone_capacity = qcow2_opts->zone_size;
+ } else if (!strcmp(qcow2_opts->zoned_profile, "zns")) {
+ s->zoned_header.zoned_profile = BLK_ZP_ZNS;
+ s->zoned_header.zone_capacity = qcow2_opts->zone_capacity;
+ }
+ s->zoned_header.zoned = BLK_Z_HM;
+ s->zoned_header.zone_size = qcow2_opts->zone_size;
+ s->zoned_header.zone_nr_conv = qcow2_opts->zone_nr_conv;
+ s->zoned_header.max_open_zones = qcow2_opts->max_open_zones;
+ s->zoned_header.max_active_zones = qcow2_opts->max_active_zones;
+ s->zoned_header.max_append_sectors = qcow2_opts->max_append_sectors;
+ }
+
/* Create a full header (including things like feature table) */
ret = qcow2_update_header(blk_bs(blk));
bdrv_graph_co_rdunlock();
@@ -3891,6 +3967,13 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v3");
}
+ /* The available zoned-profile options are zbc, which stands for
+ * ZBC/ZAC standards, and zns following NVMe ZNS spec. */
+ val = qdict_get_try_str(qdict, BLOCK_OPT_Z_PROFILE);
+ if (val) {
+ qdict_put_str(qdict, BLOCK_OPT_Z_PROFILE, val);
+ }
+
/* Change legacy command line options into QMP ones */
static const QDictRenames opt_renames[] = {
{ BLOCK_OPT_BACKING_FILE, "backing-file" },
@@ -3903,6 +3986,13 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
{ BLOCK_OPT_COMPAT_LEVEL, "version" },
{ BLOCK_OPT_DATA_FILE_RAW, "data-file-raw" },
{ BLOCK_OPT_COMPRESSION_TYPE, "compression-type" },
+ { BLOCK_OPT_Z_PROFILE, "zoned-profile"},
+ { BLOCK_OPT_Z_NR_COV, "zone-nr-conv"},
+ { BLOCK_OPT_Z_MOZ, "max-open-zones"},
+ { BLOCK_OPT_Z_MAZ, "max-active-zones"},
+ { BLOCK_OPT_Z_MAS, "max-append-sectors"},
+ { BLOCK_OPT_Z_SIZE, "zone-size"},
+ { BLOCK_OPT_Z_CAP, "zone-capacity"},
{ NULL, NULL },
};
@@ -6066,6 +6156,41 @@ static QemuOptsList qcow2_create_opts = {
.help = "Compression method used for image cluster " \
"compression", \
.def_value_str = "zlib" \
+ }, \
+ {
+ .name = BLOCK_OPT_Z_PROFILE, \
+ .type = QEMU_OPT_STRING, \
+ .help = "zoned format option for the disk img", \
+ }, \
+ { \
+ .name = BLOCK_OPT_Z_SIZE, \
+ .type = QEMU_OPT_SIZE, \
+ .help = "zone size", \
+ }, \
+ { \
+ .name = BLOCK_OPT_Z_CAP, \
+ .type = QEMU_OPT_SIZE, \
+ .help = "zone capacity", \
+ }, \
+ { \
+ .name = BLOCK_OPT_Z_NR_COV, \
+ .type = QEMU_OPT_NUMBER, \
+ .help = "numbers of conventional zones", \
+ }, \
+ { \
+ .name = BLOCK_OPT_Z_MAS, \
+ .type = QEMU_OPT_NUMBER, \
+ .help = "max append sectors", \
+ }, \
+ { \
+ .name = BLOCK_OPT_Z_MAZ, \
+ .type = QEMU_OPT_NUMBER, \
+ .help = "max active zones", \
+ }, \
+ { \
+ .name = BLOCK_OPT_Z_MOZ, \
+ .type = QEMU_OPT_NUMBER, \
+ .help = "max open zones", \
},
QCOW_COMMON_OPTIONS,
{ /* end of list */ }
diff --git a/block/qcow2.h b/block/qcow2.h
index f789ce3ae0..3694c8d217 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -236,6 +236,20 @@ typedef struct Qcow2CryptoHeaderExtension {
uint64_t length;
} QEMU_PACKED Qcow2CryptoHeaderExtension;
+typedef struct Qcow2ZonedHeaderExtension {
+ /* Zoned device attributes */
+ uint8_t zoned_profile;
+ uint8_t zoned;
+ uint16_t reserved16;
+ uint32_t zone_size;
+ uint32_t zone_capacity;
+ uint32_t nr_zones;
+ uint32_t zone_nr_conv;
+ uint32_t max_active_zones;
+ uint32_t max_open_zones;
+ uint32_t max_append_sectors;
+} QEMU_PACKED Qcow2ZonedHeaderExtension;
+
typedef struct Qcow2UnknownHeaderExtension {
uint32_t magic;
uint32_t len;
@@ -422,6 +436,13 @@ typedef struct BDRVQcow2State {
* is to convert the image with the desired compression type set.
*/
Qcow2CompressionType compression_type;
+
+ /* States of zoned device */
+ Qcow2ZonedHeaderExtension zoned_header;
+ uint32_t nr_zones_exp_open;
+ uint32_t nr_zones_imp_open;
+ uint32_t nr_zones_closed;
+ BlockZoneWps *wps;
} BDRVQcow2State;
typedef struct Qcow2COWRegion {
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 2c4618375a..ef2ba6f670 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -331,6 +331,30 @@ The fields of the bitmaps extension are:
Offset into the image file at which the bitmap directory
starts. Must be aligned to a cluster boundary.
+== Zoned extension ==
+
+The zoned extension is an optional header extension. It is required when
+using the qcow2 file as the backing image for zoned device.
+
+The fields of the zoned extension are:
+ Byte 0: zoned_profile
+ Type of zoned format. Must be `zbc` or `zns`.
+ 1: `zbc`
+ 2: `zns`
+
+ 1: zoned
+ Type of zone.
+
+ 2 - 3: Reserved, must be zero.
+
+ 4 - 7: zone_size
+ 8 - 11: zone_capacity
+ 12 - 15: nr_zones
+ 16 - 19: zone_nr_conv
+ 20 - 23: max_active_zones
+ 24 - 27: max_open_zones
+ 28 - 31: max_append_sectors
+
== Full disk encryption header pointer ==
The full disk encryption header must be present if, and only if, the
diff --git a/include/block/block-common.h b/include/block/block-common.h
index e15395f2cb..9f04a772f6 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -108,6 +108,11 @@ typedef enum BlockZoneType {
BLK_ZT_SWP = 0x3, /* Sequential writes preferred */
} BlockZoneType;
+typedef enum BlockZonedProfile {
+ BLK_ZP_ZBC = 0x1,
+ BLK_ZP_ZNS = 0x2,
+} BlockZonedProfile;
+
/*
* Zone descriptor data structure.
* Provides information on a zone with all position and size values in bytes.
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 74195c3004..1dbe820a9b 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -57,6 +57,14 @@
#define BLOCK_OPT_DATA_FILE_RAW "data_file_raw"
#define BLOCK_OPT_COMPRESSION_TYPE "compression_type"
#define BLOCK_OPT_EXTL2 "extended_l2"
+#define BLOCK_OPT_Z_PROFILE "zoned_profile"
+#define BLOCK_OPT_Z_MODEL "zoned"
+#define BLOCK_OPT_Z_SIZE "zone_size"
+#define BLOCK_OPT_Z_CAP "zone_capacity"
+#define BLOCK_OPT_Z_NR_COV "zone_nr_conv"
+#define BLOCK_OPT_Z_MAS "max_append_sectors"
+#define BLOCK_OPT_Z_MAZ "max_active_zones"
+#define BLOCK_OPT_Z_MOZ "max_open_zones"
#define BLOCK_PROBE_BUF_SIZE 512
@@ -872,12 +880,20 @@ typedef struct BlockLimits {
*/
bool has_variable_length;
+ BlockZonedProfile zoned_profile;
+
/* device zone model */
BlockZoneModel zoned;
/* zone size expressed in bytes */
uint32_t zone_size;
+ /*
+ * the number of usable logical blocks within the zone, expressed
+ * in bytes. A zone capacity is smaller or equal to the zone size.
+ */
+ uint32_t zone_capacity;
+
/* total number of zones */
uint32_t nr_zones;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2b1d493d6e..0c97ae678b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5020,24 +5020,42 @@
#
# @compression-type: The image cluster compression method
# (default: zlib, since 5.1)
+# @zoned-profile: Two zoned device protocol options, zbc or zns
+# (default: off, since 8.0)
+# @zone-size: The size of a zone of the zoned device (since 8.0)
+# @zone-capacity: The capacity of a zone of the zoned device (since 8.0)
+# @zone-nr-conv: The number of conventional zones of the zoned device
+# (since 8.0)
+# @max-open-zones: The maximal allowed open zones (since 8.0)
+# @max-active-zones: The limit of the zones that have the implicit open,
+# explicit open or closed state (since 8.0)
+# @max-append-sectors: The maximal sectors that is allowed to append write
+# (since 8.0)
#
# Since: 2.12
##
{ 'struct': 'BlockdevCreateOptionsQcow2',
- 'data': { 'file': 'BlockdevRef',
- '*data-file': 'BlockdevRef',
- '*data-file-raw': 'bool',
- '*extended-l2': 'bool',
- 'size': 'size',
- '*version': 'BlockdevQcow2Version',
- '*backing-file': 'str',
- '*backing-fmt': 'BlockdevDriver',
- '*encrypt': 'QCryptoBlockCreateOptions',
- '*cluster-size': 'size',
- '*preallocation': 'PreallocMode',
- '*lazy-refcounts': 'bool',
- '*refcount-bits': 'int',
- '*compression-type':'Qcow2CompressionType' } }
+ 'data': { 'file': 'BlockdevRef',
+ '*data-file': 'BlockdevRef',
+ '*data-file-raw': 'bool',
+ '*extended-l2': 'bool',
+ 'size': 'size',
+ '*version': 'BlockdevQcow2Version',
+ '*backing-file': 'str',
+ '*backing-fmt': 'BlockdevDriver',
+ '*encrypt': 'QCryptoBlockCreateOptions',
+ '*cluster-size': 'size',
+ '*preallocation': 'PreallocMode',
+ '*lazy-refcounts': 'bool',
+ '*refcount-bits': 'int',
+ '*compression-type': 'Qcow2CompressionType',
+ '*zoned-profile': 'str',
+ '*zone-size': 'size',
+ '*zone-capacity': 'size',
+ '*zone-nr-conv': 'uint32',
+ '*max-open-zones': 'uint32',
+ '*max-active-zones': 'uint32',
+ '*max-append-sectors': 'uint32'}}
##
# @BlockdevCreateOptionsQed:
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-14 8:58 ` [PATCH v2 2/4] qcow2: add configurations for zoned format extension Sam Li
@ 2023-08-16 19:31 ` Stefan Hajnoczi
2023-08-21 13:13 ` Markus Armbruster
2023-08-21 13:31 ` Stefan Hajnoczi
2 siblings, 0 replies; 26+ messages in thread
From: Stefan Hajnoczi @ 2023-08-16 19:31 UTC (permalink / raw)
To: Sam Li
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake, dlemoal
[-- Attachment #1: Type: text/plain, Size: 19784 bytes --]
On Mon, Aug 14, 2023 at 04:58:00PM +0800, Sam Li wrote:
> To configure the zoned format feature on the qcow2 driver, it
> requires following arguments: the device size, zoned profile,
> zoned model, zone size, zone capacity, number of conventional
> zones, limits on zone resources (max append sectors, max open
> zones, and max_active_zones). The zoned profile option is set
> to zns when using the qcow2 file as a ZNS drive.
>
> To create a qcow2 file with zoned format, use command like this:
> $ qemu-img create -f qcow2 test.qcow2 -o size=768M -o
> zone_size=64M -o zone_capacity=64M -o zone_nr_conv=0 -o
> max_append_sectors=512 -o max_open_zones=0 -o max_active_zones=0
> -o zoned_profile=zbc/zns
>
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
> block/qcow2.c | 125 +++++++++++++++++++++++++++++++
> block/qcow2.h | 21 ++++++
> docs/interop/qcow2.txt | 24 ++++++
> include/block/block-common.h | 5 ++
> include/block/block_int-common.h | 16 ++++
> qapi/block-core.json | 46 ++++++++----
> 6 files changed, 223 insertions(+), 14 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c51388e99d..c1077c4a4a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -73,6 +73,7 @@ typedef struct {
> #define QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
> #define QCOW2_EXT_MAGIC_BITMAPS 0x23852875
> #define QCOW2_EXT_MAGIC_DATA_FILE 0x44415441
> +#define QCOW2_EXT_MAGIC_ZONED_FORMAT 0x7a6264
>
> static int coroutine_fn
> qcow2_co_preadv_compressed(BlockDriverState *bs,
> @@ -210,6 +211,7 @@ qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> uint64_t offset;
> int ret;
> Qcow2BitmapHeaderExt bitmaps_ext;
> + Qcow2ZonedHeaderExtension zoned_ext;
>
> if (need_update_header != NULL) {
> *need_update_header = false;
> @@ -431,6 +433,38 @@ qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> break;
> }
>
> + case QCOW2_EXT_MAGIC_ZONED_FORMAT:
> + {
> + if (ext.len != sizeof(zoned_ext)) {
> + error_setg_errno(errp, -ret, "zoned_ext: "
ret does not contain a useful value. I suggest calling error_setg()
instead.
> + "Invalid extension length");
> + return -EINVAL;
> + }
> + ret = bdrv_pread(bs->file, offset, ext.len, &zoned_ext, 0);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "zoned_ext: "
> + "Could not read ext header");
> + return ret;
> + }
> +
> + zoned_ext.zone_size = be32_to_cpu(zoned_ext.zone_size);
> + zoned_ext.zone_capacity = be32_to_cpu(zoned_ext.zone_capacity);
> + zoned_ext.nr_zones = be32_to_cpu(zoned_ext.nr_zones);
> + zoned_ext.zone_nr_conv = be32_to_cpu(zoned_ext.zone_nr_conv);
> + zoned_ext.max_open_zones = be32_to_cpu(zoned_ext.max_open_zones);
> + zoned_ext.max_active_zones =
> + be32_to_cpu(zoned_ext.max_active_zones);
> + zoned_ext.max_append_sectors =
> + be32_to_cpu(zoned_ext.max_append_sectors);
> + s->zoned_header = zoned_ext;
I suggest adding checks here and refusing to open broken images:
if (zone_size == 0) {
error_setg(errp, "Zoned extension header zone_size field cannot be 0");
return -EINVAL;
}
if (zone_capacity > zone_size) { ... }
if (nr_zones != DIV_ROUND_UP(bs->total_size, zone_size)) { ... }
> +
> +#ifdef DEBUG_EXT
> + printf("Qcow2: Got zoned format extension: "
> + "offset=%" PRIu32 "\n", offset);
> +#endif
> + break;
> + }
> +
> default:
> /* unknown magic - save it in case we need to rewrite the header */
> /* If you add a new feature, make sure to also update the fast
> @@ -3089,6 +3123,31 @@ int qcow2_update_header(BlockDriverState *bs)
> buflen -= ret;
> }
>
> + /* Zoned devices header extension */
> + if (s->zoned_header.zoned == BLK_Z_HM) {
> + Qcow2ZonedHeaderExtension zoned_header = {
> + .zoned_profile = s->zoned_header.zoned_profile,
> + .zoned = s->zoned_header.zoned,
> + .nr_zones = cpu_to_be32(s->zoned_header.nr_zones),
> + .zone_size = cpu_to_be32(s->zoned_header.zone_size),
> + .zone_capacity = cpu_to_be32(s->zoned_header.zone_capacity),
> + .zone_nr_conv = cpu_to_be32(s->zoned_header.zone_nr_conv),
> + .max_open_zones = cpu_to_be32(s->zoned_header.max_open_zones),
> + .max_active_zones =
> + cpu_to_be32(s->zoned_header.max_active_zones),
> + .max_append_sectors =
> + cpu_to_be32(s->zoned_header.max_append_sectors)
> + };
> + ret = header_ext_add(buf, QCOW2_EXT_MAGIC_ZONED_FORMAT,
> + &zoned_header, sizeof(zoned_header),
> + buflen);
> + if (ret < 0) {
> + goto fail;
> + }
> + buf += ret;
> + buflen -= ret;
> + }
> +
> /* Keep unknown header extensions */
> QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
> ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
> @@ -3773,6 +3832,23 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
> s->image_data_file = g_strdup(data_bs->filename);
> }
>
> + if (qcow2_opts->zoned_profile) {
> + BDRVQcow2State *s = blk_bs(blk)->opaque;
> + if (!strcmp(qcow2_opts->zoned_profile, "zbc")) {
> + s->zoned_header.zoned_profile = BLK_ZP_ZBC;
> + s->zoned_header.zone_capacity = qcow2_opts->zone_size;
> + } else if (!strcmp(qcow2_opts->zoned_profile, "zns")) {
> + s->zoned_header.zoned_profile = BLK_ZP_ZNS;
> + s->zoned_header.zone_capacity = qcow2_opts->zone_capacity;
> + }
> + s->zoned_header.zoned = BLK_Z_HM;
> + s->zoned_header.zone_size = qcow2_opts->zone_size;
> + s->zoned_header.zone_nr_conv = qcow2_opts->zone_nr_conv;
> + s->zoned_header.max_open_zones = qcow2_opts->max_open_zones;
> + s->zoned_header.max_active_zones = qcow2_opts->max_active_zones;
> + s->zoned_header.max_append_sectors = qcow2_opts->max_append_sectors;
Please add input validation that rejects bad values. For example,
zone_size cannot be 0 and zone_capacity cannot be larger than zone_size.
> + }
> +
> /* Create a full header (including things like feature table) */
> ret = qcow2_update_header(blk_bs(blk));
> bdrv_graph_co_rdunlock();
> @@ -3891,6 +3967,13 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
> qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v3");
> }
>
> + /* The available zoned-profile options are zbc, which stands for
> + * ZBC/ZAC standards, and zns following NVMe ZNS spec. */
> + val = qdict_get_try_str(qdict, BLOCK_OPT_Z_PROFILE);
> + if (val) {
> + qdict_put_str(qdict, BLOCK_OPT_Z_PROFILE, val);
> + }
> +
> /* Change legacy command line options into QMP ones */
> static const QDictRenames opt_renames[] = {
> { BLOCK_OPT_BACKING_FILE, "backing-file" },
> @@ -3903,6 +3986,13 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
> { BLOCK_OPT_COMPAT_LEVEL, "version" },
> { BLOCK_OPT_DATA_FILE_RAW, "data-file-raw" },
> { BLOCK_OPT_COMPRESSION_TYPE, "compression-type" },
> + { BLOCK_OPT_Z_PROFILE, "zoned-profile"},
> + { BLOCK_OPT_Z_NR_COV, "zone-nr-conv"},
> + { BLOCK_OPT_Z_MOZ, "max-open-zones"},
> + { BLOCK_OPT_Z_MAZ, "max-active-zones"},
> + { BLOCK_OPT_Z_MAS, "max-append-sectors"},
> + { BLOCK_OPT_Z_SIZE, "zone-size"},
> + { BLOCK_OPT_Z_CAP, "zone-capacity"},
> { NULL, NULL },
> };
>
> @@ -6066,6 +6156,41 @@ static QemuOptsList qcow2_create_opts = {
> .help = "Compression method used for image cluster " \
> "compression", \
> .def_value_str = "zlib" \
> + }, \
> + {
Indentation is off and the forward slash is missing. I'm surprised this
works without the forward slash because the preprocessor should interpet
the macro as ending on this line, weird.
> + .name = BLOCK_OPT_Z_PROFILE, \
> + .type = QEMU_OPT_STRING, \
> + .help = "zoned format option for the disk img", \
> + }, \
> + { \
> + .name = BLOCK_OPT_Z_SIZE, \
> + .type = QEMU_OPT_SIZE, \
> + .help = "zone size", \
> + }, \
> + { \
> + .name = BLOCK_OPT_Z_CAP, \
> + .type = QEMU_OPT_SIZE, \
> + .help = "zone capacity", \
> + }, \
> + { \
> + .name = BLOCK_OPT_Z_NR_COV, \
Indentation is off. QEMU uses 4-space indentation.
> + .type = QEMU_OPT_NUMBER, \
> + .help = "numbers of conventional zones", \
> + }, \
> + { \
> + .name = BLOCK_OPT_Z_MAS, \
> + .type = QEMU_OPT_NUMBER, \
> + .help = "max append sectors", \
> + }, \
> + { \
> + .name = BLOCK_OPT_Z_MAZ, \
> + .type = QEMU_OPT_NUMBER, \
> + .help = "max active zones", \
> + }, \
> + { \
> + .name = BLOCK_OPT_Z_MOZ, \
> + .type = QEMU_OPT_NUMBER, \
> + .help = "max open zones", \
> },
> QCOW_COMMON_OPTIONS,
> { /* end of list */ }
> diff --git a/block/qcow2.h b/block/qcow2.h
> index f789ce3ae0..3694c8d217 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -236,6 +236,20 @@ typedef struct Qcow2CryptoHeaderExtension {
> uint64_t length;
> } QEMU_PACKED Qcow2CryptoHeaderExtension;
>
> +typedef struct Qcow2ZonedHeaderExtension {
> + /* Zoned device attributes */
> + uint8_t zoned_profile;
> + uint8_t zoned;
> + uint16_t reserved16;
> + uint32_t zone_size;
> + uint32_t zone_capacity;
> + uint32_t nr_zones;
> + uint32_t zone_nr_conv;
> + uint32_t max_active_zones;
> + uint32_t max_open_zones;
> + uint32_t max_append_sectors;
> +} QEMU_PACKED Qcow2ZonedHeaderExtension;
> +
> typedef struct Qcow2UnknownHeaderExtension {
> uint32_t magic;
> uint32_t len;
> @@ -422,6 +436,13 @@ typedef struct BDRVQcow2State {
> * is to convert the image with the desired compression type set.
> */
> Qcow2CompressionType compression_type;
> +
> + /* States of zoned device */
> + Qcow2ZonedHeaderExtension zoned_header;
> + uint32_t nr_zones_exp_open;
> + uint32_t nr_zones_imp_open;
> + uint32_t nr_zones_closed;
> + BlockZoneWps *wps;
Please add wps in the patch that uses this field. I thought wps was a
generic BlockDriverState field and didn't expect BDRVQcow2State to have
it.
> } BDRVQcow2State;
>
> typedef struct Qcow2COWRegion {
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index 2c4618375a..ef2ba6f670 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -331,6 +331,30 @@ The fields of the bitmaps extension are:
> Offset into the image file at which the bitmap directory
> starts. Must be aligned to a cluster boundary.
>
> +== Zoned extension ==
> +
> +The zoned extension is an optional header extension. It is required when
> +using the qcow2 file as the backing image for zoned device.
It's not clear here that this is about emulating a zoned storage device
rather than using qcow2 on a zoned storage device. Also, the term
"backing image" will probably be confused with qcow2's backing files
feature.
I suggest: It contains fields for emulating the zoned storage model
(https://zonedstorage.io/).
> +
> +The fields of the zoned extension are:
> + Byte 0: zoned_profile
> + Type of zoned format. Must be `zbc` or `zns`.
> + 1: `zbc`
> + 2: `zns`
> +
> + 1: zoned
> + Type of zone.
> +
> + 2 - 3: Reserved, must be zero.
> +
> + 4 - 7: zone_size
> + 8 - 11: zone_capacity
> + 12 - 15: nr_zones
> + 16 - 19: zone_nr_conv
> + 20 - 23: max_active_zones
> + 24 - 27: max_open_zones
> + 28 - 31: max_append_sectors
Please document these fields, their units, etc.
> +
> == Full disk encryption header pointer ==
>
> The full disk encryption header must be present if, and only if, the
> diff --git a/include/block/block-common.h b/include/block/block-common.h
> index e15395f2cb..9f04a772f6 100644
> --- a/include/block/block-common.h
> +++ b/include/block/block-common.h
> @@ -108,6 +108,11 @@ typedef enum BlockZoneType {
> BLK_ZT_SWP = 0x3, /* Sequential writes preferred */
> } BlockZoneType;
>
> +typedef enum BlockZonedProfile {
> + BLK_ZP_ZBC = 0x1,
> + BLK_ZP_ZNS = 0x2,
> +} BlockZonedProfile;
> +
> /*
> * Zone descriptor data structure.
> * Provides information on a zone with all position and size values in bytes.
> diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
> index 74195c3004..1dbe820a9b 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -57,6 +57,14 @@
> #define BLOCK_OPT_DATA_FILE_RAW "data_file_raw"
> #define BLOCK_OPT_COMPRESSION_TYPE "compression_type"
> #define BLOCK_OPT_EXTL2 "extended_l2"
> +#define BLOCK_OPT_Z_PROFILE "zoned_profile"
> +#define BLOCK_OPT_Z_MODEL "zoned"
> +#define BLOCK_OPT_Z_SIZE "zone_size"
> +#define BLOCK_OPT_Z_CAP "zone_capacity"
> +#define BLOCK_OPT_Z_NR_COV "zone_nr_conv"
> +#define BLOCK_OPT_Z_MAS "max_append_sectors"
> +#define BLOCK_OPT_Z_MAZ "max_active_zones"
> +#define BLOCK_OPT_Z_MOZ "max_open_zones"
>
> #define BLOCK_PROBE_BUF_SIZE 512
>
> @@ -872,12 +880,20 @@ typedef struct BlockLimits {
> */
> bool has_variable_length;
>
> + BlockZonedProfile zoned_profile;
> +
> /* device zone model */
> BlockZoneModel zoned;
>
> /* zone size expressed in bytes */
> uint32_t zone_size;
>
> + /*
> + * the number of usable logical blocks within the zone, expressed
> + * in bytes. A zone capacity is smaller or equal to the zone size.
> + */
> + uint32_t zone_capacity;
> +
> /* total number of zones */
> uint32_t nr_zones;
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 2b1d493d6e..0c97ae678b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5020,24 +5020,42 @@
> #
> # @compression-type: The image cluster compression method
> # (default: zlib, since 5.1)
> +# @zoned-profile: Two zoned device protocol options, zbc or zns
> +# (default: off, since 8.0)
> +# @zone-size: The size of a zone of the zoned device (since 8.0)
> +# @zone-capacity: The capacity of a zone of the zoned device (since 8.0)
> +# @zone-nr-conv: The number of conventional zones of the zoned device
> +# (since 8.0)
> +# @max-open-zones: The maximal allowed open zones (since 8.0)
> +# @max-active-zones: The limit of the zones that have the implicit open,
> +# explicit open or closed state (since 8.0)
> +# @max-append-sectors: The maximal sectors that is allowed to append write
> +# (since 8.0)
> #
> # Since: 2.12
> ##
> { 'struct': 'BlockdevCreateOptionsQcow2',
> - 'data': { 'file': 'BlockdevRef',
> - '*data-file': 'BlockdevRef',
> - '*data-file-raw': 'bool',
> - '*extended-l2': 'bool',
> - 'size': 'size',
> - '*version': 'BlockdevQcow2Version',
> - '*backing-file': 'str',
> - '*backing-fmt': 'BlockdevDriver',
> - '*encrypt': 'QCryptoBlockCreateOptions',
> - '*cluster-size': 'size',
> - '*preallocation': 'PreallocMode',
> - '*lazy-refcounts': 'bool',
> - '*refcount-bits': 'int',
> - '*compression-type':'Qcow2CompressionType' } }
> + 'data': { 'file': 'BlockdevRef',
> + '*data-file': 'BlockdevRef',
> + '*data-file-raw': 'bool',
> + '*extended-l2': 'bool',
> + 'size': 'size',
> + '*version': 'BlockdevQcow2Version',
> + '*backing-file': 'str',
> + '*backing-fmt': 'BlockdevDriver',
> + '*encrypt': 'QCryptoBlockCreateOptions',
> + '*cluster-size': 'size',
> + '*preallocation': 'PreallocMode',
> + '*lazy-refcounts': 'bool',
> + '*refcount-bits': 'int',
> + '*compression-type': 'Qcow2CompressionType',
> + '*zoned-profile': 'str',
> + '*zone-size': 'size',
> + '*zone-capacity': 'size',
> + '*zone-nr-conv': 'uint32',
> + '*max-open-zones': 'uint32',
> + '*max-active-zones': 'uint32',
> + '*max-append-sectors': 'uint32'}}
>
> ##
> # @BlockdevCreateOptionsQed:
> --
> 2.40.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-14 8:58 ` [PATCH v2 2/4] qcow2: add configurations for zoned format extension Sam Li
2023-08-16 19:31 ` Stefan Hajnoczi
@ 2023-08-21 13:13 ` Markus Armbruster
2023-08-28 9:05 ` Sam Li
2023-08-21 13:31 ` Stefan Hajnoczi
2 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2023-08-21 13:13 UTC (permalink / raw)
To: Sam Li
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Eric Blake, stefanha, dlemoal
Sam Li <faithilikerun@gmail.com> writes:
> To configure the zoned format feature on the qcow2 driver, it
> requires following arguments: the device size, zoned profile,
> zoned model, zone size, zone capacity, number of conventional
> zones, limits on zone resources (max append sectors, max open
> zones, and max_active_zones). The zoned profile option is set
> to zns when using the qcow2 file as a ZNS drive.
>
> To create a qcow2 file with zoned format, use command like this:
> $ qemu-img create -f qcow2 test.qcow2 -o size=768M -o
> zone_size=64M -o zone_capacity=64M -o zone_nr_conv=0 -o
> max_append_sectors=512 -o max_open_zones=0 -o max_active_zones=0
> -o zoned_profile=zbc/zns
>
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 2b1d493d6e..0c97ae678b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5020,24 +5020,42 @@
> #
> # @compression-type: The image cluster compression method
> # (default: zlib, since 5.1)
> +# @zoned-profile: Two zoned device protocol options, zbc or zns
> +# (default: off, since 8.0)
When a 'str' thing accepts a fixed set of (string) values, it most
likely should be an enum instead. Have you considered making
@zoned-profile one?
> +# @zone-size: The size of a zone of the zoned device (since 8.0)
> +# @zone-capacity: The capacity of a zone of the zoned device (since 8.0)
In bytes, I presume?
What's the difference between size and capacity?
> +# @zone-nr-conv: The number of conventional zones of the zoned device
> +# (since 8.0)
> +# @max-open-zones: The maximal allowed open zones (since 8.0)
> +# @max-active-zones: The limit of the zones that have the implicit open,
> +# explicit open or closed state (since 8.0)
Naming... if I understand the comment correctly, then @zone-nr-conv,
@max-open-zones, and @max-active-zones are all counting zones. Rename
@zone-nr-conv to @conventional-zones?
> +# @max-append-sectors: The maximal sectors that is allowed to append write
I'm not sure I understand the explanation. Elaborate for me?
> +# (since 8.0)
Please format like
#
# @zoned-profile: Two zoned device protocol options, zbc or zns
# (default: off, since 8.0)
#
# @zone-size: The size of a zone of the zoned device (since 8.0)
#
# @zone-capacity: The capacity of a zone of the zoned device
# (since 8.0)
#
# @zone-nr-conv: The number of conventional zones of the zoned device
# (since 8.0)
#
# @max-open-zones: The maximal allowed open zones (since 8.0)
#
# @max-active-zones: The limit of the zones that have the implicit
# open, explicit open or closed state (since 8.0)
#
# @max-append-sectors: The maximal sectors that is allowed to append
# write (since 8.0)
to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).
> #
> # Since: 2.12
> ##
> { 'struct': 'BlockdevCreateOptionsQcow2',
> - 'data': { 'file': 'BlockdevRef',
> - '*data-file': 'BlockdevRef',
> - '*data-file-raw': 'bool',
> - '*extended-l2': 'bool',
> - 'size': 'size',
> - '*version': 'BlockdevQcow2Version',
> - '*backing-file': 'str',
> - '*backing-fmt': 'BlockdevDriver',
> - '*encrypt': 'QCryptoBlockCreateOptions',
> - '*cluster-size': 'size',
> - '*preallocation': 'PreallocMode',
> - '*lazy-refcounts': 'bool',
> - '*refcount-bits': 'int',
> - '*compression-type':'Qcow2CompressionType' } }
> + 'data': { 'file': 'BlockdevRef',
> + '*data-file': 'BlockdevRef',
> + '*data-file-raw': 'bool',
> + '*extended-l2': 'bool',
> + 'size': 'size',
> + '*version': 'BlockdevQcow2Version',
> + '*backing-file': 'str',
> + '*backing-fmt': 'BlockdevDriver',
> + '*encrypt': 'QCryptoBlockCreateOptions',
> + '*cluster-size': 'size',
> + '*preallocation': 'PreallocMode',
> + '*lazy-refcounts': 'bool',
> + '*refcount-bits': 'int',
> + '*compression-type': 'Qcow2CompressionType',
I'd keep the existing lines unchanged. Not a demand.
> + '*zoned-profile': 'str',
> + '*zone-size': 'size',
> + '*zone-capacity': 'size',
> + '*zone-nr-conv': 'uint32',
> + '*max-open-zones': 'uint32',
> + '*max-active-zones': 'uint32',
> + '*max-append-sectors': 'uint32'}}
Keep the spaces around the } for consistency. Yes, they're kind of
ugly.
>
> ##
> # @BlockdevCreateOptionsQed:
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-21 13:13 ` Markus Armbruster
@ 2023-08-28 9:05 ` Sam Li
0 siblings, 0 replies; 26+ messages in thread
From: Sam Li @ 2023-08-28 9:05 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Eric Blake, stefanha, dlemoal
Markus Armbruster <armbru@redhat.com> 于2023年8月21日周一 21:13写道：
>
> Sam Li <faithilikerun@gmail.com> writes:
>
> > To configure the zoned format feature on the qcow2 driver, it
> > requires following arguments: the device size, zoned profile,
> > zoned model, zone size, zone capacity, number of conventional
> > zones, limits on zone resources (max append sectors, max open
> > zones, and max_active_zones). The zoned profile option is set
> > to zns when using the qcow2 file as a ZNS drive.
> >
> > To create a qcow2 file with zoned format, use command like this:
> > $ qemu-img create -f qcow2 test.qcow2 -o size=768M -o
> > zone_size=64M -o zone_capacity=64M -o zone_nr_conv=0 -o
> > max_append_sectors=512 -o max_open_zones=0 -o max_active_zones=0
> > -o zoned_profile=zbc/zns
> >
> > Signed-off-by: Sam Li <faithilikerun@gmail.com>
>
> [...]
>
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 2b1d493d6e..0c97ae678b 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -5020,24 +5020,42 @@
> > #
> > # @compression-type: The image cluster compression method
> > # (default: zlib, since 5.1)
> > +# @zoned-profile: Two zoned device protocol options, zbc or zns
> > +# (default: off, since 8.0)
>
> When a 'str' thing accepts a fixed set of (string) values, it most
> likely should be an enum instead. Have you considered making
> @zoned-profile one?
>
> > +# @zone-size: The size of a zone of the zoned device (since 8.0)
> > +# @zone-capacity: The capacity of a zone of the zoned device (since 8.0)
>
> In bytes, I presume?
Yes.
>
> What's the difference between size and capacity?
>
Zone size is the total number of logical blocks within zones in bytes.
Zone capacity is the number of usable logical blocks within zones in
bytes. A zone capacity is always smaller or equal to than zone size.
According to ZBC/ZAC standards, a zone capacity is equal to the zone
size. While in ZNS spec, it can be smaller. I will add the
documentation and below in the next patches.
> > +# @zone-nr-conv: The number of conventional zones of the zoned device
> > +# (since 8.0)
> > +# @max-open-zones: The maximal allowed open zones (since 8.0)
> > +# @max-active-zones: The limit of the zones that have the implicit open,
> > +# explicit open or closed state (since 8.0)
>
> Naming... if I understand the comment correctly, then @zone-nr-conv,
> @max-open-zones, and @max-active-zones are all counting zones. Rename
> @zone-nr-conv to @conventional-zones?
>
> > +# @max-append-sectors: The maximal sectors that is allowed to append write
>
> I'm not sure I understand the explanation. Elaborate for me?
The max_append_sector is the maximum data size (in sectors) of a zone
append request that can be successfully issued to the device. It is a
constraint on the maximum amount of data that can be appended to a
zone in a single request.
>
> > +# (since 8.0)
>
> Please format like
>
> #
> # @zoned-profile: Two zoned device protocol options, zbc or zns
> # (default: off, since 8.0)
> #
> # @zone-size: The size of a zone of the zoned device (since 8.0)
> #
> # @zone-capacity: The capacity of a zone of the zoned device
> # (since 8.0)
> #
> # @zone-nr-conv: The number of conventional zones of the zoned device
> # (since 8.0)
> #
> # @max-open-zones: The maximal allowed open zones (since 8.0)
> #
> # @max-active-zones: The limit of the zones that have the implicit
> # open, explicit open or closed state (since 8.0)
> #
> # @max-append-sectors: The maximal sectors that is allowed to append
> # write (since 8.0)
>
> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
> to conform to current conventions).
>
> > #
> > # Since: 2.12
> > ##
> > { 'struct': 'BlockdevCreateOptionsQcow2',
> > - 'data': { 'file': 'BlockdevRef',
> > - '*data-file': 'BlockdevRef',
> > - '*data-file-raw': 'bool',
> > - '*extended-l2': 'bool',
> > - 'size': 'size',
> > - '*version': 'BlockdevQcow2Version',
> > - '*backing-file': 'str',
> > - '*backing-fmt': 'BlockdevDriver',
> > - '*encrypt': 'QCryptoBlockCreateOptions',
> > - '*cluster-size': 'size',
> > - '*preallocation': 'PreallocMode',
> > - '*lazy-refcounts': 'bool',
> > - '*refcount-bits': 'int',
> > - '*compression-type':'Qcow2CompressionType' } }
> > + 'data': { 'file': 'BlockdevRef',
> > + '*data-file': 'BlockdevRef',
> > + '*data-file-raw': 'bool',
> > + '*extended-l2': 'bool',
> > + 'size': 'size',
> > + '*version': 'BlockdevQcow2Version',
> > + '*backing-file': 'str',
> > + '*backing-fmt': 'BlockdevDriver',
> > + '*encrypt': 'QCryptoBlockCreateOptions',
> > + '*cluster-size': 'size',
> > + '*preallocation': 'PreallocMode',
> > + '*lazy-refcounts': 'bool',
> > + '*refcount-bits': 'int',
> > + '*compression-type': 'Qcow2CompressionType',
>
> I'd keep the existing lines unchanged. Not a demand.
>
> > + '*zoned-profile': 'str',
> > + '*zone-size': 'size',
> > + '*zone-capacity': 'size',
> > + '*zone-nr-conv': 'uint32',
> > + '*max-open-zones': 'uint32',
> > + '*max-active-zones': 'uint32',
> > + '*max-append-sectors': 'uint32'}}
>
> Keep the spaces around the } for consistency. Yes, they're kind of
> ugly.
Thanks!
>
> >
> > ##
> > # @BlockdevCreateOptionsQed:
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-14 8:58 ` [PATCH v2 2/4] qcow2: add configurations for zoned format extension Sam Li
2023-08-16 19:31 ` Stefan Hajnoczi
2023-08-21 13:13 ` Markus Armbruster
@ 2023-08-21 13:31 ` Stefan Hajnoczi
2023-08-28 9:22 ` Sam Li
2023-08-28 14:42 ` Sam Li
2 siblings, 2 replies; 26+ messages in thread
From: Stefan Hajnoczi @ 2023-08-21 13:31 UTC (permalink / raw)
To: Sam Li
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake, dlemoal
[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]
On Mon, Aug 14, 2023 at 04:58:00PM +0800, Sam Li wrote:
> diff --git a/block/qcow2.h b/block/qcow2.h
> index f789ce3ae0..3694c8d217 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -236,6 +236,20 @@ typedef struct Qcow2CryptoHeaderExtension {
> uint64_t length;
> } QEMU_PACKED Qcow2CryptoHeaderExtension;
>
> +typedef struct Qcow2ZonedHeaderExtension {
> + /* Zoned device attributes */
> + uint8_t zoned_profile;
> + uint8_t zoned;
> + uint16_t reserved16;
> + uint32_t zone_size;
> + uint32_t zone_capacity;
Should zone capacity be stored individually for each zone (alongside the
write pointer and other per zone metadata) instead of as a global value
for all zones? My understanding is that NVMe ZNS does not have a global
value and each zone could have a different zone capacity value.
> + uint32_t nr_zones;
Is this field necessary since it can be derived from other image
options: nr_zones = DIV_ROUND_UP(total_length, zone_capacity)?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-21 13:31 ` Stefan Hajnoczi
@ 2023-08-28 9:22 ` Sam Li
2023-08-28 10:12 ` Damien Le Moal
2023-08-28 14:42 ` Sam Li
1 sibling, 1 reply; 26+ messages in thread
From: Sam Li @ 2023-08-28 9:22 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake, dlemoal
Stefan Hajnoczi <stefanha@redhat.com> 于2023年8月21日周一 21:31写道：
>
> On Mon, Aug 14, 2023 at 04:58:00PM +0800, Sam Li wrote:
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index f789ce3ae0..3694c8d217 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -236,6 +236,20 @@ typedef struct Qcow2CryptoHeaderExtension {
> > uint64_t length;
> > } QEMU_PACKED Qcow2CryptoHeaderExtension;
> >
> > +typedef struct Qcow2ZonedHeaderExtension {
> > + /* Zoned device attributes */
> > + uint8_t zoned_profile;
> > + uint8_t zoned;
> > + uint16_t reserved16;
> > + uint32_t zone_size;
> > + uint32_t zone_capacity;
>
> Should zone capacity be stored individually for each zone (alongside the
> write pointer and other per zone metadata) instead of as a global value
> for all zones? My understanding is that NVMe ZNS does not have a global
> value and each zone could have a different zone capacity value.
Though zone capacity is per-zone attribute, it remains same for all
zones in most cases. Referring to the NVMe ZNS spec, zone capacity
changes associate to RESET_ZONE op when the variable zone capacity bit
is '1'. It hasn't specifically tell what it is changed to. Current ZNS
emulation doesn't change zone capacity as well.
If the Variable Zone Capacity bit is cleared to ‘0’ in the Zone
Operation Characteristics field in the Zoned
Namespace Command Set specific Identify Namespace data structure, then
this field does not change without a change to the format of the zoned
namespace.
If the Variable Zone Capacity bit is set to ‘1’ in the Zone Operation
Characteristics field in the Zoned
Namespace Command Set specific Identify Namespace data structure, then
the zone capacity may
change upon successful completion of a Zone Management Send command
specifying the Zone Send
Action of Reset Zone.
>
> > + uint32_t nr_zones;
>
> Is this field necessary since it can be derived from other image
> options: nr_zones = DIV_ROUND_UP(total_length, zone_capacity)?
It can be dropped. I added this for reducing duplication. Thanks!
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-28 9:22 ` Sam Li
@ 2023-08-28 10:12 ` Damien Le Moal
2023-08-28 10:18 ` Sam Li
0 siblings, 1 reply; 26+ messages in thread
From: Damien Le Moal @ 2023-08-28 10:12 UTC (permalink / raw)
To: Sam Li, Stefan Hajnoczi
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake
On 8/28/23 18:22, Sam Li wrote:
> Stefan Hajnoczi <stefanha@redhat.com> 于2023年8月21日周一 21:31写道：
>>
>> On Mon, Aug 14, 2023 at 04:58:00PM +0800, Sam Li wrote:
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index f789ce3ae0..3694c8d217 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -236,6 +236,20 @@ typedef struct Qcow2CryptoHeaderExtension {
>>> uint64_t length;
>>> } QEMU_PACKED Qcow2CryptoHeaderExtension;
>>>
>>> +typedef struct Qcow2ZonedHeaderExtension {
>>> + /* Zoned device attributes */
>>> + uint8_t zoned_profile;
>>> + uint8_t zoned;
>>> + uint16_t reserved16;
>>> + uint32_t zone_size;
>>> + uint32_t zone_capacity;
>>
>> Should zone capacity be stored individually for each zone (alongside the
>> write pointer and other per zone metadata) instead of as a global value
>> for all zones? My understanding is that NVMe ZNS does not have a global
>> value and each zone could have a different zone capacity value.
>
> Though zone capacity is per-zone attribute, it remains same for all
> zones in most cases. Referring to the NVMe ZNS spec, zone capacity
> changes associate to RESET_ZONE op when the variable zone capacity bit
> is '1'. It hasn't specifically tell what it is changed to. Current ZNS
> emulation doesn't change zone capacity as well.
>
> If the Variable Zone Capacity bit is cleared to ‘0’ in the Zone
> Operation Characteristics field in the Zoned
> Namespace Command Set specific Identify Namespace data structure, then
> this field does not change without a change to the format of the zoned
> namespace.
>
> If the Variable Zone Capacity bit is set to ‘1’ in the Zone Operation
> Characteristics field in the Zoned
> Namespace Command Set specific Identify Namespace data structure, then
> the zone capacity may
> change upon successful completion of a Zone Management Send command
> specifying the Zone Send
> Action of Reset Zone.
Regardless of the variable zone capacity feature, zone capacity is per zone and
may be different between zones. That is why it is reported per zone in zone
report. The IO path code should not assume that the zone capacity is the same
for all zones.
For this particular case though, given that this is QCow2 emulation, limiting
ourselves to the same zone capacity for all zones is I think fine. But that
should be clearly stated somewhere may be...
>
>>
>>> + uint32_t nr_zones;
>>
>> Is this field necessary since it can be derived from other image
>> options: nr_zones = DIV_ROUND_UP(total_length, zone_capacity)?
>
> It can be dropped. I added this for reducing duplication. Thanks!
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-28 10:12 ` Damien Le Moal
@ 2023-08-28 10:18 ` Sam Li
2023-08-28 10:22 ` Damien Le Moal
0 siblings, 1 reply; 26+ messages in thread
From: Sam Li @ 2023-08-28 10:18 UTC (permalink / raw)
To: Damien Le Moal
Cc: Stefan Hajnoczi, qemu-devel, hare, Hanna Reitz, dmitry.fomichev,
qemu-block, Kevin Wolf, Markus Armbruster, Eric Blake
Damien Le Moal <dlemoal@kernel.org> 于2023年8月28日周一 18:13写道：
>
> On 8/28/23 18:22, Sam Li wrote:
> > Stefan Hajnoczi <stefanha@redhat.com> 于2023年8月21日周一 21:31写道：
> >>
> >> On Mon, Aug 14, 2023 at 04:58:00PM +0800, Sam Li wrote:
> >>> diff --git a/block/qcow2.h b/block/qcow2.h
> >>> index f789ce3ae0..3694c8d217 100644
> >>> --- a/block/qcow2.h
> >>> +++ b/block/qcow2.h
> >>> @@ -236,6 +236,20 @@ typedef struct Qcow2CryptoHeaderExtension {
> >>> uint64_t length;
> >>> } QEMU_PACKED Qcow2CryptoHeaderExtension;
> >>>
> >>> +typedef struct Qcow2ZonedHeaderExtension {
> >>> + /* Zoned device attributes */
> >>> + uint8_t zoned_profile;
> >>> + uint8_t zoned;
> >>> + uint16_t reserved16;
> >>> + uint32_t zone_size;
> >>> + uint32_t zone_capacity;
> >>
> >> Should zone capacity be stored individually for each zone (alongside the
> >> write pointer and other per zone metadata) instead of as a global value
> >> for all zones? My understanding is that NVMe ZNS does not have a global
> >> value and each zone could have a different zone capacity value.
> >
> > Though zone capacity is per-zone attribute, it remains same for all
> > zones in most cases. Referring to the NVMe ZNS spec, zone capacity
> > changes associate to RESET_ZONE op when the variable zone capacity bit
> > is '1'. It hasn't specifically tell what it is changed to. Current ZNS
> > emulation doesn't change zone capacity as well.
> >
> > If the Variable Zone Capacity bit is cleared to ‘0’ in the Zone
> > Operation Characteristics field in the Zoned
> > Namespace Command Set specific Identify Namespace data structure, then
> > this field does not change without a change to the format of the zoned
> > namespace.
> >
> > If the Variable Zone Capacity bit is set to ‘1’ in the Zone Operation
> > Characteristics field in the Zoned
> > Namespace Command Set specific Identify Namespace data structure, then
> > the zone capacity may
> > change upon successful completion of a Zone Management Send command
> > specifying the Zone Send
> > Action of Reset Zone.
>
> Regardless of the variable zone capacity feature, zone capacity is per zone and
> may be different between zones. That is why it is reported per zone in zone
> report. The IO path code should not assume that the zone capacity is the same
> for all zones.
How is zone capacity changed, by devices or commands? Can you give
some example please?
>
> For this particular case though, given that this is QCow2 emulation, limiting
> ourselves to the same zone capacity for all zones is I think fine. But that
> should be clearly stated somewhere may be...
I see. The qcow2 documentaion can add that.
>
> >
> >>
> >>> + uint32_t nr_zones;
> >>
> >> Is this field necessary since it can be derived from other image
> >> options: nr_zones = DIV_ROUND_UP(total_length, zone_capacity)?
> >
> > It can be dropped. I added this for reducing duplication. Thanks!
>
> --
> Damien Le Moal
> Western Digital Research
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-28 10:18 ` Sam Li
@ 2023-08-28 10:22 ` Damien Le Moal
2023-08-28 10:40 ` Sam Li
0 siblings, 1 reply; 26+ messages in thread
From: Damien Le Moal @ 2023-08-28 10:22 UTC (permalink / raw)
To: Sam Li
Cc: Stefan Hajnoczi, qemu-devel, hare, Hanna Reitz, dmitry.fomichev,
qemu-block, Kevin Wolf, Markus Armbruster, Eric Blake
On 8/28/23 19:18, Sam Li wrote:
> Damien Le Moal <dlemoal@kernel.org> 于2023年8月28日周一 18:13写道：
>>
>> On 8/28/23 18:22, Sam Li wrote:
>>> Stefan Hajnoczi <stefanha@redhat.com> 于2023年8月21日周一 21:31写道：
>>>>
>>>> On Mon, Aug 14, 2023 at 04:58:00PM +0800, Sam Li wrote:
>>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>>> index f789ce3ae0..3694c8d217 100644
>>>>> --- a/block/qcow2.h
>>>>> +++ b/block/qcow2.h
>>>>> @@ -236,6 +236,20 @@ typedef struct Qcow2CryptoHeaderExtension {
>>>>> uint64_t length;
>>>>> } QEMU_PACKED Qcow2CryptoHeaderExtension;
>>>>>
>>>>> +typedef struct Qcow2ZonedHeaderExtension {
>>>>> + /* Zoned device attributes */
>>>>> + uint8_t zoned_profile;
>>>>> + uint8_t zoned;
>>>>> + uint16_t reserved16;
>>>>> + uint32_t zone_size;
>>>>> + uint32_t zone_capacity;
>>>>
>>>> Should zone capacity be stored individually for each zone (alongside the
>>>> write pointer and other per zone metadata) instead of as a global value
>>>> for all zones? My understanding is that NVMe ZNS does not have a global
>>>> value and each zone could have a different zone capacity value.
>>>
>>> Though zone capacity is per-zone attribute, it remains same for all
>>> zones in most cases. Referring to the NVMe ZNS spec, zone capacity
>>> changes associate to RESET_ZONE op when the variable zone capacity bit
>>> is '1'. It hasn't specifically tell what it is changed to. Current ZNS
>>> emulation doesn't change zone capacity as well.
>>>
>>> If the Variable Zone Capacity bit is cleared to ‘0’ in the Zone
>>> Operation Characteristics field in the Zoned
>>> Namespace Command Set specific Identify Namespace data structure, then
>>> this field does not change without a change to the format of the zoned
>>> namespace.
>>>
>>> If the Variable Zone Capacity bit is set to ‘1’ in the Zone Operation
>>> Characteristics field in the Zoned
>>> Namespace Command Set specific Identify Namespace data structure, then
>>> the zone capacity may
>>> change upon successful completion of a Zone Management Send command
>>> specifying the Zone Send
>>> Action of Reset Zone.
>>
>> Regardless of the variable zone capacity feature, zone capacity is per zone and
>> may be different between zones. That is why it is reported per zone in zone
>> report. The IO path code should not assume that the zone capacity is the same
>> for all zones.
>
> How is zone capacity changed, by devices or commands? Can you give
> some example please?
If the device does not support variable zone capacity, the zone capacity is
fixed at device manufacturing time and never changes. It is reported per zone
and you have to make things work with whatever value you see. The user cannot
change device zone capacity.
For you qcow2 zoned image, the equivalent is to fix the zone capacity when the
image is created and not allowing to change it. And for simplicity, the same
zone capacity value can be used for all zones, so having the zone capacity
value in the header is OK.
>
>>
>> For this particular case though, given that this is QCow2 emulation, limiting
>> ourselves to the same zone capacity for all zones is I think fine. But that
>> should be clearly stated somewhere may be...
>
> I see. The qcow2 documentaion can add that.
>
>>
>>>
>>>>
>>>>> + uint32_t nr_zones;
>>>>
>>>> Is this field necessary since it can be derived from other image
>>>> options: nr_zones = DIV_ROUND_UP(total_length, zone_capacity)?
>>>
>>> It can be dropped. I added this for reducing duplication. Thanks!
>>
>> --
>> Damien Le Moal
>> Western Digital Research
>>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-28 10:22 ` Damien Le Moal
@ 2023-08-28 10:40 ` Sam Li
0 siblings, 0 replies; 26+ messages in thread
From: Sam Li @ 2023-08-28 10:40 UTC (permalink / raw)
To: Damien Le Moal
Cc: Stefan Hajnoczi, qemu-devel, hare, Hanna Reitz, dmitry.fomichev,
qemu-block, Kevin Wolf, Markus Armbruster, Eric Blake
Damien Le Moal <dlemoal@kernel.org> 于2023年8月28日周一 18:22写道：
>
> On 8/28/23 19:18, Sam Li wrote:
> > Damien Le Moal <dlemoal@kernel.org> 于2023年8月28日周一 18:13写道：
> >>
> >> On 8/28/23 18:22, Sam Li wrote:
> >>> Stefan Hajnoczi <stefanha@redhat.com> 于2023年8月21日周一 21:31写道：
> >>>>
> >>>> On Mon, Aug 14, 2023 at 04:58:00PM +0800, Sam Li wrote:
> >>>>> diff --git a/block/qcow2.h b/block/qcow2.h
> >>>>> index f789ce3ae0..3694c8d217 100644
> >>>>> --- a/block/qcow2.h
> >>>>> +++ b/block/qcow2.h
> >>>>> @@ -236,6 +236,20 @@ typedef struct Qcow2CryptoHeaderExtension {
> >>>>> uint64_t length;
> >>>>> } QEMU_PACKED Qcow2CryptoHeaderExtension;
> >>>>>
> >>>>> +typedef struct Qcow2ZonedHeaderExtension {
> >>>>> + /* Zoned device attributes */
> >>>>> + uint8_t zoned_profile;
> >>>>> + uint8_t zoned;
> >>>>> + uint16_t reserved16;
> >>>>> + uint32_t zone_size;
> >>>>> + uint32_t zone_capacity;
> >>>>
> >>>> Should zone capacity be stored individually for each zone (alongside the
> >>>> write pointer and other per zone metadata) instead of as a global value
> >>>> for all zones? My understanding is that NVMe ZNS does not have a global
> >>>> value and each zone could have a different zone capacity value.
> >>>
> >>> Though zone capacity is per-zone attribute, it remains same for all
> >>> zones in most cases. Referring to the NVMe ZNS spec, zone capacity
> >>> changes associate to RESET_ZONE op when the variable zone capacity bit
> >>> is '1'. It hasn't specifically tell what it is changed to. Current ZNS
> >>> emulation doesn't change zone capacity as well.
> >>>
> >>> If the Variable Zone Capacity bit is cleared to ‘0’ in the Zone
> >>> Operation Characteristics field in the Zoned
> >>> Namespace Command Set specific Identify Namespace data structure, then
> >>> this field does not change without a change to the format of the zoned
> >>> namespace.
> >>>
> >>> If the Variable Zone Capacity bit is set to ‘1’ in the Zone Operation
> >>> Characteristics field in the Zoned
> >>> Namespace Command Set specific Identify Namespace data structure, then
> >>> the zone capacity may
> >>> change upon successful completion of a Zone Management Send command
> >>> specifying the Zone Send
> >>> Action of Reset Zone.
> >>
> >> Regardless of the variable zone capacity feature, zone capacity is per zone and
> >> may be different between zones. That is why it is reported per zone in zone
> >> report. The IO path code should not assume that the zone capacity is the same
> >> for all zones.
> >
> > How is zone capacity changed, by devices or commands? Can you give
> > some example please?
>
> If the device does not support variable zone capacity, the zone capacity is
> fixed at device manufacturing time and never changes. It is reported per zone
> and you have to make things work with whatever value you see. The user cannot
> change device zone capacity.
>
> For you qcow2 zoned image, the equivalent is to fix the zone capacity when the
> image is created and not allowing to change it. And for simplicity, the same
> zone capacity value can be used for all zones, so having the zone capacity
> value in the header is OK.
Thanks!
>
> >
> >>
> >> For this particular case though, given that this is QCow2 emulation, limiting
> >> ourselves to the same zone capacity for all zones is I think fine. But that
> >> should be clearly stated somewhere may be...
> >
> > I see. The qcow2 documentaion can add that.
> >
> >>
> >>>
> >>>>
> >>>>> + uint32_t nr_zones;
> >>>>
> >>>> Is this field necessary since it can be derived from other image
> >>>> options: nr_zones = DIV_ROUND_UP(total_length, zone_capacity)?
> >>>
> >>> It can be dropped. I added this for reducing duplication. Thanks!
> >>
> >> --
> >> Damien Le Moal
> >> Western Digital Research
> >>
>
> --
> Damien Le Moal
> Western Digital Research
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
2023-08-21 13:31 ` Stefan Hajnoczi
2023-08-28 9:22 ` Sam Li
@ 2023-08-28 14:42 ` Sam Li
1 sibling, 0 replies; 26+ messages in thread
From: Sam Li @ 2023-08-28 14:42 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake, dlemoal
Stefan Hajnoczi <stefanha@redhat.com> 于2023年8月21日周一 21:31写道：
>
> On Mon, Aug 14, 2023 at 04:58:00PM +0800, Sam Li wrote:
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index f789ce3ae0..3694c8d217 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -236,6 +236,20 @@ typedef struct Qcow2CryptoHeaderExtension {
> > uint64_t length;
> > } QEMU_PACKED Qcow2CryptoHeaderExtension;
> >
> > +typedef struct Qcow2ZonedHeaderExtension {
> > + /* Zoned device attributes */
> > + uint8_t zoned_profile;
> > + uint8_t zoned;
> > + uint16_t reserved16;
> > + uint32_t zone_size;
> > + uint32_t zone_capacity;
>
> Should zone capacity be stored individually for each zone (alongside the
> write pointer and other per zone metadata) instead of as a global value
> for all zones? My understanding is that NVMe ZNS does not have a global
> value and each zone could have a different zone capacity value.
>
> > + uint32_t nr_zones;
>
> Is this field necessary since it can be derived from other image
> options: nr_zones = DIV_ROUND_UP(total_length, zone_capacity)?
Yes. The bs->total_sectors in refresh_limits is zero. Keeping a
persistent nr_zones helps assigning right value instead of zero.
The process is roughly like this:
*_qcow2_create: calculate nr_zones and write it to zoned_header
-> *_qcow2_update_header: update nr_zones
-> *_qcow2_read_extensions: read nr_zones in zoned_header to
Qcow2State and check if right (valid total size here)
-> *_refresh_limits(): set bl.nr_zones to zoned_header.nr_zones
Sam
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 3/4] qcow2: add zoned emulation capability
2023-08-14 8:57 [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver Sam Li
2023-08-14 8:57 ` [PATCH v2 1/4] docs/qcow2: add the zoned format feature Sam Li
2023-08-14 8:58 ` [PATCH v2 2/4] qcow2: add configurations for zoned format extension Sam Li
@ 2023-08-14 8:58 ` Sam Li
2023-08-16 21:07 ` Stefan Hajnoczi
2023-08-22 19:48 ` Stefan Hajnoczi
2023-08-14 8:58 ` [PATCH v2 4/4] iotests: test the zoned format feature for qcow2 file Sam Li
2023-08-16 7:37 ` [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver Klaus Jensen
4 siblings, 2 replies; 26+ messages in thread
From: Sam Li @ 2023-08-14 8:58 UTC (permalink / raw)
To: qemu-devel
Cc: hare, Hanna Reitz, dmitry.fomichev, qemu-block, Kevin Wolf,
Markus Armbruster, Eric Blake, stefanha, dlemoal, Sam Li
By adding zone operations and zoned metadata, the zoned emulation
capability enables full emulation support of zoned device using
a qcow2 file. The zoned device metadata includes zone type,
zoned device state and write pointer of each zone, which is stored
to an array of unsigned integers.
Each zone of a zoned device makes state transitions following
the zone state machine. The zone state machine mainly describes
five states, IMPLICIT OPEN, EXPLICIT OPEN, FULL, EMPTY and CLOSED.
READ ONLY and OFFLINE states will generally be affected by device
internal events. The operations on zones cause corresponding state
changing.
Zoned devices have a limit on zone resources, which puts constraints on
write operations into zones.
Signed-off-by: Sam Li <faithilikerun@gmail.com>
---
block/qcow2.c | 676 ++++++++++++++++++++++++++++++++++++++++-
block/qcow2.h | 2 +
docs/interop/qcow2.txt | 2 +
3 files changed, 678 insertions(+), 2 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index c1077c4a4a..5ccf79cbe7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -194,6 +194,164 @@ qcow2_extract_crypto_opts(QemuOpts *opts, const char *fmt, Error **errp)
return cryptoopts_qdict;
}
+#define QCOW2_ZT_IS_CONV(wp) (wp & 1ULL << 59)
+
+static inline int qcow2_get_wp(uint64_t wp)
+{
+ /* clear state and type information */
+ return ((wp << 5) >> 5);
+}
+
+static inline int qcow2_get_zs(uint64_t wp)
+{
+ return (wp >> 60);
+}
+
+static inline void qcow2_set_wp(uint64_t *wp, BlockZoneState zs)
+{
+ uint64_t addr = qcow2_get_wp(*wp);
+ addr |= ((uint64_t)zs << 60);
+ *wp = addr;
+}
+
+/*
+ * File wp tracking: reset zone, finish zone and append zone can
+ * change the value of write pointer. All zone operations will change
+ * the state of that/those zone.
+ * */
+static inline void qcow2_wp_tracking_helper(int index, uint64_t wp) {
+ /* format: operations, the wp. */
+ printf("wps[%d]: 0x%x\n", index, qcow2_get_wp(wp)>>BDRV_SECTOR_BITS);
+}
+
+/*
+ * Perform a state assignment and a flush operation that writes the new wp
+ * value to the dedicated location of the disk file.
+ */
+static int qcow2_write_wp_at(BlockDriverState *bs, uint64_t *wp,
+ uint32_t index, BlockZoneState zs) {
+ BDRVQcow2State *s = bs->opaque;
+ int ret;
+
+ qcow2_set_wp(wp, zs);
+ ret = bdrv_pwrite(bs->file, s->zoned_header.zonedmeta_offset
+ + sizeof(uint64_t) * index, sizeof(uint64_t), wp, 0);
+
+ if (ret < 0) {
+ goto exit;
+ }
+ qcow2_wp_tracking_helper(index, *wp);
+ return ret;
+
+exit:
+ error_report("Failed to write metadata with file");
+ return ret;
+}
+
+static int qcow2_check_active(BlockDriverState *bs)
+{
+ BDRVQcow2State *s = bs->opaque;
+
+ if (!s->zoned_header.max_active_zones) {
+ return 0;
+ }
+
+ if (s->nr_zones_exp_open + s->nr_zones_imp_open + s->nr_zones_closed
+ < s->zoned_header.max_active_zones) {
+ return 0;
+ }
+
+ return -1;
+}
+
+static int qcow2_check_open(BlockDriverState *bs)
+{
+ BDRVQcow2State *s = bs->opaque;
+ int ret;
+
+ if (!s->zoned_header.max_open_zones) {
+ return 0;
+ }
+
+ if (s->nr_zones_exp_open + s->nr_zones_imp_open
+ < s->zoned_header.max_open_zones) {
+ return 0;
+ }
+
+ if(s->nr_zones_imp_open) {
+ ret = qcow2_check_active(bs);
+ if (ret == 0) {
+ /* TODO: it takes O(n) time complexity (n = nr_zones).
+ * Optimizations required. */
+ /* close one implicitly open zones to make it available */
+ for (int i = s->zoned_header.zone_nr_conv;
+ i < bs->bl.nr_zones; ++i) {
+ uint64_t *wp = &s->wps->wp[i];
+ if (qcow2_get_zs(*wp) == BLK_ZS_IOPEN) {
+ ret = qcow2_write_wp_at(bs, wp, i, BLK_ZS_CLOSED);
+ if (ret < 0) {
+ return ret;
+ }
+ s->wps->wp[i] = *wp;
+ s->nr_zones_imp_open--;
+ s->nr_zones_closed++;
+ break;
+ }
+ }
+ return 0;
+ }
+ return ret;
+ }
+
+ return -1;
+}
+
+/*
+ * The zoned device has limited zone resources of open, closed, active
+ * zones.
+ */
+static int qcow2_check_zone_resources(BlockDriverState *bs,
+ BlockZoneState zs)
+{
+ int ret;
+
+ switch (zs) {
+ case BLK_ZS_EMPTY:
+ ret = qcow2_check_active(bs);
+ if (ret < 0) {
+ error_report("No enough active zones");
+ return ret;
+ }
+ return ret;
+ case BLK_ZS_CLOSED:
+ ret = qcow2_check_open(bs);
+ if (ret < 0) {
+ error_report("No enough open zones");
+ return ret;
+ }
+ return ret;
+ default:
+ return -EINVAL;
+ }
+
+}
+
+static inline int qcow2_refresh_zonedmeta(BlockDriverState *bs)
+{
+ int ret;
+ BDRVQcow2State *s = bs->opaque;
+ uint64_t *temp = g_malloc(s->zoned_header.zonedmeta_size);
+ ret = bdrv_pread(bs->file, s->zoned_header.zonedmeta_offset,
+ s->zoned_header.zonedmeta_size, temp, 0);
+ if (ret < 0) {
+ error_report("Can not read metadata\n");
+ return ret;
+ }
+
+ memcpy(s->wps->wp, temp, s->zoned_header.zonedmeta_size);
+ return 0;
+}
+
/*
* read qcow2 extension and fill bs
* start reading from start_offset
@@ -456,7 +614,19 @@ qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
be32_to_cpu(zoned_ext.max_active_zones);
zoned_ext.max_append_sectors =
be32_to_cpu(zoned_ext.max_append_sectors);
+ zoned_ext.zonedmeta_offset =
+ be64_to_cpu(zoned_ext.zonedmeta_offset);
+ zoned_ext.zonedmeta_size = be64_to_cpu(zoned_ext.zonedmeta_size);
s->zoned_header = zoned_ext;
+ s->wps = g_malloc(sizeof(BlockZoneWps)
+ + s->zoned_header.zonedmeta_size);
+ ret = qcow2_refresh_zonedmeta(bs);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "zonedmeta: "
+ "Could not update zoned meta");
+ return ret;
+ }
+ qemu_co_mutex_init(&s->wps->colock);
#ifdef DEBUG_EXT
printf("Qcow2: Got zoned format extension: "
@@ -2001,6 +2171,14 @@ static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp)
}
bs->bl.pwrite_zeroes_alignment = s->subcluster_size;
bs->bl.pdiscard_alignment = s->cluster_size;
+ bs->bl.zoned = s->zoned_header.zoned;
+ bs->bl.nr_zones = s->zoned_header.nr_zones;
+ bs->wps = s->wps;
+ bs->bl.max_append_sectors = s->zoned_header.max_append_sectors;
+ bs->bl.max_active_zones = s->zoned_header.max_active_zones;
+ bs->bl.max_open_zones = s->zoned_header.max_open_zones;
+ bs->bl.zone_size = s->zoned_header.zone_size;
+ bs->bl.write_granularity = BDRV_SECTOR_SIZE;
}
static int qcow2_reopen_prepare(BDRVReopenState *state,
@@ -2691,9 +2869,26 @@ qcow2_co_pwritev_part(BlockDriverState *bs, int64_t offset, int64_t bytes,
uint64_t host_offset;
QCowL2Meta *l2meta = NULL;
AioTaskPool *aio = NULL;
+ int64_t start_offset, start_bytes;
+ BlockZoneState zs;
+ int64_t end;
+ uint64_t *wp;
+ int64_t zone_size = bs->bl.zone_size;
+ int index;
trace_qcow2_writev_start_req(qemu_coroutine_self(), offset, bytes);
+ start_offset = offset;
+ start_bytes = bytes;
+ /* The offset should not less than the wp of that
+ * zone where offset starts. */
+ if (zone_size) {
+ index = start_offset / zone_size;
+ wp = &s->wps->wp[index];
+ if (offset < qcow2_get_wp(*wp)) {
+ return -EINVAL;
+ }
+ }
while (bytes != 0 && aio_task_pool_status(aio) == 0) {
l2meta = NULL;
@@ -2739,6 +2934,47 @@ qcow2_co_pwritev_part(BlockDriverState *bs, int64_t offset, int64_t bytes,
qiov_offset += cur_bytes;
trace_qcow2_writev_done_part(qemu_coroutine_self(), cur_bytes);
}
+
+ if (zone_size) {
+ index = start_offset / zone_size;
+ wp = &s->wps->wp[index];
+ uint64_t wpv = *wp;
+ if (!QCOW2_ZT_IS_CONV(wpv)) {
+ /*
+ * Implicitly open one closed zone to write if there are zone resources
+ * left.
+ */
+ zs = qcow2_get_zs(wpv);
+ if (zs == BLK_ZS_CLOSED || zs == BLK_ZS_EMPTY) {
+ ret = qcow2_check_zone_resources(bs, zs);
+ if (ret < 0) {
+ goto fail_nometa;
+ }
+
+ if (zs == BLK_ZS_CLOSED) {
+ s->nr_zones_closed--;
+ s->nr_zones_imp_open++;
+ } else {
+ s->nr_zones_imp_open++;
+ }
+ }
+
+ /* align up (start_offset, zone_size), the start offset is not
+ * necessarily power of two. */
+ end = ((start_offset + zone_size) / zone_size) * zone_size;
+ if (start_offset + start_bytes <= end) {
+ *wp = start_offset + start_bytes;
+ } else {
+ ret = -EINVAL;
+ goto fail_nometa;
+ }
+
+ ret = qcow2_write_wp_at(bs, wp, index,BLK_ZS_IOPEN);
+ if (ret < 0) {
+ goto fail_nometa;
+ }
+ }
+ }
ret = 0;
qemu_co_mutex_lock(&s->lock);
@@ -3136,7 +3372,9 @@ int qcow2_update_header(BlockDriverState *bs)
.max_active_zones =
cpu_to_be32(s->zoned_header.max_active_zones),
.max_append_sectors =
- cpu_to_be32(s->zoned_header.max_append_sectors)
+ cpu_to_be32(s->zoned_header.max_append_sectors),
+ .zonedmeta_offset = cpu_to_be64(s->zoned_header.zonedmeta_offset),
+ .zonedmeta_size = cpu_to_be64(s->zoned_header.zonedmeta_size)
};
ret = header_ext_add(buf, QCOW2_EXT_MAGIC_ZONED_FORMAT,
&zoned_header, sizeof(zoned_header),
@@ -3541,7 +3779,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
int version;
int refcount_order;
uint64_t *refcount_table;
- int ret;
+ uint64_t zoned_meta_size, zoned_clusterlen;
+ int ret, offset, i;
uint8_t compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
@@ -3847,6 +4086,48 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
s->zoned_header.max_open_zones = qcow2_opts->max_open_zones;
s->zoned_header.max_active_zones = qcow2_opts->max_active_zones;
s->zoned_header.max_append_sectors = qcow2_opts->max_append_sectors;
+ s->zoned_header.nr_zones = qcow2_opts->size / qcow2_opts->zone_size;
+
+ zoned_meta_size = sizeof(uint64_t) * s->zoned_header.nr_zones;
+ g_autofree uint64_t *meta = NULL;
+ meta = g_new0(uint64_t, s->zoned_header.nr_zones);
+
+ for (i = 0; i < s->zoned_header.zone_nr_conv; ++i) {
+ meta[i] = i * s->zoned_header.zone_size;
+ meta[i] |= 1ULL << 59;
+ }
+ for (; i < s->zoned_header.nr_zones; ++i) {
+ meta[i] = i * s->zoned_header.zone_size;
+ /* For sequential zones, the first four most significant bit
+ * indicates zone states. */
+ meta[i] |= ((uint64_t)BLK_ZS_EMPTY << 60);
+ }
+
+ offset = qcow2_alloc_clusters(blk_bs(blk), zoned_meta_size);
+ if (offset < 0) {
+ error_setg_errno(errp, -offset, "Could not allocate clusters "
+ "for zoned metadata size");
+ goto out;
+ }
+ s->zoned_header.zonedmeta_offset = offset;
+ s->zoned_header.zonedmeta_size = zoned_meta_size;
+
+ zoned_clusterlen = size_to_clusters(s, zoned_meta_size)
+ * s->cluster_size;
+ assert(qcow2_pre_write_overlap_check(bs, 0, offset,
+ zoned_clusterlen,false) == 0);
+ ret = bdrv_pwrite_zeroes(blk_bs(blk)->file, offset,
+ zoned_clusterlen, 0);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Could not zero fill zoned metadata");
+ goto out;
+ }
+ ret = bdrv_pwrite(blk_bs(blk)->file, offset, zoned_meta_size, meta, 0);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Could not write zoned metadata "
+ "to disk");
+ goto out;
+ }
}
/* Create a full header (including things like feature table) */
@@ -4190,6 +4471,393 @@ static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
return ret;
}
+static int coroutine_fn
+qcow2_co_zone_report(BlockDriverState *bs, int64_t offset,
+ unsigned int *nr_zones, BlockZoneDescriptor *zones)
+{
+ BDRVQcow2State *s = bs->opaque;
+ uint64_t zone_size = s->zoned_header.zone_size;
+ int64_t capacity = bs->total_sectors << BDRV_SECTOR_BITS;
+ int64_t size = bs->bl.nr_zones * zone_size;
+ unsigned int nrz = *nr_zones;
+ int i = 0;
+ int si;
+
+ if (offset >= capacity) {
+ error_report("offset %" PRId64 " is equal to or greater than the"
+ "device capacity %" PRId64 "", offset, capacity);
+ return -EINVAL;
+ }
+
+ if (nrz > bs->bl.nr_zones) {
+ error_report("nr_zones %" PRId32 " should not exceed the device zones"
+ "%" PRId32 "", nrz, bs->bl.nr_zones);
+ return -EINVAL;
+ }
+
+ if (zone_size > 0) {
+ si = offset / zone_size;
+ qemu_co_mutex_lock(&s->wps->colock);
+ for (; i < nrz; ++i) {
+ if (i + si >= bs->bl.nr_zones) {
+ break;
+ }
+
+ zones[i].start = (si + i) * zone_size;
+
+ /* The last zone can be smaller than the zone size */
+ if ((si + i + 1) == bs->bl.nr_zones && size > capacity) {
+ uint32_t l = zone_size - (size - capacity);
+ zones[i].length = l;
+ zones[i].cap = l;
+ } else {
+ zones[i].length = zone_size;
+ zones[i].cap = zone_size;
+ }
+
+ uint64_t wp = s->wps->wp[si + i];
+ if (QCOW2_ZT_IS_CONV(wp)) {
+ zones[i].type = BLK_ZT_CONV;
+ zones[i].state = BLK_ZS_NOT_WP;
+ /* Clear the zone type bit */
+ wp &= ~(1ULL << 59);
+ } else {
+ zones[i].type = BLK_ZT_SWR;
+ zones[i].state = qcow2_get_zs(wp);
+ /* Clear the zone state bits */
+ wp = qcow2_get_wp(wp);
+ }
+ zones[i].wp = wp;
+ }
+ qemu_co_mutex_unlock(&s->wps->colock);
+ }
+ *nr_zones = i;
+ return 0;
+}
+
+static int qcow2_open_zone(BlockDriverState *bs, uint32_t index) {
+ BDRVQcow2State *s = bs->opaque;
+ int ret;
+
+ qemu_co_mutex_lock(&s->wps->colock);
+ uint64_t *wp = &s->wps->wp[index];
+ BlockZoneState zs = qcow2_get_zs(*wp);
+
+ switch(zs) {
+ case BLK_ZS_EMPTY:
+ ret = qcow2_check_zone_resources(bs, BLK_ZS_EMPTY);
+ if (ret < 0) {
+ goto unlock;
+ }
+ break;
+ case BLK_ZS_IOPEN:
+ s->nr_zones_imp_open--;
+ break;
+ case BLK_ZS_EOPEN:
+ return 0;
+ case BLK_ZS_CLOSED:
+ ret = qcow2_check_zone_resources(bs, BLK_ZS_CLOSED);
+ if (ret < 0) {
+ goto unlock;
+ }
+ s->nr_zones_closed--;
+ break;
+ case BLK_ZS_FULL:
+ break;
+ default:
+ ret = -EINVAL;
+ goto unlock;
+ }
+ ret = qcow2_write_wp_at(bs, wp, index, BLK_ZS_EOPEN);
+ if (!ret) {
+ s->nr_zones_exp_open++;
+ }
+
+unlock:
+ qemu_co_mutex_unlock(&s->wps->colock);
+ return ret;
+}
+
+static int qcow2_close_zone(BlockDriverState *bs, uint32_t index) {
+ BDRVQcow2State *s = bs->opaque;
+ int ret;
+
+ qemu_co_mutex_lock(&s->wps->colock);
+ uint64_t *wp = &s->wps->wp[index];
+ BlockZoneState zs = qcow2_get_zs(*wp);
+
+ switch(zs) {
+ case BLK_ZS_EMPTY:
+ break;
+ case BLK_ZS_IOPEN:
+ s->nr_zones_imp_open--;
+ break;
+ case BLK_ZS_EOPEN:
+ s->nr_zones_exp_open--;
+ break;
+ case BLK_ZS_CLOSED:
+ ret = qcow2_check_zone_resources(bs, BLK_ZS_CLOSED);
+ if (ret < 0) {
+ goto unlock;
+ }
+ s->nr_zones_closed--;
+ break;
+ case BLK_ZS_FULL:
+ break;
+ default:
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ if (zs == BLK_ZS_EMPTY) {
+ ret = qcow2_write_wp_at(bs, wp, index, BLK_ZS_EMPTY);
+ } else {
+ ret = qcow2_write_wp_at(bs, wp, index, BLK_ZS_CLOSED);
+ if (!ret) {
+ s->nr_zones_closed++;
+ }
+ }
+
+unlock:
+ qemu_co_mutex_unlock(&s->wps->colock);
+ return ret;
+}
+
+static int qcow2_finish_zone(BlockDriverState *bs, uint32_t index) {
+ BDRVQcow2State *s = bs->opaque;
+ int ret;
+
+ qemu_co_mutex_lock(&s->wps->colock);
+ uint64_t *wp = &s->wps->wp[index];
+ BlockZoneState zs = qcow2_get_zs(*wp);
+
+ switch(zs) {
+ case BLK_ZS_EMPTY:
+ ret = qcow2_check_zone_resources(bs, BLK_ZS_EMPTY);
+ if (ret < 0) {
+ goto unlock;
+ }
+ break;
+ case BLK_ZS_IOPEN:
+ s->nr_zones_imp_open--;
+ break;
+ case BLK_ZS_EOPEN:
+ s->nr_zones_exp_open--;
+ break;
+ case BLK_ZS_CLOSED:
+ ret = qcow2_check_zone_resources(bs, BLK_ZS_CLOSED);
+ if (ret < 0) {
+ goto unlock;
+ }
+ s->nr_zones_closed--;
+ break;
+ case BLK_ZS_FULL:
+ ret = 0;
+ goto unlock;
+ default:
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ *wp = ((uint64_t)index + 1) * s->zoned_header.zone_size;
+ ret = qcow2_write_wp_at(bs, wp, index, BLK_ZS_FULL);
+
+unlock:
+ qemu_co_mutex_unlock(&s->wps->colock);
+ return ret;
+}
+
+static int qcow2_reset_zone(BlockDriverState *bs, uint32_t index,
+ int64_t len) {
+ BDRVQcow2State *s = bs->opaque;
+ int nrz = bs->bl.nr_zones;
+ int zone_size = bs->bl.zone_size;
+ int n, ret = 0;
+
+ qemu_co_mutex_lock(&s->wps->colock);
+ uint64_t *wp = &s->wps->wp[index];
+ if (len == bs->total_sectors << BDRV_SECTOR_BITS) {
+ n = nrz;
+ index = 0;
+ } else {
+ n = len / zone_size;
+ }
+
+ for (int i = 0; i < n; ++i) {
+ uint64_t *wp_i = (uint64_t *)(wp + i);
+ uint64_t wpi_v = *wp_i;
+ if (QCOW2_ZT_IS_CONV(wpi_v)) {
+ continue;
+ }
+
+ BlockZoneState zs = qcow2_get_zs(wpi_v);
+ switch (zs) {
+ case BLK_ZS_EMPTY:
+ break;
+ case BLK_ZS_IOPEN:
+ s->nr_zones_imp_open--;
+ break;
+ case BLK_ZS_EOPEN:
+ s->nr_zones_exp_open--;
+ break;
+ case BLK_ZS_CLOSED:
+ s->nr_zones_closed--;
+ break;
+ case BLK_ZS_FULL:
+ break;
+ default:
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ if (zs == BLK_ZS_EMPTY) {
+ continue;
+ }
+
+ *wp_i = ((uint64_t)index + i) * zone_size;
+ ret = qcow2_write_wp_at(bs, wp_i, index + i, BLK_ZS_EMPTY);
+ if (ret < 0) {
+ goto unlock;
+ }
+ /* clear data */
+ ret = qcow2_co_pwrite_zeroes(bs, qcow2_get_wp(*wp_i), zone_size, 0);
+ if (ret < 0) {
+ error_report("Failed to reset zone at 0x%" PRIx64 "", *wp_i);
+ }
+ }
+
+unlock:
+ qemu_co_mutex_unlock(&s->wps->colock);
+ return ret;
+}
+
+static int coroutine_fn qcow2_co_zone_mgmt(BlockDriverState *bs, BlockZoneOp op,
+ int64_t offset, int64_t len)
+{
+ BDRVQcow2State *s = bs->opaque;
+ int ret = 0;
+ int64_t capacity = bs->total_sectors << BDRV_SECTOR_BITS;
+ int64_t zone_size = s->zoned_header.zone_size;
+ int64_t zone_size_mask = zone_size - 1;
+ uint32_t index = offset / zone_size;
+ BlockZoneWps *wps = s->wps;
+
+ if (offset >= capacity) {
+ error_report("offset %" PRId64 " is equal to or greater than the"
+ "device capacity %" PRId64 "", offset, capacity);
+ return -EINVAL;
+ }
+
+ if (offset & zone_size_mask) {
+ error_report("sector offset %" PRId64 " is not aligned to zone size"
+ " %" PRId64 "", offset / 512, zone_size / 512);
+ return -EINVAL;
+ }
+
+ if (((offset + len) < capacity && len & zone_size_mask) ||
+ offset + len > capacity) {
+ error_report("number of sectors %" PRId64 " is not aligned to zone"
+ " size %" PRId64 "", len / 512, zone_size / 512);
+ return -EINVAL;
+ }
+
+ qemu_co_mutex_lock(&wps->colock);
+ uint64_t wpv = wps->wp[index];
+ if (QCOW2_ZT_IS_CONV(wpv) && len != capacity) {
+ error_report("zone mgmt operations are not allowed for "
+ "conventional zones");
+ ret = -EIO;
+ goto unlock;
+ }
+ qemu_co_mutex_unlock(&wps->colock);
+
+ switch(op) {
+ case BLK_ZO_OPEN:
+ ret = qcow2_open_zone(bs, index);
+ break;
+ case BLK_ZO_CLOSE:
+ ret = qcow2_close_zone(bs, index);
+ break;
+ case BLK_ZO_FINISH:
+ ret = qcow2_finish_zone(bs, index);
+ break;
+ case BLK_ZO_RESET:
+ ret = qcow2_reset_zone(bs, index, len);
+ break;
+ default:
+ error_report("Unsupported zone op: 0x%x", op);
+ ret = -ENOTSUP;
+ break;
+ }
+ return ret;
+
+unlock:
+ qemu_co_mutex_unlock(&wps->colock);
+ return ret;
+}
+
+static int coroutine_fn
+qcow2_co_zone_append(BlockDriverState *bs, int64_t *offset, QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
+{
+ assert(flags == 0);
+ int64_t capacity = bs->total_sectors << BDRV_SECTOR_BITS;
+ BDRVQcow2State *s = bs->opaque;
+ uint32_t index;
+ int ret;
+ int64_t zone_size_mask = bs->bl.zone_size - 1;
+ int64_t iov_len = 0;
+ int64_t len = 0;
+
+ if (*offset >= capacity) {
+ error_report("*offset %" PRId64 " is equal to or greater than the"
+ "device capacity %" PRId64 "", *offset, capacity);
+ return -EINVAL;
+ }
+
+ /* offset + len should not pass the end of that zone starting from offset */
+ if (*offset & zone_size_mask) {
+ error_report("sector offset %" PRId64 " is not aligned to zone size "
+ "%" PRId32 "", *offset / 512, bs->bl.zone_size / 512);
+ return -EINVAL;
+ }
+
+ int64_t wg = bs->bl.write_granularity;
+ int64_t wg_mask = wg - 1;
+ for (int i = 0; i < qiov->niov; i++) {
+ iov_len = qiov->iov[i].iov_len;
+ if (iov_len & wg_mask) {
+ error_report("len of IOVector[%d] %" PRId64 " is not aligned to "
+ "block size %" PRId64 "", i, iov_len, wg);
+ return -EINVAL;
+ }
+ }
+ len = qiov->size;
+ index = *offset / bs->bl.zone_size;
+
+ if ((len >> BDRV_SECTOR_BITS) > bs->bl.max_append_sectors) {
+ return -ENOTSUP;
+ }
+
+ qemu_co_mutex_lock(&s->wps->colock);
+ uint64_t wp = s->wps->wp[index];
+ uint64_t wp_i = qcow2_get_wp(wp);
+ ret = qcow2_co_pwritev_part(bs, wp_i, len, qiov, 0, 0);
+ if (ret == 0) {
+ *offset = wp_i;
+ /* the zone state is set to full when the wp reaches the end */
+ uint64_t wp_final = s->wps->wp[index];
+ if (!(qcow2_get_wp(wp_final) & zone_size_mask)) {
+ ret = qcow2_write_wp_at(bs, &wp_final, index, BLK_ZS_FULL);
+ }
+ } else {
+ error_report("qcow2: zap failed");
+ }
+
+ qemu_co_mutex_unlock(&s->wps->colock);
+ return ret;
+}
+
static int coroutine_fn GRAPH_RDLOCK
qcow2_co_copy_range_from(BlockDriverState *bs,
BdrvChild *src, int64_t src_offset,
@@ -6238,6 +6906,10 @@ BlockDriver bdrv_qcow2 = {
.bdrv_co_pwritev_part = qcow2_co_pwritev_part,
.bdrv_co_flush_to_os = qcow2_co_flush_to_os,
+ .bdrv_co_zone_report = qcow2_co_zone_report,
+ .bdrv_co_zone_mgmt = qcow2_co_zone_mgmt,
+ .bdrv_co_zone_append = qcow2_co_zone_append,
+
.bdrv_co_pwrite_zeroes = qcow2_co_pwrite_zeroes,
.bdrv_co_pdiscard = qcow2_co_pdiscard,
.bdrv_co_copy_range_from = qcow2_co_copy_range_from,
diff --git a/block/qcow2.h b/block/qcow2.h
index 3694c8d217..38b779ae32 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -248,6 +248,8 @@ typedef struct Qcow2ZonedHeaderExtension {
uint32_t max_active_zones;
uint32_t max_open_zones;
uint32_t max_append_sectors;
+ uint64_t zonedmeta_offset;
+ uint64_t zonedmeta_size;
} QEMU_PACKED Qcow2ZonedHeaderExtension;
typedef struct Qcow2UnknownHeaderExtension {
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index ef2ba6f670..739e2c62c6 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -354,6 +354,8 @@ The fields of the zoned extension are:
20 - 23: max_active_zones
24 - 27: max_open_zones
28 - 31: max_append_sectors
+ 32 - 39: zonedmeta_offset
+ 40 - 47: zonedmeta_size
== Full disk encryption header pointer ==
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/4] qcow2: add zoned emulation capability
2023-08-14 8:58 ` [PATCH v2 3/4] qcow2: add zoned emulation capability Sam Li
@ 2023-08-16 21:07 ` Stefan Hajnoczi
2023-08-22 19:48 ` Stefan Hajnoczi
1 sibling, 0 replies; 26+ messages in thread
From: Stefan Hajnoczi @ 2023-08-16 21:07 UTC (permalink / raw)
To: Sam Li
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake, dlemoal
[-- Attachment #1: Type: text/plain, Size: 2685 bytes --]
On Mon, Aug 14, 2023 at 04:58:01PM +0800, Sam Li wrote:
> By adding zone operations and zoned metadata, the zoned emulation
> capability enables full emulation support of zoned device using
> a qcow2 file. The zoned device metadata includes zone type,
> zoned device state and write pointer of each zone, which is stored
> to an array of unsigned integers.
>
> Each zone of a zoned device makes state transitions following
> the zone state machine. The zone state machine mainly describes
> five states, IMPLICIT OPEN, EXPLICIT OPEN, FULL, EMPTY and CLOSED.
> READ ONLY and OFFLINE states will generally be affected by device
> internal events. The operations on zones cause corresponding state
> changing.
>
> Zoned devices have a limit on zone resources, which puts constraints on
> write operations into zones.
>
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
> block/qcow2.c | 676 ++++++++++++++++++++++++++++++++++++++++-
> block/qcow2.h | 2 +
> docs/interop/qcow2.txt | 2 +
> 3 files changed, 678 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c1077c4a4a..5ccf79cbe7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -194,6 +194,164 @@ qcow2_extract_crypto_opts(QemuOpts *opts, const char *fmt, Error **errp)
> return cryptoopts_qdict;
> }
>
> +#define QCOW2_ZT_IS_CONV(wp) (wp & 1ULL << 59)
> +
> +static inline int qcow2_get_wp(uint64_t wp)
> +{
> + /* clear state and type information */
> + return ((wp << 5) >> 5);
> +}
> +
> +static inline int qcow2_get_zs(uint64_t wp)
> +{
> + return (wp >> 60);
> +}
> +
> +static inline void qcow2_set_wp(uint64_t *wp, BlockZoneState zs)
> +{
> + uint64_t addr = qcow2_get_wp(*wp);
> + addr |= ((uint64_t)zs << 60);
> + *wp = addr;
> +}
Although the function is called qcow2_set_wp() it seems to actually be
qcow2_set_zs() since it only changes the zone state, not the write
pointer. Want to rename it?
> +
> +/*
> + * File wp tracking: reset zone, finish zone and append zone can
> + * change the value of write pointer. All zone operations will change
> + * the state of that/those zone.
> + * */
> +static inline void qcow2_wp_tracking_helper(int index, uint64_t wp) {
> + /* format: operations, the wp. */
> + printf("wps[%d]: 0x%x\n", index, qcow2_get_wp(wp)>>BDRV_SECTOR_BITS);
> +}
This looks like debugging code that shouldn't go into qemu.git. Please
use tracing (docs/devel/tracing.rst) to capture internal information in
production code.
I will review more of this patch series another time because I need to
get going.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/4] qcow2: add zoned emulation capability
2023-08-14 8:58 ` [PATCH v2 3/4] qcow2: add zoned emulation capability Sam Li
2023-08-16 21:07 ` Stefan Hajnoczi
@ 2023-08-22 19:48 ` Stefan Hajnoczi
2023-08-28 11:55 ` Sam Li
1 sibling, 1 reply; 26+ messages in thread
From: Stefan Hajnoczi @ 2023-08-22 19:48 UTC (permalink / raw)
To: Sam Li
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake, dlemoal
[-- Attachment #1: Type: text/plain, Size: 5828 bytes --]
On Mon, Aug 14, 2023 at 04:58:01PM +0800, Sam Li wrote:
> By adding zone operations and zoned metadata, the zoned emulation
> capability enables full emulation support of zoned device using
> a qcow2 file. The zoned device metadata includes zone type,
> zoned device state and write pointer of each zone, which is stored
> to an array of unsigned integers.
>
> Each zone of a zoned device makes state transitions following
> the zone state machine. The zone state machine mainly describes
> five states, IMPLICIT OPEN, EXPLICIT OPEN, FULL, EMPTY and CLOSED.
> READ ONLY and OFFLINE states will generally be affected by device
> internal events. The operations on zones cause corresponding state
> changing.
>
> Zoned devices have a limit on zone resources, which puts constraints on
> write operations into zones.
>
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
> block/qcow2.c | 676 ++++++++++++++++++++++++++++++++++++++++-
> block/qcow2.h | 2 +
> docs/interop/qcow2.txt | 2 +
> 3 files changed, 678 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c1077c4a4a..5ccf79cbe7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -194,6 +194,164 @@ qcow2_extract_crypto_opts(QemuOpts *opts, const char *fmt, Error **errp)
> return cryptoopts_qdict;
> }
>
> +#define QCOW2_ZT_IS_CONV(wp) (wp & 1ULL << 59)
> +
> +static inline int qcow2_get_wp(uint64_t wp)
> +{
> + /* clear state and type information */
> + return ((wp << 5) >> 5);
> +}
> +
> +static inline int qcow2_get_zs(uint64_t wp)
> +{
> + return (wp >> 60);
> +}
> +
> +static inline void qcow2_set_wp(uint64_t *wp, BlockZoneState zs)
> +{
> + uint64_t addr = qcow2_get_wp(*wp);
> + addr |= ((uint64_t)zs << 60);
> + *wp = addr;
> +}
> +
> +/*
> + * File wp tracking: reset zone, finish zone and append zone can
> + * change the value of write pointer. All zone operations will change
> + * the state of that/those zone.
> + * */
> +static inline void qcow2_wp_tracking_helper(int index, uint64_t wp) {
> + /* format: operations, the wp. */
> + printf("wps[%d]: 0x%x\n", index, qcow2_get_wp(wp)>>BDRV_SECTOR_BITS);
> +}
> +
> +/*
> + * Perform a state assignment and a flush operation that writes the new wp
> + * value to the dedicated location of the disk file.
> + */
> +static int qcow2_write_wp_at(BlockDriverState *bs, uint64_t *wp,
> + uint32_t index, BlockZoneState zs) {
> + BDRVQcow2State *s = bs->opaque;
> + int ret;
> +
> + qcow2_set_wp(wp, zs);
> + ret = bdrv_pwrite(bs->file, s->zoned_header.zonedmeta_offset
> + + sizeof(uint64_t) * index, sizeof(uint64_t), wp, 0);
> +
> + if (ret < 0) {
> + goto exit;
Should *wp be restored to its original value to undo the effect of
qcow2_set_wp()?
> + }
> + qcow2_wp_tracking_helper(index, *wp);
> + return ret;
> +
> +exit:
> + error_report("Failed to write metadata with file");
> + return ret;
> +}
> +
> +static int qcow2_check_active(BlockDriverState *bs)
Please rename this to qcow2_check_active_zones() to avoid confusion with
other uses "active" in qcow2.
> +{
> + BDRVQcow2State *s = bs->opaque;
> +
> + if (!s->zoned_header.max_active_zones) {
> + return 0;
> + }
> +
> + if (s->nr_zones_exp_open + s->nr_zones_imp_open + s->nr_zones_closed
> + < s->zoned_header.max_active_zones) {
> + return 0;
> + }
> +
> + return -1;
> +}
(This function could return a bool instead of 0/-1 since it doesn't
really need an int.)
> +
> +static int qcow2_check_open(BlockDriverState *bs)
qcow2_check_open_zones() or, even better, qcow2_can_open_zone().
> +{
> + BDRVQcow2State *s = bs->opaque;
> + int ret;
> +
> + if (!s->zoned_header.max_open_zones) {
> + return 0;
> + }
> +
> + if (s->nr_zones_exp_open + s->nr_zones_imp_open
> + < s->zoned_header.max_open_zones) {
> + return 0;
> + }
> +
> + if(s->nr_zones_imp_open) {
> + ret = qcow2_check_active(bs);
> + if (ret == 0) {
> + /* TODO: it takes O(n) time complexity (n = nr_zones).
> + * Optimizations required. */
One solution is to keep an implicitly open list. Then this operation is
O(1).
> + /* close one implicitly open zones to make it available */
> + for (int i = s->zoned_header.zone_nr_conv;
> + i < bs->bl.nr_zones; ++i) {
> + uint64_t *wp = &s->wps->wp[i];
> + if (qcow2_get_zs(*wp) == BLK_ZS_IOPEN) {
> + ret = qcow2_write_wp_at(bs, wp, i, BLK_ZS_CLOSED);
I'm wondering if it's correct to store the zone state persistently in
the qcow2 file. If the guest or QEMU crashes, then zones will be left in
states like EOPEN. Since the guest software will have forgotten about
explicitly opened zones, the guest would need to recover zone states.
I'm not sure if existing software is designed to do that.
Damien: Should the zone state be persistent?
> + if (ret < 0) {
> + return ret;
> + }
> + s->wps->wp[i] = *wp;
> + s->nr_zones_imp_open--;
> + s->nr_zones_closed++;
> + break;
> + }
> + }
> + return 0;
This returns 0 if there are no IOPEN zones available. It needs to return
an error when there are not enough resources available.
> + }
> + return ret;
> + }
> +
> + return -1;
This function mixes 0/-1 with -errno return values. -1 is -EPERM, but I
think that's not what you want.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/4] qcow2: add zoned emulation capability
2023-08-22 19:48 ` Stefan Hajnoczi
@ 2023-08-28 11:55 ` Sam Li
2023-08-29 6:06 ` Damien Le Moal
0 siblings, 1 reply; 26+ messages in thread
From: Sam Li @ 2023-08-28 11:55 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake, dlemoal
Stefan Hajnoczi <stefanha@redhat.com> 于2023年8月23日周三 03:48写道：
>
> On Mon, Aug 14, 2023 at 04:58:01PM +0800, Sam Li wrote:
> > By adding zone operations and zoned metadata, the zoned emulation
> > capability enables full emulation support of zoned device using
> > a qcow2 file. The zoned device metadata includes zone type,
> > zoned device state and write pointer of each zone, which is stored
> > to an array of unsigned integers.
> >
> > Each zone of a zoned device makes state transitions following
> > the zone state machine. The zone state machine mainly describes
> > five states, IMPLICIT OPEN, EXPLICIT OPEN, FULL, EMPTY and CLOSED.
> > READ ONLY and OFFLINE states will generally be affected by device
> > internal events. The operations on zones cause corresponding state
> > changing.
> >
> > Zoned devices have a limit on zone resources, which puts constraints on
> > write operations into zones.
> >
> > Signed-off-by: Sam Li <faithilikerun@gmail.com>
> > ---
> > block/qcow2.c | 676 ++++++++++++++++++++++++++++++++++++++++-
> > block/qcow2.h | 2 +
> > docs/interop/qcow2.txt | 2 +
> > 3 files changed, 678 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index c1077c4a4a..5ccf79cbe7 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -194,6 +194,164 @@ qcow2_extract_crypto_opts(QemuOpts *opts, const char *fmt, Error **errp)
> > return cryptoopts_qdict;
> > }
> >
> > +#define QCOW2_ZT_IS_CONV(wp) (wp & 1ULL << 59)
> > +
> > +static inline int qcow2_get_wp(uint64_t wp)
> > +{
> > + /* clear state and type information */
> > + return ((wp << 5) >> 5);
> > +}
> > +
> > +static inline int qcow2_get_zs(uint64_t wp)
> > +{
> > + return (wp >> 60);
> > +}
> > +
> > +static inline void qcow2_set_wp(uint64_t *wp, BlockZoneState zs)
> > +{
> > + uint64_t addr = qcow2_get_wp(*wp);
> > + addr |= ((uint64_t)zs << 60);
> > + *wp = addr;
> > +}
> > +
> > +/*
> > + * File wp tracking: reset zone, finish zone and append zone can
> > + * change the value of write pointer. All zone operations will change
> > + * the state of that/those zone.
> > + * */
> > +static inline void qcow2_wp_tracking_helper(int index, uint64_t wp) {
> > + /* format: operations, the wp. */
> > + printf("wps[%d]: 0x%x\n", index, qcow2_get_wp(wp)>>BDRV_SECTOR_BITS);
> > +}
> > +
> > +/*
> > + * Perform a state assignment and a flush operation that writes the new wp
> > + * value to the dedicated location of the disk file.
> > + */
> > +static int qcow2_write_wp_at(BlockDriverState *bs, uint64_t *wp,
> > + uint32_t index, BlockZoneState zs) {
> > + BDRVQcow2State *s = bs->opaque;
> > + int ret;
> > +
> > + qcow2_set_wp(wp, zs);
> > + ret = bdrv_pwrite(bs->file, s->zoned_header.zonedmeta_offset
> > + + sizeof(uint64_t) * index, sizeof(uint64_t), wp, 0);
> > +
> > + if (ret < 0) {
> > + goto exit;
>
> Should *wp be restored to its original value to undo the effect of
> qcow2_set_wp()?
>
> > + }
> > + qcow2_wp_tracking_helper(index, *wp);
> > + return ret;
> > +
> > +exit:
> > + error_report("Failed to write metadata with file");
> > + return ret;
> > +}
> > +
> > +static int qcow2_check_active(BlockDriverState *bs)
>
> Please rename this to qcow2_check_active_zones() to avoid confusion with
> other uses "active" in qcow2.
>
> > +{
> > + BDRVQcow2State *s = bs->opaque;
> > +
> > + if (!s->zoned_header.max_active_zones) {
> > + return 0;
> > + }
> > +
> > + if (s->nr_zones_exp_open + s->nr_zones_imp_open + s->nr_zones_closed
> > + < s->zoned_header.max_active_zones) {
> > + return 0;
> > + }
> > +
> > + return -1;
> > +}
>
> (This function could return a bool instead of 0/-1 since it doesn't
> really need an int.)
>
> > +
> > +static int qcow2_check_open(BlockDriverState *bs)
>
> qcow2_check_open_zones() or, even better, qcow2_can_open_zone().
>
> > +{
> > + BDRVQcow2State *s = bs->opaque;
> > + int ret;
> > +
> > + if (!s->zoned_header.max_open_zones) {
> > + return 0;
> > + }
> > +
> > + if (s->nr_zones_exp_open + s->nr_zones_imp_open
> > + < s->zoned_header.max_open_zones) {
> > + return 0;
> > + }
> > +
> > + if(s->nr_zones_imp_open) {
> > + ret = qcow2_check_active(bs);
> > + if (ret == 0) {
> > + /* TODO: it takes O(n) time complexity (n = nr_zones).
> > + * Optimizations required. */
>
> One solution is to keep an implicitly open list. Then this operation is
> O(1).
Yes, I'll add this maybe in the v4.
>
> > + /* close one implicitly open zones to make it available */
> > + for (int i = s->zoned_header.zone_nr_conv;
> > + i < bs->bl.nr_zones; ++i) {
> > + uint64_t *wp = &s->wps->wp[i];
> > + if (qcow2_get_zs(*wp) == BLK_ZS_IOPEN) {
> > + ret = qcow2_write_wp_at(bs, wp, i, BLK_ZS_CLOSED);
>
> I'm wondering if it's correct to store the zone state persistently in
> the qcow2 file. If the guest or QEMU crashes, then zones will be left in
> states like EOPEN. Since the guest software will have forgotten about
> explicitly opened zones, the guest would need to recover zone states.
> I'm not sure if existing software is designed to do that.
>
> Damien: Should the zone state be persistent?
Pin?
>
> > + if (ret < 0) {
> > + return ret;
> > + }
> > + s->wps->wp[i] = *wp;
> > + s->nr_zones_imp_open--;
> > + s->nr_zones_closed++;
> > + break;
> > + }
> > + }
> > + return 0;
>
> This returns 0 if there are no IOPEN zones available. It needs to return
> an error when there are not enough resources available.
>
> > + }
> > + return ret;
> > + }
> > +
> > + return -1;
>
> This function mixes 0/-1 with -errno return values. -1 is -EPERM, but I
> think that's not what you want.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/4] qcow2: add zoned emulation capability
2023-08-28 11:55 ` Sam Li
@ 2023-08-29 6:06 ` Damien Le Moal
2023-08-29 6:27 ` Sam Li
0 siblings, 1 reply; 26+ messages in thread
From: Damien Le Moal @ 2023-08-29 6:06 UTC (permalink / raw)
To: Sam Li, Stefan Hajnoczi
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake
On 8/28/23 20:55, Sam Li wrote:
>>> + /* close one implicitly open zones to make it available */
>>> + for (int i = s->zoned_header.zone_nr_conv;
>>> + i < bs->bl.nr_zones; ++i) {
>>> + uint64_t *wp = &s->wps->wp[i];
>>> + if (qcow2_get_zs(*wp) == BLK_ZS_IOPEN) {
>>> + ret = qcow2_write_wp_at(bs, wp, i, BLK_ZS_CLOSED);
>>
>> I'm wondering if it's correct to store the zone state persistently in
>> the qcow2 file. If the guest or QEMU crashes, then zones will be left in
>> states like EOPEN. Since the guest software will have forgotten about
>> explicitly opened zones, the guest would need to recover zone states.
>> I'm not sure if existing software is designed to do that.
>>
>> Damien: Should the zone state be persistent?
Yes and no. Yes you need to preserve/maintain zone states but not as is.
With a real drive, if you power cycle the device, you get the following states
changes:
Before | After power cycle
----------------+-------------------
EMPTY | EMPTY
FULL | FULL
IMP. OPEN | CLOSED
EXP. OPEN | CLOSED
CLOSED | CLOSED
READ=ONLY | READ-ONLY
OFFLINE | OFFLINE
So any open (implicit or explicit) zone will show up as closed after power
cycle. That is, the number of "active" zones does not change.
For the qcow2 emulation, as long as you do not also emulate read-only and
offline zones, you actually do not need to save the zone state in the zone
metadata. On startup, you can infer the state from the zone write pointer:
zone wp == zone start -> EMPTY
zone wp >= zone capacity -> FULL
zone wp > zone start -> CLOSED
And make sure that all closed zones are counted as the initial number of active
zones. The initial number of open zones will always be 0.
So it is easy :)
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/4] qcow2: add zoned emulation capability
2023-08-29 6:06 ` Damien Le Moal
@ 2023-08-29 6:27 ` Sam Li
2023-08-29 7:14 ` Damien Le Moal
0 siblings, 1 reply; 26+ messages in thread
From: Sam Li @ 2023-08-29 6:27 UTC (permalink / raw)
To: Damien Le Moal
Cc: Stefan Hajnoczi, qemu-devel, hare, Hanna Reitz, dmitry.fomichev,
qemu-block, Kevin Wolf, Markus Armbruster, Eric Blake
Damien Le Moal <dlemoal@kernel.org> 于2023年8月29日周二 14:06写道：
>
> On 8/28/23 20:55, Sam Li wrote:
> >>> + /* close one implicitly open zones to make it available */
> >>> + for (int i = s->zoned_header.zone_nr_conv;
> >>> + i < bs->bl.nr_zones; ++i) {
> >>> + uint64_t *wp = &s->wps->wp[i];
> >>> + if (qcow2_get_zs(*wp) == BLK_ZS_IOPEN) {
> >>> + ret = qcow2_write_wp_at(bs, wp, i, BLK_ZS_CLOSED);
> >>
> >> I'm wondering if it's correct to store the zone state persistently in
> >> the qcow2 file. If the guest or QEMU crashes, then zones will be left in
> >> states like EOPEN. Since the guest software will have forgotten about
> >> explicitly opened zones, the guest would need to recover zone states.
> >> I'm not sure if existing software is designed to do that.
> >>
> >> Damien: Should the zone state be persistent?
>
> Yes and no. Yes you need to preserve/maintain zone states but not as is.
> With a real drive, if you power cycle the device, you get the following states
> changes:
>
> Before | After power cycle
> ----------------+-------------------
> EMPTY | EMPTY
> FULL | FULL
> IMP. OPEN | CLOSED
> EXP. OPEN | CLOSED
> CLOSED | CLOSED
> READ=ONLY | READ-ONLY
> OFFLINE | OFFLINE
>
> So any open (implicit or explicit) zone will show up as closed after power
> cycle. That is, the number of "active" zones does not change.
> For the qcow2 emulation, as long as you do not also emulate read-only and
> offline zones, you actually do not need to save the zone state in the zone
> metadata. On startup, you can infer the state from the zone write pointer:
>
> zone wp == zone start -> EMPTY
> zone wp >= zone capacity -> FULL
> zone wp > zone start -> CLOSED
>
> And make sure that all closed zones are counted as the initial number of active
> zones. The initial number of open zones will always be 0.
>
> So it is easy :)
Thanks for the explanations!
Read-only and offline are device internal events. Does qcow2 emulation
need to emulate that?
Current NVMe ZNS emulation in QEMU has a nvme_offline_zone() function.
Does it suggest keeping the offline state persistent?
https://github.com/qemu/qemu/blob/master/hw/nvme/ctrl.c#L3740
Sam
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/4] qcow2: add zoned emulation capability
2023-08-29 6:27 ` Sam Li
@ 2023-08-29 7:14 ` Damien Le Moal
2023-08-29 7:27 ` Sam Li
0 siblings, 1 reply; 26+ messages in thread
From: Damien Le Moal @ 2023-08-29 7:14 UTC (permalink / raw)
To: Sam Li
Cc: Stefan Hajnoczi, qemu-devel, hare, Hanna Reitz, dmitry.fomichev,
qemu-block, Kevin Wolf, Markus Armbruster, Eric Blake
On 8/29/23 15:27, Sam Li wrote:
> Damien Le Moal <dlemoal@kernel.org> 于2023年8月29日周二 14:06写道：
>>
>> On 8/28/23 20:55, Sam Li wrote:
>>>>> + /* close one implicitly open zones to make it available */
>>>>> + for (int i = s->zoned_header.zone_nr_conv;
>>>>> + i < bs->bl.nr_zones; ++i) {
>>>>> + uint64_t *wp = &s->wps->wp[i];
>>>>> + if (qcow2_get_zs(*wp) == BLK_ZS_IOPEN) {
>>>>> + ret = qcow2_write_wp_at(bs, wp, i, BLK_ZS_CLOSED);
>>>>
>>>> I'm wondering if it's correct to store the zone state persistently in
>>>> the qcow2 file. If the guest or QEMU crashes, then zones will be left in
>>>> states like EOPEN. Since the guest software will have forgotten about
>>>> explicitly opened zones, the guest would need to recover zone states.
>>>> I'm not sure if existing software is designed to do that.
>>>>
>>>> Damien: Should the zone state be persistent?
>>
>> Yes and no. Yes you need to preserve/maintain zone states but not as is.
>> With a real drive, if you power cycle the device, you get the following states
>> changes:
>>
>> Before | After power cycle
>> ----------------+-------------------
>> EMPTY | EMPTY
>> FULL | FULL
>> IMP. OPEN | CLOSED
>> EXP. OPEN | CLOSED
>> CLOSED | CLOSED
>> READ=ONLY | READ-ONLY
>> OFFLINE | OFFLINE
>>
>> So any open (implicit or explicit) zone will show up as closed after power
>> cycle. That is, the number of "active" zones does not change.
>> For the qcow2 emulation, as long as you do not also emulate read-only and
>> offline zones, you actually do not need to save the zone state in the zone
>> metadata. On startup, you can infer the state from the zone write pointer:
>>
>> zone wp == zone start -> EMPTY
>> zone wp >= zone capacity -> FULL
>> zone wp > zone start -> CLOSED
>>
>> And make sure that all closed zones are counted as the initial number of active
>> zones. The initial number of open zones will always be 0.
>>
>> So it is easy :)
>
> Thanks for the explanations!
>
> Read-only and offline are device internal events. Does qcow2 emulation
> need to emulate that?
>
> Current NVMe ZNS emulation in QEMU has a nvme_offline_zone() function.
> Does it suggest keeping the offline state persistent?
> https://github.com/qemu/qemu/blob/master/hw/nvme/ctrl.c#L3740
The offline state is useful for testing only. If a zone goes offline, it
generally means that the device is dying...
At least for now, I do not think it is needed for qcow2. That can always be
added later.
>
> Sam
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/4] qcow2: add zoned emulation capability
2023-08-29 7:14 ` Damien Le Moal
@ 2023-08-29 7:27 ` Sam Li
0 siblings, 0 replies; 26+ messages in thread
From: Sam Li @ 2023-08-29 7:27 UTC (permalink / raw)
To: Damien Le Moal
Cc: Stefan Hajnoczi, qemu-devel, hare, Hanna Reitz, dmitry.fomichev,
qemu-block, Kevin Wolf, Markus Armbruster, Eric Blake
Damien Le Moal <dlemoal@kernel.org> 于2023年8月29日周二 15:14写道：
>
> On 8/29/23 15:27, Sam Li wrote:
> > Damien Le Moal <dlemoal@kernel.org> 于2023年8月29日周二 14:06写道：
> >>
> >> On 8/28/23 20:55, Sam Li wrote:
> >>>>> + /* close one implicitly open zones to make it available */
> >>>>> + for (int i = s->zoned_header.zone_nr_conv;
> >>>>> + i < bs->bl.nr_zones; ++i) {
> >>>>> + uint64_t *wp = &s->wps->wp[i];
> >>>>> + if (qcow2_get_zs(*wp) == BLK_ZS_IOPEN) {
> >>>>> + ret = qcow2_write_wp_at(bs, wp, i, BLK_ZS_CLOSED);
> >>>>
> >>>> I'm wondering if it's correct to store the zone state persistently in
> >>>> the qcow2 file. If the guest or QEMU crashes, then zones will be left in
> >>>> states like EOPEN. Since the guest software will have forgotten about
> >>>> explicitly opened zones, the guest would need to recover zone states.
> >>>> I'm not sure if existing software is designed to do that.
> >>>>
> >>>> Damien: Should the zone state be persistent?
> >>
> >> Yes and no. Yes you need to preserve/maintain zone states but not as is.
> >> With a real drive, if you power cycle the device, you get the following states
> >> changes:
> >>
> >> Before | After power cycle
> >> ----------------+-------------------
> >> EMPTY | EMPTY
> >> FULL | FULL
> >> IMP. OPEN | CLOSED
> >> EXP. OPEN | CLOSED
> >> CLOSED | CLOSED
> >> READ=ONLY | READ-ONLY
> >> OFFLINE | OFFLINE
> >>
> >> So any open (implicit or explicit) zone will show up as closed after power
> >> cycle. That is, the number of "active" zones does not change.
> >> For the qcow2 emulation, as long as you do not also emulate read-only and
> >> offline zones, you actually do not need to save the zone state in the zone
> >> metadata. On startup, you can infer the state from the zone write pointer:
> >>
> >> zone wp == zone start -> EMPTY
> >> zone wp >= zone capacity -> FULL
> >> zone wp > zone start -> CLOSED
> >>
> >> And make sure that all closed zones are counted as the initial number of active
> >> zones. The initial number of open zones will always be 0.
> >>
> >> So it is easy :)
> >
> > Thanks for the explanations!
> >
> > Read-only and offline are device internal events. Does qcow2 emulation
> > need to emulate that?
> >
> > Current NVMe ZNS emulation in QEMU has a nvme_offline_zone() function.
> > Does it suggest keeping the offline state persistent?
> > https://github.com/qemu/qemu/blob/master/hw/nvme/ctrl.c#L3740
>
> The offline state is useful for testing only. If a zone goes offline, it
> generally means that the device is dying...
> At least for now, I do not think it is needed for qcow2. That can always be
> added later.
Ok. Then the wps of zoned metadata structure would be almost like
zoned emulation in file-posix. Current wp design can still preserve as
is. Though, it will be only in memory then.
This change will be reflected in v4 (newest v3 for now).
Sam
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 4/4] iotests: test the zoned format feature for qcow2 file
2023-08-14 8:57 [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver Sam Li
` (2 preceding siblings ...)
2023-08-14 8:58 ` [PATCH v2 3/4] qcow2: add zoned emulation capability Sam Li
@ 2023-08-14 8:58 ` Sam Li
2023-08-22 19:50 ` Stefan Hajnoczi
2023-08-16 7:37 ` [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver Klaus Jensen
4 siblings, 1 reply; 26+ messages in thread
From: Sam Li @ 2023-08-14 8:58 UTC (permalink / raw)
To: qemu-devel
Cc: hare, Hanna Reitz, dmitry.fomichev, qemu-block, Kevin Wolf,
Markus Armbruster, Eric Blake, stefanha, dlemoal, Sam Li
The zoned format feature can be tested by:
$ tests/qemu-iotests/check zoned-qcow2
Signed-off-by: Sam Li <faithilikerun@gmail.com>
---
tests/qemu-iotests/tests/zoned-qcow2 | 135 ++++++++++++++++++++++
tests/qemu-iotests/tests/zoned-qcow2.out | 140 +++++++++++++++++++++++
2 files changed, 275 insertions(+)
create mode 100755 tests/qemu-iotests/tests/zoned-qcow2
create mode 100644 tests/qemu-iotests/tests/zoned-qcow2.out
diff --git a/tests/qemu-iotests/tests/zoned-qcow2 b/tests/qemu-iotests/tests/zoned-qcow2
new file mode 100755
index 0000000000..473b462b50
--- /dev/null
+++ b/tests/qemu-iotests/tests/zoned-qcow2
@@ -0,0 +1,135 @@
+#!/usr/bin/env bash
+#
+# Test zone management operations for qcow2 file.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+status=1 # failure is the default!
+
+file_name="zbc.qcow2"
+_cleanup()
+{
+ _cleanup_test_img
+ _rm_test_img "$file_name"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ../common.rc
+. ../common.filter
+. ../common.qemu
+
+# This test only runs on Linux hosts with qcow2 image files.
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+echo
+echo "=== Initial image setup ==="
+echo
+
+$QEMU_IMG create -f qcow2 $file_name -o size=768M -o zone_size=64M \
+-o zone_capacity=64M -o zone_nr_conv=0 -o max_append_sectors=131072 \
+-o max_open_zones=0 -o max_active_zones=0 -o zoned_profile=zbc
+
+IMG="--image-opts -n driver=qcow2,file.driver=file,file.filename=$file_name"
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+echo
+echo "=== Testing a qcow2 img with zoned format ==="
+echo
+echo "case 1: test if one zone operation works"
+
+echo "(1) report zones[0]:"
+$QEMU_IO $IMG -c "zrp 0 1"
+echo
+echo "report zones[0~9]:"
+$QEMU_IO $IMG -c "zrp 0 10"
+echo
+echo "report the last zone:"
+$QEMU_IO $IMG -c "zrp 0x2C000000 2" # 0x2C000000 / 512 = 0x160000
+echo
+echo
+echo "open zones[0]:"
+$QEMU_IO $IMG -c "zo 0 0x4000000" # 0x4000000 / 512 = 0x20000
+$QEMU_IO $IMG -c "zrp 0 1"
+echo
+echo "open zones[1]"
+$QEMU_IO $IMG -c "zo 0x4000000 0x4000000"
+$QEMU_IO $IMG -c "zrp 0x4000000 1"
+echo
+echo "open the last zone"
+$QEMU_IO $IMG -c "zo 0x2C000000 0x4000000"
+$QEMU_IO $IMG -c "zrp 0x2C000000 2"
+echo
+echo
+echo "close zones[0]"
+$QEMU_IO $IMG -c "zc 0 0x4000000"
+$QEMU_IO $IMG -c "zrp 0 1"
+echo
+echo "close the last zone"
+$QEMU_IO $IMG -c "zc 0x3e70000000 0x4000000"
+$QEMU_IO $IMG -c "zrp 0x3e70000000 2"
+echo
+echo
+echo "(4) finish zones[1]"
+$QEMU_IO $IMG -c "zf 0x4000000 0x4000000"
+$QEMU_IO $IMG -c "zrp 0x4000000 1"
+echo
+echo
+echo "(5) reset zones[1]"
+$QEMU_IO $IMG -c "zrs 0x4000000 0x4000000"
+$QEMU_IO $IMG -c "zrp 0x4000000 1"
+echo
+echo
+echo "(6) append write with (4k, 8k) data" # the physical block size of the device is 4096
+$QEMU_IO $IMG -c "zrp 0 12"
+echo "Append write zones[0] one time:"
+$QEMU_IO $IMG -c "zap -p 0 0x1000 0x2000"
+$QEMU_IO $IMG -c "zrp 0 1"
+echo
+echo "Append write zones[0] twice:"
+$QEMU_IO $IMG -c "zap -p 0 0x1000 0x2000"
+$QEMU_IO $IMG -c "zrp 0 1"
+echo
+echo "Append write zones[1] one time:"
+$QEMU_IO $IMG -c "zap -p 0x4000000 0x1000 0x2000"
+$QEMU_IO $IMG -c "zrp 0x4000000 1"
+echo
+echo "Append write zones[1] twice:"
+$QEMU_IO $IMG -c "zap -p 0x4000000 0x1000 0x2000"
+$QEMU_IO $IMG -c "zrp 0x4000000 1"
+echo
+echo "Reset all:"
+$QEMU_IO $IMG -c "zrs 0 768M"
+$QEMU_IO $IMG -c "zrp 0 12"
+echo
+echo
+echo "case 2: test a sets of ops that works or not"
+
+echo "(1) append write (4k, 4k) and then write to full"
+$QEMU_IO $IMG -c "zap -p 0 0x1000 0x1000"
+echo "wrote (4k, 4k):"
+$QEMU_IO $IMG -c "zrp 0 1"
+$QEMU_IO $IMG -c "zap -p 0 0x1000 0x3ffd000"
+echo "wrote to full:"
+$QEMU_IO $IMG -c "zrp 0 1"
+echo "Reset zones[0]:"
+$QEMU_IO $IMG -c "zrs 0 64M"
+$QEMU_IO $IMG -c "zrp 0 1"
+
+echo "(2) write in zones[0], zones[3], zones[8], and then reset all"
+$QEMU_IO $IMG -c "zap -p 0 0x1000 0x1000"
+$QEMU_IO $IMG -c "zap -p 0xc000000 0x1000 0x1000"
+$QEMU_IO $IMG -c "zap -p 0x20000000 0x1000 0x1000"
+echo "wrote three zones:"
+$QEMU_IO $IMG -c "zrp 0 12"
+echo "Reset all:"
+$QEMU_IO $IMG -c "zrs 0 768M"
+$QEMU_IO $IMG -c "zrp 0 12"
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/tests/zoned-qcow2.out b/tests/qemu-iotests/tests/zoned-qcow2.out
new file mode 100644
index 0000000000..0a6b21cb55
--- /dev/null
+++ b/tests/qemu-iotests/tests/zoned-qcow2.out
@@ -0,0 +1,140 @@
+QA output created by zoned-qcow2
+
+=== Initial image setup ===
+
+Formatting 'zbc.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off compression_type=zlib zoned_profile=zbc zone_size=67108864 zone_capacity=67108864 zone_nr_conv=0 max_append_sectors=131072 max_active_zones=0 max_open_zones=0 size=805306368 lazy_refcounts=off refcount_bits=16
+
+=== Testing a qcow2 img with zoned format ===
+
+case 1: test if one zone operation works
+(1) report zones[0]:
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x0, zcond:1, [type: 2]
+
+report zones[0~9]:
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x0, zcond:1, [type: 2]
+start: 0x20000, len 0x20000, cap 0x20000, wptr 0x20000, zcond:1, [type: 2]
+start: 0x40000, len 0x20000, cap 0x20000, wptr 0x40000, zcond:1, [type: 2]
+start: 0x60000, len 0x20000, cap 0x20000, wptr 0x60000, zcond:1, [type: 2]
+start: 0x80000, len 0x20000, cap 0x20000, wptr 0x80000, zcond:1, [type: 2]
+start: 0xa0000, len 0x20000, cap 0x20000, wptr 0xa0000, zcond:1, [type: 2]
+start: 0xc0000, len 0x20000, cap 0x20000, wptr 0xc0000, zcond:1, [type: 2]
+start: 0xe0000, len 0x20000, cap 0x20000, wptr 0xe0000, zcond:1, [type: 2]
+start: 0x100000, len 0x20000, cap 0x20000, wptr 0x100000, zcond:1, [type: 2]
+start: 0x120000, len 0x20000, cap 0x20000, wptr 0x120000, zcond:1, [type: 2]
+
+report the last zone:
+start: 0x160000, len 0x20000, cap 0x20000, wptr 0x160000, zcond:1, [type: 2]
+
+
+open zones[0]:
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x0, zcond:3, [type: 2]
+
+open zones[1]
+start: 0x20000, len 0x20000, cap 0x20000, wptr 0x20000, zcond:3, [type: 2]
+
+open the last zone
+start: 0x160000, len 0x20000, cap 0x20000, wptr 0x160000, zcond:3, [type: 2]
+
+
+close zones[0]
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x0, zcond:4, [type: 2]
+
+close the last zone
+zone close failed: Input/output error
+qemu-io: offset 268167020544 is equal to or greater than thedevice capacity 805306368
+zone report failed: Invalid argument
+
+
+(4) finish zones[1]
+start: 0x20000, len 0x20000, cap 0x20000, wptr 0x40000, zcond:14, [type: 2]
+
+
+(5) reset zones[1]
+start: 0x20000, len 0x20000, cap 0x20000, wptr 0x20000, zcond:1, [type: 2]
+
+
+(6) append write with (4k, 8k) data
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x0, zcond:4, [type: 2]
+start: 0x20000, len 0x20000, cap 0x20000, wptr 0x20000, zcond:1, [type: 2]
+start: 0x40000, len 0x20000, cap 0x20000, wptr 0x40000, zcond:1, [type: 2]
+start: 0x60000, len 0x20000, cap 0x20000, wptr 0x60000, zcond:1, [type: 2]
+start: 0x80000, len 0x20000, cap 0x20000, wptr 0x80000, zcond:1, [type: 2]
+start: 0xa0000, len 0x20000, cap 0x20000, wptr 0xa0000, zcond:1, [type: 2]
+start: 0xc0000, len 0x20000, cap 0x20000, wptr 0xc0000, zcond:1, [type: 2]
+start: 0xe0000, len 0x20000, cap 0x20000, wptr 0xe0000, zcond:1, [type: 2]
+start: 0x100000, len 0x20000, cap 0x20000, wptr 0x100000, zcond:1, [type: 2]
+start: 0x120000, len 0x20000, cap 0x20000, wptr 0x120000, zcond:1, [type: 2]
+start: 0x140000, len 0x20000, cap 0x20000, wptr 0x140000, zcond:1, [type: 2]
+start: 0x160000, len 0x20000, cap 0x20000, wptr 0x160000, zcond:3, [type: 2]
+Append write zones[0] one time:
+After zap done, the append sector is 0x0
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x18, zcond:2, [type: 2]
+
+Append write zones[0] twice:
+After zap done, the append sector is 0x18
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x30, zcond:2, [type: 2]
+
+Append write zones[1] one time:
+After zap done, the append sector is 0x20000
+start: 0x20000, len 0x20000, cap 0x20000, wptr 0x20018, zcond:2, [type: 2]
+
+Append write zones[1] twice:
+After zap done, the append sector is 0x20018
+start: 0x20000, len 0x20000, cap 0x20000, wptr 0x20030, zcond:2, [type: 2]
+
+Reset all:
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x0, zcond:1, [type: 2]
+start: 0x20000, len 0x20000, cap 0x20000, wptr 0x20000, zcond:1, [type: 2]
+start: 0x40000, len 0x20000, cap 0x20000, wptr 0x40000, zcond:1, [type: 2]
+start: 0x60000, len 0x20000, cap 0x20000, wptr 0x60000, zcond:1, [type: 2]
+start: 0x80000, len 0x20000, cap 0x20000, wptr 0x80000, zcond:1, [type: 2]
+start: 0xa0000, len 0x20000, cap 0x20000, wptr 0xa0000, zcond:1, [type: 2]
+start: 0xc0000, len 0x20000, cap 0x20000, wptr 0xc0000, zcond:1, [type: 2]
+start: 0xe0000, len 0x20000, cap 0x20000, wptr 0xe0000, zcond:1, [type: 2]
+start: 0x100000, len 0x20000, cap 0x20000, wptr 0x100000, zcond:1, [type: 2]
+start: 0x120000, len 0x20000, cap 0x20000, wptr 0x120000, zcond:1, [type: 2]
+start: 0x140000, len 0x20000, cap 0x20000, wptr 0x140000, zcond:1, [type: 2]
+start: 0x160000, len 0x20000, cap 0x20000, wptr 0x160000, zcond:1, [type: 2]
+
+
+case 2: test a sets of ops that works or not
+(1) append write (4k, 4k) and then write to full
+After zap done, the append sector is 0x0
+wrote (4k, 4k):
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x10, zcond:2, [type: 2]
+After zap done, the append sector is 0x10
+wrote to full:
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x20000, zcond:14, [type: 2]
+Reset zones[0]:
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x0, zcond:1, [type: 2]
+(2) write in zones[0], zones[3], zones[8], and then reset all
+After zap done, the append sector is 0x0
+After zap done, the append sector is 0x60000
+After zap done, the append sector is 0x100000
+wrote three zones:
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x10, zcond:2, [type: 2]
+start: 0x20000, len 0x20000, cap 0x20000, wptr 0x20000, zcond:1, [type: 2]
+start: 0x40000, len 0x20000, cap 0x20000, wptr 0x40000, zcond:1, [type: 2]
+start: 0x60000, len 0x20000, cap 0x20000, wptr 0x60010, zcond:2, [type: 2]
+start: 0x80000, len 0x20000, cap 0x20000, wptr 0x80000, zcond:1, [type: 2]
+start: 0xa0000, len 0x20000, cap 0x20000, wptr 0xa0000, zcond:1, [type: 2]
+start: 0xc0000, len 0x20000, cap 0x20000, wptr 0xc0000, zcond:1, [type: 2]
+start: 0xe0000, len 0x20000, cap 0x20000, wptr 0xe0000, zcond:1, [type: 2]
+start: 0x100000, len 0x20000, cap 0x20000, wptr 0x100010, zcond:2, [type: 2]
+start: 0x120000, len 0x20000, cap 0x20000, wptr 0x120000, zcond:1, [type: 2]
+start: 0x140000, len 0x20000, cap 0x20000, wptr 0x140000, zcond:1, [type: 2]
+start: 0x160000, len 0x20000, cap 0x20000, wptr 0x160000, zcond:1, [type: 2]
+Reset all:
+start: 0x0, len 0x20000, cap 0x20000, wptr 0x0, zcond:1, [type: 2]
+start: 0x20000, len 0x20000, cap 0x20000, wptr 0x20000, zcond:1, [type: 2]
+start: 0x40000, len 0x20000, cap 0x20000, wptr 0x40000, zcond:1, [type: 2]
+start: 0x60000, len 0x20000, cap 0x20000, wptr 0x60000, zcond:1, [type: 2]
+start: 0x80000, len 0x20000, cap 0x20000, wptr 0x80000, zcond:1, [type: 2]
+start: 0xa0000, len 0x20000, cap 0x20000, wptr 0xa0000, zcond:1, [type: 2]
+start: 0xc0000, len 0x20000, cap 0x20000, wptr 0xc0000, zcond:1, [type: 2]
+start: 0xe0000, len 0x20000, cap 0x20000, wptr 0xe0000, zcond:1, [type: 2]
+start: 0x100000, len 0x20000, cap 0x20000, wptr 0x100000, zcond:1, [type: 2]
+start: 0x120000, len 0x20000, cap 0x20000, wptr 0x120000, zcond:1, [type: 2]
+start: 0x140000, len 0x20000, cap 0x20000, wptr 0x140000, zcond:1, [type: 2]
+start: 0x160000, len 0x20000, cap 0x20000, wptr 0x160000, zcond:1, [type: 2]
+*** done
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 4/4] iotests: test the zoned format feature for qcow2 file
2023-08-14 8:58 ` [PATCH v2 4/4] iotests: test the zoned format feature for qcow2 file Sam Li
@ 2023-08-22 19:50 ` Stefan Hajnoczi
0 siblings, 0 replies; 26+ messages in thread
From: Stefan Hajnoczi @ 2023-08-22 19:50 UTC (permalink / raw)
To: Sam Li
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake, dlemoal
[-- Attachment #1: Type: text/plain, Size: 573 bytes --]
On Mon, Aug 14, 2023 at 04:58:02PM +0800, Sam Li wrote:
> The zoned format feature can be tested by:
> $ tests/qemu-iotests/check zoned-qcow2
>
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
> tests/qemu-iotests/tests/zoned-qcow2 | 135 ++++++++++++++++++++++
> tests/qemu-iotests/tests/zoned-qcow2.out | 140 +++++++++++++++++++++++
> 2 files changed, 275 insertions(+)
> create mode 100755 tests/qemu-iotests/tests/zoned-qcow2
> create mode 100644 tests/qemu-iotests/tests/zoned-qcow2.out
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver
2023-08-14 8:57 [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver Sam Li
` (3 preceding siblings ...)
2023-08-14 8:58 ` [PATCH v2 4/4] iotests: test the zoned format feature for qcow2 file Sam Li
@ 2023-08-16 7:37 ` Klaus Jensen
2023-08-16 8:14 ` Sam Li
4 siblings, 1 reply; 26+ messages in thread
From: Klaus Jensen @ 2023-08-16 7:37 UTC (permalink / raw)
To: Sam Li
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake, stefanha, dlemoal
[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]
On Aug 14 16:57, Sam Li wrote:
> This patch series add a new extension - zoned format - to the
> qcow2 driver thereby allowing full zoned storage emulation on
> the qcow2 img file. Users can attach such a qcow2 file to the
> guest as a zoned device.
>
> To create a qcow2 file with zoned format, use command like this:
> $ qemu-img create -f qcow2 test.qcow2 -o size=768M -o
> zone_size=64M -o zone_capacity=64M -o zone_nr_conv=0 -o
> max_append_sectors=512 -o max_open_zones=0 -o max_active_zones=0
> -o zoned_profile=zbc
>
> Then add it to the QEMU command line:
> -blockdev node-name=drive1,driver=qcow2,file.driver=file,file.filename=../qemu/test.qcow2 \
> -device virtio-blk-pci,drive=drive1 \
>
> v1->v2:
> - add more tests to qemu-io zoned commands
> - make zone append change state to full when wp reaches end
> - add documentation to qcow2 zoned extension header
> - address review comments (Stefan):
> * fix zoned_mata allocation size
> * use bitwise or than addition
> * fix wp index overflow and locking
> * cleanups: comments, naming
>
> Sam Li (4):
> docs/qcow2: add the zoned format feature
> qcow2: add configurations for zoned format extension
> qcow2: add zoned emulation capability
> iotests: test the zoned format feature for qcow2 file
>
> block/qcow2.c | 799 ++++++++++++++++++++++-
> block/qcow2.h | 23 +
> docs/interop/qcow2.txt | 26 +
> docs/system/qemu-block-drivers.rst.inc | 39 ++
> include/block/block-common.h | 5 +
> include/block/block_int-common.h | 16 +
> qapi/block-core.json | 46 +-
> tests/qemu-iotests/tests/zoned-qcow2 | 135 ++++
> tests/qemu-iotests/tests/zoned-qcow2.out | 140 ++++
> 9 files changed, 1214 insertions(+), 15 deletions(-)
> create mode 100755 tests/qemu-iotests/tests/zoned-qcow2
> create mode 100644 tests/qemu-iotests/tests/zoned-qcow2.out
>
Hi Sam,
Thanks for this and for the RFC for hw/nvme - this is an awesome
improvement.
Can you explain the need for the zoned_profile? I understand that only
ZNS requires potentially setting zone_capacity and configuring extended
descriptors. When an image is hooked up to a block emulation device that
doesnt understand cap < size or extended descriptors, it could just
would fail on the cap < size and just ignore the extended descriptor
space. Do we really need to add the complexity of the user explicitly
having to set the profile? I also think it is fair for the QEMU zoned
block api to accomodate both variations - if a particular configuration
is supported or not is up to the emulating device.
Checking the profile from hw/nvme or hw/block/virtio is the same as
checking if cap < size or possibly the presence of extended descriptors.
Thanks,
Klaus
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver
2023-08-16 7:37 ` [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver Klaus Jensen
@ 2023-08-16 8:14 ` Sam Li
2023-08-16 18:03 ` Stefan Hajnoczi
0 siblings, 1 reply; 26+ messages in thread
From: Sam Li @ 2023-08-16 8:14 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, hare, Hanna Reitz, dmitry.fomichev, qemu-block,
Kevin Wolf, Markus Armbruster, Eric Blake, stefanha, dlemoal
Klaus Jensen <its@irrelevant.dk> 于2023年8月16日周三 15:37写道：
>
> On Aug 14 16:57, Sam Li wrote:
> > This patch series add a new extension - zoned format - to the
> > qcow2 driver thereby allowing full zoned storage emulation on
> > the qcow2 img file. Users can attach such a qcow2 file to the
> > guest as a zoned device.
> >
> > To create a qcow2 file with zoned format, use command like this:
> > $ qemu-img create -f qcow2 test.qcow2 -o size=768M -o
> > zone_size=64M -o zone_capacity=64M -o zone_nr_conv=0 -o
> > max_append_sectors=512 -o max_open_zones=0 -o max_active_zones=0
> > -o zoned_profile=zbc
> >
> > Then add it to the QEMU command line:
> > -blockdev node-name=drive1,driver=qcow2,file.driver=file,file.filename=../qemu/test.qcow2 \
> > -device virtio-blk-pci,drive=drive1 \
> >
> > v1->v2:
> > - add more tests to qemu-io zoned commands
> > - make zone append change state to full when wp reaches end
> > - add documentation to qcow2 zoned extension header
> > - address review comments (Stefan):
> > * fix zoned_mata allocation size
> > * use bitwise or than addition
> > * fix wp index overflow and locking
> > * cleanups: comments, naming
> >
> > Sam Li (4):
> > docs/qcow2: add the zoned format feature
> > qcow2: add configurations for zoned format extension
> > qcow2: add zoned emulation capability
> > iotests: test the zoned format feature for qcow2 file
> >
> > block/qcow2.c | 799 ++++++++++++++++++++++-
> > block/qcow2.h | 23 +
> > docs/interop/qcow2.txt | 26 +
> > docs/system/qemu-block-drivers.rst.inc | 39 ++
> > include/block/block-common.h | 5 +
> > include/block/block_int-common.h | 16 +
> > qapi/block-core.json | 46 +-
> > tests/qemu-iotests/tests/zoned-qcow2 | 135 ++++
> > tests/qemu-iotests/tests/zoned-qcow2.out | 140 ++++
> > 9 files changed, 1214 insertions(+), 15 deletions(-)
> > create mode 100755 tests/qemu-iotests/tests/zoned-qcow2
> > create mode 100644 tests/qemu-iotests/tests/zoned-qcow2.out
> >
>
> Hi Sam,
>
> Thanks for this and for the RFC for hw/nvme - this is an awesome
> improvement.
>
> Can you explain the need for the zoned_profile? I understand that only
> ZNS requires potentially setting zone_capacity and configuring extended
> descriptors. When an image is hooked up to a block emulation device that
> doesnt understand cap < size or extended descriptors, it could just
> would fail on the cap < size and just ignore the extended descriptor
> space. Do we really need to add the complexity of the user explicitly
> having to set the profile? I also think it is fair for the QEMU zoned
> block api to accomodate both variations - if a particular configuration
> is supported or not is up to the emulating device.
>
> Checking the profile from hw/nvme or hw/block/virtio is the same as
> checking if cap < size or possibly the presence of extended descriptors.
Hi Klaus,
Thanks for your feedback.
The zoned_profile is for users to choose the emulating device type,
either zbc or zns. It implies using virtio-blk or nvme pass through.
The zoned block api does accommodate both variations. Since the cap <
size and extended descriptor config can also infer zoned_profile, this
option can be dropped. Then the device type is determined by the
configurations. When cap = size and no extended descriptor, the img
can be used both in virtio-blk and nvme zns depending on the QEMU
command line.
Best regards,
Sam
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver
2023-08-16 8:14 ` Sam Li
@ 2023-08-16 18:03 ` Stefan Hajnoczi
0 siblings, 0 replies; 26+ messages in thread
From: Stefan Hajnoczi @ 2023-08-16 18:03 UTC (permalink / raw)
To: Sam Li
Cc: Klaus Jensen, qemu-devel, hare, Hanna Reitz, dmitry.fomichev,
qemu-block, Kevin Wolf, Markus Armbruster, Eric Blake, dlemoal
[-- Attachment #1: Type: text/plain, Size: 3895 bytes --]
On Wed, Aug 16, 2023 at 04:14:08PM +0800, Sam Li wrote:
> Klaus Jensen <its@irrelevant.dk> 于2023年8月16日周三 15:37写道：
> >
> > On Aug 14 16:57, Sam Li wrote:
> > > This patch series add a new extension - zoned format - to the
> > > qcow2 driver thereby allowing full zoned storage emulation on
> > > the qcow2 img file. Users can attach such a qcow2 file to the
> > > guest as a zoned device.
> > >
> > > To create a qcow2 file with zoned format, use command like this:
> > > $ qemu-img create -f qcow2 test.qcow2 -o size=768M -o
> > > zone_size=64M -o zone_capacity=64M -o zone_nr_conv=0 -o
> > > max_append_sectors=512 -o max_open_zones=0 -o max_active_zones=0
> > > -o zoned_profile=zbc
> > >
> > > Then add it to the QEMU command line:
> > > -blockdev node-name=drive1,driver=qcow2,file.driver=file,file.filename=../qemu/test.qcow2 \
> > > -device virtio-blk-pci,drive=drive1 \
> > >
> > > v1->v2:
> > > - add more tests to qemu-io zoned commands
> > > - make zone append change state to full when wp reaches end
> > > - add documentation to qcow2 zoned extension header
> > > - address review comments (Stefan):
> > > * fix zoned_mata allocation size
> > > * use bitwise or than addition
> > > * fix wp index overflow and locking
> > > * cleanups: comments, naming
> > >
> > > Sam Li (4):
> > > docs/qcow2: add the zoned format feature
> > > qcow2: add configurations for zoned format extension
> > > qcow2: add zoned emulation capability
> > > iotests: test the zoned format feature for qcow2 file
> > >
> > > block/qcow2.c | 799 ++++++++++++++++++++++-
> > > block/qcow2.h | 23 +
> > > docs/interop/qcow2.txt | 26 +
> > > docs/system/qemu-block-drivers.rst.inc | 39 ++
> > > include/block/block-common.h | 5 +
> > > include/block/block_int-common.h | 16 +
> > > qapi/block-core.json | 46 +-
> > > tests/qemu-iotests/tests/zoned-qcow2 | 135 ++++
> > > tests/qemu-iotests/tests/zoned-qcow2.out | 140 ++++
> > > 9 files changed, 1214 insertions(+), 15 deletions(-)
> > > create mode 100755 tests/qemu-iotests/tests/zoned-qcow2
> > > create mode 100644 tests/qemu-iotests/tests/zoned-qcow2.out
> > >
> >
> > Hi Sam,
> >
> > Thanks for this and for the RFC for hw/nvme - this is an awesome
> > improvement.
> >
> > Can you explain the need for the zoned_profile? I understand that only
> > ZNS requires potentially setting zone_capacity and configuring extended
> > descriptors. When an image is hooked up to a block emulation device that
> > doesnt understand cap < size or extended descriptors, it could just
> > would fail on the cap < size and just ignore the extended descriptor
> > space. Do we really need to add the complexity of the user explicitly
> > having to set the profile? I also think it is fair for the QEMU zoned
> > block api to accomodate both variations - if a particular configuration
> > is supported or not is up to the emulating device.
> >
> > Checking the profile from hw/nvme or hw/block/virtio is the same as
> > checking if cap < size or possibly the presence of extended descriptors.
>
> Hi Klaus,
>
> Thanks for your feedback.
>
> The zoned_profile is for users to choose the emulating device type,
> either zbc or zns. It implies using virtio-blk or nvme pass through.
> The zoned block api does accommodate both variations. Since the cap <
> size and extended descriptor config can also infer zoned_profile, this
> option can be dropped. Then the device type is determined by the
> configurations. When cap = size and no extended descriptor, the img
> can be used both in virtio-blk and nvme zns depending on the QEMU
> command line.
Dropping zoned_profile would be a nice simplification.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread