Re: [Qemu-devel] [PATCH 3/4] block/parallels: Don't update header until the first actual write

[Jeff Cody <jcody@redhat.com>]

On Fri, Oct 27, 2017 at 11:09:19AM +0200, Kevin Wolf wrote:
> Am 27.10.2017 um 10:57 hat Jeff Cody geschrieben:
> > The on disk image format 'inuse' header field is updated blindly if the
> > image is opened RDWR.  This can cause problems if the QEMU runstate is
> > set to INMIGRATE, at which point the underlying file is set to INACTIVE.
> > This causes an assert in bdrv_co_pwritev().
> > 
> > Do something similar to what is done in VHDX; latch the first write, and
> > update the header the first time we modify the file.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> 
> For VHDX, it seems that we have to have the header update in the write
> path anyway, so it might be justifiable, but I think for parallels, it's
> just ugly.
> 

A bit ugly.  I think we could get around VHDX needing to do it as well; it
does it in the write path for two scenarios:

    * First normal write, or
    * Journal log replay, if dirty, on open (if r/w)

The log check happens early in vhdx_open().  If it does not write anything,
then we can just write the headers during the open like normal, if we are
R/W (and !BDRV_O_INACTIVE, of course).

> The conservative approach to this would be doing the header write in
> .bdrv_open() only if BDRV_O_INACTIVE is cleared, and otherwise do it
> during .bdrv_invalidate_cache().

What scenarios cause BDRV_O_INACTIVE to not be set on bs, but set on
bs->file-bs?

> By the way, random design thought: It might make sense to change
> .bdrv_open() so that it always opens inactive images and then call
> .bdrv_invalidate_cache() (possibly renamed to .bdrv_activate())
> unconditionally even without migration.
> 
> Kevin