Re: [PATCH v5 2/5] qcow2: Expose bitmaps' size during measure

[Nir Soffer]

On Thu, May 21, 2020 at 4:08 PM Eric Blake <address@hidden> wrote:
>
> On 5/20/20 6:00 PM, Nir Soffer wrote:
>
> >>
> >> On the command-line side, 'qemu-img measure' gains a new --bitmaps
> >> flag.  When present, the bitmap size is rolled into the two existing
> >> measures (or errors if either the source image or destination format
> >> lacks bitmaps); when absent, there is never an error (for
> >> back-compat), but the output will instead include a new line item for
> >> bitmaps (which you would have to manually add), with that line being
> >> omitted in the same cases where passing --bitmaps would error.
> >
> > Supporting 2 ways to measure, one by specifying --bitmaps, and another
> > by adding bitmaps key is not a good idea. We really need one way.
> >
> > Each one has advantages. adding --bitmaps flag is consistent with
> > "qemu-img convert"
> > and future extensions that may require  new flag, and adding "bitmaps"
> > key is consistent
> > with "qmeu-img info", showing bitmaps when they exist.
> >
> > Adding a "bitmaps" key has an advantage that we can use it to test if 
> > qemu-img
> > supports measuring and copying bitmaps (since both features are expected to
> > be delivered at the same time). So we can avoid checking --help learn about
> > the capabilities.
> >
> > I'm ok with both options, can we have only one?
>
> That was the crux of the conversation after v3, where we were trying to
> figure out what interface you actually needed.  I implemented both to
> show the difference, but if you want only one, then my preference is to
> delete the --bitmaps option and only expose the optional 'bitmaps size'
> field in all cases where bitmaps can be copied.

I'm fine with this approach - but the bitmaps optional field should be displayed
even if there are no bitmaps in the source, so I can tell if tihs
version of qemu-img
supports measuring/copying bitmaps.

If measure reports bitmaps size we will create a large enough disk and
copy the bitmaps,
and if not we will have to drop the relevant backup history for this
vm, since the copy
will not include the bitmaps. The next backup for this vm will have to
be a full backup.

> Here's the diff that would accomplish that:

Diff does not eliminate the "bitmaps: 0" outputs, so it looks good.

>
> diff --git i/docs/tools/qemu-img.rst w/docs/tools/qemu-img.rst
> index 35050fc51070..69cd9a30373a 100644
> --- i/docs/tools/qemu-img.rst
> +++ w/docs/tools/qemu-img.rst
> @@ -597,7 +597,7 @@ Command description:
>     For more information, consult ``include/block/block.h`` in QEMU's
>     source code.
>
> -.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
> [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l
> SNAPSHOT_PARAM] FILENAME]
> +.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
> [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l
> SNAPSHOT_PARAM] FILENAME]
>
>     Calculate the file size required for a new image.  This information
>     can be used to size logical volumes or SAN LUNs appropriately for
> @@ -634,10 +634,7 @@ Command description:
>     copy bitmaps from a source image in addition to the guest-visible
>     data; the line is omitted if either source or destination lacks
>     bitmap support, or 0 if bitmaps are supported but there is nothing
> -  to copy.  If the ``--bitmaps`` option is in use, the bitmap size is
> -  instead folded into the required and fully-allocated size for
> -  convenience, rather than being a separate line item; using the
> -  option will raise an error if bitmaps are not supported.
> +  to copy.
>
>   .. option:: snapshot [--object OBJECTDEF] [--image-opts] [-U] [-q] [-l
> | -a SNAPSHOT | -c SNAPSHOT | -d SNAPSHOT] FILENAME
>
> diff --git i/qemu-img.c w/qemu-img.c
> index 1494d8f5c409..8dc4cae2c274 100644
> --- i/qemu-img.c
> +++ w/qemu-img.c
> @@ -5207,7 +5207,6 @@ static int img_measure(int argc, char **argv)
>           {"output", required_argument, 0, OPTION_OUTPUT},
>           {"size", required_argument, 0, OPTION_SIZE},
>           {"force-share", no_argument, 0, 'U'},
> -        {"bitmaps", no_argument, 0, OPTION_BITMAPS},
>           {0, 0, 0, 0}
>       };
>       OutputFormat output_format = OFORMAT_HUMAN;
> @@ -5224,7 +5223,6 @@ static int img_measure(int argc, char **argv)
>       QemuOpts *sn_opts = NULL;
>       QemuOptsList *create_opts = NULL;
>       bool image_opts = false;
> -    bool bitmaps = false;
>       uint64_t img_size = UINT64_MAX;
>       BlockMeasureInfo *info = NULL;
>       Error *local_err = NULL;
> @@ -5297,10 +5295,6 @@ static int img_measure(int argc, char **argv)
>               img_size = (uint64_t)sval;
>           }
>           break;
> -        case OPTION_BITMAPS:
> -            bitmaps = true;
> -            break;
> -        }
>       }
>
>       if (qemu_opts_foreach(&qemu_object_opts,
> @@ -5328,10 +5322,6 @@ static int img_measure(int argc, char **argv)
>           error_report("Either --size N or one filename must be
> specified.");
>           goto out;
>       }
> -    if (!filename && bitmaps) {
> -        error_report("--bitmaps is only supported with a filename.");
> -        goto out;
> -    }
>
>       if (filename) {
>           in_blk = img_open(image_opts, filename, fmt, 0,
> @@ -5387,18 +5377,6 @@ static int img_measure(int argc, char **argv)
>           goto out;
>       }
>
> -    if (bitmaps) {
> -        if (!info->has_bitmaps) {
> -            error_report("no bitmaps measured, either source or
> destination "
> -                         "format lacks bitmap support");
> -            goto out;
> -        } else {
> -            info->required += info->bitmaps;
> -            info->fully_allocated += info->bitmaps;
> -            info->has_bitmaps = false;
> -        }
> -    }
> -
>       if (output_format == OFORMAT_HUMAN) {
>           printf("required size: %" PRIu64 "\n", info->required);
>           printf("fully allocated size: %" PRIu64 "\n",
> info->fully_allocated);
> diff --git i/qemu-img-cmds.hx w/qemu-img-cmds.hx
> index e9beb15e614e..10b910b67cf8 100644
> --- i/qemu-img-cmds.hx
> +++ w/qemu-img-cmds.hx
> @@ -76,9 +76,9 @@ SRST
>   ERST
>
>   DEF("measure", img_measure,
> -"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N |
> [--object objectdef] [--image-opts] [-f fmt] [--bitmaps] [-l
> snapshot_param] filename]")
> +"measure [--output=ofmt] [-O output_fmt] [-o options] [--size N |
> [--object objectdef] [--image-opts] [-f fmt] [-l snapshot_param] filename]")
>   SRST
> -.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
> [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [--bitmaps] [-l
> SNAPSHOT_PARAM] FILENAME]
> +.. option:: measure [--output=OFMT] [-O OUTPUT_FMT] [-o OPTIONS]
> [--size N | [--object OBJECTDEF] [--image-opts] [-f FMT] [-l
> SNAPSHOT_PARAM] FILENAME]
>   ERST
>
>   DEF("snapshot", img_snapshot,
>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
>