From: Stefan Hajnoczi <stefanha@redhat.com>
To: Sam Li <faithilikerun@gmail.com>
Cc: qemu-devel@nongnu.org, dlemoal@kernel.org,
dmitry.fomichev@wdc.com, hare@suse.de,
Markus Armbruster <armbru@redhat.com>,
Kevin Wolf <kwolf@redhat.com>,
qemu-block@nongnu.org, Eric Blake <eblake@redhat.com>,
Hanna Reitz <hreitz@redhat.com>
Subject: Re: [RFC 2/4] qcow2: add configurations for zoned format extension
Date: Mon, 19 Jun 2023 12:10:31 +0200 [thread overview]
Message-ID: <20230619101031.GC2497337@fedora> (raw)
In-Reply-To: <20230605104108.125270-3-faithilikerun@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 17507 bytes --]
On Mon, Jun 05, 2023 at 06:41:06PM +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).
>
> 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
>
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
> block/qcow2.c | 119 +++++++++++++++++++++++++++++++
> block/qcow2.h | 21 ++++++
> include/block/block-common.h | 5 ++
> include/block/block_int-common.h | 8 +++
> qapi/block-core.json | 46 ++++++++----
> 5 files changed, 185 insertions(+), 14 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7f3948360d..b886dab42b 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,37 @@ 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.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;
Please validate these values. The image file is not trusted and may be
broken/corrupt. For example, zone_size=0 and nr_zones=0 must be rejected
because the code can't do anything useful when these values are zero
(similar for values that are not multiples of the block 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
> @@ -3071,6 +3104,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);
> @@ -3755,6 +3813,18 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
> s->image_data_file = g_strdup(data_bs->filename);
> }
>
> + if (!strcmp(qcow2_opts->zoned_profile, "zbc")) {
> + BDRVQcow2State *s = blk_bs(blk)->opaque;
> + s->zoned_header.zoned_profile = BLK_ZP_ZBC;
> + s->zoned_header.zoned = BLK_Z_HM;
> + s->zoned_header.zone_size = qcow2_opts->zone_size;
> + s->zoned_header.zone_capacity = qcow2_opts->zone_capacity;
> + 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;
Where is the presence of these optional qcow2_opts checked? For example,
if the user didn't specify zone_size, then they cannot create an image
with a zoned profile.
These options also need to be validated to ensure that they contain
reasonable values (e.g. not 0).
> + }
> +
> /* Create a full header (including things like feature table) */
> ret = qcow2_update_header(blk_bs(blk));
> bdrv_graph_co_rdunlock();
> @@ -3873,6 +3943,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);
> + }
What is the purpose of this code, it fetches and replaces the same qdict
element?
> +
> /* Change legacy command line options into QMP ones */
> static const QDictRenames opt_renames[] = {
> { BLOCK_OPT_BACKING_FILE, "backing-file" },
> @@ -3885,6 +3962,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 },
> };
>
> @@ -6048,6 +6132,41 @@ static QemuOptsList qcow2_create_opts = {
> .help = "Compression method used for image cluster " \
> "compression", \
> .def_value_str = "zlib" \
> + }, \
> + {
The forward slash ('\') that wraps the line is missing and indentation
is off.
> + .name = BLOCK_OPT_Z_PROFILE, \
> + .type = QEMU_OPT_STRING, \
> + .help = "zoned format option for the disk img", \
> + }, \
> + { \
Indentation is off.
> + .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.
> + .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 4f67eb912a..fe18dc4d97 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -235,6 +235,20 @@ typedef struct Qcow2CryptoHeaderExtension {
> uint64_t length;
> } QEMU_PACKED Qcow2CryptoHeaderExtension;
>
> +typedef struct Qcow2ZonedHeaderExtension {
> + /* Zoned device attributes */
> + BlockZonedProfile zoned_profile;
> + BlockZoneModel zoned;
> + 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;
> + uint8_t padding[3];
This looks strange. Why is there 3 bytes of padding at the end? Normally
padding would align to an even power-of-two number of bytes like 2, 4,
8, etc.
> +} QEMU_PACKED Qcow2ZonedHeaderExtension;
> +
> typedef struct Qcow2UnknownHeaderExtension {
> uint32_t magic;
> uint32_t len;
> @@ -419,6 +433,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;
Normally qcow2 code passes bs around, so it should be possible to access
the wps pointer without duplicating it here. This new field is not used
in this patch, so I can't tell yet how important it is. It's safer to
avoid duplicating pointers when the original pointer can be accessed
conveniently so that use-after-free, double-free, and similar memory
management bugs can be eliminated.
> } BDRVQcow2State;
>
> typedef struct Qcow2COWRegion {
> 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..4be35feaf8 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
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4bf89171c6..f9a584cbb3 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5013,24 +5013,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 8.1.
> #
> # 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 --]
next prev parent reply other threads:[~2023-06-19 10:11 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-05 10:41 [RFC 0/4] Add full zoned storage emulation to qcow2 driver Sam Li
2023-06-05 10:41 ` [RFC 1/4] docs/qcow2: add the zoned format feature Sam Li
2023-06-12 4:10 ` Stefan Hajnoczi
2023-06-13 8:01 ` Sam Li
2023-06-05 10:41 ` [RFC 2/4] qcow2: add configurations for zoned format extension Sam Li
2023-06-05 14:50 ` Eric Blake
2023-06-19 10:10 ` Stefan Hajnoczi [this message]
2023-06-19 10:32 ` Sam Li
2023-06-19 14:42 ` Stefan Hajnoczi
2023-06-19 14:50 ` Sam Li
2023-06-20 14:44 ` Stefan Hajnoczi
2023-06-20 15:07 ` Sam Li
2023-06-05 10:41 ` [RFC 3/4] qcow2: add zoned emulation capability Sam Li
2023-06-19 14:35 ` Stefan Hajnoczi
2023-06-05 10:41 ` [RFC 4/4] iotests: test the zoned format feature for qcow2 file Sam Li
2023-06-19 14:40 ` Stefan Hajnoczi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230619101031.GC2497337@fedora \
--to=stefanha@redhat.com \
--cc=armbru@redhat.com \
--cc=dlemoal@kernel.org \
--cc=dmitry.fomichev@wdc.com \
--cc=eblake@redhat.com \
--cc=faithilikerun@gmail.com \
--cc=hare@suse.de \
--cc=hreitz@redhat.com \
--cc=kwolf@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.