Re: [Qemu-devel] [PATCH v2 3/4] block/dirty-bitmaps: add block_dirty_bitmap_check function

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>> ---
>>   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