05.12.2019 23:09, Eric Blake wrote:
> On 12/5/19 1:30 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Here is double bug:
>>
>> First, return error but not set errp. This may lead to:
>> qmp block-dirty-bitmap-remove may report success when actually failed
>>
>> block-dirty-bitmap-remove used in a transaction will crash, as
>> qmp_transaction will think that it returned success and will cal
>
> call
>
>> block_dirty_bitmap_remove_commit which will crash, as state->bitmap is
>> NULL
>>
>> Second (like in anecdote), this case is not an error at all. As it is
>> documented in the comment above bdrv_co_remove_persistent_dirty_bitmap
>> definition, absence of bitmap is not an error, and similar case handled
>> at start of qcow2_co_remove_persistent_dirty_bitmap, it returns 0 when
>> there is no bitmaps at all..
>
> double .
>
>>
>> But when there are some bitmaps, but not the requested one, it return
>> error with errp unset.
>>
>> Fix that.
>>
>> Fixes: b56a1e31759b750
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
>> ---
>>
>> Hi all!
>>
>> Ohm, suddenly we faced this bug. It's a regression in 4.2. I'm very
>> sorry for introducing it, and it sad that it's found so late..
>>
>> Personally, I think that this one worth rc5, as it makes new bitmap
>> interfaces unusable. But the decision is yours.
>>
>> Last minute edit: hmm, actually, transaction action introduced in
>> 4.2, so crash is not a regression, only broken block-dirty-bitmap-remove
>> command is a regression... Maybe it's OK for stable.
>
> Libvirt REALLY wants to use transaction bitmap management (and require qemu 4.2) for its incremental backup patches that Peter is almost ready to merge in. I'm trying to ascertain:
Side question:
Is libvirt prepared to inconsistent bitmaps?
For example, migration with enabled dirty-bitmaps capability will fail if there are inconsistent bitmaps.
Also, incremental backup may be affected (you see, that you have your bitmap, try to do something with it,
aiming to start next incremental backup, but bitmap is inconsistent, and operation fails)..
In fact, we in Virtuozzo now faced with these broken migration and backup because of inconsistent bitmaps
(which were ignored in past), and we have to apply a temporary fix like
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -986,8 +986,8 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
BdrvDirtyBitmap *bitmap;
- if ((bm->flags & BME_FLAG_IN_USE) &&
- bdrv_find_dirty_bitmap(bs, bm->name))
+ if ((bm->flags & BME_FLAG_IN_USE) /* &&
+ bdrv_find_dirty_bitmap(bs, bm->name) */)
{
/*
* We already have corresponding BdrvDirtyBitmap, and bitmap in the
@@ -1000,6 +1000,13 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
* should not load it on migration target, as we already have this
* bitmap, being migrated.
*/
+
+ /*
+ * VZ7.12:
+ *
+ * We don't load inconsistent bitmaps at all, as they block
+ * migration. So the second condition is commented out.
+ */
continue;
}
And correct way is to handle inconsistent bitmaps in libvirt somehow.
Note, that the source of inconsistent bitmaps are crashes, hard resets, etc, when Qemu is unable to store bitmaps correctly.
>
> When exactly can this bug hit? Can you give a sequence of QMP commands that will trigger it for easy reproduction? Are there workarounds (such as checking that a bitmap exists prior to attempting to remove it)? If it does NOT get fixed in rc5, how will libvirt be able to probe whether the fix is in place?
>
>>
>> block/qcow2-bitmap.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 8abaf632fc..c6c8ebbe89 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1469,8 +1469,10 @@ int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> Qcow2BitmapList *bm_list;
>> if (s->nb_bitmaps == 0) {
>> - /* Absence of the bitmap is not an error: see explanation above
>> - * bdrv_remove_persistent_dirty_bitmap() definition. */
>> + /*
>> + * Absence of the bitmap is not an error: see explanation above
>> + * bdrv_co_remove_persistent_dirty_bitmap() definition.
>> + */
>> return 0;
>> }
>> @@ -1485,7 +1487,8 @@ int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> bm = find_bitmap_by_name(bm_list, name);
>> if (bm == NULL) {
>> - ret = -EINVAL;
>> + /* Absence of the bitmap is not an error, see above. */
>> + ret = 0;
>> goto out;
>> }
>>
>
--
Best regards,
Vladimir
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
