All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Eric Blake <eblake@redhat.com>, qemu-devel@nongnu.org
Cc: kwolf@redhat.com, nsoffer@redhat.com, qemu-block@nongnu.org,
	mreitz@redhat.com
Subject: Re: [PATCH v4 8/9] qemu-img: Add convert --bitmaps option
Date: Mon, 18 May 2020 16:33:22 +0300	[thread overview]
Message-ID: <f3ffd596-5f9f-53a4-b491-e4f166c8bde2@virtuozzo.com> (raw)
In-Reply-To: <20200513011648.166876-9-eblake@redhat.com>

13.05.2020 04:16, Eric Blake wrote:
> Make it easier to copy all the persistent bitmaps of (the top layer
> of) a source image along with its guest-visible contents, by adding a
> boolean flag for use with qemu-img convert.  This is basically
> shorthand, as the same effect could be accomplished with a series of
> 'qemu-img bitmap --add' and 'qemu-img bitmap --merge -b source'
> commands, or by QMP commands.
> 
> Note that this command will fail in the same scenarios where 'qemu-img
> measure --bitmaps' fails, when either the source or the destanation
> lacks persistent bitmap support altogether.
> 
> See also https://bugzilla.redhat.com/show_bug.cgi?id=1779893
> 
> While touching this, clean up a couple coding issues spotted in the
> same function: an extra blank line, and merging back-to-back 'if
> (!skip_create)' blocks.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   docs/tools/qemu-img.rst |  6 ++-
>   qemu-img.c              | 85 +++++++++++++++++++++++++++++++++++++++--
>   qemu-img-cmds.hx        |  4 +-
>   3 files changed, 89 insertions(+), 6 deletions(-)
> 

[..]

