qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v3 1/1] qemu-img: Add --target-is-zero to convert
Date: Tue, 4 Feb 2020 16:59:39 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

04.02.2020 12:52, David Edmondson wrote:
In many cases the target of a convert operation is a newly provisioned
target that the user knows is blank (filled with zeroes). In this
situation there is no requirement for qemu-img to wastefully zero out
the entire device.

Add a new option, --target-is-zero, allowing the user to indicate that
an existing target device is already zero filled.

Signed-off-by: David Edmondson <address@hidden>
---
  docs/interop/qemu-img.rst |  8 +++++++-
  qemu-img-cmds.hx          |  4 ++--
  qemu-img.c                | 25 ++++++++++++++++++++++---
  3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/docs/interop/qemu-img.rst b/docs/interop/qemu-img.rst
index fa27e5c7b453..99bdbe4740ee 100644
--- a/docs/interop/qemu-img.rst
+++ b/docs/interop/qemu-img.rst
@@ -214,6 +214,12 @@ Parameters to convert subcommand:
    will still be printed.  Areas that cannot be read from the source will be
    treated as containing only zeroes.
+.. option:: --target-is-zero
+
+  Assume that the destination is filled with zeros. This parameter is
+  mutually exclusive with the use of a backing file. It is required to

Should we mention, that s/backing file/backing file for destination/, to make 
it clean
that source backing file is unrelated?

+  also use the ``-n`` parameter to skip image creation.
+
  Parameters to dd subcommand:
.. program:: qemu-img-dd
@@ -366,7 +372,7 @@ Command description:
    4
      Error on reading data
-.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [-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] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
+.. 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] FILENAME [FILENAME2 [...]] 
OUTPUT_FILENAME
Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
    to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 3fd836ca9090..e6f98b75473f 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -39,9 +39,9 @@ SRST
  ERST
DEF("convert", img_convert,
-    "convert [--object objectdef] [--image-opts] [--target-image-opts] [-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] [-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] 
[-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] [-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,

Side question: is there plan to get rid of this duplication, and generate 
everything from rst?

diff --git a/qemu-img.c b/qemu-img.c
index 2b4562b9d9f2..e0bfc33ef4f6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -70,6 +70,7 @@ enum {
      OPTION_PREALLOCATION = 265,
      OPTION_SHRINK = 266,
      OPTION_SALVAGE = 267,
+    OPTION_TARGET_IS_ZERO = 268,
  };
typedef enum OutputFormat {
@@ -1984,10 +1985,9 @@ static int convert_do_copy(ImgConvertState *s)
      int64_t sector_num = 0;
/* Check whether we have zero initialisation or can get it efficiently */
-    if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
+    if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
+        !s->target_has_backing) {
          s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
-    } else {
-        s->has_zero_init = false;
      }
if (!s->has_zero_init && !s->target_has_backing &&
@@ -2086,6 +2086,7 @@ static int img_convert(int argc, char **argv)
              {"force-share", no_argument, 0, 'U'},
              {"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},
              {0, 0, 0, 0}
          };
          c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
@@ -2209,6 +2210,14 @@ static int img_convert(int argc, char **argv)
          case OPTION_TARGET_IMAGE_OPTS:
              tgt_image_opts = true;
              break;
+        case OPTION_TARGET_IS_ZERO:
+            /*
+             * The user asserting that the target is blank has the
+             * same effect as the target driver supporting zero
+             * initialisation.
+             */
+            s.has_zero_init = true;
+            break;
          }
      }
@@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
          warn_report("This will become an error in future QEMU versions.");
      }
+ if (s.has_zero_init && !skip_create) {
+        error_report("--target-is-zero requires use of -n flag");
+        goto fail_getopt;
+    }
+
      s.src_num = argc - optind - 1;
      out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
@@ -2380,6 +2394,11 @@ static int img_convert(int argc, char **argv)
      }
      s.target_has_backing = (bool) out_baseimg;
+ if (s.has_zero_init && s.target_has_backing) {
+        error_report("Cannot use --target-is-zero with a backing file");

And here, probably better: "with a backing file on destination"

+        goto out;
+    }
+
      if (s.src_num > 1 && out_baseimg) {
          error_report("Having a backing file for the target makes no sense when 
"
                       "concatenating multiple input images");


With or without my suggestions:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]