Re: [PATCH v9 3/7] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls

[Klaus Jensen]

On Sep 21 13:44, Damien Le Moal wrote:
> On 9/20/22 17:51, Klaus Jensen wrote:
> > On Sep 10 13:27, Sam Li wrote:
> > > Add a new zoned_host_device BlockDriver. The zoned_host_device option
> > > accepts only zoned host block devices. By adding zone management
> > > operations in this new BlockDriver, users can use the new block
> > > layer APIs including Report Zone and four zone management operations
> > > (open, close, finish, reset).
> > > 
> > > Qemu-io uses the new APIs to perform zoned storage commands of the device:
> > > zone_report(zrp), zone_open(zo), zone_close(zc), zone_reset(zrs),
> > > zone_finish(zf).
> > > 
> > > For example, to test zone_report, use following command:
> > > $ ./build/qemu-io --image-opts -n driver=zoned_host_device, 
> > > filename=/dev/nullb0
> > > -c "zrp offset nr_zones"
> > > 
> > > Signed-off-by: Sam Li <faithilikerun@gmail.com>
> > > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > > ---
> > >   block/block-backend.c             | 145 ++++++++++++++
> > >   block/file-posix.c                | 323 +++++++++++++++++++++++++++++-
> > >   block/io.c                        |  41 ++++
> > >   include/block/block-io.h          |   7 +
> > >   include/block/block_int-common.h  |  21 ++
> > >   include/block/raw-aio.h           |   6 +-
> > >   include/sysemu/block-backend-io.h |  17 ++
> > >   meson.build                       |   1 +
> > >   qapi/block-core.json              |   8 +-
> > >   qemu-io-cmds.c                    | 143 +++++++++++++
> > >   10 files changed, 708 insertions(+), 4 deletions(-)
> > > 
> > > +/*
> > > + * zone management operations - Execute an operation on a zone
> > > + */
> > > +static int coroutine_fn raw_co_zone_mgmt(BlockDriverState *bs, 
> > > BlockZoneOp op,
> > > +        int64_t offset, int64_t len) {
> > > +#if defined(CONFIG_BLKZONED)
> > > +    BDRVRawState *s = bs->opaque;
> > > +    RawPosixAIOData acb;
> > > +    int64_t zone_sector, zone_sector_mask;
> > > +    const char *zone_op_name;
> > > +    unsigned long zone_op;
> > > +    bool is_all = false;
> > > +
> > > +    zone_sector = bs->bl.zone_sectors;
> > > +    zone_sector_mask = zone_sector - 1;
> > > +    if (offset & zone_sector_mask) {
> > > +        error_report("sector offset %" PRId64 " is not aligned to zone 
> > > size "
> > > +                     "%" PRId64 "", offset, zone_sector);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    if (len & zone_sector_mask) {
> > > +        error_report("number of sectors %" PRId64 " is not aligned to 
> > > zone size"
> > > +                      " %" PRId64 "", len, zone_sector);
> > > +        return -EINVAL;
> > > +    }
> > 
> > These checks impose a power-of-two constraint on the zone size. Can they
> > be changed to divisions to lift that constraint? I don't see anything in
> > this patch set that relies on power of two zone sizes.
> 
> Given that Linux will only expose zoned devices that have a zone size that
> is a power of 2 number of LBAs, this will work as is and avoid divisions in
> the IO path. But given that zone management operations are not performance
> critical, generalizing the code should be fine.
> 

Aight. That's fine, we can relax the constraint later. But I don't see why.

> However, once we start adding the code for full zone emulation on top of a
> regular file or qcow image, sector-to-zone conversions requiring divisions
> will hurt. So I really would prefer the code be left as-is for now.
> 

Block driver based zone emulation would not hit the above code path
anyway (there would be no iotcl to call), so I don't think that is an
argument for keeping it as-is.

On the sector-to-zone convertions. Yes, they may potentially hurt, but
it would be emulation after all. Why would we care about optimizing
those conversions at the expense of not being able to emulate all valid
geometries? The performance option (which this series enables) is to use
an underlying zoned device through virtio (or, potentially, hw/nvme).
What would be the usecase for deploying a performance sensitive emulated
zoned device?

signature.asc
Description: PGP signature