Re: [PATCH v3 2/4] qcow2: add configurations for zoned format extension

[Markus Armbruster <armbru@redhat.com>]

Sam Li <faithilikerun@gmail.com> writes:

> Markus Armbruster <armbru@redhat.com> 于2023年9月1日周五 19:08写道：
>>
>> 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 profile" is gone in v3.
>>
>> > zone 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 nr_conv_zones=0 -o
>> > max_append_sectors=512 -o max_open_zones=0 -o max_active_zones=0
>> > -o zone_model=1
>> >
>> > Signed-off-by: Sam Li <faithilikerun@gmail.com>
>>
>> [...]
>>
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index 2b1d493d6e..0d8f9e0a88 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -5021,6 +5021,27 @@
>> >  # @compression-type: The image cluster compression method
>> >  #     (default: zlib, since 5.1)
>> >  #
>> > +# @zone-model: Zoned device model, 1 for host-managed and 0 for
>>
>> Why is this encoded as a number?
>>
>> If it's fundamentally a flag, use bool.
>>
>> If more models could appear in the future, make it an enum.
>>
>
> Yes, it is an enum.
>
> typedef enum BlockZoneModel {
>     BLK_Z_NONE = 0x0, /* Regular block device */
>     BLK_Z_HM = 0x1, /* Host-managed zoned block device */
>     BLK_Z_HA = 0x2, /* Host-aware zoned block device */
> } BlockZoneModel;

Please make it an enum in the QAPI schema, too.

>> > +#     non-zoned devices (default: 0, since 8.0)
>>
>> Since 8.2.  More of the same below.
>>
>> > +#
>> > +# @zone-size: Total number of logical blocks within zones in bytes
>> > +#     (since 8.0)
>> > +#
>> > +# @zone-capacity: The number of usable logical blocks within zones
>> > +#     in bytes. A zone capacity is always smaller or equal to the
>> > +#     zone size. (since 8.0)
>>
>> Two spaces between sentences for consistency, please.
>>
>> > +#
>> > +# @nr-conv-zones: The number of conventional zones of the zoned device
>> > +#     (since 8.0)
>>
>> I think @conventional-zones would be more obvious.
>>
>> > +#
>> > +# @max-open-zones: The maximal allowed open zones (since 8.0)
>>
>> Maybe "The maximum number of open zones".
>>
>> > +#
>> > +# @max-active-zones: The limit of the zones that have the implicit
>> > +#     open, explicit open or closed state (since 8.0)
>>
>> Maybe "The maximum number of zones in the implicit open, explicit open
>> or closed state".
>>
>> > +#
>> > +# @max-append-sectors: The maximal data size in sectors of a zone
>> > +#     append request that can be issued to the device. (since 8.0)
>>
>> What's the sector size, and how can the user determine it?  Why can't we
>> use bytes here?
>
> The sector size is 512 bytes.

Needs to be documented.

I believe bytes would be easier to document, which makes me suspect
they'd be the simpler interface.

>                               It's more for conventional use.

I'm afraid I don't understand this part.  Do I have to?

>> > +#
>> >  # Since: 2.12
>> >  ##
>> >  { 'struct': 'BlockdevCreateOptionsQcow2',
>> > @@ -5037,7 +5058,14 @@
>> >              '*preallocation':   'PreallocMode',
>> >              '*lazy-refcounts':  'bool',
>> >              '*refcount-bits':   'int',
>> > -            '*compression-type':'Qcow2CompressionType' } }
>> > +            '*compression-type':'Qcow2CompressionType',
>> > +            '*zone-model':         'uint8',
>> > +            '*zone-size':          'size',
>> > +            '*zone-capacity':      'size',
>> > +            '*nr-conv-zones':      'uint32',
>> > +            '*max-open-zones':     'uint32',
>> > +            '*max-active-zones':   'uint32',
>> > +            '*max-append-sectors': 'uint32' } }
>> >
>> >  ##
>> >  # @BlockdevCreateOptionsQed:
>>