Re: [Qemu-devel] [PATCH] add reopen to blockdev-transaction

[Paolo Bonzini]

> Do we need more flexibility in what is being reopened?
> 
> I see several possibilities.  In the examples below, I'm using '*' for
> any file that qemu has an open fd for, and [] to show the contents of a
> backing file field within that file (which can be relative or absolute).
> 
> 1. post-copy, oVirt creates the snapshot with a relative backing file
> name.  That is, when using blockdev-transaction with the 'reuse' flag
> set to true, qemu is merely opening the new file, ignoring the backing
> file name present in the file it just opens, and using the current open
> fd as the backing file of the just-opened fd.
> 
> 2. post-copy, letting qemu create the snapshot with absolute backing
> file name.  We are starting with:
> 
>> */store1/base <- */store1/snap1[backed by '/store1/base'] <-\
>>  /store2/base <-  /store2/snap1[backed by '/store2/base']    \- 
>> */store2/snap2[backed by '/store1/snap1'] <- qemu
> 
> and here, we actually need to rewrite the snap2 file to point to a
> different backing file.

You don't want to do this.  We could add:

* a "relative-backing-file" boolean to the snapshot JSON.  It would
compute a relative path to the current image file and store it.

* a "relative-backing-file" boolean, *or* a
"backing-file"/"backing-format" pair of strings to the mirror JSON.  The
latter is obvious, but I'm not sure I like it.  relative-path would
compute a relative path from the current image file to its backing file,
and store it.  So if you had

   /store0/templates/base <- /store0/vms/snap1

it would store ../templates/base in the newly-created image.

There's a third case, in which you do not want a snapshot to be created
at all, for example when migrating with streaming.  So overall you'd have:

* snapshot = true, relative-backing-file = false (default)
* snapshot = true, relative-backing-file = true (oVirt desired)
* snapshot = false, relative-backing-file doesn't matter

In any case, rewriting after the fact is wrong.

> Again, we don't technically need to reopen
> 'snap2', so much as we need to reopen a different 'snap1' to end with:
> 
>>  /store1/base <-  /store1/snap1[backed by '/store1/base']
>> */store2/base <- */store2/snap1[backed by '/store2/base'] <- 
>> */store2/snap2[backed by '/store2/snap1'] <- qemu

So in both cases we need to reopen the full tower here.

> Notice that in all 4 cases, the right file in current use by qemu was
> already open; what this operation is really doing is reopening an
> updated backing chain, as well as possibly discarding a mirror and/or
> rewriting the backing file of the top level file as part of opening the
> new backing chain.  I think we may need an optional argument that says
> that when reopening a file, we also want to rewrite its backing chain to
> the specified file name and type, as in:
> 
> { 'type': 'BlockdevReopen',
>   'data': { 'device': 'str', 'target': 'str', '*format': 'str',
>             '*backing': 'str', '*backing-format', 'str' } }

I think you're complicating the thing a lot.  Just ignore cases (2) and
(4), they are wrong for you and you are not going to do them anyway.
You just want to *reopen the device*.  Let QEMU pick the backing files.
 If they're wrong, something is wrong in the previous steps, and it's
there that we should expand the JSON.  (In the meanwhile, you can work
around it with reuse:true).

>> But you can even keep from your first patch the drive-reopen command and
>> not make it atomic, that shouldn't be a problem.
> 
> I'm not sure whether it makes sense for a separate drive-reopen or
> whether to just add this to blockdev-transaction (or even both); I can
> make libvirt use whichever color bikeshed we pick.  There's definitely a
> transaction aspect here

It's not so much atomicity, it's just safety.  The drive-reopen command
must be implemented in a similar way to bdrv_append; it must not do a
close+reopen in the same way as the existing blockdev-snapshot-sync
command, but that's just that blockdev-snapshot-sync was implemented
poorly.

Paolo