[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension
|
From: |
Stefan Hajnoczi |
|
Subject: |
Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension |
|
Date: |
Wed, 16 Aug 2023 15:31:07 -0400 |
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
>
signature.asc
Description: PGP signature
- [PATCH v2 0/4] Add full zoned storage emulation to qcow2 driver, Sam Li, 2023/08/14
- [PATCH v2 1/4] docs/qcow2: add the zoned format feature, Sam Li, 2023/08/14
- [PATCH v2 2/4] qcow2: add configurations for zoned format extension, Sam Li, 2023/08/14
- Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension,
Stefan Hajnoczi <=
- Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension, Markus Armbruster, 2023/08/21
- Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension, Stefan Hajnoczi, 2023/08/21
- Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension, Sam Li, 2023/08/28
- Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension, Damien Le Moal, 2023/08/28
- Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension, Sam Li, 2023/08/28
- Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension, Damien Le Moal, 2023/08/28
- Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension, Sam Li, 2023/08/28
- Re: [PATCH v2 2/4] qcow2: add configurations for zoned format extension, Sam Li, 2023/08/28
[PATCH v2 3/4] qcow2: add zoned emulation capability, Sam Li, 2023/08/14