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 <vsement...@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 :|
