Re: [Qemu-devel] [RFC PATCH 2/2] block/dirty-bitmap: implement inconsistent bit
On 2/19/19 4:00 PM, John Snow wrote: >> >> hmm, I also think we should report our deprecated status as locked for >> inconsistent bitmaps.. >> >> > > Though we're trying to deprecate the field altogether, I *could* add a > special status flag that makes it unambiguous. This will catch the > attention of anyone using the old API. Adding to the enum, even though it is going away in the future, is still helpful in the present, so I can live with that approach. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC PATCH 2/2] block/dirty-bitmap: implement inconsistent bit
20.02.2019 1:00, John Snow wrote:
>
>
> On 2/18/19 1:13 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 14.02.2019 2:36, John Snow wrote:
>>> Signed-off-by: John Snow
>>> ---
>>>block/dirty-bitmap.c | 15 +
>>>block/qcow2-bitmap.c | 42 ++-
>>>blockdev.c | 43
>>>include/block/dirty-bitmap.h | 1 +
>>>4 files changed, 81 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index b1879d7fbd..06d8ee0d79 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -589,6 +589,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>> HBitmap **out)
>>>*out = backup;
>>>}
>>>bdrv_dirty_bitmap_unlock(bitmap);
>>> +bdrv_dirty_bitmap_set_inconsistent(bitmap, false);
>>>}
>>>
>>>void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
>>> @@ -776,6 +777,13 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap
>>> *bitmap,
>>>return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
>>>}
>>>
>>> +void bdrv_dirty_bitmap_add_inconsistent_hint(Error **errp)
>>> +{
>>> +error_append_hint(errp, "Try block-dirty-bitmap-clear to mark this "
>>> + "bitmap consistent again, or
>>> block-dirty-bitmap-remove "
>>> + "to delete it.");
>>
>> bitmaps created by libvirt (or somebody) are related to some checkpoint. And
>> their name is
>> probably (Eric?) related to this checkpoint too. So, clear will never make
>> them consistent..
>> Only clear :)
>>
>> So, I don't like idea of clearing in-use bitmaps.
>>
>>> +}
>>> +
>>>void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const
>>> BdrvDirtyBitmap *src,
>>> HBitmap **backup, Error **errp)
>>>{
>>> @@ -798,6 +806,13 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest,
>>> const BdrvDirtyBitmap *src,
>>>goto out;
>>>}
>>>
>>> +if (bdrv_dirty_bitmap_inconsistent(dest)) {
>>> +error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used
>>> as"
>>> + " a merge target", dest->name);
>>> +bdrv_dirty_bitmap_add_inconsistent_hint(errp);
>>> +goto out;
>>> +}
>>
>> I think, we need common predicate which will combine busy and inconsistent,
>> as almost in all cases
>> we need both to be false to do any operation.
>>
>> bdrv_dirty_bitmap_usable() ? :)
>>
>> and pass errp to this helper.
>>
>> Actually, we already need it, to fill errp, which we almost always fill in
>> the same manner.
>>
>>> +
>>>if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
>>>error_setg(errp, "Bitmaps are incompatible and can't be merged");
>>>goto out;
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 3ee524da4b..9bd8bc417f 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>
>> hmm, I also think we should report our deprecated status as locked for
>> inconsistent bitmaps..
>>
>>
>
> Though we're trying to deprecate the field altogether, I *could* add a
> special status flag that makes it unambiguous. This will catch the
> attention of anyone using the old API.
I agree.
--
Best regards,
Vladimir
Re: [Qemu-devel] [RFC PATCH 2/2] block/dirty-bitmap: implement inconsistent bit
On 2/18/19 1:13 PM, Vladimir Sementsov-Ogievskiy wrote:
> 14.02.2019 2:36, John Snow wrote:
>> Signed-off-by: John Snow
>> ---
>> block/dirty-bitmap.c | 15 +
>> block/qcow2-bitmap.c | 42 ++-
>> blockdev.c | 43
>> include/block/dirty-bitmap.h | 1 +
>> 4 files changed, 81 insertions(+), 20 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index b1879d7fbd..06d8ee0d79 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -589,6 +589,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>> HBitmap **out)
>> *out = backup;
>> }
>> bdrv_dirty_bitmap_unlock(bitmap);
>> +bdrv_dirty_bitmap_set_inconsistent(bitmap, false);
>> }
>>
>> void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
>> @@ -776,6 +777,13 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap
>> *bitmap,
>> return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
>> }
>>
>> +void bdrv_dirty_bitmap_add_inconsistent_hint(Error **errp)
>> +{
>> +error_append_hint(errp, "Try block-dirty-bitmap-clear to mark this "
>> + "bitmap consistent again, or
>> block-dirty-bitmap-remove "
>> + "to delete it.");
>
> bitmaps created by libvirt (or somebody) are related to some checkpoint. And
> their name is
> probably (Eric?) related to this checkpoint too. So, clear will never make
> them consistent..
> Only clear :)
>
> So, I don't like idea of clearing in-use bitmaps.
>
>> +}
>> +
>> void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap
>> *src,
>>HBitmap **backup, Error **errp)
>> {
>> @@ -798,6 +806,13 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest,
>> const BdrvDirtyBitmap *src,
>> goto out;
>> }
>>
>> +if (bdrv_dirty_bitmap_inconsistent(dest)) {
>> +error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used as"
>> + " a merge target", dest->name);
>> +bdrv_dirty_bitmap_add_inconsistent_hint(errp);
>> +goto out;
>> +}
>
> I think, we need common predicate which will combine busy and inconsistent,
> as almost in all cases
> we need both to be false to do any operation.
>
> bdrv_dirty_bitmap_usable() ? :)
>
> and pass errp to this helper.
>
> Actually, we already need it, to fill errp, which we almost always fill in
> the same manner.
>
>> +
>> if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
>> error_setg(errp, "Bitmaps are incompatible and can't be merged");
>> goto out;
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 3ee524da4b..9bd8bc417f 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>
> hmm, I also think we should report our deprecated status as locked for
> inconsistent bitmaps..
>
>
Though we're trying to deprecate the field altogether, I *could* add a
special status flag that makes it unambiguous. This will catch the
attention of anyone using the old API.
--js
Re: [Qemu-devel] [RFC PATCH 2/2] block/dirty-bitmap: implement inconsistent bit
On 2/18/19 3:37 PM, Eric Blake wrote:
> On 2/18/19 12:13 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 14.02.2019 2:36, John Snow wrote:
>>> Signed-off-by: John Snow
>>> ---
>>> block/dirty-bitmap.c | 15 +
>>> block/qcow2-bitmap.c | 42 ++-
>>> blockdev.c | 43
>>> include/block/dirty-bitmap.h | 1 +
>>> 4 files changed, 81 insertions(+), 20 deletions(-)
>
>>> +void bdrv_dirty_bitmap_add_inconsistent_hint(Error **errp)
>>> +{
>>> +error_append_hint(errp, "Try block-dirty-bitmap-clear to mark this "
>>> + "bitmap consistent again, or
>>> block-dirty-bitmap-remove "
>>> + "to delete it.");
>>
>> bitmaps created by libvirt (or somebody) are related to some checkpoint. And
>> their name is
>> probably (Eric?) related to this checkpoint too. So, clear will never make
>> them consistent..
>> Only clear :)
>>
>> So, I don't like idea of clearing in-use bitmaps.
>
> It's always possible to delete a bitmap and then create a new one by the
> same name, to get the same effect of clearing an in-use bitmap. So let's
> start simple and declare that the only valid operation on an
> inconsistent bitmap is deletion.
>
OK, from the viewpoint of checkpoints specifically, that's a convincing
argument against it for now.
I'll tighten this.
Re: [Qemu-devel] [RFC PATCH 2/2] block/dirty-bitmap: implement inconsistent bit
On 2/18/19 12:13 PM, Vladimir Sementsov-Ogievskiy wrote:
> 14.02.2019 2:36, John Snow wrote:
>> Signed-off-by: John Snow
>> ---
>> block/dirty-bitmap.c | 15 +
>> block/qcow2-bitmap.c | 42 ++-
>> blockdev.c | 43
>> include/block/dirty-bitmap.h | 1 +
>> 4 files changed, 81 insertions(+), 20 deletions(-)
>> +void bdrv_dirty_bitmap_add_inconsistent_hint(Error **errp)
>> +{
>> +error_append_hint(errp, "Try block-dirty-bitmap-clear to mark this "
>> + "bitmap consistent again, or
>> block-dirty-bitmap-remove "
>> + "to delete it.");
>
> bitmaps created by libvirt (or somebody) are related to some checkpoint. And
> their name is
> probably (Eric?) related to this checkpoint too. So, clear will never make
> them consistent..
> Only clear :)
>
> So, I don't like idea of clearing in-use bitmaps.
It's always possible to delete a bitmap and then create a new one by the
same name, to get the same effect of clearing an in-use bitmap. So let's
start simple and declare that the only valid operation on an
inconsistent bitmap is deletion.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC PATCH 2/2] block/dirty-bitmap: implement inconsistent bit
14.02.2019 2:36, John Snow wrote:
> Signed-off-by: John Snow
> ---
> block/dirty-bitmap.c | 15 +
> block/qcow2-bitmap.c | 42 ++-
> blockdev.c | 43
> include/block/dirty-bitmap.h | 1 +
> 4 files changed, 81 insertions(+), 20 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index b1879d7fbd..06d8ee0d79 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -589,6 +589,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> HBitmap **out)
> *out = backup;
> }
> bdrv_dirty_bitmap_unlock(bitmap);
> +bdrv_dirty_bitmap_set_inconsistent(bitmap, false);
> }
>
> void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
> @@ -776,6 +777,13 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap
> *bitmap,
> return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
> }
>
> +void bdrv_dirty_bitmap_add_inconsistent_hint(Error **errp)
> +{
> +error_append_hint(errp, "Try block-dirty-bitmap-clear to mark this "
> + "bitmap consistent again, or block-dirty-bitmap-remove
> "
> + "to delete it.");
bitmaps created by libvirt (or somebody) are related to some checkpoint. And
their name is
probably (Eric?) related to this checkpoint too. So, clear will never make them
consistent..
Only clear :)
So, I don't like idea of clearing in-use bitmaps.
> +}
> +
> void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap
> *src,
>HBitmap **backup, Error **errp)
> {
> @@ -798,6 +806,13 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest,
> const BdrvDirtyBitmap *src,
> goto out;
> }
>
> +if (bdrv_dirty_bitmap_inconsistent(dest)) {
> +error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used as"
> + " a merge target", dest->name);
> +bdrv_dirty_bitmap_add_inconsistent_hint(errp);
> +goto out;
> +}
I think, we need common predicate which will combine busy and inconsistent, as
almost in all cases
we need both to be false to do any operation.
bdrv_dirty_bitmap_usable() ? :)
and pass errp to this helper.
Actually, we already need it, to fill errp, which we almost always fill in the
same manner.
> +
> if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
> error_setg(errp, "Bitmaps are incompatible and can't be merged");
> goto out;
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 3ee524da4b..9bd8bc417f 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
hmm, I also think we should report our deprecated status as locked for
inconsistent bitmaps..
--
Best regards,
Vladimir
[Qemu-devel] [RFC PATCH 2/2] block/dirty-bitmap: implement inconsistent bit
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 15 +
block/qcow2-bitmap.c | 42 ++-
blockdev.c | 43
include/block/dirty-bitmap.h | 1 +
4 files changed, 81 insertions(+), 20 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index b1879d7fbd..06d8ee0d79 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -589,6 +589,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap,
HBitmap **out)
*out = backup;
}
bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmap_set_inconsistent(bitmap, false);
}
void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
@@ -776,6 +777,13 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap
*bitmap,
return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
}
+void bdrv_dirty_bitmap_add_inconsistent_hint(Error **errp)
+{
+error_append_hint(errp, "Try block-dirty-bitmap-clear to mark this "
+ "bitmap consistent again, or block-dirty-bitmap-remove "
+ "to delete it.");
+}
+
void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
HBitmap **backup, Error **errp)
{
@@ -798,6 +806,13 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const
BdrvDirtyBitmap *src,
goto out;
}
+if (bdrv_dirty_bitmap_inconsistent(dest)) {
+error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used as"
+ " a merge target", dest->name);
+bdrv_dirty_bitmap_add_inconsistent_hint(errp);
+goto out;
+}
+
if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
error_setg(errp, "Bitmaps are incompatible and can't be merged");
goto out;
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3ee524da4b..9bd8bc417f 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -343,9 +343,15 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
uint32_t granularity;
BdrvDirtyBitmap *bitmap = NULL;
+granularity = 1U << bm->granularity_bits;
+bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
+if (bitmap == NULL) {
+goto fail;
+}
+
if (bm->flags & BME_FLAG_IN_USE) {
-error_setg(errp, "Bitmap '%s' is in use", bm->name);
-goto fail;
+/* Data is unusable, skip loading it */
+return bitmap;
}
ret = bitmap_table_load(bs, >table, _table);
@@ -356,12 +362,6 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
goto fail;
}
-granularity = 1U << bm->granularity_bits;
-bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
-if (bitmap == NULL) {
-goto fail;
-}
-
ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not read bitmap '%s' from image",
@@ -962,20 +962,22 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error
**errp)
}
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-if (!(bm->flags & BME_FLAG_IN_USE)) {
-BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
-if (bitmap == NULL) {
-goto fail;
-}
+BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
+if (bitmap == NULL) {
+goto fail;
+}
+
+if (bm->flags & BME_FLAG_IN_USE) {
+bdrv_dirty_bitmap_set_inconsistent(bitmap, true);
+}
-if (!(bm->flags & BME_FLAG_AUTO)) {
-bdrv_disable_dirty_bitmap(bitmap);
-}
-bdrv_dirty_bitmap_set_persistance(bitmap, true);
-bm->flags |= BME_FLAG_IN_USE;
-created_dirty_bitmaps =
-g_slist_append(created_dirty_bitmaps, bitmap);
+if (!(bm->flags & BME_FLAG_AUTO)) {
+bdrv_disable_dirty_bitmap(bitmap);
}
+bdrv_dirty_bitmap_set_persistance(bitmap, true);
+bm->flags |= BME_FLAG_IN_USE;
+created_dirty_bitmaps =
+g_slist_append(created_dirty_bitmaps, bitmap);
}
if (created_dirty_bitmaps != NULL) {
diff --git a/blockdev.c b/blockdev.c
index 23a4bf136e..12c6f706dd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1943,6 +1943,7 @@ typedef struct BlockDirtyBitmapState {
HBitmap *backup;
bool prepared;
bool was_enabled;
+bool was_inconsistent;
} BlockDirtyBitmapState;
static void block_dirty_bitmap_add_prepare(BlkActionState *common,
@@ -2016,6 +2017,7 @@ static void
block_dirty_bitmap_clear_prepare(BlkActionState *common,
return;
}
+state->was_inconsistent = bdrv_dirty_bitmap_inconsistent(state->bitmap);
bdrv_clear_dirty_bitmap(state->bitmap, >backup);
}
@@ -2027,6 +2029,9 @@ static void block_dirty_bitmap_restore(BlkActionState
*common)
