Re: [Qemu-devel] [PATCH 05/11] block: add delayed bitmap successor cleanup

[John Snow <jsnow@redhat.com>]

On 03/17/2015 02:44 PM, Max Reitz wrote:
> On 2015-03-04 at 23:15, John Snow wrote:
>> Allow bitmap successors to carry reference counts.
>>
>> We can in a later patch use this ability to clean up the dirty bitmap
>> according to both the individual job's success and the success of all
>> jobs in the transaction group.
>>
>> The code for cleaning up a bitmap is also moved from backup_run to
>> backup_complete.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block.c               | 79
>> +++++++++++++++++++++++++++++++++++++++++++++++----
>>   block/backup.c        | 23 ++++++---------
>>   include/block/block.h | 11 ++++---
>>   3 files changed, 87 insertions(+), 26 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 5eaa874..a0036af 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -51,6 +51,12 @@
>>   #include <windows.h>
>>   #endif
>> +typedef enum BitmapSuccessorAction {
>> +    SUCCESSOR_ACTION_UNDEFINED = 0,
>> +    SUCCESSOR_ACTION_ABDICATE,
>> +    SUCCESSOR_ACTION_RECLAIM
>> +} BitmapSuccessorAction;
>
> Maybe making this a QAPI enum can come in handy later. I don't know in
> which QMP commands one would use it, but if I could predict the future,
> I'd make trillions (or even billions) at the stock market.
>
> (It's just that it already looks so much alike to a QAPI enum that I
> can't help but to think of making it one)
>
>> +
>>   /**
>>    * A BdrvDirtyBitmap can be in three possible states:
>>    * (1) successor is false and disabled is false: full r/w mode
>> @@ -65,6 +71,8 @@ struct BdrvDirtyBitmap {
>>       char *name;                 /* Optional non-empty unique ID */
>>       int64_t size;               /* Size of the bitmap (Number of
>> sectors) */
>>       bool disabled;              /* Bitmap is read-only */
>> +    int successor_refcount;     /* Number of active handles to the
>> successor */
>> +    BitmapSuccessorAction act;  /* Action to take on successor upon
>> release */
>>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>>   };
>> @@ -5508,6 +5516,7 @@ int
>> bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>       /* Install the successor and freeze the parent */
>>       bitmap->successor = child;
>> +    bitmap->successor_refcount = 1;
>>       return 0;
>>   }
>> @@ -5515,9 +5524,9 @@ int
>> bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>    * 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)
>> +static BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>> +                                                   BdrvDirtyBitmap
>> *bitmap,
>> +                                                   Error **errp)
>>   {
>>       char *name;
>>       BdrvDirtyBitmap *successor = bitmap->successor;
>> @@ -5542,9 +5551,9 @@ BdrvDirtyBitmap
>> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>>    * 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)
>> +static BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>> +                                                  BdrvDirtyBitmap
>> *parent,
>> +                                                  Error **errp)
>>   {
>>       BdrvDirtyBitmap *successor = parent->successor;
>> @@ -5563,6 +5572,64 @@ BdrvDirtyBitmap
>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>>       return parent;
>>   }
>> +static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs,
>> +                                                   BdrvDirtyBitmap
>> *parent,
>> +                                                   Error **errp)
>> +{
>> +    if (parent->successor_refcount) {
>
> I don't know whether you're intending to call this function from
> anywhere outside of bdrv_dirty_bitmap_decref(), but if you don't, please
> just make this an assert(!parent->successor_refcount).
>

No, deleted.

>> +        error_setg(errp, "Cannot free the successor for bitmap '%s', "
>> +                   "because the refcount is non-zero.", parent->name);
>
> Hm, again with the full stops? *g*
>

Erased error message altogether.

>> +        return NULL;
>> +    }
>> +
>> +    switch (parent->act) {
>> +    case SUCCESSOR_ACTION_UNDEFINED:
>> +        error_setg(errp, "Cannot free the successor for bitmap '%s', "
>> +                   "because the successor action has not yet been set.",
>
> Indeed, again with the full stops. ;-)
>

And this one.

>> +                   parent->name);
>> +        return NULL;
>
> So you're (for now) only calling this function from
> bdrv_dirty_bitmap_decref(), and that function always makes sure that
> parent->act is set to SUCCESSOR_ACTION_{RECLAIM,ABDICATE}. Why not add
> an assert(parent->act != SUCCESSOR_ACTION_UNDEFINED) before the switch?
> (If that causes problems in regards to compiler warnings or something
> (not having all enum values covered in the switch), just add an
> assert(0); under "case SUCCESSOR_ACTION_UNDEFINED:".)
>

Will use g_assert_not_reached() because I like the way it reads.

>> +    case SUCCESSOR_ACTION_RECLAIM:
>> +        return bdrv_reclaim_dirty_bitmap(bs, parent, errp);
>> +    case SUCCESSOR_ACTION_ABDICATE:
>> +        return bdrv_dirty_bitmap_abdicate(bs, parent, errp);
>> +    default:
>> +        error_setg(errp,
>> +                   "Unrecognized successor action (%d), "
>> +                   "cannot free successor for bitmap '%s'",
>> +                   parent->act, parent->name);
>> +        return NULL;
>
> This can be made an assert(0), a g_assert_not_reached(), or simply an
> abort() (with the latter probably being the preferred way).
>
> So I think that all the error_setg() calls are actually cases that
> should never happen. Better make them abort() and drop the Error
> parameter (because *_decref() and *_free() functions normally (for good
> reason) don't have Error parameters...).
>

The problem all stems from when I made abdicate and reclaim callable 
interfaces in the original patch series, so they carried errp pointers 
and I let them flow outwards from there.

So, the transactionless set is still on-list, so I could clean those 
interfaces too, but I suppose I wanted to see how this series was 
received before I squashed too many things back into the v1 ... I'd 
rather leave that series be for obvious reasons.

>> +    }
>> +}
>> +
>> +BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs,
>
> I don't know whether I'm that content with the name chosen, because
> you're actually decrementing the refcount of the successor; but since
> the successor is basically a clone of the original bitmap (and I mean in
> the Star Trek sense, that it's a teleported clone and the original is
> intended to be destroyed so the successor can replace it), decrementing
> the refcount of the successor basically is equal to decrementing the
> refcount of the bitmap itself (as long as there is a successor, which
> you are asserting; maybe you want to add a comment about that to
> include/block/block.h, that one can only use this on frozen bitmaps?).
>

