On 2/25/19 9:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> 25.02.2019 16:37, Vladimir Sementsov-Ogievskiy wrote:
>> 23.02.2019 3:22, John Snow wrote:
>>> Instead of checking against busy, inconsistent, or read only directly,
>>> use a check function with permissions bits that let us streamline the
>>> checks without reproducing them in many places.
>>>
>>> As a side effect, this adds consistency checks to all the QMP
>>> interfaces for bitmap manipulation.
>>>
>>> Signed-off-by: John Snow <js...@redhat.com>
>>> ---
>>> block/dirty-bitmap.c | 39 ++++++++++++++++++++-------
>>> blockdev.c | 49 +++++++---------------------------
>>> include/block/dirty-bitmap.h | 13 ++++++++-
>>> migration/block-dirty-bitmap.c | 12 +++------
>>> nbd/server.c | 3 +--
>>> 5 files changed, 54 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 4e8f931659..2e7fd81866 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -174,7 +174,7 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap
>>> *bitmap)
>>> return bitmap->successor;
>>> }
>>> -bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
>>> +static bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
>>> return bitmap->busy;
>>> }
>>> @@ -235,6 +235,33 @@ static bool
>>> bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
>>> !bitmap->successor->disabled);
>>> }
>>> +int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
>>> + Error **errp)
>>> +{
>>> + if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
>>> + error_setg(errp, "Bitmap '%s' is currently in use by another"
>>> + " operation and cannot be used", bitmap->name);
>>> + return -1;
>>> + }
>>> +
>>> + if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
>>> + error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
>>> + bitmap->name);
>>> + return -1;
>>> + }
>>> +
>>> + if ((flags & BDRV_BITMAP_INCONSISTENT) &&
>>> + bdrv_dirty_bitmap_inconsistent(bitmap)) {
>>> + error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
>>> + bitmap->name);
>>> + error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
>>> + "this bitmap from disk");
>>> + return -1;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> /**
>>> * Create a successor bitmap destined to replace this bitmap after an
>>> operation.
>>> * Requires that the bitmap is not user_locked and has no successor.
>>> @@ -792,15 +819,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest,
>>> const BdrvDirtyBitmap *src,
>>> qemu_mutex_lock(dest->mutex);
>>> - if (bdrv_dirty_bitmap_busy(dest)) {
>>> - error_setg(errp, "Bitmap '%s' is currently in use by another"
>>> - " operation and cannot be modified", dest->name);
>>> - goto out;
>>> - }
>>> -
>>> - if (bdrv_dirty_bitmap_readonly(dest)) {
>>> - error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
>>> - dest->name);
>>> + if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
>>> goto out;
>>> }
>>> diff --git a/blockdev.c b/blockdev.c
>>> index cbce44877d..5d74479ba7 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -2007,11 +2007,7 @@ static void
>>> block_dirty_bitmap_clear_prepare(BlkActionState *common,
>>> return;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(state->bitmap)) {
>>> - error_setg(errp, "Cannot modify a bitmap in use by another
>>> operation");
>>> - return;
>>> - } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
>>> - error_setg(errp, "Cannot clear a readonly bitmap");
>>> + if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_DEFAULT, errp))
>>> {
>>> return;
>>> }
>>> @@ -2056,10 +2052,7 @@ static void
>>> block_dirty_bitmap_enable_prepare(BlkActionState *common,
>>> return;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(state->bitmap)) {
>>> - error_setg(errp,
>>> - "Bitmap '%s' is currently in use by another operation"
>>> - " and cannot be enabled", action->name);
>>> + if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO,
>>> errp)) {
>>> return;
>>> }
>>
>> hmm, preexisting, but seems a very bad idea to enable readonly bitmap.
>>
Hmm...
I think I was thinking that you might want to enable or disable the
bitmap prior to remounting RW. I think that's a valid sequence of events.
>>> @@ -2097,10 +2090,7 @@ static void
>>> block_dirty_bitmap_disable_prepare(BlkActionState *common,
>>> return;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(state->bitmap)) {
>>> - error_setg(errp,
>>> - "Bitmap '%s' is currently in use by another operation"
>>> - " and cannot be disabled", action->name);
>>> + if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO,
>>> errp)) {
>>> return;
>>> }
>>
>> as well as disable too
>>
>>> @@ -2891,10 +2881,7 @@ void qmp_block_dirty_bitmap_remove(const char *node,
>>> const char *name,
>>> return;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(bitmap)) {
>>> - error_setg(errp,
>>> - "Bitmap '%s' is currently in use by another operation
>>> and"
>>> - " cannot be removed", name);
>>> + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
>>> return;
>>> }
>>
>> yes it's OK to remove inconsistent bitmap..
>>
>> what about readonly? I don't see a problem with it, readonly just means that
>> we didn't set INUSE
>> flag for bitmap, but if we are going to remove it, why not? But it most
>> probably will fail,
>> as qcow2 image is most probably readonly too.
>>
>> any way, it seems correct to chack only for _BUSY here.
>>
Given my reasoning below, I think maybe I should just make this fail as
early as possible. We KNOW that the image is read only. We KNOW that
this will fail. Best to just make it fail very quickly.
>>> @@ -2930,13 +2917,7 @@ void qmp_block_dirty_bitmap_clear(const char *node,
>>> const char *name,
>>> return;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(bitmap)) {
>>> - error_setg(errp,
>>> - "Bitmap '%s' is currently in use by another operation"
>>> - " and cannot be cleared", name);
>>> - return;
>>> - } else if (bdrv_dirty_bitmap_readonly(bitmap)) {
>>> - error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared",
>>> name);
>>> + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>>> return;
>>> }
>>> @@ -2954,10 +2935,7 @@ void qmp_block_dirty_bitmap_enable(const char *node,
>>> const char *name,
>>> return;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(bitmap)) {
>>> - error_setg(errp,
>>> - "Bitmap '%s' is currently in use by another operation"
>>> - " and cannot be enabled", name);
>>> + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>> return;
>>> }
>>> @@ -2975,10 +2953,7 @@ void qmp_block_dirty_bitmap_disable(const char
>>> *node, const char *name,
>>> return;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(bitmap)) {
>>> - error_setg(errp,
>>> - "Bitmap '%s' is currently in use by another operation"
>>> - " and cannot be disabled", name);
>>> + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>> return;
>>> }
>>> @@ -3551,10 +3526,7 @@ static BlockJob *do_drive_backup(DriveBackup
>>> *backup, JobTxn *txn,
>>> bdrv_unref(target_bs);
>>> goto out;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(bmap)) {
>>> - error_setg(errp,
>>> - "Bitmap '%s' is currently in use by another
>>> operation"
>>> - " and cannot be used for backup", backup->bitmap);
>>> + if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>> goto out;
>>> }
>>> }
>>> @@ -3664,10 +3636,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup,
>>> JobTxn *txn,
>>> error_setg(errp, "Bitmap '%s' could not be found",
>>> backup->bitmap);
>>> goto out;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(bmap)) {
>>> - error_setg(errp,
>>> - "Bitmap '%s' is currently in use by another
>>> operation"
>>> - " and cannot be used for backup", backup->bitmap);
>>> + if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>> goto out;
>>> }
>>> }
>>
>> hmm, and doing backup on RO bitmap is questionable thing. On one hand - why
>> not, we can.
>>
>> But we will not be able to abdicate, as we can't modify bitmap.
>>
>> So, it seems better to restrict it too. If use needs to do an incremental
>> backup from
>> read-only image, not modifying a bitmap, he should either copy this bitmap
>> (through
>> merge for example) to a not-persistent one, or he need a way to skip
>> bitmap_abdicate.
>>
Hmm, you are right... we can't clear the bitmap on success, and we KNOW
we'll be unable to do that from the outset. It's better to fail early.
I'll amend this.
>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>> index b270773f7e..ee98b5014b 100644
>>> --- a/include/block/dirty-bitmap.h
>>> +++ b/include/block/dirty-bitmap.h
>>> @@ -5,6 +5,16 @@
>>> #include "qapi/qapi-types-block-core.h"
>>> #include "qemu/hbitmap.h"
>>> +typedef enum BitmapCheckFlags {
>>> + BDRV_BITMAP_BUSY = 1,
>>> + BDRV_BITMAP_RO = 2,
>>> + BDRV_BITMAP_INCONSISTENT = 4,
>>> +} BitmapCheckFlags;
>>> +
>>> +#define BDRV_BITMAP_DEFAULT (BDRV_BITMAP_BUSY | BDRV_BITMAP_RO | \
>>> + BDRV_BITMAP_INCONSISTENT)
>>> +#define BDRV_BITMAP_ALLOW_RO (BDRV_BITMAP_BUSY | BDRV_BITMAP_INCONSISTENT)
>>> +
>>> BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>> uint32_t granularity,
>>> const char *name,
>>> @@ -24,6 +34,8 @@ BdrvDirtyBitmap
>>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>>> void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
>>> BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>>> const char *name);
>>> +int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
>>> + Error **errp);
>>> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
>>> *bitmap);
>>> void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>>> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>> @@ -93,7 +105,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>> bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>>> bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>>> bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap);
>>> -bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>>> bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>> BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>>> BdrvDirtyBitmap *bitmap);
>>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>>> index 7057fff242..0fcf897f32 100644
>>> --- a/migration/block-dirty-bitmap.c
>>> +++ b/migration/block-dirty-bitmap.c
>>> @@ -274,6 +274,7 @@ static int init_dirty_bitmap_migration(void)
>>> BdrvDirtyBitmap *bitmap;
>>> DirtyBitmapMigBitmapState *dbms;
>>> BdrvNextIterator it;
>>> + Error *local_err = NULL;
>>> dirty_bitmap_mig_state.bulk_completed = false;
>>> dirty_bitmap_mig_state.prev_bs = NULL;
>>> @@ -301,15 +302,8 @@ static int init_dirty_bitmap_migration(void)
>>> goto fail;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(bitmap)) {
>>> - error_report("Can't migrate a bitmap that is in use by
>>> another operation: '%s'",
>>> - bdrv_dirty_bitmap_name(bitmap));
>>> - goto fail;
>>> - }
>>> -
>>> - if (bdrv_dirty_bitmap_readonly(bitmap)) {
>>> - error_report("Can't migrate read-only dirty bitmap: '%s",
>>> - bdrv_dirty_bitmap_name(bitmap));
>>> + if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT,
>>> &local_err)) {
>>> + error_report_err(local_err);
>>> goto fail;
>>> }
>>> diff --git a/nbd/server.c b/nbd/server.c
>>> index 02773e2836..9b87c7f243 100644
>>> --- a/nbd/server.c
>>> +++ b/nbd/server.c
>>> @@ -1510,8 +1510,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
>>> uint64_t dev_offset,
>>> goto fail;
>>> }
>>> - if (bdrv_dirty_bitmap_busy(bm)) {
>>> - error_setg(errp, "Bitmap '%s' is in use", bitmap);
>>> + if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) {
>>> goto fail;
>>> }
>>>
>>
>> and it is the only correct usage of ALLOW_RO, I think.
>>
>> hmm, doesn't this mean that we should move to "int flags" in BdrvDirtyBitmap?
>
> and this will gave us cool bdrv_create_dirty_bitmap with flags, to easily
> create different kind of bitmaps.
>
>
> And one more important question: we now creating new qapi bitmap status,
> consisting of several bool fields.
> Shouldn't we instead implement it as an array of flags?
>
>
To the above questions I am deferring to Eric's judgement[1], I happen
to like flags as well but I trust Eric's assessment that it's more
complicated than we need in this case.
I also don't want to encourage anybody adding any more :)
[1] en-US spelling suggests we ought to use "judgment", but that's
stupid and I hate it. I will not cater to prescriptivists.
