| Message ID | 20190606184159.979-3-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/dirty-bitmap: check number and size constraints against queued bitmaps | expand |
On 6/6/19 1:41 PM, John Snow wrote: > Instead of bdrv_can_store_new_bitmap, rework this as > bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry > with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free > to modify the driver state because we know we ARE adding a bitmap > instead of simply being asked if we CAN store one. > > As part of this symmetry, move this function next to > bdrv_remove_persistent_bitmap in block/dirty-bitmap.c. > > To cement this semantic distinction, use a bitmap object instead of the > (name, granularity) pair as this allows us to set persistence as a > condition of the "add" succeeding. > > Notably, the qcow2 implementation still does not actually store the new > bitmap at add time, but merely ensures that it will be able to at store > time. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > +++ b/include/block/dirty-bitmap.h > @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, > Error **errp); > void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); > void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); > +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap, > + Error **errp); Indentation looks off. Reviewed-by: Eric Blake <eblake@redhat.com>
06.06.2019 21:41, John Snow wrote: > Instead of bdrv_can_store_new_bitmap, rework this as > bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry > with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free > to modify the driver state because we know we ARE adding a bitmap > instead of simply being asked if we CAN store one. > > As part of this symmetry, move this function next to > bdrv_remove_persistent_bitmap in block/dirty-bitmap.c. > > To cement this semantic distinction, use a bitmap object instead of the > (name, granularity) pair as this allows us to set persistence as a > condition of the "add" succeeding. > > Notably, the qcow2 implementation still does not actually store the new > bitmap at add time, but merely ensures that it will be able to at store > time. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/qcow2.h | 5 ++--- > include/block/block.h | 2 -- > include/block/block_int.h | 5 ++--- > include/block/dirty-bitmap.h | 3 +++ > block.c | 22 --------------------- > block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++ > block/qcow2-bitmap.c | 24 ++++++++++++++--------- > block/qcow2.c | 2 +- > blockdev.c | 19 +++++++----------- > 9 files changed, 68 insertions(+), 52 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index fc1b0d3c1e..95d723d3c0 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -742,9 +742,8 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); > int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); > int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); > -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > - const char *name, > - uint32_t granularity, > +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap, > Error **errp); > void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, > const char *name, > diff --git a/include/block/block.h b/include/block/block.h > index f9415ed740..6d520f3b59 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, > Error **errp); > void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); > > -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, > - uint32_t granularity, Error **errp); > /** > * > * bdrv_register_buf/bdrv_unregister_buf: > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 06df2bda1b..93bbb66cd0 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -537,9 +537,8 @@ struct BlockDriver { > * field of BlockDirtyBitmap's in case of success. > */ > int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); > - bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, > - const char *name, > - uint32_t granularity, > + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap, > Error **errp); > void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, > const char *name, > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h > index 8044ace63e..c37edbe05f 100644 > --- a/include/block/dirty-bitmap.h > +++ b/include/block/dirty-bitmap.h > @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, > Error **errp); > void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); > void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); > +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap, > + Error **errp); > void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, > const char *name, > Error **errp); > diff --git a/block.c b/block.c > index e3e77feee0..6aec36b7c9 100644 > --- a/block.c > +++ b/block.c > @@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) > > parent_bs->drv->bdrv_del_child(parent_bs, child, errp); > } > - > -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, > - uint32_t granularity, Error **errp) > -{ > - BlockDriver *drv = bs->drv; > - > - if (!drv) { > - error_setg_errno(errp, ENOMEDIUM, > - "Can't store persistent bitmaps to %s", > - bdrv_get_device_or_node_name(bs)); > - return false; > - } > - > - if (!drv->bdrv_can_store_new_dirty_bitmap) { > - error_setg_errno(errp, ENOTSUP, > - "Can't store persistent bitmaps to %s", > - bdrv_get_device_or_node_name(bs)); > - return false; > - } > - > - return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); > -} > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 49646a30e6..615f8480b2 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, > } > } > > +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap, > + Error **errp) strange thing for me: "bitmap" in function name is _not_ the same thing as @bitmap argument.. as if it the same, function adds "persistent bitmap", but @bitmap is not persistent yet, so may be function, don't add persistent bitmap, but marks bitmap persistent? Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set bitmap.persistent flag" > +{ > + BlockDriver *drv = bs->drv; > + int ret; > + > + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { > + error_setg(errp, "Bitmap '%s' is already persistent, " > + "and cannot be re-added to node '%s'", > + bdrv_dirty_bitmap_name(bitmap), > + bdrv_get_device_or_node_name(bs)); > + return -EINVAL; > + } > + > + if (!drv) { > + error_setg_errno(errp, ENOMEDIUM, > + "Can't store persistent bitmaps to %s", > + bdrv_get_device_or_node_name(bs)); > + return -ENOMEDIUM; > + } > + > + if (!drv->bdrv_add_persistent_dirty_bitmap) { > + error_setg_errno(errp, ENOTSUP, > + "Can't store persistent bitmaps to %s", > + bdrv_get_device_or_node_name(bs)); > + return -ENOTSUP; > + } > + > + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp); > + if (ret == 0) { > + bdrv_dirty_bitmap_set_persistence(bitmap, true); > + } > + > + return ret; > +} > + > + > void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) > { > bdrv_dirty_bitmap_lock(bitmap); > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 83aee1a42a..c3a72ca782 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) > return 0; > } > > -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > - const char *name, > - uint32_t granularity, > +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap, > Error **errp) > { > BDRVQcow2State *s = bs->opaque; > bool found; > Qcow2BitmapList *bm_list; > + const char *name = bdrv_dirty_bitmap_name(bitmap); > + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); > + int ret = 0; dead assignment > > if (s->qcow_version < 3) { > /* Without autoclear_features, we would always have to assume > @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > * have to drop all dirty bitmaps (defeating their purpose). > */ > error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files"); > + ret = -ENOTSUP; > goto fail; > } > > - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) { > + ret = check_constraints_on_bitmap(bs, name, granularity, errp); > + if (ret) { hmm, in other places you prefere if ((ret = ...)) { syntax > goto fail; > } > > if (s->nb_bitmaps == 0) { > - return true; > + return 0; > } > > if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { > error_setg(errp, > "Maximum number of persistent bitmaps is already reached"); > + ret = -EOVERFLOW; > goto fail; > } > > @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) > { > error_setg(errp, "Not enough space in the bitmap directory"); > + ret = -EOVERFLOW; > goto fail; > } > > - if (bitmap_list_load(bs, &bm_list, errp)) { > + if ((ret = bitmap_list_load(bs, &bm_list, errp))) { interesting, now we load all bitmaps, so, may be, we don't need to load list every time, but may use bs.dirty_bitmaps to check such things.. But it seems unsafe > goto fail; > } > > found = find_bitmap_by_name(bm_list, name); > bitmap_list_free(bm_list); > if (found) { > + ret = -EEXIST; > error_setg(errp, "Bitmap with the same name is already stored"); > goto fail; > } > > - return true; > - > + return 0; > fail: > error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ", > name, bdrv_get_device_or_node_name(bs)); didn't you consider to move this error_prepend to caller and drop all these gotos? > - return false; > + return ret; > } > diff --git a/block/qcow2.c b/block/qcow2.c > index 9396d490d5..1c78eba71b 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -5229,7 +5229,7 @@ BlockDriver bdrv_qcow2 = { > .bdrv_attach_aio_context = qcow2_attach_aio_context, > > .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw, > - .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap, > + .bdrv_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap, > .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap, > }; > > diff --git a/blockdev.c b/blockdev.c > index 3f44b891eb..169a8da831 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, > disabled = false; > } > > - if (persistent) { > - aio_context = bdrv_get_aio_context(bs); > - aio_context_acquire(aio_context); > - if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) { > - goto out; > - } > - } > - > bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp); > if (bitmap == NULL) { > - goto out; > + return; > } > > if (disabled) { > bdrv_disable_dirty_bitmap(bitmap); > } > > - bdrv_dirty_bitmap_set_persistence(bitmap, persistent); > - out: > - if (aio_context) { > + if (persistent) { Good thing. I suggest to define aio_context in this block too. > + aio_context = bdrv_get_aio_context(bs); > + aio_context_acquire(aio_context); > + if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) { > + bdrv_release_dirty_bitmap(bs, bitmap); > + } > aio_context_release(aio_context); > } > } > With some my suggestions or without: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote: > 06.06.2019 21:41, John Snow wrote: >> Instead of bdrv_can_store_new_bitmap, rework this as >> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry >> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free >> to modify the driver state because we know we ARE adding a bitmap >> instead of simply being asked if we CAN store one. >> >> As part of this symmetry, move this function next to >> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c. >> >> To cement this semantic distinction, use a bitmap object instead of the >> (name, granularity) pair as this allows us to set persistence as a >> condition of the "add" succeeding. >> >> Notably, the qcow2 implementation still does not actually store the new >> bitmap at add time, but merely ensures that it will be able to at store >> time. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/qcow2.h | 5 ++--- >> include/block/block.h | 2 -- >> include/block/block_int.h | 5 ++--- >> include/block/dirty-bitmap.h | 3 +++ >> block.c | 22 --------------------- >> block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++ >> block/qcow2-bitmap.c | 24 ++++++++++++++--------- >> block/qcow2.c | 2 +- >> blockdev.c | 19 +++++++----------- >> 9 files changed, 68 insertions(+), 52 deletions(-) >> >> diff --git a/block/qcow2.h b/block/qcow2.h >> index fc1b0d3c1e..95d723d3c0 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -742,9 +742,8 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >> int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >> - const char *name, >> - uint32_t granularity, >> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> Error **errp); >> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> const char *name, >> diff --git a/include/block/block.h b/include/block/block.h >> index f9415ed740..6d520f3b59 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, >> Error **errp); >> void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); >> >> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >> - uint32_t granularity, Error **errp); >> /** >> * >> * bdrv_register_buf/bdrv_unregister_buf: >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index 06df2bda1b..93bbb66cd0 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -537,9 +537,8 @@ struct BlockDriver { >> * field of BlockDirtyBitmap's in case of success. >> */ >> int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); >> - bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, >> - const char *name, >> - uint32_t granularity, >> + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> Error **errp); >> void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, >> const char *name, >> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >> index 8044ace63e..c37edbe05f 100644 >> --- a/include/block/dirty-bitmap.h >> +++ b/include/block/dirty-bitmap.h >> @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, >> Error **errp); >> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); >> void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); >> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> + Error **errp); >> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> const char *name, >> Error **errp); >> diff --git a/block.c b/block.c >> index e3e77feee0..6aec36b7c9 100644 >> --- a/block.c >> +++ b/block.c >> @@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) >> >> parent_bs->drv->bdrv_del_child(parent_bs, child, errp); >> } >> - >> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >> - uint32_t granularity, Error **errp) >> -{ >> - BlockDriver *drv = bs->drv; >> - >> - if (!drv) { >> - error_setg_errno(errp, ENOMEDIUM, >> - "Can't store persistent bitmaps to %s", >> - bdrv_get_device_or_node_name(bs)); >> - return false; >> - } >> - >> - if (!drv->bdrv_can_store_new_dirty_bitmap) { >> - error_setg_errno(errp, ENOTSUP, >> - "Can't store persistent bitmaps to %s", >> - bdrv_get_device_or_node_name(bs)); >> - return false; >> - } >> - >> - return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); >> -} >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 49646a30e6..615f8480b2 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >> } >> } >> >> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> + Error **errp) > > strange thing for me: "bitmap" in function name is _not_ the same > thing as @bitmap argument.. as if it the same, > function adds "persistent bitmap", but @bitmap is not persistent yet, > so may be function, don't add persistent bitmap, but marks bitmap persistent? > > > Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set > bitmap.persistent flag" > Yeah, I meant "Add bitmap to file", where the persistence is implied because it's part of the file. The bitmap is indeed not YET persistent, but becomes so as a condition of this call succeeding. Do you have a suggestion for a better name? I liked the symmetry with 'remove' so that there was an obvious "add bitmap" and "remove bitmap". Of course, when you remove, it is indeed persistent, so "remove_persistent_dirty_bitmap" makes sense there. > >> +{ >> + BlockDriver *drv = bs->drv; >> + int ret; >> + >> + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { >> + error_setg(errp, "Bitmap '%s' is already persistent, " >> + "and cannot be re-added to node '%s'", >> + bdrv_dirty_bitmap_name(bitmap), >> + bdrv_get_device_or_node_name(bs)); >> + return -EINVAL; >> + } >> + >> + if (!drv) { >> + error_setg_errno(errp, ENOMEDIUM, >> + "Can't store persistent bitmaps to %s", >> + bdrv_get_device_or_node_name(bs)); >> + return -ENOMEDIUM; >> + } >> + >> + if (!drv->bdrv_add_persistent_dirty_bitmap) { >> + error_setg_errno(errp, ENOTSUP, >> + "Can't store persistent bitmaps to %s", >> + bdrv_get_device_or_node_name(bs)); >> + return -ENOTSUP; >> + } >> + >> + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp); >> + if (ret == 0) { >> + bdrv_dirty_bitmap_set_persistence(bitmap, true); >> + } >> + >> + return ret; >> +} >> + >> + >> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> { >> bdrv_dirty_bitmap_lock(bitmap); >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >> index 83aee1a42a..c3a72ca782 100644 >> --- a/block/qcow2-bitmap.c >> +++ b/block/qcow2-bitmap.c >> @@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) >> return 0; >> } >> >> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >> - const char *name, >> - uint32_t granularity, >> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> Error **errp) >> { >> BDRVQcow2State *s = bs->opaque; >> bool found; >> Qcow2BitmapList *bm_list; >> + const char *name = bdrv_dirty_bitmap_name(bitmap); >> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); >> + int ret = 0; > > dead assignment > Will take care of. >> >> if (s->qcow_version < 3) { >> /* Without autoclear_features, we would always have to assume >> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >> * have to drop all dirty bitmaps (defeating their purpose). >> */ >> error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files"); >> + ret = -ENOTSUP; >> goto fail; >> } >> >> - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) { >> + ret = check_constraints_on_bitmap(bs, name, granularity, errp); >> + if (ret) { > > hmm, in other places you prefere > if ((ret = ...)) { > syntax > I have to get rid of those anyway, they violate our coding style. I'll be replacing them all with this full style. >> goto fail; >> } >> >> if (s->nb_bitmaps == 0) { >> - return true; >> + return 0; >> } >> >> if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { >> error_setg(errp, >> "Maximum number of persistent bitmaps is already reached"); >> + ret = -EOVERFLOW; >> goto fail; >> } >> >> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >> QCOW2_MAX_BITMAP_DIRECTORY_SIZE) >> { >> error_setg(errp, "Not enough space in the bitmap directory"); >> + ret = -EOVERFLOW; >> goto fail; >> } >> >> - if (bitmap_list_load(bs, &bm_list, errp)) { >> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) { > > interesting, now we load all bitmaps, so, may be, we don't need to load list every time, > but may use bs.dirty_bitmaps to check such things.. But it seems unsafe > Yeah, I would like to cache this list eventually, but I ran into a lot of hassle with it last time I tried and put it off for now. I think we should definitely be able to. And in fact, if we did, we could even add these bitmaps to the bm_list as soon as _add_ is called and drop the need for the queued counters. >> goto fail; >> } >> >> found = find_bitmap_by_name(bm_list, name); >> bitmap_list_free(bm_list); >> if (found) { >> + ret = -EEXIST; >> error_setg(errp, "Bitmap with the same name is already stored"); >> goto fail; >> } >> >> - return true; >> - >> + return 0; >> fail: >> error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ", >> name, bdrv_get_device_or_node_name(bs)); > > didn't you consider to move this error_prepend to caller and drop all these gotos? > Nope! But I'll do that. >> - return false; >> + return ret; >> } >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 9396d490d5..1c78eba71b 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -5229,7 +5229,7 @@ BlockDriver bdrv_qcow2 = { >> .bdrv_attach_aio_context = qcow2_attach_aio_context, >> >> .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw, >> - .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap, >> + .bdrv_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap, >> .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap, >> }; >> >> diff --git a/blockdev.c b/blockdev.c >> index 3f44b891eb..169a8da831 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, >> disabled = false; >> } >> >> - if (persistent) { >> - aio_context = bdrv_get_aio_context(bs); >> - aio_context_acquire(aio_context); >> - if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) { >> - goto out; >> - } >> - } >> - >> bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp); >> if (bitmap == NULL) { >> - goto out; >> + return; >> } >> >> if (disabled) { >> bdrv_disable_dirty_bitmap(bitmap); >> } >> >> - bdrv_dirty_bitmap_set_persistence(bitmap, persistent); >> - out: >> - if (aio_context) { >> + if (persistent) { > > Good thing. I suggest to define aio_context in this block too. > Oh, sure. >> + aio_context = bdrv_get_aio_context(bs); >> + aio_context_acquire(aio_context); >> + if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) { >> + bdrv_release_dirty_bitmap(bs, bitmap); >> + } >> aio_context_release(aio_context); >> } >> } >> > > With some my suggestions or without: > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > I will respin anyway, so I will take your suggestions. --js
On 6/7/19 1:10 PM, John Snow wrote: > > > On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote: >> 06.06.2019 21:41, John Snow wrote: >>> Instead of bdrv_can_store_new_bitmap, rework this as >>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry >>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free >>> to modify the driver state because we know we ARE adding a bitmap >>> instead of simply being asked if we CAN store one. >>> >>> As part of this symmetry, move this function next to >>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c. >>> >>> To cement this semantic distinction, use a bitmap object instead of the >>> (name, granularity) pair as this allows us to set persistence as a >>> condition of the "add" succeeding. >>> >>> Notably, the qcow2 implementation still does not actually store the new >>> bitmap at add time, but merely ensures that it will be able to at store >>> time. >>> >>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >>> + BdrvDirtyBitmap *bitmap, >>> + Error **errp) >> >> strange thing for me: "bitmap" in function name is _not_ the same >> thing as @bitmap argument.. as if it the same, >> function adds "persistent bitmap", but @bitmap is not persistent yet, >> so may be function, don't add persistent bitmap, but marks bitmap persistent? >> >> >> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set >> bitmap.persistent flag" >> > > Yeah, I meant "Add bitmap to file", where the persistence is implied > because it's part of the file. The bitmap is indeed not YET persistent, > but becomes so as a condition of this call succeeding. > > Do you have a suggestion for a better name? I liked the symmetry with > 'remove' so that there was an obvious "add bitmap" and "remove bitmap". > > Of course, when you remove, it is indeed persistent, so > "remove_persistent_dirty_bitmap" makes sense there. > Or maybe even merely 'bdrv_add_dirty_bitmap' with doc comments stating that it associates an existing non-persistent bitmap with the bdrv storage and marks it persistent if successful.
07.06.2019 21:10, John Snow wrote: > > > On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote: >> 06.06.2019 21:41, John Snow wrote: >>> Instead of bdrv_can_store_new_bitmap, rework this as >>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry >>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free >>> to modify the driver state because we know we ARE adding a bitmap >>> instead of simply being asked if we CAN store one. >>> >>> As part of this symmetry, move this function next to >>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c. >>> >>> To cement this semantic distinction, use a bitmap object instead of the >>> (name, granularity) pair as this allows us to set persistence as a >>> condition of the "add" succeeding. >>> >>> Notably, the qcow2 implementation still does not actually store the new >>> bitmap at add time, but merely ensures that it will be able to at store >>> time. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> block/qcow2.h | 5 ++--- >>> include/block/block.h | 2 -- >>> include/block/block_int.h | 5 ++--- >>> include/block/dirty-bitmap.h | 3 +++ >>> block.c | 22 --------------------- >>> block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++ >>> block/qcow2-bitmap.c | 24 ++++++++++++++--------- >>> block/qcow2.c | 2 +- >>> blockdev.c | 19 +++++++----------- >>> 9 files changed, 68 insertions(+), 52 deletions(-) >>> >>> diff --git a/block/qcow2.h b/block/qcow2.h >>> index fc1b0d3c1e..95d723d3c0 100644 >>> --- a/block/qcow2.h >>> +++ b/block/qcow2.h >>> @@ -742,9 +742,8 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >>> int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >>> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >>> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>> - const char *name, >>> - uint32_t granularity, >>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >>> + BdrvDirtyBitmap *bitmap, >>> Error **errp); >>> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>> const char *name, >>> diff --git a/include/block/block.h b/include/block/block.h >>> index f9415ed740..6d520f3b59 100644 >>> --- a/include/block/block.h >>> +++ b/include/block/block.h >>> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, >>> Error **errp); >>> void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); >>> >>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >>> - uint32_t granularity, Error **errp); >>> /** >>> * >>> * bdrv_register_buf/bdrv_unregister_buf: >>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>> index 06df2bda1b..93bbb66cd0 100644 >>> --- a/include/block/block_int.h >>> +++ b/include/block/block_int.h >>> @@ -537,9 +537,8 @@ struct BlockDriver { >>> * field of BlockDirtyBitmap's in case of success. >>> */ >>> int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); >>> - bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, >>> - const char *name, >>> - uint32_t granularity, >>> + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs, >>> + BdrvDirtyBitmap *bitmap, >>> Error **errp); >>> void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, >>> const char *name, >>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >>> index 8044ace63e..c37edbe05f 100644 >>> --- a/include/block/dirty-bitmap.h >>> +++ b/include/block/dirty-bitmap.h >>> @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, >>> Error **errp); >>> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); >>> void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); >>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >>> + BdrvDirtyBitmap *bitmap, >>> + Error **errp); >>> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>> const char *name, >>> Error **errp); >>> diff --git a/block.c b/block.c >>> index e3e77feee0..6aec36b7c9 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) >>> >>> parent_bs->drv->bdrv_del_child(parent_bs, child, errp); >>> } >>> - >>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >>> - uint32_t granularity, Error **errp) >>> -{ >>> - BlockDriver *drv = bs->drv; >>> - >>> - if (!drv) { >>> - error_setg_errno(errp, ENOMEDIUM, >>> - "Can't store persistent bitmaps to %s", >>> - bdrv_get_device_or_node_name(bs)); >>> - return false; >>> - } >>> - >>> - if (!drv->bdrv_can_store_new_dirty_bitmap) { >>> - error_setg_errno(errp, ENOTSUP, >>> - "Can't store persistent bitmaps to %s", >>> - bdrv_get_device_or_node_name(bs)); >>> - return false; >>> - } >>> - >>> - return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); >>> -} >>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>> index 49646a30e6..615f8480b2 100644 >>> --- a/block/dirty-bitmap.c >>> +++ b/block/dirty-bitmap.c >>> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>> } >>> } >>> >>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >>> + BdrvDirtyBitmap *bitmap, >>> + Error **errp) >> >> strange thing for me: "bitmap" in function name is _not_ the same >> thing as @bitmap argument.. as if it the same, >> function adds "persistent bitmap", but @bitmap is not persistent yet, >> so may be function, don't add persistent bitmap, but marks bitmap persistent? >> >> >> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set >> bitmap.persistent flag" >> > > Yeah, I meant "Add bitmap to file", where the persistence is implied > because it's part of the file. The bitmap is indeed not YET persistent, > but becomes so as a condition of this call succeeding. > > Do you have a suggestion for a better name? I liked the symmetry with > 'remove' so that there was an obvious "add bitmap" and "remove bitmap". > > Of course, when you remove, it is indeed persistent, so > "remove_persistent_dirty_bitmap" makes sense there. > >> >>> +{ >>> + BlockDriver *drv = bs->drv; >>> + int ret; >>> + >>> + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { >>> + error_setg(errp, "Bitmap '%s' is already persistent, " >>> + "and cannot be re-added to node '%s'", >>> + bdrv_dirty_bitmap_name(bitmap), >>> + bdrv_get_device_or_node_name(bs)); >>> + return -EINVAL; >>> + } >>> + >>> + if (!drv) { >>> + error_setg_errno(errp, ENOMEDIUM, >>> + "Can't store persistent bitmaps to %s", >>> + bdrv_get_device_or_node_name(bs)); >>> + return -ENOMEDIUM; >>> + } >>> + >>> + if (!drv->bdrv_add_persistent_dirty_bitmap) { >>> + error_setg_errno(errp, ENOTSUP, >>> + "Can't store persistent bitmaps to %s", >>> + bdrv_get_device_or_node_name(bs)); >>> + return -ENOTSUP; >>> + } >>> + >>> + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp); >>> + if (ret == 0) { >>> + bdrv_dirty_bitmap_set_persistence(bitmap, true); >>> + } >>> + >>> + return ret; >>> +} >>> + >>> + >>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >>> { >>> bdrv_dirty_bitmap_lock(bitmap); >>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>> index 83aee1a42a..c3a72ca782 100644 >>> --- a/block/qcow2-bitmap.c >>> +++ b/block/qcow2-bitmap.c >>> @@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) >>> return 0; >>> } >>> >>> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>> - const char *name, >>> - uint32_t granularity, >>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >>> + BdrvDirtyBitmap *bitmap, >>> Error **errp) >>> { >>> BDRVQcow2State *s = bs->opaque; >>> bool found; >>> Qcow2BitmapList *bm_list; >>> + const char *name = bdrv_dirty_bitmap_name(bitmap); >>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); >>> + int ret = 0; >> >> dead assignment >> > > Will take care of. > >>> >>> if (s->qcow_version < 3) { >>> /* Without autoclear_features, we would always have to assume >>> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>> * have to drop all dirty bitmaps (defeating their purpose). >>> */ >>> error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files"); >>> + ret = -ENOTSUP; >>> goto fail; >>> } >>> >>> - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) { >>> + ret = check_constraints_on_bitmap(bs, name, granularity, errp); >>> + if (ret) { >> >> hmm, in other places you prefere >> if ((ret = ...)) { >> syntax >> > > I have to get rid of those anyway, they violate our coding style. I'll > be replacing them all with this full style. > >>> goto fail; >>> } >>> >>> if (s->nb_bitmaps == 0) { >>> - return true; >>> + return 0; >>> } >>> >>> if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { >>> error_setg(errp, >>> "Maximum number of persistent bitmaps is already reached"); >>> + ret = -EOVERFLOW; >>> goto fail; >>> } >>> >>> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>> QCOW2_MAX_BITMAP_DIRECTORY_SIZE) >>> { >>> error_setg(errp, "Not enough space in the bitmap directory"); >>> + ret = -EOVERFLOW; >>> goto fail; >>> } >>> >>> - if (bitmap_list_load(bs, &bm_list, errp)) { >>> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) { >> >> interesting, now we load all bitmaps, so, may be, we don't need to load list every time, >> but may use bs.dirty_bitmaps to check such things.. But it seems unsafe >> > > Yeah, I would like to cache this list eventually, but I ran into a lot > of hassle with it last time I tried and put it off for now. > > I think we should definitely be able to. > > And in fact, if we did, we could even add these bitmaps to the bm_list > as soon as _add_ is called and drop the need for the queued counters. Personally I like the old _can_add_ :) But you want to make this function really do something with driver, so "_can_" is not good ofcourse and _add_ is better. And any kind of _mark_persistent_ or _make_persistent_ will be in a bad relation with simple setter _set_persistence() which we already have and which doesn't imply any complicated logic.. On the other hand I still not sure that we need track "queued" bitmaps in qcow2 driver, as we can calculate their number and needed size of directory in any moment, not extending the qcow2 state.. > >>> goto fail; >>> } >>> >>> found = find_bitmap_by_name(bm_list, name); >>> bitmap_list_free(bm_list); >>> if (found) { >>> + ret = -EEXIST; >>> error_setg(errp, "Bitmap with the same name is already stored"); >>> goto fail; >>> } >>> >>> - return true; >>> - >>> + return 0; >>> fail: >>> error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ", >>> name, bdrv_get_device_or_node_name(bs)); >> >> didn't you consider to move this error_prepend to caller and drop all these gotos? >> > > Nope! But I'll do that. > >>> - return false; >>> + return ret; >>> } >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index 9396d490d5..1c78eba71b 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -5229,7 +5229,7 @@ BlockDriver bdrv_qcow2 = { >>> .bdrv_attach_aio_context = qcow2_attach_aio_context, >>> >>> .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw, >>> - .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap, >>> + .bdrv_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap, >>> .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap, >>> }; >>> >>> diff --git a/blockdev.c b/blockdev.c >>> index 3f44b891eb..169a8da831 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, >>> disabled = false; >>> } >>> >>> - if (persistent) { >>> - aio_context = bdrv_get_aio_context(bs); >>> - aio_context_acquire(aio_context); >>> - if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) { >>> - goto out; >>> - } >>> - } >>> - >>> bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp); >>> if (bitmap == NULL) { >>> - goto out; >>> + return; >>> } >>> >>> if (disabled) { >>> bdrv_disable_dirty_bitmap(bitmap); >>> } >>> >>> - bdrv_dirty_bitmap_set_persistence(bitmap, persistent); >>> - out: >>> - if (aio_context) { >>> + if (persistent) { >> >> Good thing. I suggest to define aio_context in this block too. >> > > Oh, sure. > >>> + aio_context = bdrv_get_aio_context(bs); >>> + aio_context_acquire(aio_context); >>> + if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) { >>> + bdrv_release_dirty_bitmap(bs, bitmap); >>> + } >>> aio_context_release(aio_context); >>> } >>> } >>> >> >> With some my suggestions or without: >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> > > I will respin anyway, so I will take your suggestions. > > --js >
07.06.2019 21:17, Vladimir Sementsov-Ogievskiy wrote: > 07.06.2019 21:10, John Snow wrote: >> >> >> On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 06.06.2019 21:41, John Snow wrote: >>>> Instead of bdrv_can_store_new_bitmap, rework this as >>>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry >>>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free >>>> to modify the driver state because we know we ARE adding a bitmap >>>> instead of simply being asked if we CAN store one. >>>> >>>> As part of this symmetry, move this function next to >>>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c. >>>> >>>> To cement this semantic distinction, use a bitmap object instead of the >>>> (name, granularity) pair as this allows us to set persistence as a >>>> condition of the "add" succeeding. >>>> >>>> Notably, the qcow2 implementation still does not actually store the new >>>> bitmap at add time, but merely ensures that it will be able to at store >>>> time. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>>   block/qcow2.h               | 5 ++--- >>>>   include/block/block.h       | 2 -- >>>>   include/block/block_int.h   | 5 ++--- >>>>   include/block/dirty-bitmap.h | 3 +++ >>>>   block.c                     | 22 --------------------- >>>>   block/dirty-bitmap.c        | 38 ++++++++++++++++++++++++++++++++++++ >>>>   block/qcow2-bitmap.c        | 24 ++++++++++++++--------- >>>>   block/qcow2.c               | 2 +- >>>>   blockdev.c                  | 19 +++++++----------- >>>>   9 files changed, 68 insertions(+), 52 deletions(-) >>>> >>>> diff --git a/block/qcow2.h b/block/qcow2.h >>>> index fc1b0d3c1e..95d723d3c0 100644 >>>> --- a/block/qcow2.h >>>> +++ b/block/qcow2.h >>>> @@ -742,9 +742,8 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >>>>   int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >>>>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >>>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >>>> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>> -                                     const char *name, >>>> -                                     uint32_t granularity, >>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>> +                                     BdrvDirtyBitmap *bitmap, >>>>                                         Error **errp); >>>>   void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>>>                                             const char *name, >>>> diff --git a/include/block/block.h b/include/block/block.h >>>> index f9415ed740..6d520f3b59 100644 >>>> --- a/include/block/block.h >>>> +++ b/include/block/block.h >>>> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, >>>>                       Error **errp); >>>>   void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); >>>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >>>> -                                    uint32_t granularity, Error **errp); >>>>   /** >>>>    * >>>>    * bdrv_register_buf/bdrv_unregister_buf: >>>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>>> index 06df2bda1b..93bbb66cd0 100644 >>>> --- a/include/block/block_int.h >>>> +++ b/include/block/block_int.h >>>> @@ -537,9 +537,8 @@ struct BlockDriver { >>>>        * field of BlockDirtyBitmap's in case of success. >>>>        */ >>>>       int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); >>>> -   bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, >>>> -                                           const char *name, >>>> -                                           uint32_t granularity, >>>> +   int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs, >>>> +                                           BdrvDirtyBitmap *bitmap, >>>>                                               Error **errp); >>>>       void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, >>>>                                                   const char *name, >>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >>>> index 8044ace63e..c37edbe05f 100644 >>>> --- a/include/block/dirty-bitmap.h >>>> +++ b/include/block/dirty-bitmap.h >>>> @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, >>>>                               Error **errp); >>>>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); >>>>   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); >>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>> +                                     BdrvDirtyBitmap *bitmap, >>>> +                                     Error **errp); >>>>   void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>>>                                            const char *name, >>>>                                            Error **errp); >>>> diff --git a/block.c b/block.c >>>> index e3e77feee0..6aec36b7c9 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) >>>>       parent_bs->drv->bdrv_del_child(parent_bs, child, errp); >>>>   } >>>> - >>>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >>>> -                                    uint32_t granularity, Error **errp) >>>> -{ >>>> -   BlockDriver *drv = bs->drv; >>>> - >>>> -   if (!drv) { >>>> -       error_setg_errno(errp, ENOMEDIUM, >>>> -                        "Can't store persistent bitmaps to %s", >>>> -                        bdrv_get_device_or_node_name(bs)); >>>> -       return false; >>>> -   } >>>> - >>>> -   if (!drv->bdrv_can_store_new_dirty_bitmap) { >>>> -       error_setg_errno(errp, ENOTSUP, >>>> -                        "Can't store persistent bitmaps to %s", >>>> -                        bdrv_get_device_or_node_name(bs)); >>>> -       return false; >>>> -   } >>>> - >>>> -   return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); >>>> -} >>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>> index 49646a30e6..615f8480b2 100644 >>>> --- a/block/dirty-bitmap.c >>>> +++ b/block/dirty-bitmap.c >>>> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>>>       } >>>>   } >>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>> +                                    BdrvDirtyBitmap *bitmap, >>>> +                                    Error **errp) >>> >>> strange thing for me: "bitmap" in function name is _not_ the same >>> thing as @bitmap argument.. as if it the same, >>> function adds "persistent bitmap", but @bitmap is not persistent yet, >>> so may be function, don't add persistent bitmap, but marks bitmap persistent? >>> >>> >>> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set >>> bitmap.persistent flag" >>> >> >> Yeah, I meant "Add bitmap to file", where the persistence is implied >> because it's part of the file. The bitmap is indeed not YET persistent, >> but becomes so as a condition of this call succeeding. >> >> Do you have a suggestion for a better name? I liked the symmetry with >> 'remove' so that there was an obvious "add bitmap" and "remove bitmap". >> >> Of course, when you remove, it is indeed persistent, so >> "remove_persistent_dirty_bitmap" makes sense there. >> >>> >>>> +{ >>>> +   BlockDriver *drv = bs->drv; >>>> +   int ret; >>>> + >>>> +   if (bdrv_dirty_bitmap_get_persistence(bitmap)) { >>>> +       error_setg(errp, "Bitmap '%s' is already persistent, " >>>> +                  "and cannot be re-added to node '%s'", >>>> +                  bdrv_dirty_bitmap_name(bitmap), >>>> +                  bdrv_get_device_or_node_name(bs)); >>>> +       return -EINVAL; >>>> +   } >>>> + >>>> +   if (!drv) { >>>> +       error_setg_errno(errp, ENOMEDIUM, >>>> +                        "Can't store persistent bitmaps to %s", >>>> +                        bdrv_get_device_or_node_name(bs)); >>>> +       return -ENOMEDIUM; >>>> +   } >>>> + >>>> +   if (!drv->bdrv_add_persistent_dirty_bitmap) { >>>> +       error_setg_errno(errp, ENOTSUP, >>>> +                        "Can't store persistent bitmaps to %s", >>>> +                        bdrv_get_device_or_node_name(bs)); >>>> +       return -ENOTSUP; >>>> +   } >>>> + >>>> +   ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp); >>>> +   if (ret == 0) { >>>> +       bdrv_dirty_bitmap_set_persistence(bitmap, true); >>>> +   } >>>> + >>>> +   return ret; >>>> +} >>>> + >>>> + >>>>   void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >>>>   { >>>>       bdrv_dirty_bitmap_lock(bitmap); >>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>>> index 83aee1a42a..c3a72ca782 100644 >>>> --- a/block/qcow2-bitmap.c >>>> +++ b/block/qcow2-bitmap.c >>>> @@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) >>>>       return 0; >>>>   } >>>> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>> -                                     const char *name, >>>> -                                     uint32_t granularity, >>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>> +                                     BdrvDirtyBitmap *bitmap, >>>>                                         Error **errp) >>>>   { >>>>       BDRVQcow2State *s = bs->opaque; >>>>       bool found; >>>>       Qcow2BitmapList *bm_list; >>>> +   const char *name = bdrv_dirty_bitmap_name(bitmap); >>>> +   uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); >>>> +   int ret = 0; >>> >>> dead assignment >>> >> >> Will take care of. >> >>>>       if (s->qcow_version < 3) { >>>>           /* Without autoclear_features, we would always have to assume >>>> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>>            * have to drop all dirty bitmaps (defeating their purpose). >>>>            */ >>>>           error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files"); >>>> +       ret = -ENOTSUP; >>>>           goto fail; >>>>       } >>>> -   if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) { >>>> +   ret = check_constraints_on_bitmap(bs, name, granularity, errp); >>>> +   if (ret) { >>> >>> hmm, in other places you prefere >>> if ((ret = ...)) { >>> syntax >>> >> >> I have to get rid of those anyway, they violate our coding style. I'll >> be replacing them all with this full style. >> >>>>           goto fail; >>>>       } >>>>       if (s->nb_bitmaps == 0) { >>>> -       return true; >>>> +       return 0; >>>>       } >>>>       if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { >>>>           error_setg(errp, >>>>                      "Maximum number of persistent bitmaps is already reached"); >>>> +       ret = -EOVERFLOW; >>>>           goto fail; >>>>       } >>>> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>>           QCOW2_MAX_BITMAP_DIRECTORY_SIZE) >>>>       { >>>>           error_setg(errp, "Not enough space in the bitmap directory"); >>>> +       ret = -EOVERFLOW; >>>>           goto fail; >>>>       } >>>> -   if (bitmap_list_load(bs, &bm_list, errp)) { >>>> +   if ((ret = bitmap_list_load(bs, &bm_list, errp))) { >>> >>> interesting, now we load all bitmaps, so, may be, we don't need to load list every time, >>> but may use bs.dirty_bitmaps to check such things.. But it seems unsafe >>> >> >> Yeah, I would like to cache this list eventually, but I ran into a lot >> of hassle with it last time I tried and put it off for now. >> >> I think we should definitely be able to. >> >> And in fact, if we did, we could even add these bitmaps to the bm_list >> as soon as _add_ is called and drop the need for the queued counters. > > Personally I like the old _can_add_ :) Haha, I already forget that it was _can_store_, which is anyway in a bad relation with the fact that we want to count bitmaps that are "queued". > > But you want to make this function really do something with driver, so "_can_" is not good > ofcourse and _add_ is better. And any kind of _mark_persistent_ or _make_persistent_ will > be in a bad relation with simple setter _set_persistence() which we already have and which > doesn't imply any complicated logic.. > > On the other hand I still not sure that we need track "queued" bitmaps in qcow2 driver, as we > can calculate their number and needed size of directory in any moment, not extending the qcow2 > state.. > >> >>>>           goto fail; >>>>       } >>>>       found = find_bitmap_by_name(bm_list, name); >>>>       bitmap_list_free(bm_list); >>>>       if (found) { >>>> +       ret = -EEXIST; >>>>           error_setg(errp, "Bitmap with the same name is already stored"); >>>>           goto fail; >>>>       } >>>> -   return true; >>>> - >>>> +   return 0; >>>>   fail: >>>>       error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ", >>>>                     name, bdrv_get_device_or_node_name(bs)); >>> >>> didn't you consider to move this error_prepend to caller and drop all these gotos? >>> >> >> Nope! But I'll do that. >> >>>> -   return false; >>>> +   return ret; >>>>   } >>>> diff --git a/block/qcow2.c b/block/qcow2.c >>>> index 9396d490d5..1c78eba71b 100644 >>>> --- a/block/qcow2.c >>>> +++ b/block/qcow2.c >>>> @@ -5229,7 +5229,7 @@ BlockDriver bdrv_qcow2 = { >>>>       .bdrv_attach_aio_context = qcow2_attach_aio_context, >>>>       .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw, >>>> -   .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap, >>>> +   .bdrv_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap, >>>>       .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap, >>>>   }; >>>> diff --git a/blockdev.c b/blockdev.c >>>> index 3f44b891eb..169a8da831 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, >>>>           disabled = false; >>>>       } >>>> -   if (persistent) { >>>> -       aio_context = bdrv_get_aio_context(bs); >>>> -       aio_context_acquire(aio_context); >>>> -       if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) { >>>> -           goto out; >>>> -       } >>>> -   } >>>> - >>>>       bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp); >>>>       if (bitmap == NULL) { >>>> -       goto out; >>>> +       return; >>>>       } >>>>       if (disabled) { >>>>           bdrv_disable_dirty_bitmap(bitmap); >>>>       } >>>> -   bdrv_dirty_bitmap_set_persistence(bitmap, persistent); >>>> - out: >>>> -   if (aio_context) { >>>> +   if (persistent) { >>> >>> Good thing. I suggest to define aio_context in this block too. >>> >> >> Oh, sure. >> >>>> +       aio_context = bdrv_get_aio_context(bs); >>>> +       aio_context_acquire(aio_context); >>>> +       if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) { >>>> +           bdrv_release_dirty_bitmap(bs, bitmap); >>>> +       } >>>>           aio_context_release(aio_context); >>>>       } >>>>   } >>>> >>> >>> With some my suggestions or without: >>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> >> >> I will respin anyway, so I will take your suggestions. >> >> --js >> > >
On 6/7/19 2:17 PM, Vladimir Sementsov-Ogievskiy wrote: > 07.06.2019 21:10, John Snow wrote: >> >> >> On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 06.06.2019 21:41, John Snow wrote: >>>> Instead of bdrv_can_store_new_bitmap, rework this as >>>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry >>>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free >>>> to modify the driver state because we know we ARE adding a bitmap >>>> instead of simply being asked if we CAN store one. >>>> >>>> As part of this symmetry, move this function next to >>>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c. >>>> >>>> To cement this semantic distinction, use a bitmap object instead of the >>>> (name, granularity) pair as this allows us to set persistence as a >>>> condition of the "add" succeeding. >>>> >>>> Notably, the qcow2 implementation still does not actually store the new >>>> bitmap at add time, but merely ensures that it will be able to at store >>>> time. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> block/qcow2.h | 5 ++--- >>>> include/block/block.h | 2 -- >>>> include/block/block_int.h | 5 ++--- >>>> include/block/dirty-bitmap.h | 3 +++ >>>> block.c | 22 --------------------- >>>> block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++ >>>> block/qcow2-bitmap.c | 24 ++++++++++++++--------- >>>> block/qcow2.c | 2 +- >>>> blockdev.c | 19 +++++++----------- >>>> 9 files changed, 68 insertions(+), 52 deletions(-) >>>> >>>> diff --git a/block/qcow2.h b/block/qcow2.h >>>> index fc1b0d3c1e..95d723d3c0 100644 >>>> --- a/block/qcow2.h >>>> +++ b/block/qcow2.h >>>> @@ -742,9 +742,8 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >>>> int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >>>> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >>>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >>>> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>> - const char *name, >>>> - uint32_t granularity, >>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>> + BdrvDirtyBitmap *bitmap, >>>> Error **errp); >>>> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>>> const char *name, >>>> diff --git a/include/block/block.h b/include/block/block.h >>>> index f9415ed740..6d520f3b59 100644 >>>> --- a/include/block/block.h >>>> +++ b/include/block/block.h >>>> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, >>>> Error **errp); >>>> void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); >>>> >>>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >>>> - uint32_t granularity, Error **errp); >>>> /** >>>> * >>>> * bdrv_register_buf/bdrv_unregister_buf: >>>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>>> index 06df2bda1b..93bbb66cd0 100644 >>>> --- a/include/block/block_int.h >>>> +++ b/include/block/block_int.h >>>> @@ -537,9 +537,8 @@ struct BlockDriver { >>>> * field of BlockDirtyBitmap's in case of success. >>>> */ >>>> int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); >>>> - bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, >>>> - const char *name, >>>> - uint32_t granularity, >>>> + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs, >>>> + BdrvDirtyBitmap *bitmap, >>>> Error **errp); >>>> void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, >>>> const char *name, >>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >>>> index 8044ace63e..c37edbe05f 100644 >>>> --- a/include/block/dirty-bitmap.h >>>> +++ b/include/block/dirty-bitmap.h >>>> @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, >>>> Error **errp); >>>> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); >>>> void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); >>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>> + BdrvDirtyBitmap *bitmap, >>>> + Error **errp); >>>> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>>> const char *name, >>>> Error **errp); >>>> diff --git a/block.c b/block.c >>>> index e3e77feee0..6aec36b7c9 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) >>>> >>>> parent_bs->drv->bdrv_del_child(parent_bs, child, errp); >>>> } >>>> - >>>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >>>> - uint32_t granularity, Error **errp) >>>> -{ >>>> - BlockDriver *drv = bs->drv; >>>> - >>>> - if (!drv) { >>>> - error_setg_errno(errp, ENOMEDIUM, >>>> - "Can't store persistent bitmaps to %s", >>>> - bdrv_get_device_or_node_name(bs)); >>>> - return false; >>>> - } >>>> - >>>> - if (!drv->bdrv_can_store_new_dirty_bitmap) { >>>> - error_setg_errno(errp, ENOTSUP, >>>> - "Can't store persistent bitmaps to %s", >>>> - bdrv_get_device_or_node_name(bs)); >>>> - return false; >>>> - } >>>> - >>>> - return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); >>>> -} >>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>> index 49646a30e6..615f8480b2 100644 >>>> --- a/block/dirty-bitmap.c >>>> +++ b/block/dirty-bitmap.c >>>> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>>> } >>>> } >>>> >>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>> + BdrvDirtyBitmap *bitmap, >>>> + Error **errp) >>> >>> strange thing for me: "bitmap" in function name is _not_ the same >>> thing as @bitmap argument.. as if it the same, >>> function adds "persistent bitmap", but @bitmap is not persistent yet, >>> so may be function, don't add persistent bitmap, but marks bitmap persistent? >>> >>> >>> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set >>> bitmap.persistent flag" >>> >> >> Yeah, I meant "Add bitmap to file", where the persistence is implied >> because it's part of the file. The bitmap is indeed not YET persistent, >> but becomes so as a condition of this call succeeding. >> >> Do you have a suggestion for a better name? I liked the symmetry with >> 'remove' so that there was an obvious "add bitmap" and "remove bitmap". >> >> Of course, when you remove, it is indeed persistent, so >> "remove_persistent_dirty_bitmap" makes sense there. >> >>> >>>> +{ >>>> + BlockDriver *drv = bs->drv; >>>> + int ret; >>>> + >>>> + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { >>>> + error_setg(errp, "Bitmap '%s' is already persistent, " >>>> + "and cannot be re-added to node '%s'", >>>> + bdrv_dirty_bitmap_name(bitmap), >>>> + bdrv_get_device_or_node_name(bs)); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if (!drv) { >>>> + error_setg_errno(errp, ENOMEDIUM, >>>> + "Can't store persistent bitmaps to %s", >>>> + bdrv_get_device_or_node_name(bs)); >>>> + return -ENOMEDIUM; >>>> + } >>>> + >>>> + if (!drv->bdrv_add_persistent_dirty_bitmap) { >>>> + error_setg_errno(errp, ENOTSUP, >>>> + "Can't store persistent bitmaps to %s", >>>> + bdrv_get_device_or_node_name(bs)); >>>> + return -ENOTSUP; >>>> + } >>>> + >>>> + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp); >>>> + if (ret == 0) { >>>> + bdrv_dirty_bitmap_set_persistence(bitmap, true); >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> + >>>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >>>> { >>>> bdrv_dirty_bitmap_lock(bitmap); >>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>>> index 83aee1a42a..c3a72ca782 100644 >>>> --- a/block/qcow2-bitmap.c >>>> +++ b/block/qcow2-bitmap.c >>>> @@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) >>>> return 0; >>>> } >>>> >>>> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>> - const char *name, >>>> - uint32_t granularity, >>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>> + BdrvDirtyBitmap *bitmap, >>>> Error **errp) >>>> { >>>> BDRVQcow2State *s = bs->opaque; >>>> bool found; >>>> Qcow2BitmapList *bm_list; >>>> + const char *name = bdrv_dirty_bitmap_name(bitmap); >>>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); >>>> + int ret = 0; >>> >>> dead assignment >>> >> >> Will take care of. >> >>>> >>>> if (s->qcow_version < 3) { >>>> /* Without autoclear_features, we would always have to assume >>>> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>> * have to drop all dirty bitmaps (defeating their purpose). >>>> */ >>>> error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files"); >>>> + ret = -ENOTSUP; >>>> goto fail; >>>> } >>>> >>>> - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) { >>>> + ret = check_constraints_on_bitmap(bs, name, granularity, errp); >>>> + if (ret) { >>> >>> hmm, in other places you prefere >>> if ((ret = ...)) { >>> syntax >>> >> >> I have to get rid of those anyway, they violate our coding style. I'll >> be replacing them all with this full style. >> >>>> goto fail; >>>> } >>>> >>>> if (s->nb_bitmaps == 0) { >>>> - return true; >>>> + return 0; >>>> } >>>> >>>> if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { >>>> error_setg(errp, >>>> "Maximum number of persistent bitmaps is already reached"); >>>> + ret = -EOVERFLOW; >>>> goto fail; >>>> } >>>> >>>> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>> QCOW2_MAX_BITMAP_DIRECTORY_SIZE) >>>> { >>>> error_setg(errp, "Not enough space in the bitmap directory"); >>>> + ret = -EOVERFLOW; >>>> goto fail; >>>> } >>>> >>>> - if (bitmap_list_load(bs, &bm_list, errp)) { >>>> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) { >>> >>> interesting, now we load all bitmaps, so, may be, we don't need to load list every time, >>> but may use bs.dirty_bitmaps to check such things.. But it seems unsafe >>> >> >> Yeah, I would like to cache this list eventually, but I ran into a lot >> of hassle with it last time I tried and put it off for now. >> >> I think we should definitely be able to. >> >> And in fact, if we did, we could even add these bitmaps to the bm_list >> as soon as _add_ is called and drop the need for the queued counters. > > Personally I like the old _can_add_ :) > > But you want to make this function really do something with driver, so "_can_" is not good > ofcourse and _add_ is better. And any kind of _mark_persistent_ or _make_persistent_ will > be in a bad relation with simple setter _set_persistence() which we already have and which > doesn't imply any complicated logic.. > I think it's a simpler design to have "add" and "remove" callbacks and be more direct about it. Of course, the qcow2 implementation is free to avoid a write to disk on add if it wishes, but I don't like the idea of having to call "can_store" and then later a "do_store" or any such thing. You could say that can_store is the check and then mark persistent is the actual action, but then why keep them separate? I like the link between calling the driver and the later store to be obvious. I feel like marking the bitmap persistent without the knowledge or consent of the driver is bound to cause trouble sooner or later, so I'd rather make the persistent call a condition of the store check succeeding. > On the other hand I still not sure that we need track "queued" bitmaps in qcow2 driver, as we > can calculate their number and needed size of directory in any moment, not extending the qcow2 > state.. > Do we want to add an O(N) check because we don't want to spend 12 bytes? --js
08.06.2019 1:08, John Snow wrote: > > > On 6/7/19 2:17 PM, Vladimir Sementsov-Ogievskiy wrote: >> 07.06.2019 21:10, John Snow wrote: >>> >>> >>> On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> 06.06.2019 21:41, John Snow wrote: >>>>> Instead of bdrv_can_store_new_bitmap, rework this as >>>>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry >>>>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free >>>>> to modify the driver state because we know we ARE adding a bitmap >>>>> instead of simply being asked if we CAN store one. >>>>> >>>>> As part of this symmetry, move this function next to >>>>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c. >>>>> >>>>> To cement this semantic distinction, use a bitmap object instead of the >>>>> (name, granularity) pair as this allows us to set persistence as a >>>>> condition of the "add" succeeding. >>>>> >>>>> Notably, the qcow2 implementation still does not actually store the new >>>>> bitmap at add time, but merely ensures that it will be able to at store >>>>> time. >>>>> >>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>>> --- >>>>> block/qcow2.h | 5 ++--- >>>>> include/block/block.h | 2 -- >>>>> include/block/block_int.h | 5 ++--- >>>>> include/block/dirty-bitmap.h | 3 +++ >>>>> block.c | 22 --------------------- >>>>> block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++ >>>>> block/qcow2-bitmap.c | 24 ++++++++++++++--------- >>>>> block/qcow2.c | 2 +- >>>>> blockdev.c | 19 +++++++----------- >>>>> 9 files changed, 68 insertions(+), 52 deletions(-) >>>>> >>>>> diff --git a/block/qcow2.h b/block/qcow2.h >>>>> index fc1b0d3c1e..95d723d3c0 100644 >>>>> --- a/block/qcow2.h >>>>> +++ b/block/qcow2.h >>>>> @@ -742,9 +742,8 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); >>>>> int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); >>>>> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); >>>>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); >>>>> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>>> - const char *name, >>>>> - uint32_t granularity, >>>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>>> + BdrvDirtyBitmap *bitmap, >>>>> Error **errp); >>>>> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>>>> const char *name, >>>>> diff --git a/include/block/block.h b/include/block/block.h >>>>> index f9415ed740..6d520f3b59 100644 >>>>> --- a/include/block/block.h >>>>> +++ b/include/block/block.h >>>>> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, >>>>> Error **errp); >>>>> void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); >>>>> >>>>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >>>>> - uint32_t granularity, Error **errp); >>>>> /** >>>>> * >>>>> * bdrv_register_buf/bdrv_unregister_buf: >>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>>>> index 06df2bda1b..93bbb66cd0 100644 >>>>> --- a/include/block/block_int.h >>>>> +++ b/include/block/block_int.h >>>>> @@ -537,9 +537,8 @@ struct BlockDriver { >>>>> * field of BlockDirtyBitmap's in case of success. >>>>> */ >>>>> int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); >>>>> - bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, >>>>> - const char *name, >>>>> - uint32_t granularity, >>>>> + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs, >>>>> + BdrvDirtyBitmap *bitmap, >>>>> Error **errp); >>>>> void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, >>>>> const char *name, >>>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >>>>> index 8044ace63e..c37edbe05f 100644 >>>>> --- a/include/block/dirty-bitmap.h >>>>> +++ b/include/block/dirty-bitmap.h >>>>> @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, >>>>> Error **errp); >>>>> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); >>>>> void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); >>>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>>> + BdrvDirtyBitmap *bitmap, >>>>> + Error **errp); >>>>> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>>>> const char *name, >>>>> Error **errp); >>>>> diff --git a/block.c b/block.c >>>>> index e3e77feee0..6aec36b7c9 100644 >>>>> --- a/block.c >>>>> +++ b/block.c >>>>> @@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) >>>>> >>>>> parent_bs->drv->bdrv_del_child(parent_bs, child, errp); >>>>> } >>>>> - >>>>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, >>>>> - uint32_t granularity, Error **errp) >>>>> -{ >>>>> - BlockDriver *drv = bs->drv; >>>>> - >>>>> - if (!drv) { >>>>> - error_setg_errno(errp, ENOMEDIUM, >>>>> - "Can't store persistent bitmaps to %s", >>>>> - bdrv_get_device_or_node_name(bs)); >>>>> - return false; >>>>> - } >>>>> - >>>>> - if (!drv->bdrv_can_store_new_dirty_bitmap) { >>>>> - error_setg_errno(errp, ENOTSUP, >>>>> - "Can't store persistent bitmaps to %s", >>>>> - bdrv_get_device_or_node_name(bs)); >>>>> - return false; >>>>> - } >>>>> - >>>>> - return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); >>>>> -} >>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>>> index 49646a30e6..615f8480b2 100644 >>>>> --- a/block/dirty-bitmap.c >>>>> +++ b/block/dirty-bitmap.c >>>>> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, >>>>> } >>>>> } >>>>> >>>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>>> + BdrvDirtyBitmap *bitmap, >>>>> + Error **errp) >>>> >>>> strange thing for me: "bitmap" in function name is _not_ the same >>>> thing as @bitmap argument.. as if it the same, >>>> function adds "persistent bitmap", but @bitmap is not persistent yet, >>>> so may be function, don't add persistent bitmap, but marks bitmap persistent? >>>> >>>> >>>> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set >>>> bitmap.persistent flag" >>>> >>> >>> Yeah, I meant "Add bitmap to file", where the persistence is implied >>> because it's part of the file. The bitmap is indeed not YET persistent, >>> but becomes so as a condition of this call succeeding. >>> >>> Do you have a suggestion for a better name? I liked the symmetry with >>> 'remove' so that there was an obvious "add bitmap" and "remove bitmap". >>> >>> Of course, when you remove, it is indeed persistent, so >>> "remove_persistent_dirty_bitmap" makes sense there. >>> >>>> >>>>> +{ >>>>> + BlockDriver *drv = bs->drv; >>>>> + int ret; >>>>> + >>>>> + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { >>>>> + error_setg(errp, "Bitmap '%s' is already persistent, " >>>>> + "and cannot be re-added to node '%s'", >>>>> + bdrv_dirty_bitmap_name(bitmap), >>>>> + bdrv_get_device_or_node_name(bs)); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + if (!drv) { >>>>> + error_setg_errno(errp, ENOMEDIUM, >>>>> + "Can't store persistent bitmaps to %s", >>>>> + bdrv_get_device_or_node_name(bs)); >>>>> + return -ENOMEDIUM; >>>>> + } >>>>> + >>>>> + if (!drv->bdrv_add_persistent_dirty_bitmap) { >>>>> + error_setg_errno(errp, ENOTSUP, >>>>> + "Can't store persistent bitmaps to %s", >>>>> + bdrv_get_device_or_node_name(bs)); >>>>> + return -ENOTSUP; >>>>> + } >>>>> + >>>>> + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp); >>>>> + if (ret == 0) { >>>>> + bdrv_dirty_bitmap_set_persistence(bitmap, true); >>>>> + } >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> + >>>>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >>>>> { >>>>> bdrv_dirty_bitmap_lock(bitmap); >>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>>>> index 83aee1a42a..c3a72ca782 100644 >>>>> --- a/block/qcow2-bitmap.c >>>>> +++ b/block/qcow2-bitmap.c >>>>> @@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) >>>>> return 0; >>>>> } >>>>> >>>>> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>>> - const char *name, >>>>> - uint32_t granularity, >>>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, >>>>> + BdrvDirtyBitmap *bitmap, >>>>> Error **errp) >>>>> { >>>>> BDRVQcow2State *s = bs->opaque; >>>>> bool found; >>>>> Qcow2BitmapList *bm_list; >>>>> + const char *name = bdrv_dirty_bitmap_name(bitmap); >>>>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); >>>>> + int ret = 0; >>>> >>>> dead assignment >>>> >>> >>> Will take care of. >>> >>>>> >>>>> if (s->qcow_version < 3) { >>>>> /* Without autoclear_features, we would always have to assume >>>>> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>>> * have to drop all dirty bitmaps (defeating their purpose). >>>>> */ >>>>> error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files"); >>>>> + ret = -ENOTSUP; >>>>> goto fail; >>>>> } >>>>> >>>>> - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) { >>>>> + ret = check_constraints_on_bitmap(bs, name, granularity, errp); >>>>> + if (ret) { >>>> >>>> hmm, in other places you prefere >>>> if ((ret = ...)) { >>>> syntax >>>> >>> >>> I have to get rid of those anyway, they violate our coding style. I'll >>> be replacing them all with this full style. >>> >>>>> goto fail; >>>>> } >>>>> >>>>> if (s->nb_bitmaps == 0) { >>>>> - return true; >>>>> + return 0; >>>>> } >>>>> >>>>> if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { >>>>> error_setg(errp, >>>>> "Maximum number of persistent bitmaps is already reached"); >>>>> + ret = -EOVERFLOW; >>>>> goto fail; >>>>> } >>>>> >>>>> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>>>> QCOW2_MAX_BITMAP_DIRECTORY_SIZE) >>>>> { >>>>> error_setg(errp, "Not enough space in the bitmap directory"); >>>>> + ret = -EOVERFLOW; >>>>> goto fail; >>>>> } >>>>> >>>>> - if (bitmap_list_load(bs, &bm_list, errp)) { >>>>> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) { >>>> >>>> interesting, now we load all bitmaps, so, may be, we don't need to load list every time, >>>> but may use bs.dirty_bitmaps to check such things.. But it seems unsafe >>>> >>> >>> Yeah, I would like to cache this list eventually, but I ran into a lot >>> of hassle with it last time I tried and put it off for now. >>> >>> I think we should definitely be able to. >>> >>> And in fact, if we did, we could even add these bitmaps to the bm_list >>> as soon as _add_ is called and drop the need for the queued counters. >> >> Personally I like the old _can_add_ :) >> >> But you want to make this function really do something with driver, so "_can_" is not good >> ofcourse and _add_ is better. And any kind of _mark_persistent_ or _make_persistent_ will >> be in a bad relation with simple setter _set_persistence() which we already have and which >> doesn't imply any complicated logic.. >> > > I think it's a simpler design to have "add" and "remove" callbacks and > be more direct about it. Of course, the qcow2 implementation is free to > avoid a write to disk on add if it wishes, but I don't like the idea of > having to call "can_store" and then later a "do_store" or any such thing. > > You could say that can_store is the check and then mark persistent is > the actual action, but then why keep them separate? > > I like the link between calling the driver and the later store to be > obvious. I feel like marking the bitmap persistent without the knowledge > or consent of the driver is bound to cause trouble sooner or later, so > I'd rather make the persistent call a condition of the store check > succeeding. > >> On the other hand I still not sure that we need track "queued" bitmaps in qcow2 driver, as we >> can calculate their number and needed size of directory in any moment, not extending the qcow2 >> state.. >> > > Do we want to add an O(N) check because we don't want to spend 12 bytes? We have O(N) check anyway, as we should check for existent bitmap name
diff --git a/block/qcow2.h b/block/qcow2.h index fc1b0d3c1e..95d723d3c0 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -742,9 +742,8 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, - const char *name, - uint32_t granularity, +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, Error **errp); void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, diff --git a/include/block/block.h b/include/block/block.h index f9415ed740..6d520f3b59 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, Error **errp); void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, - uint32_t granularity, Error **errp); /** * * bdrv_register_buf/bdrv_unregister_buf: diff --git a/include/block/block_int.h b/include/block/block_int.h index 06df2bda1b..93bbb66cd0 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -537,9 +537,8 @@ struct BlockDriver { * field of BlockDirtyBitmap's in case of success. */ int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp); - bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, - const char *name, - uint32_t granularity, + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, Error **errp); void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, const char *name, diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 8044ace63e..c37edbe05f 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, Error **errp); void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, + Error **errp); void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, Error **errp); diff --git a/block.c b/block.c index e3e77feee0..6aec36b7c9 100644 --- a/block.c +++ b/block.c @@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) parent_bs->drv->bdrv_del_child(parent_bs, child, errp); } - -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, - uint32_t granularity, Error **errp) -{ - BlockDriver *drv = bs->drv; - - if (!drv) { - error_setg_errno(errp, ENOMEDIUM, - "Can't store persistent bitmaps to %s", - bdrv_get_device_or_node_name(bs)); - return false; - } - - if (!drv->bdrv_can_store_new_dirty_bitmap) { - error_setg_errno(errp, ENOTSUP, - "Can't store persistent bitmaps to %s", - bdrv_get_device_or_node_name(bs)); - return false; - } - - return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); -} diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 49646a30e6..615f8480b2 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, } } +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, + Error **errp) +{ + BlockDriver *drv = bs->drv; + int ret; + + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { + error_setg(errp, "Bitmap '%s' is already persistent, " + "and cannot be re-added to node '%s'", + bdrv_dirty_bitmap_name(bitmap), + bdrv_get_device_or_node_name(bs)); + return -EINVAL; + } + + if (!drv) { + error_setg_errno(errp, ENOMEDIUM, + "Can't store persistent bitmaps to %s", + bdrv_get_device_or_node_name(bs)); + return -ENOMEDIUM; + } + + if (!drv->bdrv_add_persistent_dirty_bitmap) { + error_setg_errno(errp, ENOTSUP, + "Can't store persistent bitmaps to %s", + bdrv_get_device_or_node_name(bs)); + return -ENOTSUP; + } + + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp); + if (ret == 0) { + bdrv_dirty_bitmap_set_persistence(bitmap, true); + } + + return ret; +} + + void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { bdrv_dirty_bitmap_lock(bitmap); diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 83aee1a42a..c3a72ca782 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) return 0; } -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, - const char *name, - uint32_t granularity, +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, Error **errp) { BDRVQcow2State *s = bs->opaque; bool found; Qcow2BitmapList *bm_list; + const char *name = bdrv_dirty_bitmap_name(bitmap); + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); + int ret = 0; if (s->qcow_version < 3) { /* Without autoclear_features, we would always have to assume @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, * have to drop all dirty bitmaps (defeating their purpose). */ error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files"); + ret = -ENOTSUP; goto fail; } - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) { + ret = check_constraints_on_bitmap(bs, name, granularity, errp); + if (ret) { goto fail; } if (s->nb_bitmaps == 0) { - return true; + return 0; } if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) { error_setg(errp, "Maximum number of persistent bitmaps is already reached"); + ret = -EOVERFLOW; goto fail; } @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { error_setg(errp, "Not enough space in the bitmap directory"); + ret = -EOVERFLOW; goto fail; } - if (bitmap_list_load(bs, &bm_list, errp)) { + if ((ret = bitmap_list_load(bs, &bm_list, errp))) { goto fail; } found = find_bitmap_by_name(bm_list, name); bitmap_list_free(bm_list); if (found) { + ret = -EEXIST; error_setg(errp, "Bitmap with the same name is already stored"); goto fail; } - return true; - + return 0; fail: error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ", name, bdrv_get_device_or_node_name(bs)); - return false; + return ret; } diff --git a/block/qcow2.c b/block/qcow2.c index 9396d490d5..1c78eba71b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -5229,7 +5229,7 @@ BlockDriver bdrv_qcow2 = { .bdrv_attach_aio_context = qcow2_attach_aio_context, .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw, - .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap, + .bdrv_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap, .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap, }; diff --git a/blockdev.c b/blockdev.c index 3f44b891eb..169a8da831 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name, disabled = false; } - if (persistent) { - aio_context = bdrv_get_aio_context(bs); - aio_context_acquire(aio_context); - if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) { - goto out; - } - } - bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp); if (bitmap == NULL) { - goto out; + return; } if (disabled) { bdrv_disable_dirty_bitmap(bitmap); } - bdrv_dirty_bitmap_set_persistence(bitmap, persistent); - out: - if (aio_context) { + if (persistent) { + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) { + bdrv_release_dirty_bitmap(bs, bitmap); + } aio_context_release(aio_context); } }
Instead of bdrv_can_store_new_bitmap, rework this as bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free to modify the driver state because we know we ARE adding a bitmap instead of simply being asked if we CAN store one. As part of this symmetry, move this function next to bdrv_remove_persistent_bitmap in block/dirty-bitmap.c. To cement this semantic distinction, use a bitmap object instead of the (name, granularity) pair as this allows us to set persistence as a condition of the "add" succeeding. Notably, the qcow2 implementation still does not actually store the new bitmap at add time, but merely ensures that it will be able to at store time. Signed-off-by: John Snow <jsnow@redhat.com> --- block/qcow2.h | 5 ++--- include/block/block.h | 2 -- include/block/block_int.h | 5 ++--- include/block/dirty-bitmap.h | 3 +++ block.c | 22 --------------------- block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++ block/qcow2-bitmap.c | 24 ++++++++++++++--------- block/qcow2.c | 2 +- blockdev.c | 19 +++++++----------- 9 files changed, 68 insertions(+), 52 deletions(-)