> 
> +static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
> +{
> +    BdrvDirtyBitmap *bm;
> +    Error *err = NULL;
> +    BlockDirtyBitmapMergeSource *merge;
> +    BlockDirtyBitmapMergeSourceList *list;
> +
> +    FOR_EACH_DIRTY_BITMAP(src, bm) {
> +        const char *name;
> +
> +        if (!bdrv_dirty_bitmap_get_persistence(bm)) {
> +            continue;
> +        }
> +        name = bdrv_dirty_bitmap_name(bm);
> +        qmp_block_dirty_bitmap_add(dst->node_name, name,
> +                                   true, bdrv_dirty_bitmap_granularity(bm),
> +                                   true, true,
> +                                   true, !bdrv_dirty_bitmap_enabled(bm),
> +                                   &err);
> +        if (err) {
> +            error_reportf_err(err, "Failed to create bitmap %s: ", name);
> +            return -1;
> +        }
> +
> +        merge = g_new0(BlockDirtyBitmapMergeSource, 1);
> +        merge->type = QTYPE_QDICT;
> +        merge->u.external.node = g_strdup(src->node_name);
> +        merge->u.external.name = g_strdup(name);
> +        list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
> +        list->value = merge;
> +        qmp_block_dirty_bitmap_merge(dst->node_name, name, list, &err);
> +        qapi_free_BlockDirtyBitmapMergeSourceList(list);

^ duplicated code chunk with "case BITMAP_MERGE". Small helper function will not hurt.

> +        if (err) {
> +            error_reportf_err(err, "Failed to populate bitmap %s: ", name);
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>   #define MAX_BUF_SECTORS 32768
> 
>   static int img_convert(int argc, char **argv)
> @@ -2130,6 +2172,8 @@ static int img_convert(int argc, char **argv)
>       int64_t ret = -EINVAL;
>       bool force_share = false;
>       bool explict_min_sparse = false;
> +    bool bitmaps = false;
> +    size_t nbitmaps = 0;
> 
>       ImgConvertState s = (ImgConvertState) {
>           /* Need at least 4k of zeros for sparse detection */
> @@ -2149,6 +2193,7 @@ static int img_convert(int argc, char **argv)
>               {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
>               {"salvage", no_argument, 0, OPTION_SALVAGE},
>               {"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
> +            {"bitmaps", no_argument, 0, OPTION_BITMAPS},
>               {0, 0, 0, 0}
>           };
>           c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
> @@ -2272,6 +2317,9 @@ static int img_convert(int argc, char **argv)
>                */
>               s.has_zero_init = true;
>               break;
> +        case OPTION_BITMAPS:
> +            bitmaps = true;
> +            break;
>           }
>       }
> 
> @@ -2333,7 +2381,6 @@ static int img_convert(int argc, char **argv)
>           goto fail_getopt;
>       }
> 
> -
>       /* ret is still -EINVAL until here */
>       ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
>       if (ret < 0) {
> @@ -2493,6 +2540,27 @@ static int img_convert(int argc, char **argv)
>           }
>       }
> 
> +    /* Determine how many bitmaps need copying */
> +    if (bitmaps) {
> +        BdrvDirtyBitmap *bm;
> +
> +        if (s.src_num > 1) {
> +            error_report("Copying bitmaps only possible with single source");
> +            ret = -1;
> +            goto out;
> +        }
> +        if (!bdrv_supports_persistent_dirty_bitmap(blk_bs(s.src[0]))) {
> +            error_report("Source lacks bitmap support");
> +            ret = -1;
> +            goto out;
> +        }
> +        FOR_EACH_DIRTY_BITMAP(blk_bs(s.src[0]), bm) {
> +            if (bdrv_dirty_bitmap_get_persistence(bm)) {

Just note, that we can't have not persistent bitmaps here.. But the check doesn't hurt.

> +                nbitmaps++;
> +            }
> +        }
> +    }
> +
>       /*
>        * The later open call will need any decryption secrets, and
>        * bdrv_create() will purge "opts", so extract them now before
> @@ -2501,9 +2569,7 @@ static int img_convert(int argc, char **argv)
>       if (!skip_create) {
>           open_opts = qdict_new();
>           qemu_opt_foreach(opts, img_add_key_secrets, open_opts, &error_abort);
> -    }
> 
> -    if (!skip_create) {
>           /* Create the new image */
>           ret = bdrv_create(drv, out_filename, opts, &local_err);
>           if (ret < 0) {
> @@ -2541,6 +2607,13 @@ static int img_convert(int argc, char **argv)
>       }
>       out_bs = blk_bs(s.target);
> 
> +    if (nbitmaps > 0 && !bdrv_supports_persistent_dirty_bitmap(out_bs)) {
> +        error_report("Format driver '%s' does not support bitmaps",
> +                     out_fmt);
> +        ret = -1;
> +        goto out;
> +    }
> +
>       if (s.compressed && !block_driver_can_compress(out_bs->drv)) {
>           error_report("Compression not supported for this file format");
>           ret = -1;
> @@ -2600,6 +2673,12 @@ static int img_convert(int argc, char **argv)
>       }
> 
>       ret = convert_do_copy(&s);
> +
> +    /* Now copy the bitmaps */
> +    if (nbitmaps > 0 && ret == 0) {
> +        ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs);
> +    }
> +
>   out:
>       if (!ret) {
>           qemu_progress_print(100, 0);
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index bfcd9f32dddf..2791c4f58ddd 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -46,9 +46,9 @@ SRST
>   ERST
> 
>   DEF("convert", img_convert,
> -    "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
> +    "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
>   SRST
> -.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
> +.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
>   ERST
> 
>   DEF("create", img_create,
> 


-- 
Best regards,
Vladimir


  reply	other threads:[~2020-05-18 13:34 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13  1:16 [PATCH v4 0/9] qemu-img: Add convert --bitmaps Eric Blake
2020-05-13  1:16 ` [PATCH v4 1/9] docs: Sort sections on qemu-img subcommand parameters Eric Blake
2020-05-14  5:02   ` Vladimir Sementsov-Ogievskiy
2020-05-13  1:16 ` [PATCH v4 2/9] qemu-img: Fix stale comments on doc location Eric Blake
2020-05-14  5:06   ` Vladimir Sementsov-Ogievskiy
2020-05-13  1:16 ` [PATCH v4 3/9] block: Make it easier to learn which BDS support bitmaps Eric Blake
2020-05-14  5:19   ` Vladimir Sementsov-Ogievskiy
2020-05-14 14:09     ` Eric Blake
2020-05-13  1:16 ` [PATCH v4 4/9] blockdev: Promote several bitmap functions to non-static Eric Blake
2020-05-14  5:45   ` Vladimir Sementsov-Ogievskiy
2020-05-14 11:45   ` Vladimir Sementsov-Ogievskiy
2020-05-14 14:10     ` Eric Blake
2020-05-13  1:16 ` [PATCH v4 5/9] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
2020-05-14  6:21   ` Vladimir Sementsov-Ogievskiy
2020-05-14 14:15     ` Eric Blake
2020-05-13  1:16 ` [PATCH v4 6/9] qemu-img: Add bitmap sub-command Eric Blake
2020-05-14  6:45   ` Vladimir Sementsov-Ogievskiy
2020-05-14 14:20     ` Eric Blake
2020-05-14 15:09       ` Vladimir Sementsov-Ogievskiy
2020-05-18 11:42   ` Vladimir Sementsov-Ogievskiy
2020-05-18 19:07     ` Eric Blake
2020-05-18 19:38       ` Vladimir Sementsov-Ogievskiy
2020-05-13  1:16 ` [PATCH v4 7/9] qcow2: Expose bitmaps' size during measure Eric Blake
2020-05-18 13:07   ` Vladimir Sementsov-Ogievskiy
2020-05-18 19:17     ` Eric Blake
2020-05-18 19:47       ` Vladimir Sementsov-Ogievskiy
2020-05-13  1:16 ` [PATCH v4 8/9] qemu-img: Add convert --bitmaps option Eric Blake
2020-05-18 13:33   ` Vladimir Sementsov-Ogievskiy [this message]
2020-05-13  1:16 ` [PATCH v4 9/9] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
2020-05-18 14:43   ` Vladimir Sementsov-Ogievskiy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f3ffd596-5f9f-53a4-b491-e4f166c8bde2@virtuozzo.com \
    --to=vsementsov@virtuozzo.com \
    --cc=eblake@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=nsoffer@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.