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.
>
>> @@ -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.
>
>> @@ -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.
>
>> 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?
--
Best regards,
Vladimir
