[Qemu-devel] [PATCH RFC for-3.0-rc3 0/3] qemu-img: Disable copy offloading by default

[Qemu-devel] [PATCH RFC for-3.0-rc3 0/3] qemu-img: Disable copy offloading by default

[Fam Zheng]

Kevin pointed out that both glibc and kernel provides a slow fallback of
copy_file_range which hurts thin provisioning. This is particularly true for
thin LVs, because host_device driver cannot get allocation info from the
volume, and copy_file_range is called on every sectors, making the dst fully
allocated.

NFS mount points also doesn't support SEEK_DATA well, so the allocation
information is unknown to QEMU.

That leaves only iscsi:// which seems to do what we want so far, but it is a
smaller use case.

Add an option to qemu-img convert, "-C", to enable (attempting) copy offloading
explicitly. And mark it incompatible with "-S" and "-c".

Fam Zheng (3):
  Revert "qemu-img: Document copy offloading implications with -S and
    -c"
  qemu-img: Add -C option for convert with copy offloading
  iotests: Add test for 'qemu-img convert -C' compatibility

 qemu-img-cmds.hx           |  2 +-
 qemu-img.c                 | 21 +++++++++++++++++----
 qemu-img.texi              | 14 +++++++++-----
 tests/qemu-iotests/082     |  8 ++++++++
 tests/qemu-iotests/082.out | 11 +++++++++++
 5 files changed, 46 insertions(+), 10 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH RFC for-3.0-rc3 1/3] Revert "qemu-img: Document copy offloading implications with -S and -c"

[Fam Zheng]

This reverts commit eb461485f4558e362fab905735b50987505bca44.

Now that we introduce an explicit option, these implicit rules are not
used.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img.texi | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qemu-img.texi b/qemu-img.texi
index 5853cd18d1..aeb1b9e66c 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -96,8 +96,7 @@ will enumerate information about backing files in a disk image chain. Refer
 below for further description.
 
 @item -c
-indicates that target image must be compressed (qcow format only). If this
-option is used, copy offloading will not be attempted.
+indicates that target image must be compressed (qcow format only)
 
 @item -h
 with or without a command shows help and lists the supported formats
@@ -116,8 +115,7 @@ in case both @var{-q} and @var{-p} options are used.
 indicates the consecutive number of bytes that must contain only zeros
 for qemu-img to create a sparse image during conversion. This value is rounded
 down to the nearest 512 bytes. You may use the common size suffixes like
-@code{k} for kilobytes. If this option is used, copy offloading will not be
-attempted.
+@code{k} for kilobytes.
 
 @item -t @var{cache}
 specifies the cache mode that should be used with the (destination) file. See
-- 
2.17.1

