Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device

[Liu, Changpeng]

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:address@hidden
> Sent: Wednesday, July 26, 2017 6:35 PM
> To: Liu, Changpeng <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden
> Subject: Re: [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host
> device
> 
> On Thu, Jul 27, 2017 at 10:00:50AM +0800, Changpeng Liu wrote:
> > +static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t
> *config)
> > +{
> > +    VHostUserBlk *s = VHOST_USER_BLK(vdev);
> > +    struct virtio_blk_config blkcfg;
> > +
> > +    memcpy(&blkcfg, config, sizeof(blkcfg));
> > +
> > +    if (blkcfg.wce != s->config_wce) {
> > +        error_report("vhost-user-blk: does not support the operation");
> 
> If vhost-user-blk doesn't support the operation then please remove the
> VIRTIO_BLK_F_CONFIG_WCE feature bit.  That way the guest knows it cannot
> modify this field.
> 
> > +static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> > +{
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > +    VHostUserBlk *s = VHOST_USER_BLK(vdev);
> > +    int ret;
> > +    uint64_t size;
> > +
> > +    if (!s->chardev.chr) {
> > +        error_setg(errp, "vhost-user-blk: chardev is mandatary");
> 
> mandatory
> 
> > +        return;
> > +    }
> > +
> > +    if (!s->num_queues) {
> > +        error_setg(errp, "vhost-user-blk: invalid number of IO queues");
> > +        return;
> > +    }
> > +
> > +    if (!s->queue_size) {
> > +        error_setg(errp, "vhost-user-blk: invalid count of the IO queue");
> 
> "count of the IO queue" sounds like number of queues.  I suggest saying
> "queue size must be non-zero".
> 
> > +        return;
> > +    }
> > +
> > +    if (!s->size) {
> > +        error_setg(errp, "vhost-user-blk: block capacity must be assigned,"
> > +                  "size can be specified by GiB or MiB");
> > +        return;
> > +    }
> > +
> > +    ret = qemu_strtosz_MiB(s->size, NULL, &size);
> > +    if (ret < 0) {
> > +        error_setg(errp, "vhost-user-blk: invalid size %s in GiB/MiB", 
> > s->size);
> > +        return;
> > +    }
> > +    s->capacity = size / 512;
> > +
> > +    /* block size with default 512 Bytes */
> > +    if (!s->blkcfg.logical_block_size) {
> > +        s->blkcfg.logical_block_size = 512;
> > +    }
> > +
> > +    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
> > +                sizeof(struct virtio_blk_config));
> > +    virtio_add_queue(vdev, s->queue_size, vhost_user_blk_handle_output);
> > +
> > +    s->dev.nvqs = s->num_queues;
> > +    s->dev.vqs = g_new(struct vhost_virtqueue, s->dev.nvqs);
> 
> Please test multi-queue, it's currently broken.  virtio_add_queue() must
> be called for each queue.
Yes, should move virtio_add_queue into loop.
> 
> > +    s->dev.vq_index = 0;
> > +    s->dev.backend_features = 0;
> > +
> > +    ret = vhost_dev_init(&s->dev, (void *)&s->chardev,
> 
> The compiler automatically converts any pointer type to void * without a
> warning in C.  (This is different from C++!)
> 
> The (void *) cast can be dropped.
> 
> > +                         VHOST_BACKEND_TYPE_USER, 0);
> > +    if (ret < 0) {
> > +        error_setg(errp, "vhost-user-blk: vhost initialization failed: %s",
> > +                   strerror(-ret));
> 
> If realize fails unrealize() will not be called.  Cleanup must be
> performed here.
> 
> > +        return;
> > +    }
> > +}
> > +
> > +static void vhost_user_blk_device_unrealize(DeviceState *dev, Error **errp)
> > +{
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > +    VHostUserBlk *s = VHOST_USER_BLK(dev);
> > +
> > +    vhost_user_blk_set_status(vdev, 0);
> > +    vhost_dev_cleanup(&s->dev);
> > +    g_free(s->dev.vqs);
> > +    virtio_cleanup(vdev);
> > +}
> > +
> > +static void vhost_user_blk_instance_init(Object *obj)
> > +{
> > +    VHostUserBlk *s = VHOST_USER_BLK(obj);
> > +
> > +    device_add_bootindex_property(obj, &s->bootindex, "bootindex",
> > +                                  "/address@hidden,0", DEVICE(obj), NULL);
> > +}
> > +
> > +static const VMStateDescription vmstate_vhost_user_blk = {
> > +    .name = "vhost-user-blk",
> > +    .minimum_version_id = 2,
> > +    .version_id = 2,
> 
> Why is version_id 2?  There has never been a vhost-user-blk device in
> qemu.git before, so I would expect version to be 1.
> 
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_VIRTIO_DEVICE,
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> > +static Property vhost_user_blk_properties[] = {
> > +    DEFINE_PROP_CHR("chardev", VHostUserBlk, chardev),
> > +    DEFINE_BLOCK_PROPERTIES(VHostUserBlk, blkcfg),
> 
> DEFINE_BLOCK_PROPERTIES cannot be used directly because it includes the
> 'drive' (blkcfg.blk) parameter.  The command-line should not allow a
> drive= parameter.
> 
> Also, parameters like the discard granularity, optimal I/O size, logical
> block size, etc can be initialized to sensible defaults by QEMU's block
> layer when drive= is used.  Since you are bypassing QEMU's block layer
> there is no way for QEMU to set good defaults.
> 
> Are you relying on the managment tools populating these parameters with
> the correct values from the vhost-user-blk process?  Or did you have
> some other mechanism in mind?
Actually for this part and your comments about block resize, I think maybe add 
several
additional vhost user messages such like: 
VHOST_USER_GET_BLK_CFG/VHOST_USER_SET_BLK_CFG
makes the code more clear, I'm okay to add such messages.
> 
> > +    DEFINE_BLOCK_CHS_PROPERTIES(VHostUserBlk, blkcfg),
> > +    DEFINE_PROP_STRING("size", VHostUserBlk, size),
> 
> virtio-blk supports online resize.  QEMU and the vhost-user-blk process
> need a way to exchange disk capacity information for online resize.
> 
> Please add the necessary vhost-user protocol messages now so size
> information can be automatically exchanged and updated for resize.
> 
> > +typedef struct VHostUserBlk {
> > +    VirtIODevice parent_obj;
> > +    CharBackend chardev;
> > +    Error *migration_blocker;
> 
> This field is unused, please drop it.