[Qemu-devel] [PATCH] qemu-img convert: Fix copy length for the iteration

[Qemu-devel] [PATCH] qemu-img convert: Fix copy length for the iteration

[Kevin Wolf]

qemu-img converts already checks the block status of the next sectors to
copy and skips them if it knows that they don't need to be copied (e.g.
because they are zero and the destination is zero-initialised).

It doesn't, however, limit the number of sectors read to the number of
allocated sectors when copying data yet. This means that when using -B,
some data is needlessly copied from the backing file into the overlay.
For images without backing files, it means that more data is read than
is necessary, even though the zero buffer detection will still keep
those sectors sparse.

This patch changes qemu-img to respect the number of allocated sectors
and limit its requests accordingly when processing data, which fixes -B
to expose the expected behaviour and is a performance optimisation for
other cases.

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c                 |  1 +
 tests/qemu-iotests/122     | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/122.out | 11 +++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 69 insertions(+)
 create mode 100755 tests/qemu-iotests/122
 create mode 100644 tests/qemu-iotests/122.out

diff --git a/qemu-img.c b/qemu-img.c
index e148af8..b94afe2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1809,6 +1809,7 @@ restart:
             }
 
             n = MIN(nb_sectors, bufsectors);
+            n = MIN(n, sector_num_next_status - sector_num);
 
             /* round down request length to an aligned sector, but
              * do not bother doing this on short requests. They happen
diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
new file mode 100755
index 0000000..0106e8a
--- /dev/null
+++ b/tests/qemu-iotests/122
@@ -0,0 +1,56 @@
+#!/bin/bash
+#
+# Test image sparseness after qemu-img convert
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+TEST_IMG=$TEST_IMG.base _make_test_img 64M
+_make_test_img -b $TEST_IMG.base
+$QEMU_IO -c "write -P 0x11 0 64M" $TEST_IMG.base 2>&1 | _filter_qemu_io | _filter_testdir
+$QEMU_IO -c "write -P 0x22 0 3M" $TEST_IMG 2>&1 | _filter_qemu_io | _filter_testdir
+$QEMU_IMG convert -O $IMGFMT -B $TEST_IMG.base $TEST_IMG $TEST_IMG.orig
+$QEMU_IMG map $TEST_IMG.orig | _filter_qemu_img_map
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
new file mode 100644
index 0000000..6e4a760
--- /dev/null
+++ b/tests/qemu-iotests/122.out
@@ -0,0 +1,11 @@
+QA output created by 122
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file='TEST_DIR/t.IMGFMT.base'
+wrote 67108864/67108864 bytes at offset 0
+64 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 3145728/3145728 bytes at offset 0
+3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset          Length          File
+0               0x300000        TEST_DIR/t.IMGFMT.orig
+0x300000        0x3d00000       TEST_DIR/t.IMGFMT.base
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 4b2b93b..28c78e5 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -117,3 +117,4 @@
 113 rw auto quick
 114 rw auto quick
 116 rw auto quick
+122 rw auto quick
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] qemu-img convert: Fix copy length for the iteration

[Max Reitz]

On 2015-02-11 at 06:08, Kevin Wolf wrote:
> qemu-img converts already checks the block status of the next sectors to
> copy and skips them if it knows that they don't need to be copied (e.g.
> because they are zero and the destination is zero-initialised).
>
> It doesn't, however, limit the number of sectors read to the number of
> allocated sectors when copying data yet. This means that when using -B,
> some data is needlessly copied from the backing file into the overlay.
> For images without backing files, it means that more data is read than
> is necessary, even though the zero buffer detection will still keep
> those sectors sparse.
>
> This patch changes qemu-img to respect the number of allocated sectors
> and limit its requests accordingly when processing data, which fixes -B
> to expose the expected behaviour and is a performance optimisation for
> other cases.
>
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   qemu-img.c                 |  1 +
>   tests/qemu-iotests/122     | 56 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/122.out | 11 +++++++++
>   tests/qemu-iotests/group   |  1 +
>   4 files changed, 69 insertions(+)
>   create mode 100755 tests/qemu-iotests/122
>   create mode 100644 tests/qemu-iotests/122.out
>
> diff --git a/qemu-img.c b/qemu-img.c
> index e148af8..b94afe2 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1809,6 +1809,7 @@ restart:
>               }
>   
>               n = MIN(nb_sectors, bufsectors);
> +            n = MIN(n, sector_num_next_status - sector_num);

This needs to be done only if sector_num_next_status > sector_num, 
otherwise we'll run into an infinite loop.

Apparently I accidentally broke -n for qemu-img convert, but once I get 
it working again, you can see that this command:

$ qemu-img convert -n -O raw test.qcow2 nbd://localhost:10809

does not terminate (because out_baseimg is NULL and has_zero_init is 
false due to nbd not supporting that, which means that 
sector_num_next_status is never set to anything but 0).

Other than that, this looks good to me.

Max

>               /* round down request length to an aligned sector, but
>                * do not bother doing this on short requests. They happen
> diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
> new file mode 100755
> index 0000000..0106e8a
> --- /dev/null
> +++ b/tests/qemu-iotests/122
> @@ -0,0 +1,56 @@
> +#!/bin/bash
> +#
> +# Test image sparseness after qemu-img convert
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=kwolf@redhat.com
> +
> +seq="$(basename $0)"
> +echo "QA output created by $seq"
> +
> +here="$PWD"
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +	_cleanup_test_img
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto generic
> +_supported_os Linux
> +
> +
> +TEST_IMG=$TEST_IMG.base _make_test_img 64M
> +_make_test_img -b $TEST_IMG.base
> +$QEMU_IO -c "write -P 0x11 0 64M" $TEST_IMG.base 2>&1 | _filter_qemu_io | _filter_testdir
> +$QEMU_IO -c "write -P 0x22 0 3M" $TEST_IMG 2>&1 | _filter_qemu_io | _filter_testdir
> +$QEMU_IMG convert -O $IMGFMT -B $TEST_IMG.base $TEST_IMG $TEST_IMG.orig
> +$QEMU_IMG map $TEST_IMG.orig | _filter_qemu_img_map
> +
> +# success, all done
> +echo '*** done'
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
> new file mode 100644
> index 0000000..6e4a760
> --- /dev/null
> +++ b/tests/qemu-iotests/122.out
> @@ -0,0 +1,11 @@
> +QA output created by 122
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file='TEST_DIR/t.IMGFMT.base'
> +wrote 67108864/67108864 bytes at offset 0
> +64 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 3145728/3145728 bytes at offset 0
> +3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Offset          Length          File
> +0               0x300000        TEST_DIR/t.IMGFMT.orig
> +0x300000        0x3d00000       TEST_DIR/t.IMGFMT.base
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 4b2b93b..28c78e5 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -117,3 +117,4 @@
>   113 rw auto quick
>   114 rw auto quick
>   116 rw auto quick
> +122 rw auto quick

Re: [Qemu-devel] [PATCH] qemu-img convert: Fix copy length for the iteration

[Kevin Wolf]

Am 11.02.2015 um 15:39 hat Max Reitz geschrieben:
> On 2015-02-11 at 06:08, Kevin Wolf wrote:
> >qemu-img converts already checks the block status of the next sectors to
> >copy and skips them if it knows that they don't need to be copied (e.g.
> >because they are zero and the destination is zero-initialised).
> >
> >It doesn't, however, limit the number of sectors read to the number of
> >allocated sectors when copying data yet. This means that when using -B,
> >some data is needlessly copied from the backing file into the overlay.
> >For images without backing files, it means that more data is read than
> >is necessary, even though the zero buffer detection will still keep
> >those sectors sparse.
> >
> >This patch changes qemu-img to respect the number of allocated sectors
> >and limit its requests accordingly when processing data, which fixes -B
> >to expose the expected behaviour and is a performance optimisation for
> >other cases.
> >
> >Reported-by: Max Reitz <mreitz@redhat.com>
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >---
> >  qemu-img.c                 |  1 +
> >  tests/qemu-iotests/122     | 56 ++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/122.out | 11 +++++++++
> >  tests/qemu-iotests/group   |  1 +
> >  4 files changed, 69 insertions(+)
> >  create mode 100755 tests/qemu-iotests/122
> >  create mode 100644 tests/qemu-iotests/122.out
> >
> >diff --git a/qemu-img.c b/qemu-img.c
> >index e148af8..b94afe2 100644
> >--- a/qemu-img.c
> >+++ b/qemu-img.c
> >@@ -1809,6 +1809,7 @@ restart:
> >              }
> >              n = MIN(nb_sectors, bufsectors);
> >+            n = MIN(n, sector_num_next_status - sector_num);
> 
> This needs to be done only if sector_num_next_status > sector_num,
> otherwise we'll run into an infinite loop.
> 
> Apparently I accidentally broke -n for qemu-img convert, but once I
> get it working again, you can see that this command:
> 
> $ qemu-img convert -n -O raw test.qcow2 nbd://localhost:10809
> 
> does not terminate (because out_baseimg is NULL and has_zero_init is
> false due to nbd not supporting that, which means that
> sector_num_next_status is never set to anything but 0).
> 
> Other than that, this looks good to me.

Except that it means that the whole function is crap.

We should unconditionally be doing the bdrv_co_get_block_status() and
save at least the reads if we can. This would also enable us to use
bdrv_co_write_zeroes() for the cases where an explicit zero write is
needed.

I guess it's time to rewrite the whole thing...

Kevin