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

[Vladimir Sementsov-Ogievskiy]

01.03.2019 22:15, 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.
> 
> Included in this patch are permissions changes that simply add the
> inconsistent check to existing permissions call spots, without
> addressing existing bugs.
> 
> In general, this means that busy+readonly checks become BDRV_BITMAP_DEFAULT,
> which checks against all three conditions. busy-only checks become
> BDRV_BITMAP_ALLOW_RO.

so, it's a conversion + check on inconsistance. backup/disable/enable 
correctness
postponed.

you've left bdrv_dirty_bitmap_create_successor as is..

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> 
> Notably, remove allows inconsistent bitmaps, so it doesn't follow the pattern.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>   include/block/dirty-bitmap.h   | 13 ++++++++-
>   block/dirty-bitmap.c           | 38 +++++++++++++++++++-------
>   blockdev.c                     | 49 +++++++---------------------------
>   migration/block-dirty-bitmap.c | 12 +++------
>   nbd/server.c                   |  3 +--
>   5 files changed, 54 insertions(+), 61 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index bd1b6479df..2a78243954 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(const 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(const 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/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 71e0098396..769668ccdc 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(const 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(const 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 marked busy and has no successor.
> @@ -794,17 +821,10 @@ 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);
> +    if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
>           goto out;
>       }
>   
> -    if (bdrv_dirty_bitmap_readonly(dest)) {
> -        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> -                   dest->name);
> -        goto out;
> -    }
>   
>       if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
>           error_setg(errp, "Bitmaps are incompatible and can't be merged");
> 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;
>       }
>   
> @@ -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;
>       }
>   
> @@ -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;
>       }
>   
> @@ -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;
>           }
>       }
> 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;
>           }
>   
> 


-- 
Best regards,
Vladimir