Re: [Qemu-devel] [PATCH v11 08/13] block: Add bitmap successors

[John Snow <jsnow@redhat.com>]

On 01/13/2015 04:24 AM, Fam Zheng wrote:
> On Mon, 01/12 11:31, John Snow wrote:
>> A bitmap successor is an anonymous BdrvDirtyBitmap that is intended to
>> be created just prior to a sensitive operation (e.g. Incremental Backup)
>> that can either succeed or fail, but during the course of which we still
>> want a bitmap tracking writes.
>>
>> On creating a successor, we "freeze" the parent bitmap which prevents
>> its deletion, enabling, anonymization, or creating a bitmap with the
>> same name.
>>
>> On success, the parent bitmap can "abdicate" responsibility to the
>> successor, which will inherit its name. The successor will have been
>> tracking writes during the course of the backup operation. The parent
>> will be safely deleted.
>>
>> On failure, we can "reclaim" the successor from the parent, unifying
>> them such that the resulting bitmap describes all writes occurring since
>> the last successful backup, for instance. Reclamation will thaw the
>> parent, but not explicitly re-enable it.
>
> If we compare this with image snapshot and overlay, it fits the current backing
> chain model very well.  Given that we're on the way to persistent dirty bitmap,
> which will probably be read/written with general block.c code, I'm wondering if
> it will be any better to reuse the block layer backing file code to handle
> "successor" logic?
>
> Also with the frozen feature built in dirty bitmaps, is it okay to drop
> "enabled" state?
>
> I think there are three states for a bitmap:
>
> 1) Active, no successor (similar to an read-write top image)
> 2) Frozen, no successor (similar to an read-only top image)
> 3) Frozen, with successor (similar to an read-only backing file, with an
>     overlap)
>
> Admittedly, more code is needed than this patch, in order to glue hbitmap and
> block layer together, but it would probably make live migration of dirty bitmap
> very easy, but I'm not sure without a closer look.
>
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block.c               | 123 ++++++++++++++++++++++++++++++++++++++++++++++++--
>>   blockdev.c            |  14 ++++++
>>   include/block/block.h |  10 ++++
>>   3 files changed, 144 insertions(+), 3 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index bd4b449..3f33b9d 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -53,10 +53,12 @@
>>
>>   struct BdrvDirtyBitmap {
>>       HBitmap *bitmap;
>> +    BdrvDirtyBitmap *successor;
>>       int64_t size;
>>       int64_t granularity;
>>       char *name;
>>       bool enabled;
>> +    bool frozen;
>>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>>   };
>>
>> @@ -5342,6 +5344,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
>>
>>   void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>>   {
>> +    assert(!bitmap->frozen);
>>       g_free(bitmap->name);
>>       bitmap->name = NULL;
>>   }
>> @@ -5379,9 +5382,114 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>       return bitmap;
>>   }
>>
>> +/**
>> +  * A frozen bitmap cannot be renamed, deleted, cleared,
>> +  * or set. A frozen bitmap can only abdicate, reclaim,
>> +  * or thaw.
>> +  */
>> +static void bdrv_freeze_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>> +{
>> +    bitmap->frozen = true;
>> +}
>> +
>> +static void bdrv_thaw_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>> +{
>> +    bitmap->frozen = false;
>> +}
>> +
>> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
>> +{
>> +    return bitmap->frozen;
>> +}
>> +
>>   bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
>>   {
>> -    return bitmap->enabled;
>> +    return bitmap->enabled && !bitmap->frozen;
>
> An indication that "enabled" could be replaced by "frozen". Otherwise it looks
> confusing to me.
>
>> +}
>> +
>> +/**
>> + * Create a successor bitmap destined to replace this bitmap after an operation.
>> + * Requires that the bitmap is not frozen and has no successor.
>> + */
>> +int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>> +                                       BdrvDirtyBitmap *bitmap, Error **errp)
>> +{
>> +    uint64_t granularity;
>> +
>> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> +        error_setg(errp,
>> +                   "Cannot create a successor for a bitmap currently in-use.");
>> +        return -1;
>> +    } else if (bitmap->successor) {
>> +        error_setg(errp, "Cannot create a successor for a bitmap that "
>> +                   "already has one.");
>> +        return -1;
>> +    }
>> +
>> +    /* Create an anonymous successor */
>> +    granularity = bdrv_dirty_bitmap_granularity(bs, bitmap);
>> +    bitmap->successor = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
>> +    if (!bitmap->successor) {
>> +        return -1;
>> +    }
>> +
>> +    /* Successor will be on or off based on our current state.
>> +     * Parent will be disabled and frozen. */
>> +    bitmap->successor->enabled = bitmap->enabled;
>> +    bdrv_disable_dirty_bitmap(bitmap);
>> +    bdrv_freeze_dirty_bitmap(bitmap);
>> +    return 0;
>> +}
>> +
>> +/**
>> + * For a bitmap with a successor, yield our name to the successor,
>> + * Delete the old bitmap, and return a handle to the new bitmap.
>> + */
>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>> +                                            BdrvDirtyBitmap *bitmap,
>> +                                            Error **errp)
>> +{
>> +    char *name;
>> +    BdrvDirtyBitmap *successor = bitmap->successor;
>> +
>> +    if (successor == NULL) {
>> +        error_setg(errp, "Cannot relinquish control if "
>> +                   "there's no successor present.\n");
>> +        return NULL;
>> +    }
>> +
>> +    name = bitmap->name;
>> +    bitmap->name = NULL;
>> +    successor->name = name;
>> +
>> +    bdrv_thaw_dirty_bitmap(bitmap);
>> +    bdrv_release_dirty_bitmap(bs, bitmap);
>> +
>> +    return successor;
>> +}
>> +
>> +/**
>> + * In cases of failure where we can no longer safely delete the parent,
>> + * We may wish to re-join the parent and child/successor.
>> + * The merged parent will be un-frozen, but not explicitly re-enabled.
>> + */
>> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>> +                                           BdrvDirtyBitmap *parent,
>> +                                           Error **errp)
>> +{
>> +    BdrvDirtyBitmap *successor = parent->successor;
>> +    if (!successor) {
>> +        error_setg(errp, "Cannot reclaim a successor when none is present.\n");
>> +        return NULL;
>> +    }
>> +
>> +    hbitmap_merge(parent->bitmap, successor->bitmap);
>> +    bdrv_release_dirty_bitmap(bs, successor);
>> +
>> +    bdrv_thaw_dirty_bitmap(parent);
>> +    parent->successor = NULL;
>> +
>> +    return parent;
>>   }
>>
>>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>> @@ -5389,6 +5497,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>>       BdrvDirtyBitmap *bm, *next;
>>       QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>>           if (bm == bitmap) {
>> +            assert(!bm->frozen);
>>               QLIST_REMOVE(bitmap, list);
>>               hbitmap_free(bitmap->bitmap);
>>               g_free(bitmap->name);
>> @@ -5405,6 +5514,7 @@ void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>
>>   void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>   {
>> +    assert(!bitmap->frozen);
>>       bitmap->enabled = true;
>>   }
>>
>> @@ -5483,7 +5593,9 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>>   void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>>                                int64_t cur_sector, int nr_sectors)
>>   {
>> -    hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>> +    if (!bitmap->frozen) {
>
> Probably just assert it's not frozen?
>
>> +        hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>> +    }
>>   }
>>
>>   /**
>> @@ -5492,7 +5604,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>>    */
>>   void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>>   {
>> -    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
>> +    if (!bitmap->frozen) {
>> +        hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
>
> The same: probably just assert it's not frozen?
>

Looking at this again, I see what I was trying to accomplish. Similar to 
how (!enabled) will silently ignore writes, I just wanted frozen bitmaps 
to silently ignore attempts to clear or reset bits, not assert or error out.

Just in case, in some future state, there is a more generic "set/clear 
bits" mechanism that may blindly try to clear bits to all attached 
bitmaps of a particular BDS, I didn't want to throw an error in that 
case -- just ignore it.

It would be a logical error to try to clear a bitmap we KNOW is frozen, 
but there may be cases where clears and resets may be done with less 
discrimination.

I think I will leave this as-is, but I can write some extra commentary 
for it. Is that OK?

--JS

>> +    }
>>   }
>>
>>   static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>> @@ -5512,6 +5626,9 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
>>   {
>>       BdrvDirtyBitmap *bitmap;
>>       QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>> +        if (bitmap->frozen) {
>> +            continue;
>> +        }
>>           hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>>       }
>>   }
>> diff --git a/blockdev.c b/blockdev.c
>> index 118cb6c..8dde72a 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1988,9 +1988,16 @@ void qmp_block_dirty_bitmap_remove(const char *node_ref, const char *name,
>>       aio_context = bdrv_get_aio_context(bs);
>>       aio_context_acquire(aio_context);
>>
>> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> +        error_setg(errp,
>> +                   "Bitmap %s is currently frozen and cannot be removed yet.\n",
>> +                   name);
>> +        goto out;
>> +    }
>>       bdrv_dirty_bitmap_make_anon(bs, bitmap);
>>       bdrv_release_dirty_bitmap(bs, bitmap);
>>
>> + out:
>>       aio_context_release(aio_context);
>>   }
>>
>> @@ -2009,8 +2016,15 @@ void qmp_block_dirty_bitmap_enable(const char *node_ref, const char *name,
>>       aio_context = bdrv_get_aio_context(bs);
>>       aio_context_acquire(aio_context);
>>
>> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> +        error_setg(errp, "Bitmap %s is frozen and cannot be enabled yet.\n",
>> +                   name);
>> +        goto out;
>> +    }
>> +
>>       bdrv_enable_dirty_bitmap(bitmap);
>>
>> + out:
>>       aio_context_release(aio_context);
>>   }
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index a5bc249..6138544 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -433,6 +433,15 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>                                             int granularity,
>>                                             const char *name,
>>                                             Error **errp);
>> +int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>> +                                       BdrvDirtyBitmap *bitmap,
>> +                                       Error **errp);
>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>> +                                            BdrvDirtyBitmap *bitmap,
>> +                                            Error **errp);
>> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>> +                                           BdrvDirtyBitmap *bitmap,
>> +                                           Error **errp);
>>   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>>                                           const char *name);
>>   void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>> @@ -444,6 +453,7 @@ uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>>   uint64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
>>                                         BdrvDirtyBitmap *bitmap);
>>   bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
>> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
>>   int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
>>   void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>>                              int64_t cur_sector, int nr_sectors);
>> --
>> 1.9.3
>>
>