Re: [PATCH v6 6/8] vduse-blk: Implement vduse-blk export

[Yongji Xie]

On Fri, May 27, 2022 at 11:34 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 23.05.2022 um 10:46 hat Xie Yongji geschrieben:
> > This implements a VDUSE block backends based on
> > the libvduse library. We can use it to export the BDSs
> > for both VM and container (host) usage.
> >
> > The new command-line syntax is:
> >
> > $ qemu-storage-daemon \
> >     --blockdev file,node-name=drive0,filename=test.img \
> >     --export vduse-blk,node-name=drive0,id=vduse-export0,writable=on
> >
> > After the qemu-storage-daemon started, we need to use
> > the "vdpa" command to attach the device to vDPA bus:
> >
> > $ vdpa dev add name vduse-export0 mgmtdev vduse
> >
> > Also the device must be removed via the "vdpa" command
> > before we stop the qemu-storage-daemon.
> >
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  MAINTAINERS                   |   4 +-
> >  block/export/export.c         |   6 +
> >  block/export/meson.build      |   5 +
> >  block/export/vduse-blk.c      | 307 ++++++++++++++++++++++++++++++++++
> >  block/export/vduse-blk.h      |  20 +++
> >  meson.build                   |  13 ++
> >  meson_options.txt             |   2 +
> >  qapi/block-export.json        |  28 +++-
> >  scripts/meson-buildoptions.sh |   4 +
> >  9 files changed, 385 insertions(+), 4 deletions(-)
> >  create mode 100644 block/export/vduse-blk.c
> >  create mode 100644 block/export/vduse-blk.h
>
> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> > index 0685cb8b9a..e4bd4de363 100644
> > --- a/qapi/block-export.json
> > +++ b/qapi/block-export.json
> > @@ -177,6 +177,23 @@
> >              '*allow-other': 'FuseExportAllowOther' },
> >    'if': 'CONFIG_FUSE' }
> >
> > +##
> > +# @BlockExportOptionsVduseBlk:
> > +#
> > +# A vduse-blk block export.
> > +#
> > +# @num-queues: the number of virtqueues. Defaults to 1.
> > +# @queue-size: the size of virtqueue. Defaults to 256.
> > +# @logical-block-size: Logical block size in bytes. Range [512, PAGE_SIZE]
> > +#                      and must be power of 2. Defaults to 512 bytes.
> > +#
> > +# Since: 7.1
> > +##
> > +{ 'struct': 'BlockExportOptionsVduseBlk',
> > +  'data': { '*num-queues': 'uint16',
> > +            '*queue-size': 'uint16',
> > +            '*logical-block-size': 'size'} }
> > +
> >  ##
> >  # @NbdServerAddOptions:
> >  #
> > @@ -280,6 +297,7 @@
> >  # @nbd: NBD export
> >  # @vhost-user-blk: vhost-user-blk export (since 5.2)
> >  # @fuse: FUSE export (since: 6.0)
> > +# @vduse-blk: vduse-blk export (since 7.1)
> >  #
> >  # Since: 4.2
> >  ##
> > @@ -287,7 +305,8 @@
> >    'data': [ 'nbd',
> >              { 'name': 'vhost-user-blk',
> >                'if': 'CONFIG_VHOST_USER_BLK_SERVER' },
> > -            { 'name': 'fuse', 'if': 'CONFIG_FUSE' } ] }
> > +            { 'name': 'fuse', 'if': 'CONFIG_FUSE' },
> > +            { 'name': 'vduse-blk', 'if': 'CONFIG_VDUSE_BLK_EXPORT' } ] }
> >
> >  ##
> >  # @BlockExportOptions:
> > @@ -295,7 +314,8 @@
> >  # Describes a block export, i.e. how single node should be exported on an
> >  # external interface.
> >  #
> > -# @id: A unique identifier for the block export (across all export types)
> > +# @id: A unique identifier for the block export (across the host for 
> > vduse-blk
> > +#      export type or across all export types for other types)
>
> I find this sentence a bit confusing, but more importantly, it shows
> that you are using one value for two different purposes: The ID to
> identfy the export within QEMU (must be distinct from any other exports
> in the same QEMU process, but can overlap with names used by other
> processes), and the VDUSE name to uniquely identify it on the host (must
> be distinct from other VDUSE devices on the same host, but can overlap
> with other export types like NBD in the same process).
>
> We can fix this on top, but I would suggest having a separate option for
> the VDUSE device name, like BlockExportOptionsNbdBase contains a 'name'
> option for the export name that is different from the export ID in QEMU.
>

Yes, we discussed whether we need a separate option before. I think I
can send a patch to do it.

Thanks,
Yongji