| Message ID | 20180416114414.18406-6-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | Dirty bitmaps fixing and refactoring | expand |
On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote: > bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction > commit, avoiding any rollback. > > After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it. > I'm trying to remember why we ever bothered doing it the other way. Is it because of timings with respect to other operations and we wanted the "clear" to take effect sooner rather than later to co-operate with other commands? (You and Nikolay hinted about similar problems in the Libvirt PULL api thread, too. It continues to be an issue.) Hopping in my time machine: https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01229.html (Oh, I wrote it...) Looks like it was a conscious decision to avoid ordering conflicts with other block jobs. > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/block_int.h | 1 - > block/dirty-bitmap.c | 9 --------- > blockdev.c | 16 +--------------- > 3 files changed, 1 insertion(+), 25 deletions(-) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 189666efa5..22059c8119 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -1087,7 +1087,6 @@ bool blk_dev_is_medium_locked(BlockBackend *blk); > void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes); > > void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out); > -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in); > > void bdrv_inc_in_flight(BlockDriverState *bs); > void bdrv_dec_in_flight(BlockDriverState *bs); > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 1812e17549..3c69a8eed9 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -607,15 +607,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) > bdrv_dirty_bitmap_unlock(bitmap); > } > > -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in) > -{ > - HBitmap *tmp = bitmap->bitmap; > - assert(bdrv_dirty_bitmap_enabled(bitmap)); > - assert(!bdrv_dirty_bitmap_readonly(bitmap)); > - bitmap->bitmap = in; > - hbitmap_free(tmp); > -} > - > uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap, > uint64_t offset, uint64_t bytes) > { > diff --git a/blockdev.c b/blockdev.c > index c31bf3d98d..88eae60c1c 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState { > BlkActionState common; > BdrvDirtyBitmap *bitmap; > BlockDriverState *bs; > - HBitmap *backup; > bool prepared; > } BlockDirtyBitmapState; > > @@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, > error_setg(errp, "Cannot clear a readonly bitmap"); > return; > } > - > - bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); > -} > - > -static void block_dirty_bitmap_clear_abort(BlkActionState *common) > -{ > - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > - common, common); > - > - if (state->backup) { > - bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup); > - } > } > > static void block_dirty_bitmap_clear_commit(BlkActionState *common) > @@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common) > BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > common, common); > > - hbitmap_free(state->backup); > + bdrv_clear_dirty_bitmap(state->bitmap, NULL); > } > > static void abort_prepare(BlkActionState *common, Error **errp) > @@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = { > .instance_size = sizeof(BlockDirtyBitmapState), > .prepare = block_dirty_bitmap_clear_prepare, > .commit = block_dirty_bitmap_clear_commit, > - .abort = block_dirty_bitmap_clear_abort, > } > }; > >
On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote: > bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction > commit, avoiding any rollback. > > After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- Sorry if it was my idea entirely that led you on this wild goose chase, I had forgotten :(
20.04.2018 01:47, John Snow wrote: > > On 04/16/2018 07:44 AM, Vladimir Sementsov-Ogievskiy wrote: >> bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction >> commit, avoiding any rollback. >> >> After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it. >> > I'm trying to remember why we ever bothered doing it the other way. Is > it because of timings with respect to other operations and we wanted the > "clear" to take effect sooner rather than later to co-operate with other > commands? > > (You and Nikolay hinted about similar problems in the Libvirt PULL api > thread, too. It continues to be an issue.) > > Hopping in my time machine: > > https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01229.html > > (Oh, I wrote it...) > > Looks like it was a conscious decision to avoid ordering conflicts with > other block jobs. ohh, interesting.. so, transactions are tricky. Don't you think, that this letter shows, that backup transaction is wrong, rather than clear? I think, when we write a transaction, we should understand, that full effect of the operation will be achieved only after corresponding .commit. So, backup should not do in .prepare something dependent on other operations. > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> include/block/block_int.h | 1 - >> block/dirty-bitmap.c | 9 --------- >> blockdev.c | 16 +--------------- >> 3 files changed, 1 insertion(+), 25 deletions(-) >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index 189666efa5..22059c8119 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -1087,7 +1087,6 @@ bool blk_dev_is_medium_locked(BlockBackend *blk); >> void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes); >> >> void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out); >> -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in); >> >> void bdrv_inc_in_flight(BlockDriverState *bs); >> void bdrv_dec_in_flight(BlockDriverState *bs); >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 1812e17549..3c69a8eed9 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -607,15 +607,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) >> bdrv_dirty_bitmap_unlock(bitmap); >> } >> >> -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in) >> -{ >> - HBitmap *tmp = bitmap->bitmap; >> - assert(bdrv_dirty_bitmap_enabled(bitmap)); >> - assert(!bdrv_dirty_bitmap_readonly(bitmap)); >> - bitmap->bitmap = in; >> - hbitmap_free(tmp); >> -} >> - >> uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap, >> uint64_t offset, uint64_t bytes) >> { >> diff --git a/blockdev.c b/blockdev.c >> index c31bf3d98d..88eae60c1c 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState { >> BlkActionState common; >> BdrvDirtyBitmap *bitmap; >> BlockDriverState *bs; >> - HBitmap *backup; >> bool prepared; >> } BlockDirtyBitmapState; >> >> @@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, >> error_setg(errp, "Cannot clear a readonly bitmap"); >> return; >> } >> - >> - bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); >> -} >> - >> -static void block_dirty_bitmap_clear_abort(BlkActionState *common) >> -{ >> - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, >> - common, common); >> - >> - if (state->backup) { >> - bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup); >> - } >> } >> >> static void block_dirty_bitmap_clear_commit(BlkActionState *common) >> @@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common) >> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, >> common, common); >> >> - hbitmap_free(state->backup); >> + bdrv_clear_dirty_bitmap(state->bitmap, NULL); >> } >> >> static void abort_prepare(BlkActionState *common, Error **errp) >> @@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = { >> .instance_size = sizeof(BlockDirtyBitmapState), >> .prepare = block_dirty_bitmap_clear_prepare, >> .commit = block_dirty_bitmap_clear_commit, >> - .abort = block_dirty_bitmap_clear_abort, >> } >> }; >> >>
diff --git a/include/block/block_int.h b/include/block/block_int.h index 189666efa5..22059c8119 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1087,7 +1087,6 @@ bool blk_dev_is_medium_locked(BlockBackend *blk); void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes); void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out); -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in); void bdrv_inc_in_flight(BlockDriverState *bs); void bdrv_dec_in_flight(BlockDriverState *bs); diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 1812e17549..3c69a8eed9 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -607,15 +607,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) bdrv_dirty_bitmap_unlock(bitmap); } -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in) -{ - HBitmap *tmp = bitmap->bitmap; - assert(bdrv_dirty_bitmap_enabled(bitmap)); - assert(!bdrv_dirty_bitmap_readonly(bitmap)); - bitmap->bitmap = in; - hbitmap_free(tmp); -} - uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap, uint64_t offset, uint64_t bytes) { diff --git a/blockdev.c b/blockdev.c index c31bf3d98d..88eae60c1c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState { BlkActionState common; BdrvDirtyBitmap *bitmap; BlockDriverState *bs; - HBitmap *backup; bool prepared; } BlockDirtyBitmapState; @@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, error_setg(errp, "Cannot clear a readonly bitmap"); return; } - - bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); -} - -static void block_dirty_bitmap_clear_abort(BlkActionState *common) -{ - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); - - if (state->backup) { - bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup); - } } static void block_dirty_bitmap_clear_commit(BlkActionState *common) @@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common) BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, common, common); - hbitmap_free(state->backup); + bdrv_clear_dirty_bitmap(state->bitmap, NULL); } static void abort_prepare(BlkActionState *common, Error **errp) @@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = { .instance_size = sizeof(BlockDirtyBitmapState), .prepare = block_dirty_bitmap_clear_prepare, .commit = block_dirty_bitmap_clear_commit, - .abort = block_dirty_bitmap_clear_abort, } };
bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction commit, avoiding any rollback. After this, bdrv_undo_clear_dirty_bitmap() becomes unused, so, drop it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block_int.h | 1 - block/dirty-bitmap.c | 9 --------- blockdev.c | 16 +--------------- 3 files changed, 1 insertion(+), 25 deletions(-)