> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefa...@redhat.com]
> Sent: Wednesday, July 26, 2017 6:35 PM
> To: Liu, Changpeng <changpeng....@intel.com>
> Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; fel...@nutanix.com;
> m...@redhat.com
> 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",
> > + "/disk@0,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.