Re: [Qemu-devel] [PATCH] block: record new size in bdrv_dirty_bitmap_truncate

[Kevin Wolf <kwolf@redhat.com>]

Am 08.06.2015 um 22:49 hat John Snow geschrieben:
> ce1ffea8 neglected to update the BdrvDirtyBitmap structure
> itself for internal consistency. It's currently not an issue,
> but for migration and persistence series this will cause headaches.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

I know nothing about dirty bitmaps, but this one looks obvious enough,
I'll apply it.

> diff --git a/block.c b/block.c
> index 2b9ceae..2786e47 100644
> --- a/block.c
> +++ b/block.c
> @@ -3224,6 +3224,7 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>              continue;
>          }
>          hbitmap_truncate(bitmap->bitmap, size);
> +        bitmap->size = size;
>      }
>  }

However, I'm left wondering whether that 'continue' in the context of
that hunk is right. More context:

    QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
        if (bdrv_dirty_bitmap_frozen(bitmap)) {
            continue;
        }
        hbitmap_truncate(bitmap->bitmap, size);
    }

If the image just shrunk, the frozen bitmap covers parts of the image
that don't exist any more. When they are read out for the backup, that
request would fail.

If the image was extended, the frozen bitmap covers only part of the
image. There are a few bitmap functions that don't check the size and
would just work beyond the end of the bitmap if called with a now valid
sector number that is outside the image.

In practice, I don't think any of these happen because of op blockers
that prevent resizing while a backup is in progress, but should
!bdrv_dirty_bitmap_frozen(bitmap) be asserted then rather than just
skipping the bitmap?

Kevin