Re: [PATCH v4 4/6] vduse-blk: implements vduse-blk export

[Yongji Xie]

On Wed, Apr 27, 2022 at 1:03 AM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 06.04.2022 um 09:59 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>
>
> The request handling code is almos the same as for the vhost-user-blk
> export. I wonder if we could share this code instead of copying.
>

I think we can. Will do it v5.

> The main difference seems to be that you chose not to support discard
> and write_zeroes yet. I'm curious if there is a reason why the
> vhost-user-blk code wouldn't work for vdpa there?
>

They are different protocols. The data plane is similar, so we can
share some codes. But the control plane is different, e.g., vhost-user
can only work for guests but vdpa can work for both guests and hosts.

> > +    features = vduse_get_virtio_features() |
> > +               (1ULL << VIRTIO_BLK_F_SIZE_MAX) |
> > +               (1ULL << VIRTIO_BLK_F_SEG_MAX) |
> > +               (1ULL << VIRTIO_BLK_F_TOPOLOGY) |
> > +               (1ULL << VIRTIO_BLK_F_BLK_SIZE);
> > +
> > +    if (num_queues > 1) {
> > +        features |= 1ULL << VIRTIO_BLK_F_MQ;
> > +    }
> > +    if (!vblk_exp->writable) {
> > +        features |= 1ULL << VIRTIO_BLK_F_RO;
> > +    }
>
> VIRTIO_BLK_F_FLUSH seems to be missing even though the flush command is
> implemented.
>

Oops. Will fix it.

> (This is not a full review yet, just two or three things I noticed while
> having a quick look.)
>

Thank you for your time!

Thanks,
Yongji