qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v15 3/8] block: add block layer APIs resembling Linux ZonedBl


From: Sam Li
Subject: Re: [PATCH v15 3/8] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls
Date: Tue, 28 Feb 2023 20:00:11 +0800

Kevin Wolf <kwolf@redhat.com> 于2023年2月28日周二 19:54写道:
>
> Am 27.02.2023 um 20:14 hat Stefan Hajnoczi geschrieben:
> > On Mon, Feb 27, 2023 at 07:20:14PM +0100, Kevin Wolf wrote:
> > > Am 29.01.2023 um 11:28 hat Sam Li geschrieben:
> > > > Add zoned device option to host_device BlockDriver. It will be 
> > > > presented only
> > > > for zoned host block devices. By adding zone management operations to 
> > > > the
> > > > host_block_device BlockDriver, users can use the new block layer APIs
> > > > including Report Zone and four zone management operations
> > > > (open, close, finish, reset, reset_all).
> > > >
> > > > 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=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>
> > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > >  block/block-backend.c             | 147 ++++++++++++++
> > > >  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 |  18 ++
> > > >  meson.build                       |   4 +
> > > >  qemu-io-cmds.c                    | 149 ++++++++++++++
> > > >  9 files changed, 715 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/block/block-backend.c b/block/block-backend.c
> > > > index ba7bf1d6bc..a4847b9131 100644
> > > > --- a/block/block-backend.c
> > > > +++ b/block/block-backend.c
> > > > @@ -1451,6 +1451,15 @@ typedef struct BlkRwCo {
> > > >      void *iobuf;
> > > >      int ret;
> > > >      BdrvRequestFlags flags;
> > > > +    union {
> > > > +        struct {
> > > > +            unsigned int *nr_zones;
> > > > +            BlockZoneDescriptor *zones;
> > > > +        } zone_report;
> > > > +        struct {
> > > > +            unsigned long op;
> > > > +        } zone_mgmt;
> > > > +    };
> > > >  } BlkRwCo;
> > >
> > > Should we use a different struct for blk_aio_zone_*() so that we don't
> > > need to touch the one for the normal I/O path? My concern is that
> > > increasing the size of the struct (currently 32 bytes) might negatively
> > > impact the performance even of non-zoned devices. Maybe it turns out
> > > that it wasn't really necessary in the end (have we done any
> > > benchmarks?), but I don't think it can hurt anyway.
> > >
> > > With this changed, you can add to the series:
> > > Acked-by: Kevin Wolf <kwolf@redhat.com>
> >
> > There are unused fields in BlkRwCo and BlkAioEmAIOCB, so changing the
> > size of the struct isn't necessary. ioctl/flush/pdiscard already use
> > BlkAioEmAIOCB/BlkRwCo for non-read/write operations, including using the
> > iobuf field for different types, so it wouldn't be weird:
> >
> >   typedef struct BlkRwCo {
> >       BlockBackend *blk;
> >       int64_t offset;
> >       void *iobuf;
> >             ^^^^^ used for preadv/pwritev qiov, ioctl buf, and NULL for
> >                   other request types. zone_report could put the
> >                   BlockZoneDescriptor pointer here. zone_mgmt could put
> >                   op here.
> >       int ret;
> >       BdrvRequestFlags flags;
> >   } BlkRwCo;
> >
> >   typedef struct BlkAioEmAIOCB {
> >       BlockAIOCB common;
> >       BlkRwCo rwco;
> >       int64_t bytes;
> >               ^^^^^ zone_report could put the nr_zones pointer here
> >       bool has_returned;
> >   } BlkAioEmAIOCB;
> >
> > Does that sound okay?
>
> Might not be great for readability, but good enough for me.
>
> Kevin

I see. Will change it accordingly. Thanks!

Sam



reply via email to

[Prev in Thread] Current Thread [Next in Thread]