[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC 2/4] qcow2: add configurations for zoned format extension
|
From: |
Sam Li |
|
Subject: |
Re: [RFC 2/4] qcow2: add configurations for zoned format extension |
|
Date: |
Mon, 19 Jun 2023 18:32:52 +0800 |
Stefan Hajnoczi <stefanha@redhat.com> 于2023年6月19日周一 18:10写道：
>
> 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?
It creates a string configuration for zoned_profile and matches the
user input to that config.
>
> > +
> > /* 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.
It is calculated as 3 if sizeof(zoned+zoned_profile) = 8. Else if it's
16, the padding is 2.
>
> > +} 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.
I see. Thanks!
>
> > } 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
> >
[RFC 3/4] qcow2: add zoned emulation capability, Sam Li, 2023/06/05
[RFC 4/4] iotests: test the zoned format feature for qcow2 file, Sam Li, 2023/06/05