| Message ID | 20170123121036.4823-23-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 23.01.2017 13:10, Vladimir Sementsov-Ogievskiy wrote: > Remove persistent bitmap from its storage on bdrv_release_dirty_bitmap. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/dirty-bitmap.c | 21 ++++++++++++++++++--- > include/block/block_int.h | 3 +++ > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 775181c9ab..0977df6309 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -297,7 +297,8 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) > > static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, > BdrvDirtyBitmap *bitmap, > - bool only_named) > + bool only_named, > + bool deep) I'd rather call it "remove_persistent" or something, which is bad grammar, but it's better at getting the point across. > { > BdrvDirtyBitmap *bm, *next; > QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { > @@ -305,6 +306,19 @@ static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, > assert(!bm->active_iterators); > assert(!bdrv_dirty_bitmap_frozen(bm)); > assert(!bm->meta); > + > + if (deep && bm->persistent && bs->drv && > + bs->drv->bdrv_remove_persistent_dirty_bitmap) > + { > + Error *local_err = NULL; > + bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, bm->name, > + &local_err); > + if (local_err != NULL) { > + error_report_err(local_err); This looks like maybe it's the wrong thing to do... I do agree that sometimes it may not be fatal, but sometimes it probably is. > + } > + } > + > + > QLIST_REMOVE(bm, list); > hbitmap_free(bm->bitmap); > g_free(bm->name); > @@ -322,16 +336,17 @@ static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, > > void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > { > - bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false); > + bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false, true); The @deep parameter (or however you decide to call it) should be available to this function's caller, too. I don't believe qcow2's load_bitmap() wants to delete the persistent bitmap in the error path; and block-dirty-bitmap-remove should probably allow the user to decide what to do. Which brings me to the error thing above: If the user (or management tool) decides that block-dirty-bitmap-remove should remove the persistent bitmap, I believe that the operation should be aborted if the persistent bitmap cannot be removed. It should not just go on and release the in-memory bitmap but abort altogether. If the user just wants to release that in-memory bitmap then, they can still go on an call block-dirty-bitmap-remove with deep=false. > } > > /** > * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()). > * There must not be any frozen bitmaps attached. > + * This function do not remove persistent bitmaps from the storage. *does Max > */ > void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) > { > - bdrv_do_release_matching_dirty_bitmap(bs, NULL, true); > + bdrv_do_release_matching_dirty_bitmap(bs, NULL, true, false); > } > > void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) > diff --git a/include/block/block_int.h b/include/block/block_int.h > index b38b3aa198..3e080d6d68 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -326,6 +326,9 @@ struct BlockDriver { > Error **errp); > bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, const char *name, > uint32_t granularity, Error **errp); > + void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, > + const char *name, > + Error **errp); > > QLIST_ENTRY(BlockDriver) list; > }; >
29.01.2017 19:54, Max Reitz wrote: > On 23.01.2017 13:10, Vladimir Sementsov-Ogievskiy wrote: >> Remove persistent bitmap from its storage on bdrv_release_dirty_bitmap. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/dirty-bitmap.c | 21 ++++++++++++++++++--- >> include/block/block_int.h | 3 +++ >> 2 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 775181c9ab..0977df6309 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -297,7 +297,8 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) >> >> static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, >> BdrvDirtyBitmap *bitmap, >> - bool only_named) >> + bool only_named, >> + bool deep) > I'd rather call it "remove_persistent" or something, which is bad > grammar, but it's better at getting the point across. > >> { >> BdrvDirtyBitmap *bm, *next; >> QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { >> @@ -305,6 +306,19 @@ static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, >> assert(!bm->active_iterators); >> assert(!bdrv_dirty_bitmap_frozen(bm)); >> assert(!bm->meta); >> + >> + if (deep && bm->persistent && bs->drv && >> + bs->drv->bdrv_remove_persistent_dirty_bitmap) >> + { >> + Error *local_err = NULL; >> + bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, bm->name, >> + &local_err); >> + if (local_err != NULL) { >> + error_report_err(local_err); > This looks like maybe it's the wrong thing to do... I do agree that > sometimes it may not be fatal, but sometimes it probably is. > >> + } >> + } >> + >> + >> QLIST_REMOVE(bm, list); >> hbitmap_free(bm->bitmap); >> g_free(bm->name); >> @@ -322,16 +336,17 @@ static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, >> >> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) >> { >> - bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false); >> + bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false, true); > The @deep parameter (or however you decide to call it) should be > available to this function's caller, too. I don't believe qcow2's > load_bitmap() wants to delete the persistent bitmap in the error path; > and block-dirty-bitmap-remove should probably allow the user to decide > what to do. > > Which brings me to the error thing above: If the user (or management > tool) decides that block-dirty-bitmap-remove should remove the > persistent bitmap, I believe that the operation should be aborted if the > persistent bitmap cannot be removed. It should not just go on and > release the in-memory bitmap but abort altogether. If the user just > wants to release that in-memory bitmap then, they can still go on an > call block-dirty-bitmap-remove with deep=false. I've started to rewrite it in that way and it seems like deep almost always will be false. What about just move call to bs->drv->bdrv_remove_persistent_dirty_bitmap() directly to qmp_block_dirty_bitmap_remove and add parameter 'remove_persistent' (or 'remove_from_storage', to make it maximum descriptive) only to this qmp command? Or even without that parameter, as leaving inconsistent version of bitmap in the storage doesn't seem useful. > >> } >> >> /** >> * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()). >> * There must not be any frozen bitmaps attached. >> + * This function do not remove persistent bitmaps from the storage. > *does > > Max > >> */ >> void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) >> { >> - bdrv_do_release_matching_dirty_bitmap(bs, NULL, true); >> + bdrv_do_release_matching_dirty_bitmap(bs, NULL, true, false); >> } >> >> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index b38b3aa198..3e080d6d68 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -326,6 +326,9 @@ struct BlockDriver { >> Error **errp); >> bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, const char *name, >> uint32_t granularity, Error **errp); >> + void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, >> + const char *name, >> + Error **errp); >> >> QLIST_ENTRY(BlockDriver) list; >> }; >> >
On 31.01.2017 17:03, Vladimir Sementsov-Ogievskiy wrote: > 29.01.2017 19:54, Max Reitz wrote: >> On 23.01.2017 13:10, Vladimir Sementsov-Ogievskiy wrote: >>> Remove persistent bitmap from its storage on bdrv_release_dirty_bitmap. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block/dirty-bitmap.c | 21 ++++++++++++++++++--- >>> include/block/block_int.h | 3 +++ >>> 2 files changed, 21 insertions(+), 3 deletions(-) >>> >>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>> index 775181c9ab..0977df6309 100644 >>> --- a/block/dirty-bitmap.c >>> +++ b/block/dirty-bitmap.c >>> @@ -297,7 +297,8 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState >>> *bs) >>> static void >>> bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, >>> BdrvDirtyBitmap >>> *bitmap, >>> - bool only_named) >>> + bool only_named, >>> + bool deep) >> I'd rather call it "remove_persistent" or something, which is bad >> grammar, but it's better at getting the point across. >> >>> { >>> BdrvDirtyBitmap *bm, *next; >>> QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { >>> @@ -305,6 +306,19 @@ static void >>> bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, >>> assert(!bm->active_iterators); >>> assert(!bdrv_dirty_bitmap_frozen(bm)); >>> assert(!bm->meta); >>> + >>> + if (deep && bm->persistent && bs->drv && >>> + bs->drv->bdrv_remove_persistent_dirty_bitmap) >>> + { >>> + Error *local_err = NULL; >>> + bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, >>> bm->name, >>> + >>> &local_err); >>> + if (local_err != NULL) { >>> + error_report_err(local_err); >> This looks like maybe it's the wrong thing to do... I do agree that >> sometimes it may not be fatal, but sometimes it probably is. >> >>> + } >>> + } >>> + >>> + >>> QLIST_REMOVE(bm, list); >>> hbitmap_free(bm->bitmap); >>> g_free(bm->name); >>> @@ -322,16 +336,17 @@ static void >>> bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, >>> void bdrv_release_dirty_bitmap(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap) >>> { >>> - bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false); >>> + bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false, true); >> The @deep parameter (or however you decide to call it) should be >> available to this function's caller, too. I don't believe qcow2's >> load_bitmap() wants to delete the persistent bitmap in the error path; >> and block-dirty-bitmap-remove should probably allow the user to decide >> what to do. >> >> Which brings me to the error thing above: If the user (or management >> tool) decides that block-dirty-bitmap-remove should remove the >> persistent bitmap, I believe that the operation should be aborted if the >> persistent bitmap cannot be removed. It should not just go on and >> release the in-memory bitmap but abort altogether. If the user just >> wants to release that in-memory bitmap then, they can still go on an >> call block-dirty-bitmap-remove with deep=false. > > I've started to rewrite it in that way and it seems like deep almost > always will be false. What about just move call to > bs->drv->bdrv_remove_persistent_dirty_bitmap() directly to > qmp_block_dirty_bitmap_remove and add parameter > 'remove_persistent' (or 'remove_from_storage', to make it maximum > descriptive) only to this qmp command? Or you could add an explicit bdrv_remove_persistent_dirty_bitmap() which just wraps the bs->drv function. > Or even without that parameter, as leaving inconsistent version of > bitmap in the storage doesn't seem useful. Hm, right, the bitmap will be automatically removed from the image once it's closed, right? Max
On 01.02.2017 00:00, Max Reitz wrote: > On 31.01.2017 17:03, Vladimir Sementsov-Ogievskiy wrote: [...] >> I've started to rewrite it in that way and it seems like deep almost >> always will be false. What about just move call to >> bs->drv->bdrv_remove_persistent_dirty_bitmap() directly to >> qmp_block_dirty_bitmap_remove and add parameter >> 'remove_persistent' (or 'remove_from_storage', to make it maximum >> descriptive) only to this qmp command? > > Or you could add an explicit bdrv_remove_persistent_dirty_bitmap() which > just wraps the bs->drv function. > >> Or even without that parameter, as leaving inconsistent version of >> bitmap in the storage doesn't seem useful. > > Hm, right, the bitmap will be automatically removed from the image once > it's closed, right? [No, it won't.] But you're right, if a persistent bitmap is completely removed by the user, it should probably be always removed from the image file, too. The only use I could think of for dropping a bitmap from memory but wanting to keep it in the file is for keeping it at a certain state. It would make more sense to allow the user to disable a bitmap to achieve this, though. We don't have a block-dirty-bitmap-disable command yet, though, but I guess it shouldn't be too hard to add...? (Anyway, it's something that's out of scope for this series, I think.) Max
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 775181c9ab..0977df6309 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -297,7 +297,8 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, - bool only_named) + bool only_named, + bool deep) { BdrvDirtyBitmap *bm, *next; QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { @@ -305,6 +306,19 @@ static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, assert(!bm->active_iterators); assert(!bdrv_dirty_bitmap_frozen(bm)); assert(!bm->meta); + + if (deep && bm->persistent && bs->drv && + bs->drv->bdrv_remove_persistent_dirty_bitmap) + { + Error *local_err = NULL; + bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, bm->name, + &local_err); + if (local_err != NULL) { + error_report_err(local_err); + } + } + + QLIST_REMOVE(bm, list); hbitmap_free(bm->bitmap); g_free(bm->name); @@ -322,16 +336,17 @@ static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs, void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) { - bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false); + bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false, true); } /** * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()). * There must not be any frozen bitmaps attached. + * This function do not remove persistent bitmaps from the storage. */ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) { - bdrv_do_release_matching_dirty_bitmap(bs, NULL, true); + bdrv_do_release_matching_dirty_bitmap(bs, NULL, true, false); } void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) diff --git a/include/block/block_int.h b/include/block/block_int.h index b38b3aa198..3e080d6d68 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -326,6 +326,9 @@ struct BlockDriver { Error **errp); bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, const char *name, uint32_t granularity, Error **errp); + void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs, + const char *name, + Error **errp); QLIST_ENTRY(BlockDriver) list; };
Remove persistent bitmap from its storage on bdrv_release_dirty_bitmap. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/dirty-bitmap.c | 21 ++++++++++++++++++--- include/block/block_int.h | 3 +++ 2 files changed, 21 insertions(+), 3 deletions(-)