Re: [PATCH v9 1/7] include: add zoned device structs

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 9/19/22 09:50, Sam Li wrote:
> Stefan Hajnoczi <stefanha@redhat.com> 于2022年9月18日周日 04:17写道：
>>
>> On Thu, Sep 15, 2022 at 06:06:38PM +0800, Sam Li wrote:
>>> Eric Blake <eblake@redhat.com> 于2022年9月15日周四 16:05写道：
>>>>
>>>> On Sat, Sep 10, 2022 at 01:27:53PM +0800, Sam Li wrote:
>>>>> Signed-off-by: Sam Li <faithilikerun@gmail.com>
>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>>>> ---
>>>>>  include/block/block-common.h | 43 ++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 43 insertions(+)
>>>>>
>>>>> diff --git a/include/block/block-common.h b/include/block/block-common.h
>>>>> index fdb7306e78..36bd0e480e 100644
>>>>> --- a/include/block/block-common.h
>>>>> +++ b/include/block/block-common.h
>>>>> @@ -49,6 +49,49 @@ typedef struct BlockDriver BlockDriver;
>>>>>  typedef struct BdrvChild BdrvChild;
>>>>>  typedef struct BdrvChildClass BdrvChildClass;
>>>>>
>>>>> +typedef enum BlockZoneOp {
>>>>> +    BLK_ZO_OPEN,
>>>>> +    BLK_ZO_CLOSE,
>>>>> +    BLK_ZO_FINISH,
>>>>> +    BLK_ZO_RESET,
>>>>> +} BlockZoneOp;
>>>>> +
>>>>> +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;
>>>>> +
>>>>> +typedef enum BlockZoneCondition {
>>>>> +    BLK_ZS_NOT_WP = 0x0,
>>>>> +    BLK_ZS_EMPTY = 0x1,
>>>>> +    BLK_ZS_IOPEN = 0x2,
>>>>> +    BLK_ZS_EOPEN = 0x3,
>>>>> +    BLK_ZS_CLOSED = 0x4,
>>>>> +    BLK_ZS_RDONLY = 0xD,
>>>>> +    BLK_ZS_FULL = 0xE,
>>>>> +    BLK_ZS_OFFLINE = 0xF,
>>>>> +} BlockZoneCondition;
>>>>> +
>>>>> +typedef enum BlockZoneType {
>>>>> +    BLK_ZT_CONV = 0x1, /* Conventional random writes supported */
>>>>> +    BLK_ZT_SWR = 0x2, /* Sequential writes required */
>>>>> +    BLK_ZT_SWP = 0x3, /* Sequential writes preferred */
>>>>> +} BlockZoneType;
>>>>> +
>>>>> +/*
>>>>> + * Zone descriptor data structure.
>>>>> + * Provides information on a zone with all position and size values in bytes.
>>>>
>>>> I'm glad that you chose bytes here for use in qemu.  But since the
>>>> kernel struct blk_zone uses sectors instead of bytes, is it worth
>>>> adding a sentence that we intentionally use bytes here, different from
>>>> Linux, to make it easier for reviewers to realize that scaling when
>>>> translating between qemu and kernel is necessary?
>>>
>>> Sorry about the unit mistake. The zone information is in sectors which
>>> is the same as kernel struct blk_zone. I think adding a sentence to
>>> inform the sector unit makes it clear what the zone descriptor is.
>>
>> I'd make the units bytes for consistency with the rest of the QEMU block
>> layer. For example, the MapEntry structure that "qemu-img map" reports
>> has names with similar fields and they are in bytes:
>>
>>   struct MapEntry {
>>       int64_t start;
>>       int64_t length;
>>
> 
> I think the zone descriptor uses sector units because ioctl() will
> report zones in sector units. Making blk_zone.offset =
> zone_descriptor.offset is more convenient than using byte units where
> it needs make conversions twice(sector -> byte -> sector in zone
> descriptors and offset argument in bdrv_co_zone_report). The MapEntry
> uses byte units because lseek() in bdrv_co_block_status suggests the
> file offset is set to bytes and I think it may be why the rest of the
> block layer uses bytes(not sure).
> 
> I do not object to using bytes here but it would require some
> compromises. If I was wrong about anything, please let me know.

The conversion can be done using 9-bits left and right shifts, which are
cheap to do. I think it is important to be consistent with qemu block API,
so using for the API bytes is preferred. That will avoid confusions.

> 
> 
> Sam

-- 
Damien Le Moal
Western Digital Research