Re: [PATCH 3/4] virtiofsd: Specify size of notification buffer using config space

[Stefan Hajnoczi]

On Fri, Nov 15, 2019 at 03:55:42PM -0500, Vivek Goyal wrote:
> diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c
> index 411114c9b3..982b6ad0bd 100644
> --- a/contrib/virtiofsd/fuse_virtio.c
> +++ b/contrib/virtiofsd/fuse_virtio.c
> @@ -109,7 +109,8 @@ static uint64_t fv_get_features(VuDev *dev)
>      uint64_t features;
>  
>      features = 1ull << VIRTIO_F_VERSION_1 |
> -               1ull << VIRTIO_FS_F_NOTIFICATION;
> +               1ull << VIRTIO_FS_F_NOTIFICATION |
> +               1ull << VHOST_USER_F_PROTOCOL_FEATURES;

This is not needed since VHOST_USER_F_PROTOCOL_FEATURES is already added
by vu_get_features_exec():

  vu_get_features_exec(VuDev *dev, VhostUserMsg *vmsg)
  {
      vmsg->payload.u64 =
          1ULL << VHOST_F_LOG_ALL |
          1ULL << VHOST_USER_F_PROTOCOL_FEATURES;

      if (dev->iface->get_features) {
          vmsg->payload.u64 |= dev->iface->get_features(dev);
      }

>  
>      return features;
>  }
> @@ -927,6 +928,27 @@ static bool fv_queue_order(VuDev *dev, int qidx)
>      return false;
>  }
>  
> +static uint64_t fv_get_protocol_features(VuDev *dev)
> +{
> +     return 1ull << VHOST_USER_PROTOCOL_F_CONFIG;
> +}

Please change vu_get_protocol_features_exec() in a separate patch so
that devices don't need this boilerplate .get_protocol_features() code:

  static bool
  vu_get_protocol_features_exec(VuDev *dev, VhostUserMsg *vmsg)
  {
      ...
 -    if (dev->iface->get_config && dev->iface->set_config) {
 +    if (dev->iface->get_config || dev->iface->set_config) {
          features |= 1ULL << VHOST_USER_PROTOCOL_F_CONFIG;
      }

> +
> +static int fv_get_config(VuDev *dev, uint8_t *config, uint32_t len)
> +{
> +     struct virtio_fs_config fscfg = {};
> +
> +     fuse_log(FUSE_LOG_DEBUG, "%s:Setting notify_buf_size=%d\n", __func__,
> +                 sizeof(struct fuse_notify_lock_out));
> +     /*
> +      * As of now only notification related to lock is supported. As more
> +      * notification types are supported, bump up the size accordingly.
> +      */
> +     fscfg.notify_buf_size = sizeof(struct fuse_notify_lock_out);

Missing cpu_to_le32().

I'm not sure about specifying the size precisely down to the last byte
because any change to guest-visible aspects of the device (like VIRTIO
Configuration Space) are not compatible across live migration.  It will
be necessary to introduce a device version command-line option for live
migration compatibility so that existing guests can be migrated to a new
virtiofsd without the device changing underneath them.

How about rounding this up to 4 KB?

>  static void vuf_get_config(VirtIODevice *vdev, uint8_t *config)
>  {
>      VHostUserFS *fs = VHOST_USER_FS(vdev);
>      struct virtio_fs_config fscfg = {};
> +    int ret;
> +
> +    /*
> +     * As of now we only get notification buffer size from device. And that's
> +     * needed only if notification queue is enabled.
> +     */
> +    if (fs->notify_enabled) {
> +        ret = vhost_dev_get_config(&fs->vhost_dev, (uint8_t *)&fs->fscfg,
> +                                   sizeof(struct virtio_fs_config));
> +     if (ret < 0) {

Indentation.

> +            error_report("vhost-user-fs: get device config space failed."
> +                         " ret=%d\n", ret);
> +            return;
> +        }

Missing le32_to_cpu() for notify_buf_size.

> +    }
>  
>      memcpy((char *)fscfg.tag, fs->conf.tag,
>             MIN(strlen(fs->conf.tag) + 1, sizeof(fscfg.tag)));
>  
>      virtio_stl_p(vdev, &fscfg.num_request_queues, 
> fs->conf.num_request_queues);
> +    virtio_stl_p(vdev, &fscfg.notify_buf_size, fs->fscfg.notify_buf_size);
>  
>      memcpy(config, &fscfg, sizeof(fscfg));
>  }
> @@ -545,6 +569,8 @@ static void vuf_device_realize(DeviceState *dev, Error 
> **errp)
>      fs->vhost_dev.nvqs = 2 + fs->conf.num_request_queues;
>  
>      fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs);
> +
> +    vhost_dev_set_config_notifier(&fs->vhost_dev, &fs_ops);

Is this really needed since vhost_user_fs_handle_config_change() ignores
it?

signature.asc
Description: PGP signature