Re: [Qemu-devel] [PATCH v2 5/7] blockdev: refactor block-dirty-bitmap-clear transaction

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> > ---
> >   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,
> >       }
> >   };
> >   


--
Best regards,
Vladimir