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

[Stefan Hajnoczi]

On Sat, Jul 29, 2017 at 03:21:16AM +0000, Liu, Changpeng wrote:
> 
> 
> > -----Original Message-----
> > From: Stefan Hajnoczi [mailto:address@hidden
> > Sent: Friday, July 28, 2017 6:36 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:08:49AM +0000, Liu, Changpeng wrote:
> > > > -----Original Message-----
> > > > From: Stefan Hajnoczi [mailto:address@hidden
> > > > Sent: Thursday, July 27, 2017 5:49 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 12:29:45AM +0000, Liu, Changpeng wrote:
> > > > > > > +    .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.
> > > >
> > > > New messages for virtio config space read/write might be useful for
> > > > other devices besides virtio-blk.
> > > I mean all the block device information like: block_size/capacity are 
> > > stored in
> > another process,
> > > so add a new vhost user message to Qemu vhost-user-blk can get those
> > information when Qemu get
> > > started, once those messages stored to virtio_pci config space, we will 
> > > not
> > change it.
> > 
> > No, I think updates are necessary:
> > 
> > The virtio block device can do online disk resize, so it will be
> > necessary to change the capacity field from the vhost-user-blk process
> > at runtime and raise a config change notification interrupt.
> > 
> > The virtio block device also has a config space field ("wce") that is
> > writable by the guest.  Supporting this feature also requires virtio
> > config space support in vhost-user.
> > 
> > If you focus on adding generic virtio config space messages to
> > vhost-user then these virtio block features can be supported by
> > vhost-user-blk.
> > 
> > Regarding the two approaches of adding "block device information" as you
> > have suggested versus adding generic virtio config space support as I'm
> > suggesting, from a protocol design perspective it's nicer to have
> > generic messages that are usable by all device types.  I'm not aware of
> > a reason why high-level "block device information" is needed since QEMU
> > will just put that into the config space but otherwise does not
> > interpret the information.
> Agreed, adding a vhost message  to get/set generic vitio_pci device config 
> space 
> is clear to me now, it's better than only for vhost-user-blk messages.
> 
> Here I still have an concern about *resize* feature for vhost-user-blk, 
> currently
> Qemu vhost-user-blk is the client for vhost user messages, does this means
> the I/O processing process must send a new vhost message back to Qemu
> vhost-user-blk driver that the capacity of the block device is changed? Or you
> have better idea to do this? Thanks.

A vhost-user process -> vhost-user master message is not necessary.  An
eventfd can be used to signal config changes instead.

I have CCed Marc-André because I don't know much about the vhost-user
protocol design.

Here is what I suggest for virtio config space:

typedef enum VhostUserRequest {
    ...

    /* Submitted by the vhost-user master when the guest writes to
     * virtio config space and also after live migration on the
     * destination host.
     */
    VHOST_USER_SET_CONFIG,

    /* Submitted by the vhost-user master to fetch the contents of the
     * virtio config space.  The vhost-user master may cache the
     * contents to avoid repeated VHOST_USER_GET_CONFIG calls.
     */
    VHOST_USER_GET_CONFIG,

    ...
};

struct VuDev {
    ...
    int config_change_fd;
    ...
};

/**
 * vu_config_change_notify:
 * @dev: a VuDev context
 *
 * Notify the vhost-user master that the device has updated virtio
 * config space.  The master must raise a config change interrupt in the
 * guest and invalidate any cached virtio config space contents so that
 * the next guest read results in a VHOST_USER_GET_CONFIG request.
 */
void vu_config_change_notify(VuDev *dev);

signature.asc
Description: PGP signature