[Qemu-devel] [PATCH RFC for-3.0-rc3 2/3] qemu-img: Add -C option for convert with copy offloading

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img-cmds.hx |  2 +-
 qemu-img.c       | 21 +++++++++++++++++----
 qemu-img.texi    |  8 +++++++-
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 69758fb6e8..1526f327a5 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -44,7 +44,7 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] [-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 [--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")
 STEXI
 @item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] [-U] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index 9b7506b8ae..1acddf693c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2024,11 +2024,12 @@ static int img_convert(int argc, char **argv)
          skip_create = false, progress = false, tgt_image_opts = false;
     int64_t ret = -EINVAL;
     bool force_share = false;
+    bool explict_min_sparse = false;
 
     ImgConvertState s = (ImgConvertState) {
         /* Need at least 4k of zeros for sparse detection */
         .min_sparse         = 8,
-        .copy_range         = true,
+        .copy_range         = false,
         .buf_sectors        = IO_BUF_SIZE / BDRV_SECTOR_SIZE,
         .wr_in_order        = true,
         .num_coroutines     = 8,
@@ -2043,7 +2044,7 @@ static int img_convert(int argc, char **argv)
             {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, ":hf:O:B:co:l:S:pt:T:qnm:WU",
+        c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2067,9 +2068,11 @@ static int img_convert(int argc, char **argv)
         case 'B':
             out_baseimg = optarg;
             break;
+        case 'C':
+            s.copy_range = true;
+            break;
         case 'c':
             s.compressed = true;
-            s.copy_range = false;
             break;
         case 'o':
             if (!is_valid_option_list(optarg)) {
@@ -2112,7 +2115,7 @@ static int img_convert(int argc, char **argv)
             }
 
             s.min_sparse = sval / BDRV_SECTOR_SIZE;
-            s.copy_range = false;
+            explict_min_sparse = true;
             break;
         }
         case 'p':
@@ -2172,6 +2175,16 @@ static int img_convert(int argc, char **argv)
         goto fail_getopt;
     }
 
+    if (s.compressed && s.copy_range) {
+        error_report("Cannot enable copy offloading when -c is used");
+        goto fail_getopt;
+    }
+
+    if (explict_min_sparse && s.copy_range) {
+        error_report("Cannot enable copy offloading when -S is used");
+        goto fail_getopt;
+    }
+
     if (tgt_image_opts && !skip_create) {
         error_report("--target-image-opts requires use of -n flag");
         goto fail_getopt;
diff --git a/qemu-img.texi b/qemu-img.texi
index aeb1b9e66c..3b6710a580 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -169,6 +169,12 @@ Number of parallel coroutines for the convert process
 Allow out-of-order writes to the destination. This option improves performance,
 but is only recommended for preallocated devices like host devices or other
 raw block devices.
+@item -C
+Try to use copy offloading to move data from source image to target. This may
+improve performance if the data is remote, such as with NFS or iSCSI backends,
+but will not automatically sparsify zero sectors, and may result in a fully
+allocated target image depending on the host support for getting allocation
+information.
 @end table
 
 Parameters to dd subcommand:
@@ -319,7 +325,7 @@ Error on reading data
 
 @end table
 
-@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] [-U] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] [-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} [@var{filename2} [...]] @var{output_filename}
 
 Convert the disk image @var{filename} or a snapshot @var{snapshot_param}
 to disk image @var{output_filename} using format @var{output_fmt}. It can be optionally compressed (@code{-c}
-- 
2.17.1

[Qemu-devel] [PATCH RFC for-3.0-rc3 3/3] iotests: Add test for 'qemu-img convert -C' compatibility

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/082     |  8 ++++++++
 tests/qemu-iotests/082.out | 11 +++++++++++
 2 files changed, 19 insertions(+)

diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
index a872f771a6..3e605d52d1 100755
--- a/tests/qemu-iotests/082
+++ b/tests/qemu-iotests/082
@@ -157,6 +157,14 @@ run_qemu_img convert -o help
 # Try help option for a format that does not support creation
 run_qemu_img convert -O bochs -o help
 
+echo
+echo === convert: -C and other options ===
+
+# Adding the help option to a command without other -o options
+run_qemu_img convert -C -S 4k -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
+run_qemu_img convert -C -S 8k -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
+run_qemu_img convert -C -c -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
+
 echo
 echo === amend: Options specified more than once ===
 
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 60ef87c276..19e9fb13ff 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -508,6 +508,17 @@ size             Virtual disk size
 Testing: convert -O bochs -o help
 qemu-img: Format driver 'bochs' does not support image creation
 
+=== convert: -C and other options ===
+
+Testing: convert -C -S 4k -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.target
+qemu-img: Cannot enable copy offloading when -S is used
+
+Testing: convert -C -S 8k -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.target
+qemu-img: Cannot enable copy offloading when -S is used
+
+Testing: convert -C -c -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.target
+qemu-img: Cannot enable copy offloading when -c is used
+
 === amend: Options specified more than once ===
 
 Testing: amend -f foo -f qcow2 -o lazy_refcounts=on TEST_DIR/t.qcow2
-- 
2.17.1

Re: [Qemu-devel] [PATCH RFC for-3.0-rc3 0/3] qemu-img: Disable copy offloading by default

[Kevin Wolf]

Am 27.07.2018 um 05:33 hat Fam Zheng geschrieben:
> Kevin pointed out that both glibc and kernel provides a slow fallback of
> copy_file_range which hurts thin provisioning. This is particularly true for
> thin LVs, because host_device driver cannot get allocation info from the
> volume, and copy_file_range is called on every sectors, making the dst fully
> allocated.
> 
> NFS mount points also doesn't support SEEK_DATA well, so the allocation
> information is unknown to QEMU.
> 
> That leaves only iscsi:// which seems to do what we want so far, but it is a
> smaller use case.
> 
> Add an option to qemu-img convert, "-C", to enable (attempting) copy offloading
> explicitly. And mark it incompatible with "-S" and "-c".

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Not sure why you made this an RFC only, but I think we absolutely need
this. People are used to using 'qemu-img convert' to compact images and
this would regress with automatic copy offloading.

Do you think we need more discussion?

Kevin

Re: [Qemu-devel] [PATCH RFC for-3.0-rc3 0/3] qemu-img: Disable copy offloading by default

[Fam Zheng]

On Fri, Jul 27, 2018 at 6:29 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 27.07.2018 um 05:33 hat Fam Zheng geschrieben:
> > Kevin pointed out that both glibc and kernel provides a slow fallback of
> > copy_file_range which hurts thin provisioning. This is particularly true for
> > thin LVs, because host_device driver cannot get allocation info from the
> > volume, and copy_file_range is called on every sectors, making the dst fully
> > allocated.
> >
> > NFS mount points also doesn't support SEEK_DATA well, so the allocation
> > information is unknown to QEMU.
> >
> > That leaves only iscsi:// which seems to do what we want so far, but it is a
> > smaller use case.
> >
> > Add an option to qemu-img convert, "-C", to enable (attempting) copy offloading
> > explicitly. And mark it incompatible with "-S" and "-c".
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
> Not sure why you made this an RFC only, but I think we absolutely need
> this. People are used to using 'qemu-img convert' to compact images and
> this would regress with automatic copy offloading.
>
> Do you think we need more discussion?

I think merging this for 3.0 is the right thing do to.

What worries me is the general usability of the feature. We could
probably explore ideas about how we can better take advantage of copy
offloading. I don't think counting on the user to make the right
decision between disk efficiency (thin provisioning) and BW efficiency
(copy offloading) will ever work. Even if we don't care about breaking
the default '-S 4k' behavior, the lack of SEEK_DATA/SEEK_HOLE support
on host NFS and block devices will make it very hard to use. Making it
worse, if the network to NFS server is good enough, convert with
pread64/pwrite64 with host page cache is also more efficient than
copy_file_range, so we'll be slower by trying to play clever. :(

Any thought?

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH RFC for-3.0-rc3 0/3] qemu-img: Disable copy offloading by default

