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

[Stefan Hajnoczi]

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.

> > It's worth checking how virtio config space is live migrated and how to
> > do that consistently if the vhost-user process is involved.
> Virtio will config spaces for virtio_blk, and even the config space migrated 
> to another machine, it should be
> same with the another I/O process, did I get your comments ? Thanks. 

The guest-writable "wce" config space field is an example that shows the
vhost-user-blk process on the destination host does not have all the
state necessary.  QEMU needs to migrate the config space and send it to
the vhost-user-blk process on the destination.

Stefan

signature.asc
Description: PGP signature