On Mon, Oct 04, 2021 at 05:01:07PM -0400, Vivek Goyal wrote: > On Mon, Oct 04, 2021 at 03:30:38PM +0100, Stefan Hajnoczi wrote: > > On Thu, Sep 30, 2021 at 11:30:32AM -0400, Vivek Goyal wrote: > > > Add a notification queue which will be used to send async notifications > > > for file lock availability. > > > > > > Signed-off-by: Vivek Goyal > > > Signed-off-by: Ioannis Angelakopoulos > > > --- > > > hw/virtio/vhost-user-fs-pci.c | 4 +- > > > hw/virtio/vhost-user-fs.c | 62 +++++++++++++++++++++++++-- > > > include/hw/virtio/vhost-user-fs.h | 2 + > > > tools/virtiofsd/fuse_i.h | 1 + > > > tools/virtiofsd/fuse_virtio.c | 70 +++++++++++++++++++++++-------- > > > 5 files changed, 116 insertions(+), 23 deletions(-) > > > > > > diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c > > > index 2ed8492b3f..cdb9471088 100644 > > > --- a/hw/virtio/vhost-user-fs-pci.c > > > +++ b/hw/virtio/vhost-user-fs-pci.c > > > @@ -41,8 +41,8 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > > > if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { > > > - /* Also reserve config change and hiprio queue vectors */ > > > - vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2; > > > + /* Also reserve config change, hiprio and notification queue vectors */ > > > + vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 3; > > > } > > > > > > qdev_realize(vdev, BUS(&vpci_dev->bus), errp); > > > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c > > > index d1efbc5b18..6bafcf0243 100644 > > > --- a/hw/virtio/vhost-user-fs.c > > > +++ b/hw/virtio/vhost-user-fs.c > > > @@ -31,6 +31,7 @@ static const int user_feature_bits[] = { > > > VIRTIO_F_NOTIFY_ON_EMPTY, > > > VIRTIO_F_RING_PACKED, > > > VIRTIO_F_IOMMU_PLATFORM, > > > + VIRTIO_FS_F_NOTIFICATION, > > > > > > VHOST_INVALID_FEATURE_BIT > > > }; > > > @@ -147,7 +148,7 @@ static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > */ > > > } > > > > > > -static void vuf_create_vqs(VirtIODevice *vdev) > > > +static void vuf_create_vqs(VirtIODevice *vdev, bool notification_vq) > > > { > > > VHostUserFS *fs = VHOST_USER_FS(vdev); > > > unsigned int i; > > > @@ -155,6 +156,15 @@ static void vuf_create_vqs(VirtIODevice *vdev) > > > /* Hiprio queue */ > > > fs->hiprio_vq = virtio_add_queue(vdev, fs->conf.queue_size, > > > vuf_handle_output); > > > + /* > > > + * Notification queue. Feature negotiation happens later. So at this > > > + * point of time we don't know if driver will use notification queue > > > + * or not. > > > + */ > > > + if (notification_vq) { > > > + fs->notification_vq = virtio_add_queue(vdev, fs->conf.queue_size, > > > + vuf_handle_output); > > > + } > > > > > > /* Request queues */ > > > fs->req_vqs = g_new(VirtQueue *, fs->conf.num_request_queues); > > > @@ -163,8 +173,12 @@ static void vuf_create_vqs(VirtIODevice *vdev) > > > vuf_handle_output); > > > } > > > > > > - /* 1 high prio queue, plus the number configured */ > > > - fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues; > > > + /* 1 high prio queue, 1 notification queue plus the number configured */ > > > + if (notification_vq) { > > > + fs->vhost_dev.nvqs = 2 + fs->conf.num_request_queues; > > > + } else { > > > + fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues; > > > + } > > > fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs); > > > } > > > > > > @@ -176,6 +190,11 @@ static void vuf_cleanup_vqs(VirtIODevice *vdev) > > > virtio_delete_queue(fs->hiprio_vq); > > > fs->hiprio_vq = NULL; > > > > > > + if (fs->notification_vq) { > > > + virtio_delete_queue(fs->notification_vq); > > > + } > > > + fs->notification_vq = NULL; > > > + > > > for (i = 0; i < fs->conf.num_request_queues; i++) { > > > virtio_delete_queue(fs->req_vqs[i]); > > > } > > > @@ -194,9 +213,43 @@ static uint64_t vuf_get_features(VirtIODevice *vdev, > > > { > > > VHostUserFS *fs = VHOST_USER_FS(vdev); > > > > > > + virtio_add_feature(&features, VIRTIO_FS_F_NOTIFICATION); > > > + > > > return vhost_get_features(&fs->vhost_dev, user_feature_bits, features); > > > } > > > > > > +static void vuf_set_features(VirtIODevice *vdev, uint64_t features) > > > +{ > > > + VHostUserFS *fs = VHOST_USER_FS(vdev); > > > + > > > + if (virtio_has_feature(features, VIRTIO_FS_F_NOTIFICATION)) { > > > + fs->notify_enabled = true; > > > + /* > > > + * If guest first booted with no notification queue support and > > > + * later rebooted with kernel which supports notification, we > > > + * can end up here > > > + */ > > > + if (!fs->notification_vq) { > > > + vuf_cleanup_vqs(vdev); > > > + vuf_create_vqs(vdev, true); > > > + } > > > > I would simplify things by unconditionally creating the notification vq > > for the device and letting the vhost-user device backend decide whether > > it wants to handle the vq or not. > > If the backend doesn't implement the > > vq then it also won't advertise VIRTIO_FS_F_NOTIFICATION so the guest > > driver won't submit virtqueue buffers. > > I think I am did not understand the idea. This code deals with that > both qemu and vhost-user device can deal with notification queue. But > driver can't deal with it. > > So if we first booted into a guest kernel which does not support > notification queue, then we will not have instantiated notification > queue. But later we reboot guest into a newer kernel and now it > has capability to deal with notification queues, so we create it > now. > > IIUC, you are suggesting that somehow keep notification queue > instantiated even if guest driver does not support notifications, so > that we will not have to get into the exercise of cleaning up queues > and re-instantiating these? Yes. > But I think we can't keep notification queue around if driver does > not support it. Because it changes queue index. queue index 1 will > belong to request queue if notifications are not enabled otherwise > it will belong to notification queue. So If I always instantiate > notification queue, then guest and qemu/virtiofsd will have > different understanding of which queue index belongs to what > queue. The meaning of the virtqueue doesn't matter. That only matters to virtiofsd when processing virtqueues. Since QEMU's -device vhost-user-fs doesn't process virtqueues there's no difference between hipri, request, and notification virtqueues. I'm not 100% sure that the vhost-user code is set up to work smoothly in this fashion, but I think it should be possible to make this work and the end result will be simpler. Stefan