| Message ID | 20191014115126.15360-2-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix qcow2_can_store_new_dirty_bitmap | expand |
On 14.10.19 13:51, Vladimir Sementsov-Ogievskiy wrote: > qcow2_can_store_new_dirty_bitmap works wrong, as it considers only > bitmaps already stored in the qcow2 image and ignores persistent > BdrvDirtyBitmap objects. > > So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 > bitmaps on open, so there should not be any bitmap in the image for > which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of > corruption, and no reason to check for corruptions here (open() and > close() are better places for it). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2-bitmap.c | 41 ++++++++++++++++++----------------------- > 1 file changed, 18 insertions(+), 23 deletions(-) Am I right in interpreting qcow2_load_dirty_bitmaps() in such a way that every persistent dirty bitmap will cause a runtime dirty bitmap to be created? If so: Reviewed-by: Max Reitz <mreitz@redhat.com>
25.10.2019 15:50, Max Reitz wrote: > On 14.10.19 13:51, Vladimir Sementsov-Ogievskiy wrote: >> qcow2_can_store_new_dirty_bitmap works wrong, as it considers only >> bitmaps already stored in the qcow2 image and ignores persistent >> BdrvDirtyBitmap objects. >> >> So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 >> bitmaps on open, so there should not be any bitmap in the image for >> which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of >> corruption, and no reason to check for corruptions here (open() and >> close() are better places for it). >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2-bitmap.c | 41 ++++++++++++++++++----------------------- >> 1 file changed, 18 insertions(+), 23 deletions(-) > > Am I right in interpreting qcow2_load_dirty_bitmaps() in such a way that > every persistent dirty bitmap will cause a runtime dirty bitmap to be > created? Yes, we load all the bitmaps. Every bitmap stored in qcow2 image is loaded and corresponding BdrvDirtyBitmap is created. If bitmap has IN_USE flag in the image, created BdrvDirtyBitmap is marked inconsistent, but it is still there. If bitmap doesn't have AUTO flag, it becomes disabled bitmap. > > If so: > > Reviewed-by: Max Reitz <mreitz@redhat.com> >
On 14.10.19 13:51, Vladimir Sementsov-Ogievskiy wrote: > qcow2_can_store_new_dirty_bitmap works wrong, as it considers only > bitmaps already stored in the qcow2 image and ignores persistent > BdrvDirtyBitmap objects. > > So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 > bitmaps on open, so there should not be any bitmap in the image for > which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of > corruption, and no reason to check for corruptions here (open() and > close() are better places for it). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2-bitmap.c | 41 ++++++++++++++++++----------------------- > 1 file changed, 18 insertions(+), 23 deletions(-) Thanks, applied to my block-next branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next Max
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 98294a7696..d5131181a3 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1671,8 +1671,14 @@ bool coroutine_fn qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, Error **errp) { BDRVQcow2State *s = bs->opaque; - bool found; - Qcow2BitmapList *bm_list; + BdrvDirtyBitmap *bitmap; + uint64_t bitmap_directory_size = 0; + uint32_t nb_bitmaps = 0; + + if (bdrv_find_dirty_bitmap(bs, name)) { + error_setg(errp, "Bitmap already exists: %s", name); + return false; + } if (s->qcow_version < 3) { /* Without autoclear_features, we would always have to assume @@ -1688,38 +1694,27 @@ bool coroutine_fn qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, goto fail; } - if (s->nb_bitmaps == 0) { - return true; + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { + nb_bitmaps++; + bitmap_directory_size += + calc_dir_entry_size(strlen(bdrv_dirty_bitmap_name(bitmap)), 0); + } } + nb_bitmaps++; + bitmap_directory_size += calc_dir_entry_size(strlen(name), 0); - if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { + if (nb_bitmaps > QCOW2_MAX_BITMAPS) { error_setg(errp, "Maximum number of persistent bitmaps is already reached"); goto fail; } - if (s->bitmap_directory_size + calc_dir_entry_size(strlen(name), 0) > - QCOW2_MAX_BITMAP_DIRECTORY_SIZE) - { + if (bitmap_directory_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { error_setg(errp, "Not enough space in the bitmap directory"); goto fail; } - qemu_co_mutex_lock(&s->lock); - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, - s->bitmap_directory_size, errp); - qemu_co_mutex_unlock(&s->lock); - if (bm_list == NULL) { - goto fail; - } - - found = find_bitmap_by_name(bm_list, name); - bitmap_list_free(bm_list); - if (found) { - error_setg(errp, "Bitmap with the same name is already stored"); - goto fail; - } - return true; fail:
qcow2_can_store_new_dirty_bitmap works wrong, as it considers only bitmaps already stored in the qcow2 image and ignores persistent BdrvDirtyBitmap objects. So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2 bitmaps on open, so there should not be any bitmap in the image for which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of corruption, and no reason to check for corruptions here (open() and close() are better places for it). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2-bitmap.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-)