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

[Kevin Wolf <kwolf@redhat.com>]

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.

>  # @node-name: The node name of the block node to be exported (since: 5.2)
>  #
> @@ -331,7 +351,9 @@
>        'vhost-user-blk': { 'type': 'BlockExportOptionsVhostUserBlk',
>                            'if': 'CONFIG_VHOST_USER_BLK_SERVER' },
>        'fuse': { 'type': 'BlockExportOptionsFuse',
> -                'if': 'CONFIG_FUSE' }
> +                'if': 'CONFIG_FUSE' },
> +      'vduse-blk': { 'type': 'BlockExportOptionsVduseBlk',
> +                     'if': 'CONFIG_VDUSE_BLK_EXPORT' }
>     } }

Kevin