Re: [PATCH 2/4] virtiofd: Create a notification queue

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> 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?

The kill_fd is internal to virtiofsd; it's only used as a way for the
main thread to cause the queue thread to exit;  if you've not got the
thread, you don't need the kill_fd.

Dave

> > 
> > > @@ -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
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK