Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Stefan Hajnoczi]

On Fri, Mar 31, 2017 at 11:25:02AM -0700, ashish mittal wrote:
> On Mon, Mar 27, 2017 at 6:04 PM, ashish mittal <address@hidden> wrote:
> > On Mon, Mar 27, 2017 at 10:27 AM, Stefan Hajnoczi <address@hidden> wrote:
> >> On Sun, Mar 26, 2017 at 07:50:35PM -0700, Ashish Mittal wrote:
> >>
> >> Have you tested live migration?
> >>
> >> If live migration is not supported then a migration blocker should be
> >> added using migrate_add_blocker().
> >>
> >
> > We do support live migration. We have been testing a fork of this code
> > (slightly different version) with live migration.

The reason I ask is because this patch doesn't implement the
BlockDriver bdrv_invalidate_cache()/bdrv_inactivate() callbacks.  These
functions are invoked during live migration so that the block driver can
ensure data consistency.

Since the destination QEMU process is launched while the source QEMU is
still running and making changes to the disk, some block drivers need to
discard metadata at migration handover time that was read upon opening
the image on the destination.  The guarantees that they see the latest
metadata.

Not sure if libvxhs caches anything that might get stale during live
migration, but I wanted to raise this question?

Regarding a fork of this code that you haven't posted to the mailing
list, it doesn't exist as far as anyone here is concerned :).  Therefore
either the code on the mailing list needs to support migration or it
must register a migration blocker to prevent migration.

> >>> +static BlockDriver bdrv_vxhs = {
> >>> +    .format_name                  = "vxhs",
> >>> +    .protocol_name                = "vxhs",
> >>> +    .instance_size                = sizeof(BDRVVXHSState),
> >>> +    .bdrv_file_open               = vxhs_open,
> >>> +    .bdrv_parse_filename          = vxhs_parse_filename,
> >>> +    .bdrv_close                   = vxhs_close,
> >>> +    .bdrv_getlength               = vxhs_getlength,
> >>> +    .bdrv_aio_readv               = vxhs_aio_readv,
> >>> +    .bdrv_aio_writev              = vxhs_aio_writev,
> >>
> >> Missing .bdrv_aio_flush().  Does VxHS promise that every completed write
> >> request is persistent?
> >>
> >
> > Yes, every acknowledged write request is persistent.
> >
> >> In that case it may be better to disable the emulated disk write cache
> >> so the guest operating system and application know not to send flush
> >> commands.
> >
> > We do pass "cache=none" on the qemu command line for every block
> > device. Are there any other code changes necessary? Any pointers will
> > help.
> >
> 
> Upon further reading, I now understand that cache=none will not
> disable the emulated disk write cache. I am trying to understand if -
> (1) It should still not be a problem since flush will just be a no-op for us.

The guest operating system and applications may take different code
paths depending on the state of the disk write cache.

Useless vmexits can be eliminated if the guest doesn't need to send
flush commands.  Hence the file system and applications may perform
better.

> (2) Is there a way, or reason, to disable the emulated disk write
> cache in the code for vxhs? I think passing WCE=0 to the guest has
> something to do with this, although I have yet to figure out what that
> means.

Right, WCE == "Write Cache Enable".  If you disable the write cache then
the guest's SCSI disk or virtio-blk drivers will notice that the disk
does not require flush commands.

Try launching a guest with -drive if=none,id=drive0,cache=directsync,...
and you should see that the write cache is disabled:

  # cat /sys/block/vda/queue/write_cache

> (3) Is this a must for merge?

This doesn't affect the block driver code so no change is necessary.

signature.asc
Description: PGP signature