[Nir Soffer]

On Fri, Jul 27, 2018 at 3:15 PM Fam Zheng <famz@redhat.com> wrote:

> On Fri, Jul 27, 2018 at 6:29 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 27.07.2018 um 05:33 hat Fam Zheng geschrieben:
> > > Kevin pointed out that both glibc and kernel provides a slow fallback
> of
> > > copy_file_range which hurts thin provisioning. This is particularly
> true for
> > > thin LVs, because host_device driver cannot get allocation info from
> the
> > > volume, and copy_file_range is called on every sectors, making the dst
> fully
> > > allocated.
> > >
> > > NFS mount points also doesn't support SEEK_DATA well, so the allocation
> > > information is unknown to QEMU.
>

NFS >= 4.2 supports SEEK_DATA/HOLE.


> > >
> > > That leaves only iscsi:// which seems to do what we want so far, but
> it is a
> > > smaller use case.
> > >
> > > Add an option to qemu-img convert, "-C", to enable (attempting) copy
> offloading
> > > explicitly. And mark it incompatible with "-S" and "-c".
> >
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> >
> > Not sure why you made this an RFC only, but I think we absolutely need
> > this. People are used to using 'qemu-img convert' to compact images and
> > this would regress with automatic copy offloading.
> >
> > Do you think we need more discussion?
>
> I think merging this for 3.0 is the right thing do to.
>
> What worries me is the general usability of the feature. We could
> probably explore ideas about how we can better take advantage of copy
> offloading. I don't think counting on the user to make the right
> decision between disk efficiency (thin provisioning) and BW efficiency

