Bitmaps data is not critical, and we should not fail the migration (or
use postcopy recovering) because of dirty-bitmaps migration failure.
Instead we should just lose unfinished bitmaps.
Still we have to report io stream violation errors, as they affect the
whole migration stream.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
migration/block-dirty-bitmap.c | 152 +++++++++++++++++++++++++--------
1 file changed, 117 insertions(+), 35 deletions(-)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index eb4ffeac4d..c24d4614bf 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -145,6 +145,15 @@ typedef struct DBMLoadState {
bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
+ /*
+ * cancelled
+ * Incoming migration is cancelled for some reason. That means that we
+ * still should read our chunks from migration stream, to not affect other
+ * migration objects (like RAM), but just ignore them and do not touch any
+ * bitmaps or nodes.
+ */
+ bool cancelled;
+
GSList *bitmaps;
QemuMutex lock; /* protect bitmaps */
} DBMLoadState;
@@ -531,6 +540,10 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
uint8_t flags = qemu_get_byte(f);
LoadBitmapState *b;
+ if (s->cancelled) {
+ return 0;
+ }
+
if (s->bitmap) {
error_report("Bitmap with the same name ('%s') already exists on "
"destination", bdrv_dirty_bitmap_name(s->bitmap));
@@ -613,13 +626,47 @@ void dirty_bitmap_mig_before_vm_start(void)
qemu_mutex_unlock(&s->lock);
}
+static void cancel_incoming_locked(DBMLoadState *s)
+{
+ GSList *item;
+
+ if (s->cancelled) {
+ return;
+ }
+
+ s->cancelled = true;
+ s->bs = NULL;
+ s->bitmap = NULL;
+
+ /* Drop all unfinished bitmaps */
+ for (item = s->bitmaps; item; item = g_slist_next(item)) {
+ LoadBitmapState *b = item->data;
+
+ /*
+ * Bitmap must be unfinished, as finished bitmaps should already be
+ * removed from the list.
+ */
+ assert(!s->before_vm_start_handled || !b->migrated);
+ if (bdrv_dirty_bitmap_has_successor(b->bitmap)) {
+ bdrv_reclaim_dirty_bitmap(b->bitmap, &error_abort);
+ }
+ bdrv_release_dirty_bitmap(b->bitmap);
+ }
+
+ g_slist_free_full(s->bitmaps, g_free);
+ s->bitmaps = NULL;
+}
+
static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
{
GSList *item;
trace_dirty_bitmap_load_complete();
- bdrv_dirty_bitmap_deserialize_finish(s->bitmap);
- qemu_mutex_lock(&s->lock);
+ if (s->cancelled) {
+ return;
+ }
+
+ bdrv_dirty_bitmap_deserialize_finish(s->bitmap);
if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
@@ -637,8 +684,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
break;
}
}
-
- qemu_mutex_unlock(&s->lock);
}
static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
@@ -650,15 +695,32 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
trace_dirty_bitmap_load_bits_zeroes();
- bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte, nr_bytes,
- false);
+ if (!s->cancelled) {
+ bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte,
+ nr_bytes, false);
+ }
} else {
size_t ret;
uint8_t *buf;
uint64_t buf_size = qemu_get_be64(f);
- uint64_t needed_size =
- bdrv_dirty_bitmap_serialization_size(s->bitmap,
- first_byte, nr_bytes);
+ uint64_t needed_size;
+
+ buf = g_malloc(buf_size);
+ ret = qemu_get_buffer(f, buf, buf_size);
+ if (ret != buf_size) {
+ error_report("Failed to read bitmap bits");
+ g_free(buf);
+ return -EIO;
+ }
+
+ if (s->cancelled) {
+ g_free(buf);
+ return 0;
+ }
+
+ needed_size = bdrv_dirty_bitmap_serialization_size(s->bitmap,
+ first_byte,
+ nr_bytes);
if (needed_size > buf_size ||
buf_size > QEMU_ALIGN_UP(needed_size, 4 * sizeof(long))
@@ -667,15 +729,8 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
error_report("Migrated bitmap granularity doesn't "
"match the destination bitmap '%s' granularity",
bdrv_dirty_bitmap_name(s->bitmap));
- return -EINVAL;
- }
-
- buf = g_malloc(buf_size);
- ret = qemu_get_buffer(f, buf, buf_size);
- if (ret != buf_size) {
- error_report("Failed to read bitmap bits");
- g_free(buf);
- return -EIO;
+ cancel_incoming_locked(s);
+ return 0;
}
bdrv_dirty_bitmap_deserialize_part(s->bitmap, buf, first_byte, nr_bytes,
@@ -700,14 +755,16 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
error_report("Unable to read node name string");
return -EINVAL;
}
- s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
- if (!s->bs) {
- error_report_err(local_err);
- return -EINVAL;
+ if (!s->cancelled) {
+ s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
+ if (!s->bs) {
+ error_report_err(local_err);
+ cancel_incoming_locked(s);
+ }
}
- } else if (!s->bs && !nothing) {
+ } else if (!s->bs && !nothing && !s->cancelled) {
error_report("Error: block device name is not set");
- return -EINVAL;
+ cancel_incoming_locked(s);
}
if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
@@ -715,24 +772,38 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
error_report("Unable to read bitmap name string");
return -EINVAL;
}
- s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
-
- /* bitmap may be NULL here, it wouldn't be an error if it is the
- * first occurrence of the bitmap */
- if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
- error_report("Error: unknown dirty bitmap "
- "'%s' for block device '%s'",
- s->bitmap_name, s->node_name);
- return -EINVAL;
+ if (!s->cancelled) {
+ s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
+
+ /*
+ * bitmap may be NULL here, it wouldn't be an error if it is the
+ * first occurrence of the bitmap
+ */
+ if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
+ error_report("Error: unknown dirty bitmap "
+ "'%s' for block device '%s'",
+ s->bitmap_name, s->node_name);
+ cancel_incoming_locked(s);
+ }
}
- } else if (!s->bitmap && !nothing) {
+ } else if (!s->bitmap && !nothing && !s->cancelled) {
error_report("Error: block device name is not set");
- return -EINVAL;
+ cancel_incoming_locked(s);
}
return 0;
}
+/*
+ * dirty_bitmap_load
+ *
+ * Load sequence of dirty bitmap chunks. Return error only on fatal io stream
+ * violations. On other errors just cancel bitmaps incoming migration and return
+ * 0.
+ *
+ * Note, than when incoming bitmap migration is canceled, we still must read all
+ * our chunks (and just ignore them), to not affect other migration objects.
+ */
static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
{
DBMLoadState *s = &((DBMState *)opaque)->load;
@@ -741,12 +812,19 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
trace_dirty_bitmap_load_enter();
if (version_id != 1) {
+ qemu_mutex_lock(&s->lock);
+ cancel_incoming_locked(s);
+ qemu_mutex_unlock(&s->lock);
return -EINVAL;
}
do {
+ qemu_mutex_lock(&s->lock);
+
ret = dirty_bitmap_load_header(f, s);
if (ret < 0) {
+ cancel_incoming_locked(s);
+ qemu_mutex_unlock(&s->lock);
return ret;
}
@@ -763,8 +841,12 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
}
if (ret) {
+ cancel_incoming_locked(s);
+ qemu_mutex_unlock(&s->lock);
return ret;
}
+
+ qemu_mutex_unlock(&s->lock);
} while (!(s->flags & DIRTY_BITMAP_MIG_FLAG_EOS));
trace_dirty_bitmap_load_success();
--
2.21.0
On 7/24/20 3:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> Bitmaps data is not critical, and we should not fail the migration (or
> use postcopy recovering) because of dirty-bitmaps migration failure.
> Instead we should just lose unfinished bitmaps.
>
> Still we have to report io stream violation errors, as they affect the
> whole migration stream.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> migration/block-dirty-bitmap.c | 152 +++++++++++++++++++++++++--------
> 1 file changed, 117 insertions(+), 35 deletions(-)
>
> @@ -650,15 +695,32 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
>
> if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
> trace_dirty_bitmap_load_bits_zeroes();
> - bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte, nr_bytes,
> - false);
> + if (!s->cancelled) {
> + bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte,
> + nr_bytes, false);
> + }
> } else {
> size_t ret;
> uint8_t *buf;
> uint64_t buf_size = qemu_get_be64(f);
Pre-existing, but if I understand, we are reading a value from the
migration stream...
> - uint64_t needed_size =
> - bdrv_dirty_bitmap_serialization_size(s->bitmap,
> - first_byte, nr_bytes);
> + uint64_t needed_size;
> +
> + buf = g_malloc(buf_size);
> + ret = qemu_get_buffer(f, buf, buf_size);
...and using it to malloc memory. Is that a potential risk of a
malicious stream causing us to allocate too much memory in relation to
the guest's normal size? If so, fixing that should be done separately.
I'm not a migration expert, but the patch looks reasonable to me.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
24.07.2020 20:35, Eric Blake wrote:
> On 7/24/20 3:43 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Bitmaps data is not critical, and we should not fail the migration (or
>> use postcopy recovering) because of dirty-bitmaps migration failure.
>> Instead we should just lose unfinished bitmaps.
>>
>> Still we have to report io stream violation errors, as they affect the
>> whole migration stream.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> migration/block-dirty-bitmap.c | 152 +++++++++++++++++++++++++--------
>> 1 file changed, 117 insertions(+), 35 deletions(-)
>>
>
>> @@ -650,15 +695,32 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
>> if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
>> trace_dirty_bitmap_load_bits_zeroes();
>> - bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte, nr_bytes,
>> - false);
>> + if (!s->cancelled) {
>> + bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte,
>> + nr_bytes, false);
>> + }
>> } else {
>> size_t ret;
>> uint8_t *buf;
>> uint64_t buf_size = qemu_get_be64(f);
>
> Pre-existing, but if I understand, we are reading a value from the migration stream...
Hmm, actually, this becomes worse after patch, as before patch we had the check, that the size at least corresponds to the bitmap.. But we want to relax things in cancelled mode (and we may not have any bitmap). Most correct thing is to use read in a loop to just skip the data from stream if we are in cancelled mode (something like nbd_drop()).
I can fix this with a follow-up patch.
>
>> - uint64_t needed_size =
>> - bdrv_dirty_bitmap_serialization_size(s->bitmap,
>> - first_byte, nr_bytes);
>> + uint64_t needed_size;
>> +
>> + buf = g_malloc(buf_size);
>> + ret = qemu_get_buffer(f, buf, buf_size);
>
> ...and using it to malloc memory. Is that a potential risk of a malicious stream causing us to allocate too much memory in relation to the guest's normal size? If so, fixing that should be done separately.
>
> I'm not a migration expert, but the patch looks reasonable to me.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
--
Best regards,
Vladimir
* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> 24.07.2020 20:35, Eric Blake wrote:
> > On 7/24/20 3:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > Bitmaps data is not critical, and we should not fail the migration (or
> > > use postcopy recovering) because of dirty-bitmaps migration failure.
> > > Instead we should just lose unfinished bitmaps.
> > >
> > > Still we have to report io stream violation errors, as they affect the
> > > whole migration stream.
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > > Â migration/block-dirty-bitmap.c | 152 +++++++++++++++++++++++++--------
> > > Â 1 file changed, 117 insertions(+), 35 deletions(-)
> > >
> >
> > > @@ -650,15 +695,32 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
> > > Â Â Â Â Â if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
> > > Â Â Â Â Â Â Â Â Â trace_dirty_bitmap_load_bits_zeroes();
> > > -Â Â Â Â Â Â Â bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte, nr_bytes,
> > > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â false);
> > > +Â Â Â Â Â Â Â if (!s->cancelled) {
> > > +Â Â Â Â Â Â Â Â Â Â Â bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte,
> > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â nr_bytes, false);
> > > +Â Â Â Â Â Â Â }
> > > Â Â Â Â Â } else {
> > > Â Â Â Â Â Â Â Â Â size_t ret;
> > > Â Â Â Â Â Â Â Â Â uint8_t *buf;
> > > Â Â Â Â Â Â Â Â Â uint64_t buf_size = qemu_get_be64(f);
> >
> > Pre-existing, but if I understand, we are reading a value from the migration stream...
>
> Hmm, actually, this becomes worse after patch, as before patch we had the check, that the size at least corresponds to the bitmap.. But we want to relax things in cancelled mode (and we may not have any bitmap). Most correct thing is to use read in a loop to just skip the data from stream if we are in cancelled mode (something like nbd_drop()).
>
> I can fix this with a follow-up patch.
If the size is bogus, it's probably not worth trying to skip anything
because it could be just a broken/misaligned stream.
Dave
> >
> > > -Â Â Â Â Â Â Â uint64_t needed_size =
> > > -Â Â Â Â Â Â Â Â Â Â Â bdrv_dirty_bitmap_serialization_size(s->bitmap,
> > > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â first_byte, nr_bytes);
> > > +Â Â Â Â Â Â Â uint64_t needed_size;
> > > +
> > > +Â Â Â Â Â Â Â buf = g_malloc(buf_size);
> > > +Â Â Â Â Â Â Â ret = qemu_get_buffer(f, buf, buf_size);
> >
> > ...and using it to malloc memory. Is that a potential risk of a malicious stream causing us to allocate too much memory in relation to the guest's normal size? If so, fixing that should be done separately.
> >
> > I'm not a migration expert, but the patch looks reasonable to me.
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> >
>
>
> --
> Best regards,
> Vladimir
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
27.07.2020 14:16, Dr. David Alan Gilbert wrote:
> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
>> 24.07.2020 20:35, Eric Blake wrote:
>>> On 7/24/20 3:43 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Bitmaps data is not critical, and we should not fail the migration (or
>>>> use postcopy recovering) because of dirty-bitmaps migration failure.
>>>> Instead we should just lose unfinished bitmaps.
>>>>
>>>> Still we have to report io stream violation errors, as they affect the
>>>> whole migration stream.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>> Â migration/block-dirty-bitmap.c | 152 +++++++++++++++++++++++++--------
>>>> Â 1 file changed, 117 insertions(+), 35 deletions(-)
>>>>
>>>
>>>> @@ -650,15 +695,32 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
>>>> Â Â Â Â Â if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
>>>> Â Â Â Â Â Â Â Â Â trace_dirty_bitmap_load_bits_zeroes();
>>>> -Â Â Â Â Â Â Â bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte, nr_bytes,
>>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â false);
>>>> +Â Â Â Â Â Â Â if (!s->cancelled) {
>>>> +Â Â Â Â Â Â Â Â Â Â Â bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte,
>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â nr_bytes, false);
>>>> +Â Â Â Â Â Â Â }
>>>> Â Â Â Â Â } else {
>>>> Â Â Â Â Â Â Â Â Â size_t ret;
>>>> Â Â Â Â Â Â Â Â Â uint8_t *buf;
>>>> Â Â Â Â Â Â Â Â Â uint64_t buf_size = qemu_get_be64(f);
>>>
>>> Pre-existing, but if I understand, we are reading a value from the migration stream...
>>
>> Hmm, actually, this becomes worse after patch, as before patch we had the check, that the size at least corresponds to the bitmap.. But we want to relax things in cancelled mode (and we may not have any bitmap). Most correct thing is to use read in a loop to just skip the data from stream if we are in cancelled mode (something like nbd_drop()).
>>
>> I can fix this with a follow-up patch.
>
> If the size is bogus, it's probably not worth trying to skip anything
> because it could be just a broken/misaligned stream.
>
The problem is that, when we are already in "skipping" mode, we don't have actual bitmap to understand, is size look reasonable or not. We can probably just invent some heuristic constant (100M for example?), so that any size less will be silently skipped, and any size above will be considered as stream violation and cancel postcopy process.
>
>>>
>>>> -Â Â Â Â Â Â Â uint64_t needed_size =
>>>> -Â Â Â Â Â Â Â Â Â Â Â bdrv_dirty_bitmap_serialization_size(s->bitmap,
>>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â first_byte, nr_bytes);
>>>> +Â Â Â Â Â Â Â uint64_t needed_size;
>>>> +
>>>> +Â Â Â Â Â Â Â buf = g_malloc(buf_size);
>>>> +Â Â Â Â Â Â Â ret = qemu_get_buffer(f, buf, buf_size);
>>>
>>> ...and using it to malloc memory. Is that a potential risk of a malicious stream causing us to allocate too much memory in relation to the guest's normal size? If so, fixing that should be done separately.
>>>
>>> I'm not a migration expert, but the patch looks reasonable to me.
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>
>>
>> --
>> Best regards,
>> Vladimir
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
--
Best regards,
Vladimir
* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> Bitmaps data is not critical, and we should not fail the migration (or
> use postcopy recovering) because of dirty-bitmaps migration failure.
> Instead we should just lose unfinished bitmaps.
>
> Still we have to report io stream violation errors, as they affect the
> whole migration stream.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> migration/block-dirty-bitmap.c | 152 +++++++++++++++++++++++++--------
> 1 file changed, 117 insertions(+), 35 deletions(-)
>
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index eb4ffeac4d..c24d4614bf 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -145,6 +145,15 @@ typedef struct DBMLoadState {
>
> bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
>
> + /*
> + * cancelled
> + * Incoming migration is cancelled for some reason. That means that we
> + * still should read our chunks from migration stream, to not affect other
> + * migration objects (like RAM), but just ignore them and do not touch any
> + * bitmaps or nodes.
> + */
> + bool cancelled;
> +
> GSList *bitmaps;
> QemuMutex lock; /* protect bitmaps */
> } DBMLoadState;
> @@ -531,6 +540,10 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
> uint8_t flags = qemu_get_byte(f);
> LoadBitmapState *b;
>
> + if (s->cancelled) {
> + return 0;
> + }
> +
> if (s->bitmap) {
> error_report("Bitmap with the same name ('%s') already exists on "
> "destination", bdrv_dirty_bitmap_name(s->bitmap));
> @@ -613,13 +626,47 @@ void dirty_bitmap_mig_before_vm_start(void)
> qemu_mutex_unlock(&s->lock);
> }
>
> +static void cancel_incoming_locked(DBMLoadState *s)
> +{
> + GSList *item;
> +
> + if (s->cancelled) {
> + return;
> + }
> +
> + s->cancelled = true;
> + s->bs = NULL;
> + s->bitmap = NULL;
> +
> + /* Drop all unfinished bitmaps */
> + for (item = s->bitmaps; item; item = g_slist_next(item)) {
> + LoadBitmapState *b = item->data;
> +
> + /*
> + * Bitmap must be unfinished, as finished bitmaps should already be
> + * removed from the list.
> + */
> + assert(!s->before_vm_start_handled || !b->migrated);
> + if (bdrv_dirty_bitmap_has_successor(b->bitmap)) {
> + bdrv_reclaim_dirty_bitmap(b->bitmap, &error_abort);
> + }
> + bdrv_release_dirty_bitmap(b->bitmap);
> + }
> +
> + g_slist_free_full(s->bitmaps, g_free);
> + s->bitmaps = NULL;
> +}
> +
> static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
> {
> GSList *item;
> trace_dirty_bitmap_load_complete();
> - bdrv_dirty_bitmap_deserialize_finish(s->bitmap);
>
> - qemu_mutex_lock(&s->lock);
> + if (s->cancelled) {
> + return;
> + }
> +
> + bdrv_dirty_bitmap_deserialize_finish(s->bitmap);
>
> if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
> bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
> @@ -637,8 +684,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
> break;
> }
> }
> -
> - qemu_mutex_unlock(&s->lock);
> }
>
> static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
> @@ -650,15 +695,32 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
>
> if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
> trace_dirty_bitmap_load_bits_zeroes();
> - bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte, nr_bytes,
> - false);
> + if (!s->cancelled) {
> + bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte,
> + nr_bytes, false);
> + }
> } else {
> size_t ret;
> uint8_t *buf;
> uint64_t buf_size = qemu_get_be64(f);
> - uint64_t needed_size =
> - bdrv_dirty_bitmap_serialization_size(s->bitmap,
> - first_byte, nr_bytes);
> + uint64_t needed_size;
> +
> + buf = g_malloc(buf_size);
> + ret = qemu_get_buffer(f, buf, buf_size);
> + if (ret != buf_size) {
> + error_report("Failed to read bitmap bits");
> + g_free(buf);
> + return -EIO;
> + }
> +
> + if (s->cancelled) {
> + g_free(buf);
> + return 0;
> + }
> +
> + needed_size = bdrv_dirty_bitmap_serialization_size(s->bitmap,
> + first_byte,
> + nr_bytes);
>
> if (needed_size > buf_size ||
> buf_size > QEMU_ALIGN_UP(needed_size, 4 * sizeof(long))
> @@ -667,15 +729,8 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
> error_report("Migrated bitmap granularity doesn't "
> "match the destination bitmap '%s' granularity",
> bdrv_dirty_bitmap_name(s->bitmap));
> - return -EINVAL;
> - }
> -
> - buf = g_malloc(buf_size);
> - ret = qemu_get_buffer(f, buf, buf_size);
> - if (ret != buf_size) {
> - error_report("Failed to read bitmap bits");
> - g_free(buf);
> - return -EIO;
> + cancel_incoming_locked(s);
> + return 0;
> }
>
> bdrv_dirty_bitmap_deserialize_part(s->bitmap, buf, first_byte, nr_bytes,
> @@ -700,14 +755,16 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
> error_report("Unable to read node name string");
> return -EINVAL;
> }
> - s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
> - if (!s->bs) {
> - error_report_err(local_err);
> - return -EINVAL;
> + if (!s->cancelled) {
> + s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
> + if (!s->bs) {
> + error_report_err(local_err);
> + cancel_incoming_locked(s);
> + }
> }
> - } else if (!s->bs && !nothing) {
> + } else if (!s->bs && !nothing && !s->cancelled) {
> error_report("Error: block device name is not set");
> - return -EINVAL;
> + cancel_incoming_locked(s);
> }
>
> if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> @@ -715,24 +772,38 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
> error_report("Unable to read bitmap name string");
> return -EINVAL;
> }
> - s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
> -
> - /* bitmap may be NULL here, it wouldn't be an error if it is the
> - * first occurrence of the bitmap */
> - if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
> - error_report("Error: unknown dirty bitmap "
> - "'%s' for block device '%s'",
> - s->bitmap_name, s->node_name);
> - return -EINVAL;
> + if (!s->cancelled) {
> + s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
> +
> + /*
> + * bitmap may be NULL here, it wouldn't be an error if it is the
> + * first occurrence of the bitmap
> + */
> + if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
> + error_report("Error: unknown dirty bitmap "
> + "'%s' for block device '%s'",
> + s->bitmap_name, s->node_name);
> + cancel_incoming_locked(s);
> + }
> }
> - } else if (!s->bitmap && !nothing) {
> + } else if (!s->bitmap && !nothing && !s->cancelled) {
> error_report("Error: block device name is not set");
> - return -EINVAL;
> + cancel_incoming_locked(s);
> }
>
> return 0;
> }
>
> +/*
> + * dirty_bitmap_load
> + *
> + * Load sequence of dirty bitmap chunks. Return error only on fatal io stream
> + * violations. On other errors just cancel bitmaps incoming migration and return
> + * 0.
> + *
> + * Note, than when incoming bitmap migration is canceled, we still must read all
> + * our chunks (and just ignore them), to not affect other migration objects.
> + */
> static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
> {
> DBMLoadState *s = &((DBMState *)opaque)->load;
> @@ -741,12 +812,19 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
> trace_dirty_bitmap_load_enter();
>
> if (version_id != 1) {
> + qemu_mutex_lock(&s->lock);
> + cancel_incoming_locked(s);
> + qemu_mutex_unlock(&s->lock);
> return -EINVAL;
> }
>
> do {
> + qemu_mutex_lock(&s->lock);
Would QEMU_LOCK_GUARD(&s->lock) work there?
It avoids the need to catch the unlock on each of the failure cases.
Dave
> ret = dirty_bitmap_load_header(f, s);
> if (ret < 0) {
> + cancel_incoming_locked(s);
> + qemu_mutex_unlock(&s->lock);
> return ret;
> }
>
> @@ -763,8 +841,12 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
> }
>
> if (ret) {
> + cancel_incoming_locked(s);
> + qemu_mutex_unlock(&s->lock);
> return ret;
> }
> +
> + qemu_mutex_unlock(&s->lock);
> } while (!(s->flags & DIRTY_BITMAP_MIG_FLAG_EOS));
>
> trace_dirty_bitmap_load_success();
> --
> 2.21.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
27.07.2020 14:23, Dr. David Alan Gilbert wrote:
> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
>> Bitmaps data is not critical, and we should not fail the migration (or
>> use postcopy recovering) because of dirty-bitmaps migration failure.
>> Instead we should just lose unfinished bitmaps.
>>
>> Still we have to report io stream violation errors, as they affect the
>> whole migration stream.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> migration/block-dirty-bitmap.c | 152 +++++++++++++++++++++++++--------
>> 1 file changed, 117 insertions(+), 35 deletions(-)
>>
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index eb4ffeac4d..c24d4614bf 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -145,6 +145,15 @@ typedef struct DBMLoadState {
>>
>> bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
>>
>> + /*
>> + * cancelled
>> + * Incoming migration is cancelled for some reason. That means that we
>> + * still should read our chunks from migration stream, to not affect other
>> + * migration objects (like RAM), but just ignore them and do not touch any
>> + * bitmaps or nodes.
>> + */
>> + bool cancelled;
>> +
>> GSList *bitmaps;
>> QemuMutex lock; /* protect bitmaps */
>> } DBMLoadState;
>> @@ -531,6 +540,10 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
>> uint8_t flags = qemu_get_byte(f);
>> LoadBitmapState *b;
>>
>> + if (s->cancelled) {
>> + return 0;
>> + }
>> +
>> if (s->bitmap) {
>> error_report("Bitmap with the same name ('%s') already exists on "
>> "destination", bdrv_dirty_bitmap_name(s->bitmap));
>> @@ -613,13 +626,47 @@ void dirty_bitmap_mig_before_vm_start(void)
>> qemu_mutex_unlock(&s->lock);
>> }
>>
>> +static void cancel_incoming_locked(DBMLoadState *s)
>> +{
>> + GSList *item;
>> +
>> + if (s->cancelled) {
>> + return;
>> + }
>> +
>> + s->cancelled = true;
>> + s->bs = NULL;
>> + s->bitmap = NULL;
>> +
>> + /* Drop all unfinished bitmaps */
>> + for (item = s->bitmaps; item; item = g_slist_next(item)) {
>> + LoadBitmapState *b = item->data;
>> +
>> + /*
>> + * Bitmap must be unfinished, as finished bitmaps should already be
>> + * removed from the list.
>> + */
>> + assert(!s->before_vm_start_handled || !b->migrated);
>> + if (bdrv_dirty_bitmap_has_successor(b->bitmap)) {
>> + bdrv_reclaim_dirty_bitmap(b->bitmap, &error_abort);
>> + }
>> + bdrv_release_dirty_bitmap(b->bitmap);
>> + }
>> +
>> + g_slist_free_full(s->bitmaps, g_free);
>> + s->bitmaps = NULL;
>> +}
>> +
>> static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
>> {
>> GSList *item;
>> trace_dirty_bitmap_load_complete();
>> - bdrv_dirty_bitmap_deserialize_finish(s->bitmap);
>>
>> - qemu_mutex_lock(&s->lock);
>> + if (s->cancelled) {
>> + return;
>> + }
>> +
>> + bdrv_dirty_bitmap_deserialize_finish(s->bitmap);
>>
>> if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
>> bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
>> @@ -637,8 +684,6 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
>> break;
>> }
>> }
>> -
>> - qemu_mutex_unlock(&s->lock);
>> }
>>
>> static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
>> @@ -650,15 +695,32 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
>>
>> if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
>> trace_dirty_bitmap_load_bits_zeroes();
>> - bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte, nr_bytes,
>> - false);
>> + if (!s->cancelled) {
>> + bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte,
>> + nr_bytes, false);
>> + }
>> } else {
>> size_t ret;
>> uint8_t *buf;
>> uint64_t buf_size = qemu_get_be64(f);
>> - uint64_t needed_size =
>> - bdrv_dirty_bitmap_serialization_size(s->bitmap,
>> - first_byte, nr_bytes);
>> + uint64_t needed_size;
>> +
>> + buf = g_malloc(buf_size);
>> + ret = qemu_get_buffer(f, buf, buf_size);
>> + if (ret != buf_size) {
>> + error_report("Failed to read bitmap bits");
>> + g_free(buf);
>> + return -EIO;
>> + }
>> +
>> + if (s->cancelled) {
>> + g_free(buf);
>> + return 0;
>> + }
>> +
>> + needed_size = bdrv_dirty_bitmap_serialization_size(s->bitmap,
>> + first_byte,
>> + nr_bytes);
>>
>> if (needed_size > buf_size ||
>> buf_size > QEMU_ALIGN_UP(needed_size, 4 * sizeof(long))
>> @@ -667,15 +729,8 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
>> error_report("Migrated bitmap granularity doesn't "
>> "match the destination bitmap '%s' granularity",
>> bdrv_dirty_bitmap_name(s->bitmap));
>> - return -EINVAL;
>> - }
>> -
>> - buf = g_malloc(buf_size);
>> - ret = qemu_get_buffer(f, buf, buf_size);
>> - if (ret != buf_size) {
>> - error_report("Failed to read bitmap bits");
>> - g_free(buf);
>> - return -EIO;
>> + cancel_incoming_locked(s);
>> + return 0;
>> }
>>
>> bdrv_dirty_bitmap_deserialize_part(s->bitmap, buf, first_byte, nr_bytes,
>> @@ -700,14 +755,16 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
>> error_report("Unable to read node name string");
>> return -EINVAL;
>> }
>> - s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
>> - if (!s->bs) {
>> - error_report_err(local_err);
>> - return -EINVAL;
>> + if (!s->cancelled) {
>> + s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
>> + if (!s->bs) {
>> + error_report_err(local_err);
>> + cancel_incoming_locked(s);
>> + }
>> }
>> - } else if (!s->bs && !nothing) {
>> + } else if (!s->bs && !nothing && !s->cancelled) {
>> error_report("Error: block device name is not set");
>> - return -EINVAL;
>> + cancel_incoming_locked(s);
>> }
>>
>> if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
>> @@ -715,24 +772,38 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
>> error_report("Unable to read bitmap name string");
>> return -EINVAL;
>> }
>> - s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
>> -
>> - /* bitmap may be NULL here, it wouldn't be an error if it is the
>> - * first occurrence of the bitmap */
>> - if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
>> - error_report("Error: unknown dirty bitmap "
>> - "'%s' for block device '%s'",
>> - s->bitmap_name, s->node_name);
>> - return -EINVAL;
>> + if (!s->cancelled) {
>> + s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
>> +
>> + /*
>> + * bitmap may be NULL here, it wouldn't be an error if it is the
>> + * first occurrence of the bitmap
>> + */
>> + if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
>> + error_report("Error: unknown dirty bitmap "
>> + "'%s' for block device '%s'",
>> + s->bitmap_name, s->node_name);
>> + cancel_incoming_locked(s);
>> + }
>> }
>> - } else if (!s->bitmap && !nothing) {
>> + } else if (!s->bitmap && !nothing && !s->cancelled) {
>> error_report("Error: block device name is not set");
>> - return -EINVAL;
>> + cancel_incoming_locked(s);
>> }
>>
>> return 0;
>> }
>>
>> +/*
>> + * dirty_bitmap_load
>> + *
>> + * Load sequence of dirty bitmap chunks. Return error only on fatal io stream
>> + * violations. On other errors just cancel bitmaps incoming migration and return
>> + * 0.
>> + *
>> + * Note, than when incoming bitmap migration is canceled, we still must read all
>> + * our chunks (and just ignore them), to not affect other migration objects.
>> + */
>> static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
>> {
>> DBMLoadState *s = &((DBMState *)opaque)->load;
>> @@ -741,12 +812,19 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
>> trace_dirty_bitmap_load_enter();
>>
>> if (version_id != 1) {
>> + qemu_mutex_lock(&s->lock);
>> + cancel_incoming_locked(s);
>> + qemu_mutex_unlock(&s->lock);
>> return -EINVAL;
>> }
>>
>> do {
>> + qemu_mutex_lock(&s->lock);
>
> Would QEMU_LOCK_GUARD(&s->lock) work there?
> It avoids the need to catch the unlock on each of the failure cases.
>
Yes it should work, thanks. I just didn't know about this thing.
>
>> ret = dirty_bitmap_load_header(f, s);
>> if (ret < 0) {
>> + cancel_incoming_locked(s);
>> + qemu_mutex_unlock(&s->lock);
>> return ret;
>> }
>>
>> @@ -763,8 +841,12 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
>> }
>>
>> if (ret) {
>> + cancel_incoming_locked(s);
>> + qemu_mutex_unlock(&s->lock);
>> return ret;
>> }
>> +
>> + qemu_mutex_unlock(&s->lock);
>> } while (!(s->flags & DIRTY_BITMAP_MIG_FLAG_EOS));
>>
>> trace_dirty_bitmap_load_success();
>> --
>> 2.21.0
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.