On Fri, Nov 22, 2019 at 10:19:03AM +0000, Stefan Hajnoczi wrote:
> On Fri, Nov 15, 2019 at 03:55:41PM -0500, Vivek Goyal wrote:
> > /* Callback from libvhost-user */
> > static void fv_set_features(VuDev *dev, uint64_t features)
> > {
> > + struct fv_VuDev *vud = container_of(dev, struct fv_VuDev, dev);
> > + struct fuse_session *se = vud->se;
> > +
> > + if ((1 << VIRTIO_FS_F_NOTIFICATION) & features) {
>
> For consistency 1ull should be used. That way the reader does not have
> to check the bit position to verify that the bitmap isn't truncated at
> 32 bits.
Ok, will do.
>
> > + vud->notify_enabled = true;
> > + se->notify_enabled = true;
>
> Only one copy of this field is needed. vud has a pointer to se.
I need to access ->notify_enabled in passthrough_ll.c to determine if
notification queue is enabled or not. That determines if async locks are
supported or not. And based on that either -EOPNOTSUPP is returned or
a response to wait is returned.
I did not see passthrough_ll.c accessing vud. I did see it having access
to session object though. So I created a copy there.
But I am open to suggestions on what's the best way to access this
information in passthrough_ll.c
>
> > + }
> > }
> >
> > /*
> > @@ -662,6 +671,65 @@ static void fv_queue_worker(gpointer data, gpointer
> > user_data)
> > free(req);
> > }
> >
> > +static void *fv_queue_notify_thread(void *opaque)
> > +{
> > + struct fv_QueueInfo *qi = opaque;
> > +
> > + fuse_log(FUSE_LOG_INFO, "%s: Start for queue %d kick_fd %d\n",
> > __func__,
> > + qi->qidx, qi->kick_fd);
> > +
> > + while (1) {
> > + struct pollfd pf[2];
> > +
> > + pf[0].fd = qi->kick_fd;
> > + pf[0].events = POLLIN;
> > + pf[0].revents = 0;
> > + pf[1].fd = qi->kill_fd;
> > + pf[1].events = POLLIN;
> > + pf[1].revents = 0;
> > +
> > + fuse_log(FUSE_LOG_DEBUG, "%s: Waiting for Queue %d event\n",
> > __func__,
> > + qi->qidx);
> > + int poll_res = ppoll(pf, 2, NULL, NULL);
> > +
> > + if (poll_res == -1) {
> > + if (errno == EINTR) {
> > + fuse_log(FUSE_LOG_INFO, "%s: ppoll interrupted, going
> > around\n",
> > + __func__);
> > + continue;
> > + }
> > + fuse_log(FUSE_LOG_ERR, "fv_queue_thread ppoll: %m\n");
> > + break;
> > + }
> > + assert(poll_res >= 1);
> > + if (pf[0].revents & (POLLERR | POLLHUP | POLLNVAL)) {
> > + fuse_log(FUSE_LOG_ERR, "%s: Unexpected poll revents %x Queue
> > %d\n",
> > + __func__, pf[0].revents, qi->qidx);
> > + break;
> > + }
> > + if (pf[1].revents & (POLLERR | POLLHUP | POLLNVAL)) {
> > + fuse_log(FUSE_LOG_ERR, "%s: Unexpected poll revents %x Queue
> > %d"
> > + "killfd\n", __func__, pf[1].revents, qi->qidx);
> > + break;
> > + }
> > + if (pf[1].revents) {
> > + fuse_log(FUSE_LOG_INFO, "%s: kill event on queue %d -
> > quitting\n",
> > + __func__, qi->qidx);
> > + break;
> > + }
> > + assert(pf[0].revents & POLLIN);
> > + fuse_log(FUSE_LOG_DEBUG, "%s: Got queue event on Queue %d\n",
> > __func__,
> > + qi->qidx);
> > +
> > + eventfd_t evalue;
> > + if (eventfd_read(qi->kick_fd, &evalue)) {
> > + fuse_log(FUSE_LOG_ERR, "Eventfd_read for queue: %m\n");
> > + break;
> > + }
> > + }
> > + return NULL;
> > +}
>
> It's difficult to review function without any actual functionality using
> the virtqueue. I'm not sure a thread is even needed since the device
> only needs to get a buffer when it has a notification for the driver.
> I'll have to wait for the following patches to see what happens here...
This might very well be redundant. I am not sure. Can get rid of
this thread if not needed at all. So we don't need to monitor even
kill_fd and take any special action?
>
> > @@ -378,12 +382,23 @@ static void vuf_set_status(VirtIODevice *vdev,
> > uint8_t status)
> > }
> > }
> >
> > -static uint64_t vuf_get_features(VirtIODevice *vdev,
> > - uint64_t requested_features,
> > - Error **errp)
> > +static uint64_t vuf_get_features(VirtIODevice *vdev, uint64_t features,
> > + Error **errp)
> > {
> > - /* No feature bits used yet */
> > - return requested_features;
> > + 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;
>
> This field is unused, please remove it.
vuf_get_config() uses it.
Thanks
Vivek
