On Fri, Apr 23, 2021 at 03:59:00PM +0300, 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.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
> ---
>
> 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)
NB, it'll need to be 6.1 by time this patch has a chance to merge
> +''''''''''''''''''''''''''''
> +
> +Use ``blockdev-backup`` in pair with ``blockdev-add`` instead.
> +
> System accelerators
> -------------------
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6d227924d0..8e2c6e1622 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1642,6 +1642,9 @@
> # The operation can be stopped before it has completed using the
> # block-job-cancel command.
> #
> +# Features:
> +# @deprecated: This command is deprecated. Use @blockdev-backup instead.
> +#
> # Returns: - nothing on success
> # - If @device is not a valid block device, GenericError
> #
> @@ -1657,7 +1660,7 @@
> #
> ##
> { 'command': 'drive-backup', 'boxed': true,
> - 'data': 'DriveBackup' }
> + 'data': 'DriveBackup', 'features': ['deprecated'] }
>
> ##
> # @blockdev-backup:
> --
> 2.29.2
>
>
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 :|