I could get clever with the name and call it bdrv_frozen_bitmap_decref, 
which hopefully still shows membership to the bdrv_dirty_bitmap class of 
functions but clarifies its usage sufficiently.

>> +                                          BdrvDirtyBitmap *parent,
>> +                                          int ret,
>> +                                          Error **errp)
>> +{
>> +    assert(bdrv_dirty_bitmap_frozen(parent));
>> +    assert(parent->successor);
>> +
>> +    if (ret) {
>> +        parent->act = SUCCESSOR_ACTION_RECLAIM;
>> +    } else if (parent->act != SUCCESSOR_ACTION_RECLAIM) {
>> +        parent->act = SUCCESSOR_ACTION_ABDICATE;
>> +    }
>> +
>> +    parent->successor_refcount--;
>> +    if (parent->successor_refcount == 0) {
>> +        return bdrv_free_bitmap_successor(bs, parent, errp);
>
> If you drop the Error parameter from bdrv_free_bitmap_successor(), you
> can drop it from this function, too.
>

Done.

>> +    }
>> +    return parent;
>> +}
>> +
>> +void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent)
>> +{
>> +    assert(bdrv_dirty_bitmap_frozen(parent));
>> +    assert(parent->successor);
>> +
>> +    parent->successor_refcount++;
>> +}
>> +
>>   static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t
>> size)
>>   {
>>       /* Should only be frozen during a block backup job, which should
>> have
>> diff --git a/block/backup.c b/block/backup.c
>> index 41bd9af..4332df4 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void
>> *opaque)
>>       bdrv_unref(s->target);
>> +    if (s->sync_bitmap) {
>> +        BdrvDirtyBitmap *bm;
>> +        bm = bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap,
>> data->ret, NULL);
>> +        assert(bm);
>
> You can use &error_abort as the Error object and drop the assert(); or,
> if you are dropping the Error parameter, there is no need to check the
> return value at all, because it will always be non-NULL (there won't be
> any code path in the function returning NULL at all). Maybe you can even
> drop the return value, too.
>
> I just looked through the series: Actually, you're never using the Error
> parameter for bdrv_dirty_bitmap_decref() at all. Seems to me like you
> really should drop it (and maybe the return value along with it).
>

I actually use this parameter to return the "new bitmap" (if any) after 
the decrement operation. I wanted to leave the window open for merge 
optimizations, so I tell the caller which bitmap remains after the 
operation.

I will cull the errp, but will likely leave the return code.

>> +    }
>> +
>>       block_job_completed(job, data->ret);
>>       g_free(data);
>>   }
>> @@ -419,19 +425,6 @@ leave:
>>       qemu_co_rwlock_wrlock(&job->flush_rwlock);
>>       qemu_co_rwlock_unlock(&job->flush_rwlock);
>> -    if (job->sync_bitmap) {
>> -        BdrvDirtyBitmap *bm;
>> -        if (ret < 0) {
>> -            /* Merge the successor back into the parent, delete
>> nothing. */
>> -            bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
>> -            assert(bm);
>> -            bdrv_enable_dirty_bitmap(job->sync_bitmap);
>
> Hm, what is that function call doing here? It feels like it shouldn't
> have been part of your transactionless series (because other than this,
> there is no caller of bdrv_{en,dis}able_dirty_bitmap() at all).
>
> (You're silently removing it here; removing it is fine, but I guess it
> shouldn't have been there in the first place)
>

Fixed in v4 for the transactionless series.

>> -        } else {
>> -            /* Everything is fine, delete this bitmap and install the
>> backup. */
>> -            bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
>> -            assert(bm);
>> -        }
>> -    }
>>       hbitmap_free(job->bitmap);
>>       bdrv_iostatus_disable(target);
>> @@ -535,6 +528,8 @@ void backup_start(BlockDriverState *bs,
>> BlockDriverState *target,
>>    error:
>>       if (sync_bitmap) {
>> -        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
>> +        BdrvDirtyBitmap *ret;
>> +        ret = bdrv_dirty_bitmap_decref(bs, sync_bitmap, -1, NULL);
>> +        assert(ret);
>>       }
>>   }
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 3a85690..d7859a7 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -458,12 +458,11 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState
>> *bs);
>>   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_dirty_bitmap_decref(BlockDriverState *bs,
>> +                                          BdrvDirtyBitmap *parent,
>> +                                          int ret,
>> +                                          Error **errp);
>> +void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent);
>>   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>>                                           const char *name);
>>   void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
>
> I'm happy with this patch if you drop the Error parameter from
> bdrv_dirty_bitmap_decref() (possibly dropping the return value, too) and
> turn all the error cases into assertions.
>
> Max
>