(copy offloading) will ever work. Even if we don't care about breaking
> the default '-S 4k' behavior, the lack of SEEK_DATA/SEEK_HOLE support
> on host NFS and block devices will make it very hard to use. Making it
> worse, if the network to NFS server is good enough, convert with
> pread64/pwrite64 with host page cache

is also more efficient than
> copy_file_range, so we'll be slower by trying to play clever. :(
>

In oVirt we always disable caching (-t none -T none), so using
host page cache is not an option.

I think adding an option for copy offloading is the right thing. This way
we can introduce the feature early without breaking the rest of the stuck.

For long term it would be nicer if qemu could select the best way to do
the copy automatically, keeping the allocation policy specified by the user.
oVirt still have bugs related to converting preallocated images to sparse
and sparse images to preallocated. Users like to have control on this, so
qemu-img cannot change the policy.

Do we have some documentation on the useful cases for copy
offloading? Did we benchmark this with different kind of storage?

The interesting use case for oVirt are:
- block storage: copying rregular LV on shared storage to
  another LV on same VG, but may be on a different PV.
- file storage: copying files on same NFS or GlusterFS mont.
- copying between different servers or file types, I guess
  copy_file_range will not help in this case, right?

Nir

Re: [Qemu-devel] [PATCH RFC for-3.0-rc3 0/3] qemu-img: Disable copy offloading by default

[Kevin Wolf]

Am 27.07.2018 um 14:14 hat Fam Zheng geschrieben:
> On Fri, Jul 27, 2018 at 6:29 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 27.07.2018 um 05:33 hat Fam Zheng geschrieben:
> > > Kevin pointed out that both glibc and kernel provides a slow fallback of
> > > copy_file_range which hurts thin provisioning. This is particularly true for
> > > thin LVs, because host_device driver cannot get allocation info from the
> > > volume, and copy_file_range is called on every sectors, making the dst fully
> > > allocated.
> > >
> > > NFS mount points also doesn't support SEEK_DATA well, so the allocation
> > > information is unknown to QEMU.
> > >
> > > That leaves only iscsi:// which seems to do what we want so far, but it is a
> > > smaller use case.
> > >
> > > Add an option to qemu-img convert, "-C", to enable (attempting) copy offloading
> > > explicitly. And mark it incompatible with "-S" and "-c".
> >
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> >
> > Not sure why you made this an RFC only, but I think we absolutely need
> > this. People are used to using 'qemu-img convert' to compact images and
> > this would regress with automatic copy offloading.
> >
> > Do you think we need more discussion?
> 
> I think merging this for 3.0 is the right thing do to.

Thanks, applied to the block branch.

> What worries me is the general usability of the feature. We could
> probably explore ideas about how we can better take advantage of copy
> offloading. I don't think counting on the user to make the right
> decision between disk efficiency (thin provisioning) and BW efficiency
> (copy offloading) will ever work.

Ultimately it is a decision that QEMU can't make, though. The user needs
to tell us at least whether they are trying to compact the image or
whether they just want to get it copied as fast as possible.

Even if we know this, of course, we may still be lacking information
about the storage to make the best decision.

> Even if we don't care about breaking the default '-S 4k' behavior, the
> lack of SEEK_DATA/SEEK_HOLE support on host NFS and block devices will
> make it very hard to use.

I hope that NFS 4.2 will gain some ground in the future. Without it,
thin provisioning with raw images on NFS is basically impossible.

I'm not aware of problems with block devices?

> Making it worse, if the network to NFS server is good enough, convert
> with pread64/pwrite64 with host page cache is also more efficient than
> copy_file_range, so we'll be slower by trying to play clever. :(

Really? I could understand not being much faster, but being actually
slower sounds wrong. Are we slower because of overhead in
copy_file_range() itself or something during preparation?

Kevin