Re: [Qemu-devel] [libvirt] [PATCH 1/2] qapi: deprecate drive-mirror and drive-backup

[John Snow <jsnow@redhat.com>]

On 8/15/19 3:44 AM, Peter Krempa wrote:
> On Wed, Aug 14, 2019 at 15:22:15 -0400, John Snow wrote:
>>
>>
>> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> It's hard and not necessary to maintain outdated versions of these
>>> commands.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>  qemu-deprecated.texi  |  4 ++++
>>>  qapi/block-core.json  |  4 ++++
>>>  qapi/transaction.json |  2 +-
>>>  blockdev.c            | 10 ++++++++++
>>>  4 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>>> index fff07bb2a3..2753fafd0b 100644
>>> --- a/qemu-deprecated.texi
>>> +++ b/qemu-deprecated.texi
>>> @@ -179,6 +179,10 @@ and accurate ``query-qmp-schema'' command.
>>>  Character devices creating sockets in client mode should not specify
>>>  the 'wait' field, which is only applicable to sockets in server mode
>>>  
>>> +@subsection drive-mirror, drive-backup and drive-backup transaction action (since 4.2)
>>> +
>>> +Use blockdev-mirror and blockdev-backup instead.
>>> +
>>>  @section Human Monitor Protocol (HMP) commands
>>>  
>>>  @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 3.1)
> 
> [...]
> 
>>> @@ -3831,6 +3838,9 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>>>      const char *format = arg->format;
>>>      int ret;
>>>  
>>> +    warn_report("drive-mirror command is deprecated and will disappear in "
>>> +                "future. Use blockdev-mirror instead");
>>> +
>>>      bs = qmp_get_root_bs(arg->device, errp);
>>>      if (!bs) {
>>>          return;
>>>
>>
>> Hm!
>>
>> I wonder if this is ever-so-slightly too soon for our friends over at
>> the libvirt project.
>>
>> I don't think they have fully moved away from the non-blockdev
>> interfaces *just yet*, and I might encourage seeing the first full
>> libvirt release that does support and use it before we start the
>> deprecation clock.
>>
>> (Juuuust in case.)
>>
>> That's just me being very, very cautious though.
>>
>> Peter Krempa, how do you feel about this?
> 
> Thanks for the heads up!
> 

You're welcome!

> Currently libvirt does not use 'drive-backup' at all so that one can be
> deprecated immediately.
> 
> In case of 'drive-mirror' the situation is a bit more complex:
> 
> Libvirt uses 'drive-mirror' currently in the following places
> 
> 1) virDomainBlockCopy API
> With blockdev integration enabled this will go away. Pathces are being
> reviewed:
> 
> https://www.redhat.com/archives/libvir-list/2019-August/msg00295.html
> 
> 2) VM migration with non-shared storage
> Currently uses 'drive-mirror' in most cases but there is pre-existing
> integration for blockdev-mirror for nbd+tls. I need to make sure that
> the blockdev version will be used unconditionally once the integration
> is enabled. This is a TODO.
> 
> There is also one gotcha. In case when an 'sd' card device is used for
> the VM, libvirt disables all of blockdev, because SD cards can't be
> expressed with blockdev. There's too many code paths which would need
> checking to be worth it. To be fair, I'm not even sure when a sd card
> can be emulated by qemu as all of my basic tests failed and I did not
> care more.
> 
> For libvirt to enable blockdev there's one more part missing and that's
> snapshot integration. I'm currently testing patches to integrate it with
> external snapshots, which should be posted soon.
> 
> I also found a bug in qemu, which prevents creation of internal
> snapshots when -blockdev is used:
> 
> When savevm HMP command is used (via QMP->HMP bridge) qemu invokes
> save_snapshot(), which calls bdrv_all_can_snapshot(). That function uses
> bdrv_next() to iterate all nodes which correspond to a block backend
> first, but then also iterates any other node which is monitor-owned.
> 
> Since with blockdev all nodes including the ones for the 'file' protocol
> are monitor-owned, and 'file' does not support snapshots that check
> fails. A simple hack of skipping the second part in bdrv_next() allows
> to do a snapshot actually. Kevin told me that the idea is that also
> non-attached nodes should be considered for internal snapshod which is
> okay in my opinion, but given how the snapshot works for the files
> attached to backeds (and also in pre-blockdev use) only the top level of
> a chain should ever be considered for snapshot.
> 
> So the summary is, that I'm pretty hopeful that we should be able to get
> rid of all reasonable uses of drive-mirror very soon after I finish
> snapshot integration. The only question is how much
> we care about SD card users being able to do a drive-mirror in the
> future.
> 

OK. It sounds like we should hold off on deprecating these for now
because it's not certain which libvirt release will no longer need them,
but it sounds like it's hopefully not far off.

--js