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

[Sam Li]

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.


Sam