Re: [PATCH] qapi: deprecate drive-backup

[Daniel P . Berrangé]

On Mon, Apr 26, 2021 at 09:00:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 26.04.2021 20:34, John Snow wrote:
> > On 4/23/21 8:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > Modern way is using blockdev-add + blockdev-backup, which provides a
> > > lot more control on how target is opened.
> > > 
> > > As example of drive-backup problems consider the following:
> > > 
> > > User of drive-backup expects that target will be opened in the same
> > > cache and aio mode as source. Corresponding logic is in
> > > drive_backup_prepare(), where we take bs->open_flags of source.
> > > 
> > > It works rather bad if source was added by blockdev-add. Assume source
> > > is qcow2 image. On blockdev-add we should specify aio and cache options
> > > for file child of qcow2 node. What happens next:
> > > 
> > > drive_backup_prepare() looks at bs->open_flags of qcow2 source node.
> > > But there no BDRV_O_NOCAHE neither BDRV_O_NATIVE_AIO: BDRV_O_NOCAHE is
> > > places in bs->file->bs->open_flags, and BDRV_O_NATIVE_AIO is nowhere,
> > > as file-posix parse options and simply set s->use_linux_aio.
> > > 
> > 
> > No complaints from me, especially if Virtuozzo is on board. I would like to 
> > see some documentation changes alongside this deprecation, though.
> > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > > 
> > > Hi all! I remember, I suggested to deprecate drive-backup some time ago,
> > > and nobody complain.. But that old patch was inside the series with
> > > other more questionable deprecations and it did not landed.
> > > 
> > > Let's finally deprecate what should be deprecated long ago.
> > > 
> > > We now faced a problem in our downstream, described in commit message.
> > > In downstream I've fixed it by simply enabling O_DIRECT and linux_aio
> > > unconditionally for drive_backup target. But actually this just shows
> > > that using drive-backup in blockdev era is a bad idea. So let's motivate
> > > everyone (including Virtuozzo of course) to move to new interfaces and
> > > avoid problems with all that outdated option inheritance.
> > > 
> > >   docs/system/deprecated.rst | 5 +++++
> > >   qapi/block-core.json       | 5 ++++-
> > >   2 files changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> > > index 80cae86252..b6f5766e17 100644
> > > --- a/docs/system/deprecated.rst
> > > +++ b/docs/system/deprecated.rst
> > > @@ -186,6 +186,11 @@ Use the more generic commands ``block-export-add`` 
> > > and ``block-export-del``
> > >   instead.  As part of this deprecation, where ``nbd-server-add`` used a
> > >   single ``bitmap``, the new ``block-export-add`` uses a list of 
> > > ``bitmaps``.
> > > +``drive-backup`` (since 6.0)
> > > +''''''''''''''''''''''''''''
> > > +
> > > +Use ``blockdev-backup`` in pair with ``blockdev-add`` instead.
> > > +
> > 
> > 1) Let's add a sphinx reference to 
> > https://qemu-project.gitlab.io/qemu/interop/live-block-operations.html#live-disk-backup-drive-backup-and-blockdev-backup
> > 
> > 
> > 2) Just a thought, not a request: We also may wish to update 
> > https://qemu-project.gitlab.io/qemu/interop/bitmaps.html to use the new, 
> > preferred method. However, this doc is a bit old and is in need of an 
> > overhaul anyway (Especially to add the NBD pull workflow.) Since the doc is 
> > in need of an overhaul anyway, can we ask Kashyap to help us here, if he 
> > has time?
> > 
> > 
> > 3) Let's add a small explanation here that outlines the differences in 
> > using these two commands. Here's a suggestion:
> > 
> > This change primarily separates the creation/opening process of the backup 
> > target with explicit, separate steps. BlockdevBackup uses mostly the same 
> > arguments as DriveBackup, except the "format" and "mode" options are 
> > removed in favor of using explicit "blockdev-create" and "blockdev-add" 
> > calls.
> > 
> > The "target" argument changes semantics. It no longer accepts filenames, 
> > and will now additionally accept arbitrary node names in addition to device 
> > names.
> > 
> > 
> > 4) Also not a request: If we want to go above and beyond, it might be nice 
> > to spell out the exact steps required to transition from the old interface 
> > to the new one. Here's a (hasty) suggestion for how that might look:
> > 
> > - The MODE argument is deprecated.
> >   - "existing" is replaced by using "blockdev-add" commands.
> >   - "absolute-paths" is replaced by using "blockdev-add" and
> >     "blockdev-create" commands.
> > 
> > - The FORMAT argument is deprecated.
> >   - Format information is given to "blockdev-add"/"blockdev-create".
> > 
> > - The TARGET argument has new semantics:
> >   - Filenames are no longer supported, use blockdev-add/blockdev-create
> >     as necessary instead.
> >   - Device targets remain supported.
> > 
> > 
> > Example:
> > 
> > drive-backup $ARGS format=$FORMAT mode=$MODE target=$FILENAME becomes:
> > 
> > (taking some liberties with syntax to just illustrate the idea ...)
> > 
> > blockdev-create options={
> >     "driver": "file",
> >     "filename": $FILENAME,
> >     "size": 0,
> > }
> > 
> > blockdev-add arguments={
> >     "driver": "file",
> >     "filename": $FILENAME,
> >     "node-name": "Example_Filenode0"
> > }
> > 
> > blockdev-create options={
> >     "driver": $FORMAT,
> >     "file": "Example_Filenode0",
> >     "size": $SIZE,
> > }
> > 
> > blockdev-add arguments={
> >     "driver": $FORMAT,
> >     "file": "Example_Filenode0",
> >     "node-name": "Example_Formatnode0",
> > }
> > 
> > blockdev-backup arguments={
> >     $ARGS ...,
> >     "target": "Example_Formatnode0",
> > }
> > 
> 
> Good ideas. Hmm. Do you think that the whole explanation with examples should 
> be here, in docs/system/deprecated.rst ? Seems too big in comparison with 
> other deprecations

No, the deprecations entry should be short.

If the replacement is so complex that it requires us to provide examples,
that's a sign that the replacement is insufficiently documented in its
own right. IOW, add all this docs info to a suitable place in the main
QEMU documentation, and just link to that from the deprecations page.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|