Re: [Qemu-block] [PATCH v3] block: maintain persistent disabled bitmaps
On 2018-02-02 17:07, Vladimir Sementsov-Ogievskiy wrote: > To maintain load/store disabled bitmap there is new approach: > > - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored > - store enabled bitmaps as "auto" to qcow2 > - store disabled bitmaps without "auto" flag to qcow2 > - on qcow2 open load "auto" bitmaps as enabled and others >as disabled (except in_use bitmaps) > > Also, adjust iotests 165 and 176 appropriately. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- Thanks, applied to my block tree: https://github.com/XanClic/qemu/commits/block Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v3] block: maintain persistent disabled bitmaps
On 2018-02-02 17:18, Eric Blake wrote:
> On 02/02/2018 10:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> To maintain load/store disabled bitmap there is new approach:
>>
>> - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
>> - store enabled bitmaps as "auto" to qcow2
>> - store disabled bitmaps without "auto" flag to qcow2
>> - on qcow2 open load "auto" bitmaps as enabled and others
>>as disabled (except in_use bitmaps)
>>
>> Also, adjust iotests 165 and 176 appropriately.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>
>> +++ b/qemu-doc.texi
>> @@ -2749,6 +2749,13 @@ used and it will be removed with no replacement.
>> The ``convert -s snapshot_id_or_name'' argument is obsoleted
>> by the ``convert -l snapshot_param'' argument instead.
>>
>> +@section QEMU Machine Protocol (QMP) commands
>> +
>> +@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
>> +
>> +"autoload" parameter is now ignored. All bitmaps are automatically loaded
>> +from qcow2 image.
>
> Won't later patches be adding the ability to enable/disable bitmaps,
> which then affects whether they are autoloaded? So we don't forget to
> revisit this text in that patch, a better wording might be:
>
> The "autoload" parameter is ignored; all enabled persistent dirty
> bitmaps are automatically loaded from a qcow2 image, regardless of the
> initial setting requested in this parameter.
>
>
>> @@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
>> }
>>
>> /* Called with BQL taken. */
>> -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
>> -{
>> -qemu_mutex_lock(bitmap->mutex);
>> -bitmap->autoload = autoload;
>> -qemu_mutex_unlock(bitmap->mutex);
>> -}
>> -
>> -bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
>> -{
>> -return bitmap->autoload;
>> -}
>
> Will later patches be reintroducing these functions for learning which
> bitmaps are enabled/disabled? But I'm okay with deleting them in this
> patch, even if that is more churn.
You mean bdrv_enable_dirty_bitmap(), bdrv_disable_dirty_bitmap(), and
bdrv_dirty_bitmap_enabled()? ;-)
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v3] block: maintain persistent disabled bitmaps
02.02.2018 19:18, Eric Blake wrote:
On 02/02/2018 10:07 AM, Vladimir Sementsov-Ogievskiy wrote:
To maintain load/store disabled bitmap there is new approach:
- deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
- store enabled bitmaps as "auto" to qcow2
- store disabled bitmaps without "auto" flag to qcow2
- on qcow2 open load "auto" bitmaps as enabled and others
as disabled (except in_use bitmaps)
Also, adjust iotests 165 and 176 appropriately.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
+++ b/qemu-doc.texi
@@ -2749,6 +2749,13 @@ used and it will be removed with no replacement.
The ``convert -s snapshot_id_or_name'' argument is obsoleted
by the ``convert -l snapshot_param'' argument instead.
+@section QEMU Machine Protocol (QMP) commands
+
+@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
+
+"autoload" parameter is now ignored. All bitmaps are automatically loaded
+from qcow2 image.
Won't later patches be adding the ability to enable/disable bitmaps,
which then affects whether they are autoloaded? So we don't forget to
revisit this text in that patch, a better wording might be:
The "autoload" parameter is ignored; all enabled persistent dirty
bitmaps are automatically loaded from a qcow2 image, regardless of the
initial setting requested in this parameter.
hmm.. no. all bitmaps are loaded, even disabled ones. Before this patch
there is
no way to have disabled bitmap in qemu (by loading or by creating).
After the patch, we have a theoretical way of creating such bitmap in
qcow2 image, then
it will be loaded as disabled.
Also, we can store bitmap with persistent=true and autoload=false before
this patch, and there is no way
to load this bitmap before this patch, but after this patch it will be
loaded as disabled.
@@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
}
/* Called with BQL taken. */
-void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
-{
-qemu_mutex_lock(bitmap->mutex);
-bitmap->autoload = autoload;
-qemu_mutex_unlock(bitmap->mutex);
-}
-
-bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
-{
-return bitmap->autoload;
-}
Will later patches be reintroducing these functions for learning which
bitmaps are enabled/disabled? But I'm okay with deleting them in this
patch, even if that is more churn.
no, actually the aim of the patch is to drop buggy relation between
autoload qmp parameter
and auto qcow2 flag (which is more like "enabled" then "autoload"). Look
at "Reasoning" in head
letter for details.
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v3] block: maintain persistent disabled bitmaps
On 02/02/2018 10:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> To maintain load/store disabled bitmap there is new approach:
>
> - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
> - store enabled bitmaps as "auto" to qcow2
> - store disabled bitmaps without "auto" flag to qcow2
> - on qcow2 open load "auto" bitmaps as enabled and others
>as disabled (except in_use bitmaps)
>
> Also, adjust iotests 165 and 176 appropriately.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> +++ b/qemu-doc.texi
> @@ -2749,6 +2749,13 @@ used and it will be removed with no replacement.
> The ``convert -s snapshot_id_or_name'' argument is obsoleted
> by the ``convert -l snapshot_param'' argument instead.
>
> +@section QEMU Machine Protocol (QMP) commands
> +
> +@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> +
> +"autoload" parameter is now ignored. All bitmaps are automatically loaded
> +from qcow2 image.
Won't later patches be adding the ability to enable/disable bitmaps,
which then affects whether they are autoloaded? So we don't forget to
revisit this text in that patch, a better wording might be:
The "autoload" parameter is ignored; all enabled persistent dirty
bitmaps are automatically loaded from a qcow2 image, regardless of the
initial setting requested in this parameter.
> @@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
> }
>
> /* Called with BQL taken. */
> -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
> -{
> -qemu_mutex_lock(bitmap->mutex);
> -bitmap->autoload = autoload;
> -qemu_mutex_unlock(bitmap->mutex);
> -}
> -
> -bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
> -{
> -return bitmap->autoload;
> -}
Will later patches be reintroducing these functions for learning which
bitmaps are enabled/disabled? But I'm okay with deleting them in this
patch, even if that is more churn.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-block] [PATCH v3] block: maintain persistent disabled bitmaps
To maintain load/store disabled bitmap there is new approach: - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored - store enabled bitmaps as "auto" to qcow2 - store disabled bitmaps without "auto" flag to qcow2 - on qcow2 open load "auto" bitmaps as enabled and others as disabled (except in_use bitmaps) Also, adjust iotests 165 and 176 appropriately. Signed-off-by: Vladimir Sementsov-Ogievskiy --- v3: fix iotests add info into qemu-doc.texi rm John's r-b (sorry=( v2: add John's r-b fix spelling bitmaps-api and bitmaps-postcopy migrations depend on this patch, so let's discuss (and merge if in case of consent) as soon as possible. Reasoning: Firstly, consider only enabled bitmaps (for now, actually, we can't get disabled bitmap in Qemu, as block-dirty-bitmap-enable command is not merged yet). We (Virtuozzo) always use persistent=true and autoload=true. As, to maintain dirty bitmap (enabled) valid, we must to load it every time, when we load our disk for r/w, to track disk changes. I do not think, that something like "not loading enabled dirty bitmaps, when open disk for read-only" gives real benefits (and now it is not possible, anyway, as loading depends on qcow2 bitmap "auto" flag, not on r-w mode). So, this flag is useless for now. Moreover, if we save bitmap with autoload=false, it will not be loaded next time, and will become invalid on first write to the disk. Creating bitmap with flags "persistent=true, autoload=false", actually means "make it disabled after storing" (weird semantic, isn't it?), so it will not be automatically updated more. So, this flag is a bit dangerous. Let's move to disabled bitmaps. Assume, that my patches will be merged, and we will really have a possibility of enable/disable bitmaps when we want. So, it's natural to expect, that if we have persistent-enabled bitmaps and persistent-disabled bitmaps, then enabled one will be enabled on next Qemu start and disabled will be disabled. How to achieve this? Let's start from qcow2. We need a stored information on "is this bitmap enabled or not". But we actually have it. Let's look on qcow2 "auto" flag definiton: 1: auto The bitmap must reflect all changes of the virtual disk by any application that would write to this qcow2 file (including writes, snapshot switching, etc.). The type of this bitmap must be 'dirty tracking bitmap'. Isn't it a definition of "enabled" bitmap? And yes, current mapping from qapi flag "autoload" to qcow2 flag "auto" is not good mapping. So, it looks ok, to map !BdrvDirtyBitmap.disabled to "auto". If we have in future some bitmaps in qcow2, which are not enabled and not disabled (something more complicated), we can introduce new flags or even new bitmap type. (from qcow2 spec): 16:type This field describes the sort of the bitmap. Values: 1: Dirty tracking bitmap Values 0, 2 - 255 are reserved. So, it looks like we _do not need_ qcow2 format changing for now. It already maintain enabled (auto=true) and disabled (auto=false) bitmaps (may be, additional note in spec about mapping of this flag in Qemu will be not bad) And actually, we do not have anything about "load this bitmap or not" in qcow2. And I do not think that we need. Qemu (and user) should decide, which bitmaps to load. (and it is obvious, that we must load "auto" bitmaps, to not break them) Then about qapi. What will occur, if we store disabled-persistent-autolading bitmap? It will be enabled on next Qemu start! And it shows that mapping "autoload"->"auto" is definitely bad. So, as we do not want information on "load or not the bitmap" in qcow2 (ok, I don't want, but I think Kevin and Max will agree, as it keeps qcow2 format more generic and separated from Qemu specifics), we see again, that "autoload" is useless, dangerous and wrong. If we agreed, that for now auto = !BdrvDirtyBitmap.disabled and "autoload" is deprecated, we need to decide, which disabled bitmaps we want to load. The simplest way to solve the problem is to load all bitmaps, mapping BdrvDirtyBitmap.disabled = !auto. In future, if we need, we'll be able to introduce some cmd-line flags to select disabled bitmaps for loading or separate qmp-command to load them (and do not load them on start). Or we can go another way, and continue loading all disabled bitmaps, but in "lazy mode", so bitmap is not actually loaded, only its name and some metadata. And we can actually load it, if user enables or exports it. It looks very interesting approach, as we do not lose RAM on (possibly) a lot of not needed bitmaps, but we can manage them (for example, remove). Any way, loading all bitmaps looks like a good start. qemu-doc.texi| 7 +++ qapi/block-core.json | 6 +++--- block/qcow2.h| 2 +- include/block/dirty-bitmap.h | 1 - block/dirty-bitmap.c | 18 -- block/qcow2-bitmap.c | 12 +++- block/qcow2.c
