Re: [Qemu-devel] [PATCH v2 10/11] block/backup: support bitmap sync modes for non-bitmap backups
On 7/16/19 1:18 AM, Markus Armbruster wrote: > John Snow writes: > >> Accept bitmaps and sync policies for the other backup modes. >> This allows us to do things like create a bitmap synced to a full backup >> without a transaction, or start a resumable backup process. >> >> Some combinations don't make sense, though: >> >> - NEVER policy combined with any non-BITMAP mode doesn't do anything, >> because the bitmap isn't used for input or output. >> It's harmless, but is almost certainly never what the user wanted. >> >> - sync=NONE is more questionable. It can't use on-success because this >> job never completes with success anyway, and the resulting artifact >> of 'always' is suspect: because we start with a full bitmap and only >> copy out segments that get written to, the final output bitmap will >> always be ... a fully set bitmap. >> >> Maybe there's contexts in which bitmaps make sense for sync=none, >> but not without more severe changes to the current job, and omitting >> it here doesn't prevent us from adding it later. >> >> Signed-off-by: John Snow >> --- > [...] >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 5a578806c5..099e4f37b2 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1352,13 +1352,15 @@ >> # @speed: the maximum speed, in bytes per second. The default is 0, >> # for unlimited. >> # >> -# @bitmap: the name of a dirty bitmap if sync is "bitmap" or "incremental". >> +# @bitmap: The name of a dirty bitmap to use. >> # Must be present if sync is "bitmap" or "incremental". >> +# Can be present if sync is "full" or "top". >> # Must not be present otherwise. >> # (Since 2.4 (drive-backup), 3.1 (blockdev-backup)) >> # >> # @bitmap-mode: Specifies the type of data the bitmap should contain after >> -# the operation concludes. Must be present if sync is >> "bitmap". >> +# the operation concludes. >> +# Must be present if a bitmap was provided, >> # Must NOT be present otherwise. (Since 4.2) >> # >> # @compress: true to compress data, if the target format supports it. > > Do you expect management applications will want to know about the > presence of this patch? > It's all going to be queued up for 4.2, so management can gate on the presence of the "bitmap-mode" field. The text being replaced here is intermediate only. Bitmaps can't be used with traditional backup modes without the bitmap-mode parameter, so I believe this is OK. --js
Re: [Qemu-devel] [PATCH v2 10/11] block/backup: support bitmap sync modes for non-bitmap backups
On 16.07.19 02:01, John Snow wrote:
> Accept bitmaps and sync policies for the other backup modes.
> This allows us to do things like create a bitmap synced to a full backup
> without a transaction, or start a resumable backup process.
>
> Some combinations don't make sense, though:
>
> - NEVER policy combined with any non-BITMAP mode doesn't do anything,
> because the bitmap isn't used for input or output.
> It's harmless, but is almost certainly never what the user wanted.
>
> - sync=NONE is more questionable. It can't use on-success because this
> job never completes with success anyway, and the resulting artifact
> of 'always' is suspect: because we start with a full bitmap and only
> copy out segments that get written to, the final output bitmap will
> always be ... a fully set bitmap.
>
> Maybe there's contexts in which bitmaps make sense for sync=none,
> but not without more severe changes to the current job, and omitting
> it here doesn't prevent us from adding it later.
>
> Signed-off-by: John Snow
> ---
> block/backup.c | 8 +---
> blockdev.c | 22 ++
> qapi/block-core.json | 6 --
> 3 files changed, 27 insertions(+), 9 deletions(-)
Reviewed-by: Max Reitz
(I’ve seen Markus’s concern, but I think management applications can
just see whether specifying sync={full,top} + bitmap works if they want
to use it.)
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 10/11] block/backup: support bitmap sync modes for non-bitmap backups
John Snow writes: > Accept bitmaps and sync policies for the other backup modes. > This allows us to do things like create a bitmap synced to a full backup > without a transaction, or start a resumable backup process. > > Some combinations don't make sense, though: > > - NEVER policy combined with any non-BITMAP mode doesn't do anything, > because the bitmap isn't used for input or output. > It's harmless, but is almost certainly never what the user wanted. > > - sync=NONE is more questionable. It can't use on-success because this > job never completes with success anyway, and the resulting artifact > of 'always' is suspect: because we start with a full bitmap and only > copy out segments that get written to, the final output bitmap will > always be ... a fully set bitmap. > > Maybe there's contexts in which bitmaps make sense for sync=none, > but not without more severe changes to the current job, and omitting > it here doesn't prevent us from adding it later. > > Signed-off-by: John Snow > --- [...] > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 5a578806c5..099e4f37b2 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1352,13 +1352,15 @@ > # @speed: the maximum speed, in bytes per second. The default is 0, > # for unlimited. > # > -# @bitmap: the name of a dirty bitmap if sync is "bitmap" or "incremental". > +# @bitmap: The name of a dirty bitmap to use. > # Must be present if sync is "bitmap" or "incremental". > +# Can be present if sync is "full" or "top". > # Must not be present otherwise. > # (Since 2.4 (drive-backup), 3.1 (blockdev-backup)) > # > # @bitmap-mode: Specifies the type of data the bitmap should contain after > -# the operation concludes. Must be present if sync is "bitmap". > +# the operation concludes. > +# Must be present if a bitmap was provided, > # Must NOT be present otherwise. (Since 4.2) > # > # @compress: true to compress data, if the target format supports it. Do you expect management applications will want to know about the presence of this patch?
