[Qemu-devel] [PATCH v3 0/4] qemu-img: fix bugs when cluster size is larger than the default value

[Qemu-devel] [PATCH v3 0/4] qemu-img: fix bugs when cluster size is larger than the default value

[Hu Tao]

This series fixes several bugs of qcow2 when doing preallocation with a
cluster_size larger than the default value.

Hu Tao (4):
  qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()
  qcow2: fix offset overflow in qcow2_alloc_clusters_at()
  qcow2: check for NULL l2meta
  qemu-iotests: add test for qcow2 preallocation with different cluster
    sizes

 block/qcow2-cluster.c      | 14 ++++------
 block/qcow2-refcount.c     |  8 +++++-
 block/qcow2.c              | 31 +++++++++++----------
 block/qcow2.h              |  2 +-
 tests/qemu-iotests/079     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/079.out | 32 +++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 trace-events               |  2 +-
 8 files changed, 134 insertions(+), 25 deletions(-)
 create mode 100755 tests/qemu-iotests/079
 create mode 100644 tests/qemu-iotests/079.out

-- 
1.8.5.2.229.g4448466

[Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Hu Tao]

n_start can be actually calculated from offset. The number of
sectors to be allocated(n_end - n_start) can be passed in in
num. By removing n_start and n_end, we can save two parameters.

The side effect is there is a bug in qcow2.c:preallocate() that
passes incorrect n_start to qcow2_alloc_cluster_offset() is
fixed. The bug can be triggerred by a larger cluster size than
the default value(65536), for example:

./qemu-img create -f qcow2 \
  -o 'cluster_size=131072,preallocation=metadata' file.img 4G

Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2-cluster.c | 14 ++++++--------
 block/qcow2.c         |  6 +++---
 block/qcow2.h         |  2 +-
 trace-events          |  2 +-
 4 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 8534084..c57f39d 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1182,7 +1182,7 @@ fail:
  * Return 0 on success and -errno in error cases
  */
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
-    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
+    int *num, uint64_t *host_offset, QCowL2Meta **m)
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t start, remaining;
@@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
     uint64_t cur_bytes;
     int ret;
 
-    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
-                                      n_start, n_end);
+    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
 
-    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
-    offset = start_of_cluster(s, offset);
+    assert((offset & ~BDRV_SECTOR_MASK) == 0);
 
 again:
-    start = offset + (n_start << BDRV_SECTOR_BITS);
-    remaining = (n_end - n_start) << BDRV_SECTOR_BITS;
+    start = offset;
+    remaining = *num << BDRV_SECTOR_BITS;
     cluster_offset = 0;
     *host_offset = 0;
     cur_bytes = 0;
@@ -1284,7 +1282,7 @@ again:
         }
     }
 
-    *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS);
+    *num -= remaining >> BDRV_SECTOR_BITS;
     assert(*num > 0);
     assert(*host_offset != 0);
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 8ec9db1..a0596ec 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1016,14 +1016,14 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
 
         trace_qcow2_writev_start_part(qemu_coroutine_self());
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
-        n_end = index_in_cluster + remaining_sectors;
+        cur_nr_sectors = remaining_sectors;
         if (s->crypt_method &&
             n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
             n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
         }
 
         ret = qcow2_alloc_cluster_offset(bs, sector_num << 9,
-            index_in_cluster, n_end, &cur_nr_sectors, &cluster_offset, &l2meta);
+            &cur_nr_sectors, &cluster_offset, &l2meta);
         if (ret < 0) {
             goto fail;
         }
@@ -1400,7 +1400,7 @@ static int preallocate(BlockDriverState *bs)
 
     while (nb_sectors) {
         num = MIN(nb_sectors, INT_MAX >> 9);
-        ret = qcow2_alloc_cluster_offset(bs, offset, 0, num, &num,
+        ret = qcow2_alloc_cluster_offset(bs, offset, &num,
                                          &host_offset, &meta);
         if (ret < 0) {
             return ret;
diff --git a/block/qcow2.h b/block/qcow2.h
index 303eb26..84e1344 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -468,7 +468,7 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
 int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     int *num, uint64_t *cluster_offset);
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
-    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m);
+    int *num, uint64_t *host_offset, QCowL2Meta **m);
 uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
                                          uint64_t offset,
                                          int compressed_size);
diff --git a/trace-events b/trace-events
index 9f4456a..9b4e586 100644
--- a/trace-events
+++ b/trace-events
@@ -494,7 +494,7 @@ qcow2_writev_done_part(void *co, int cur_nr_sectors) "co %p cur_nr_sectors %d"
 qcow2_writev_data(void *co, uint64_t offset) "co %p offset %" PRIx64
 
 # block/qcow2-cluster.c
-qcow2_alloc_clusters_offset(void *co, uint64_t offset, int n_start, int n_end) "co %p offet %" PRIx64 " n_start %d n_end %d"
+qcow2_alloc_clusters_offset(void *co, uint64_t offset, int num) "co %p offet %" PRIx64 " num %d"
 qcow2_handle_copied(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64
 qcow2_handle_alloc(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64
 qcow2_do_alloc_clusters_offset(void *co, uint64_t guest_offset, uint64_t host_offset, int nb_clusters) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " nb_clusters %d"
-- 
1.8.5.2.229.g4448466

[Qemu-devel] [PATCH v3 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()

[Hu Tao]

When cluster size is big enough it can lead offset overflow
in qcow2_alloc_clusters_at(). This patch fixes it.

The allocation each time is stopped at L2 table boundary
(see handle_alloc()), so the possible maximum bytes could be

  2^(cluster_bits - 3 + cluster_bits)

so int is safe for cluster_bits<=17, unsafe otherwise.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2-refcount.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c974abe..8712d8b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -676,7 +676,13 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
     BDRVQcowState *s = bs->opaque;
     uint64_t cluster_index;
     uint64_t old_free_cluster_index;
-    int i, refcount, ret;
+    uint64_t i;
+    int refcount, ret;
+
+    assert(nb_clusters >= 0);
+    if (nb_clusters == 0) {
+        return 0;
+    }
 
     /* Check how many clusters there are free */
     cluster_index = offset >> s->cluster_bits;
-- 
1.8.5.2.229.g4448466

[Qemu-devel] [PATCH v3 3/4] qcow2: check for NULL l2meta

[Hu Tao]

In case of do preallocating metadata with a large cluster size,
qcow2_alloc_cluster_offset() can allocate nothing and returns
a NULL l2meta. This patch checks for it and link2 l2 with only
valid l2meta.

Replace 9 and 512 with BDRV_SECTOR_BITS, BDRV_SECTOR_SIZE
respectively while at the function.

Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index a0596ec..e4e06f9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1395,22 +1395,24 @@ static int preallocate(BlockDriverState *bs)
     int ret;
     QCowL2Meta *meta;
 
-    nb_sectors = bdrv_getlength(bs) >> 9;
+    nb_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
     offset = 0;
 
     while (nb_sectors) {
-        num = MIN(nb_sectors, INT_MAX >> 9);
+        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
         ret = qcow2_alloc_cluster_offset(bs, offset, &num,
                                          &host_offset, &meta);
         if (ret < 0) {
             return ret;
         }
 
-        ret = qcow2_alloc_cluster_link_l2(bs, meta);
-        if (ret < 0) {
-            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
-                                    QCOW2_DISCARD_NEVER);
-            return ret;
+        if (meta) {
+            ret = qcow2_alloc_cluster_link_l2(bs, meta);
+            if (ret < 0) {
+                qcow2_free_any_clusters(bs, meta->alloc_offset,
+                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
+                return ret;
+            }
         }
 
         /* There are no dependent requests, but we need to remove our request
@@ -1422,7 +1424,7 @@ static int preallocate(BlockDriverState *bs)
         /* TODO Preallocate data if requested */
 
         nb_sectors -= num;
-        offset += num << 9;
+        offset += num << BDRV_SECTOR_BITS;
     }
 
     /*
@@ -1431,9 +1433,10 @@ static int preallocate(BlockDriverState *bs)
      * EOF). Extend the image to the last allocated sector.
      */
     if (host_offset != 0) {
-        uint8_t buf[512];
-        memset(buf, 0, 512);
-        ret = bdrv_write(bs->file, (host_offset >> 9) + num - 1, buf, 1);
+        uint8_t buf[BDRV_SECTOR_SIZE];
+        memset(buf, 0, BDRV_SECTOR_SIZE);
+        ret = bdrv_write(bs->file, (host_offset >> BDRV_SECTOR_BITS) + num - 1,
+                         buf, 1);
         if (ret < 0) {
             return ret;
         }
-- 
1.8.5.2.229.g4448466

[Qemu-devel] [PATCH v3 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes

[Hu Tao]

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---

Note: the current largest test case number is 074, but Kevin suggested picking
079 as there are in-flight patches taken lower numbers.

 tests/qemu-iotests/079     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/079.out | 32 +++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 102 insertions(+)
 create mode 100755 tests/qemu-iotests/079
 create mode 100644 tests/qemu-iotests/079.out

diff --git a/tests/qemu-iotests/079 b/tests/qemu-iotests/079
new file mode 100755
index 0000000..5df045b
--- /dev/null
+++ b/tests/qemu-iotests/079
@@ -0,0 +1,69 @@
+#!/bin/bash
+#
+# Test qcow2 preallocation with different cluster_sizes
+#
+# Copyright (C) 2014 Fujitsu.
+#
+# 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=hutao@cn.fujitsu.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 file
+_supported_os Linux
+
+function filter_test_dir()
+{
+    sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
+        -e "s#$TEST_DIR#TEST_DIR#g"
+}
+
+function test_qemu_img()
+{
+    echo qemu-img "$@" | filter_test_dir
+    $QEMU_IMG "$@" 2>&1 | filter_test_dir
+    echo
+}
+
+echo "=== Check option preallocation and cluster_size ==="
+echo
+cluster_sizes="16384 32768 65536 131072 262144 524288 1048576 2097152 4194304"
+
+for s in $cluster_sizes; do
+    test_qemu_img create -f $IMGFMT -o preallocation=metadata,cluster_size=$s "$TEST_IMG" 4G
+done
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/079.out b/tests/qemu-iotests/079.out
new file mode 100644
index 0000000..ef4b8c9
--- /dev/null
+++ b/tests/qemu-iotests/079.out
@@ -0,0 +1,32 @@
+QA output created by 079
+=== Check option preallocation and cluster_size ===
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=16384 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=16384 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=32768 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=32768 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=65536 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=131072 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=131072 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=262144 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=262144 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=524288 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=524288 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=1048576 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=1048576 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=2097152 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=2097152 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=4194304 TEST_DIR/t.qcow2 4G
+qemu-img: TEST_DIR/t.qcow2: Cluster size must be a power of two between 512 and 2048k
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=4194304 preallocation='metadata' lazy_refcounts=off
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index cc750c9..3bb22c2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -79,3 +79,4 @@
 070 rw auto
 073 rw auto
 074 rw auto
+079 rw auto
-- 
1.8.5.2.229.g4448466

Re: [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Kevin Wolf]

Am 22.01.2014 um 07:57 hat Hu Tao geschrieben:
> n_start can be actually calculated from offset. The number of
> sectors to be allocated(n_end - n_start) can be passed in in
> num. By removing n_start and n_end, we can save two parameters.
> 
> The side effect is there is a bug in qcow2.c:preallocate() that
> passes incorrect n_start to qcow2_alloc_cluster_offset() is
> fixed. The bug can be triggerred by a larger cluster size than
> the default value(65536), for example:
> 
> ./qemu-img create -f qcow2 \
>   -o 'cluster_size=131072,preallocation=metadata' file.img 4G
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2-cluster.c | 14 ++++++--------
>  block/qcow2.c         |  6 +++---
>  block/qcow2.h         |  2 +-
>  trace-events          |  2 +-
>  4 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 8534084..c57f39d 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1182,7 +1182,7 @@ fail:
>   * Return 0 on success and -errno in error cases
>   */
>  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
> +    int *num, uint64_t *host_offset, QCowL2Meta **m)
>  {
>      BDRVQcowState *s = bs->opaque;
>      uint64_t start, remaining;
> @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
>      uint64_t cur_bytes;
>      int ret;
>  
> -    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
> -                                      n_start, n_end);
> +    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
>  
> -    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
> -    offset = start_of_cluster(s, offset);
> +    assert((offset & ~BDRV_SECTOR_MASK) == 0);
>  
>  again:
> -    start = offset + (n_start << BDRV_SECTOR_BITS);
> -    remaining = (n_end - n_start) << BDRV_SECTOR_BITS;
> +    start = offset;
> +    remaining = *num << BDRV_SECTOR_BITS;
>      cluster_offset = 0;
>      *host_offset = 0;
>      cur_bytes = 0;
> @@ -1284,7 +1282,7 @@ again:
>          }
>      }
>  
> -    *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS);
> +    *num -= remaining >> BDRV_SECTOR_BITS;
>      assert(*num > 0);
>      assert(*host_offset != 0);
>  
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8ec9db1..a0596ec 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1016,14 +1016,14 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>  
>          trace_qcow2_writev_start_part(qemu_coroutine_self());
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -        n_end = index_in_cluster + remaining_sectors;
> +        cur_nr_sectors = remaining_sectors;
>          if (s->crypt_method &&
>              n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
>              n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
>          }

You don't want to change n_end here any more, this should affect
cur_nr_sectors now. n_end becomes completely unused then and can be
removed.

I wonder why the compiler doesn't complain here, this is uninitialised
use and a write-only variable at the same time.

Kevin

Re: [Qemu-devel] [PATCH v3 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes

[Kevin Wolf]

Am 22.01.2014 um 07:57 hat Hu Tao geschrieben:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> 
> Note: the current largest test case number is 074, but Kevin suggested picking
> 079 as there are in-flight patches taken lower numbers.
> 
>  tests/qemu-iotests/079     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/079.out | 32 +++++++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 102 insertions(+)
>  create mode 100755 tests/qemu-iotests/079
>  create mode 100644 tests/qemu-iotests/079.out
> 
> diff --git a/tests/qemu-iotests/079 b/tests/qemu-iotests/079
> new file mode 100755
> index 0000000..5df045b
> --- /dev/null
> +++ b/tests/qemu-iotests/079
> @@ -0,0 +1,69 @@
> +#!/bin/bash
> +#
> +# Test qcow2 preallocation with different cluster_sizes
> +#
> +# Copyright (C) 2014 Fujitsu.
> +#
> +# 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=hutao@cn.fujitsu.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 file
> +_supported_os Linux
> +
> +function filter_test_dir()
> +{
> +    sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
> +        -e "s#$TEST_DIR#TEST_DIR#g"
> +}

Can't you use _filter_testdir() from common.filter?

If you need the additional $IMGPROTO: filter that is missing there, I
think we can add it to the common.filter function.

Kevin

Re: [Qemu-devel] [PATCH v3 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()

[Max Reitz]

On 22.01.2014 07:57, Hu Tao wrote:
> When cluster size is big enough it can lead offset overflow
> in qcow2_alloc_clusters_at(). This patch fixes it.
>
> The allocation each time is stopped at L2 table boundary
> (see handle_alloc()), so the possible maximum bytes could be
>
>    2^(cluster_bits - 3 + cluster_bits)
>
> so int is safe for cluster_bits<=17, unsafe otherwise.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/qcow2-refcount.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v3 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes

[Hu Tao]

On Wed, Jan 22, 2014 at 11:02:09AM +0100, Kevin Wolf wrote:
> Am 22.01.2014 um 07:57 hat Hu Tao geschrieben:
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> > 
> > Note: the current largest test case number is 074, but Kevin suggested picking
> > 079 as there are in-flight patches taken lower numbers.
> > 
> >  tests/qemu-iotests/079     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/079.out | 32 +++++++++++++++++++++
> >  tests/qemu-iotests/group   |  1 +
> >  3 files changed, 102 insertions(+)
> >  create mode 100755 tests/qemu-iotests/079
> >  create mode 100644 tests/qemu-iotests/079.out
> > 
> > diff --git a/tests/qemu-iotests/079 b/tests/qemu-iotests/079
> > new file mode 100755
> > index 0000000..5df045b
> > --- /dev/null
> > +++ b/tests/qemu-iotests/079
> > @@ -0,0 +1,69 @@
> > +#!/bin/bash
> > +#
> > +# Test qcow2 preallocation with different cluster_sizes
> > +#
> > +# Copyright (C) 2014 Fujitsu.
> > +#
> > +# 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=hutao@cn.fujitsu.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 file
> > +_supported_os Linux
> > +
> > +function filter_test_dir()
> > +{
> > +    sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
> > +        -e "s#$TEST_DIR#TEST_DIR#g"
> > +}
> 
> Can't you use _filter_testdir() from common.filter?

Yes, it works. Thanks!

Re: [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Hu Tao]

On Wed, Jan 22, 2014 at 10:56:58AM +0100, Kevin Wolf wrote:
> Am 22.01.2014 um 07:57 hat Hu Tao geschrieben:
> > n_start can be actually calculated from offset. The number of
> > sectors to be allocated(n_end - n_start) can be passed in in
> > num. By removing n_start and n_end, we can save two parameters.
> > 
> > The side effect is there is a bug in qcow2.c:preallocate() that
> > passes incorrect n_start to qcow2_alloc_cluster_offset() is
> > fixed. The bug can be triggerred by a larger cluster size than
> > the default value(65536), for example:
> > 
> > ./qemu-img create -f qcow2 \
> >   -o 'cluster_size=131072,preallocation=metadata' file.img 4G
> > 
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  block/qcow2-cluster.c | 14 ++++++--------
> >  block/qcow2.c         |  6 +++---
> >  block/qcow2.h         |  2 +-
> >  trace-events          |  2 +-
> >  4 files changed, 11 insertions(+), 13 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index 8534084..c57f39d 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -1182,7 +1182,7 @@ fail:
> >   * Return 0 on success and -errno in error cases
> >   */
> >  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> > -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
> > +    int *num, uint64_t *host_offset, QCowL2Meta **m)
> >  {
> >      BDRVQcowState *s = bs->opaque;
> >      uint64_t start, remaining;
> > @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> >      uint64_t cur_bytes;
> >      int ret;
> >  
> > -    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
> > -                                      n_start, n_end);
> > +    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
> >  
> > -    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
> > -    offset = start_of_cluster(s, offset);
> > +    assert((offset & ~BDRV_SECTOR_MASK) == 0);
> >  
> >  again:
> > -    start = offset + (n_start << BDRV_SECTOR_BITS);
> > -    remaining = (n_end - n_start) << BDRV_SECTOR_BITS;
> > +    start = offset;
> > +    remaining = *num << BDRV_SECTOR_BITS;
> >      cluster_offset = 0;
> >      *host_offset = 0;
> >      cur_bytes = 0;
> > @@ -1284,7 +1282,7 @@ again:
> >          }
> >      }
> >  
> > -    *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS);
> > +    *num -= remaining >> BDRV_SECTOR_BITS;
> >      assert(*num > 0);
> >      assert(*host_offset != 0);
> >  
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 8ec9db1..a0596ec 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1016,14 +1016,14 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
> >  
> >          trace_qcow2_writev_start_part(qemu_coroutine_self());
> >          index_in_cluster = sector_num & (s->cluster_sectors - 1);
> > -        n_end = index_in_cluster + remaining_sectors;
> > +        cur_nr_sectors = remaining_sectors;
> >          if (s->crypt_method &&
> >              n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
> >              n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
> >          }
> 
> You don't want to change n_end here any more, this should affect
> cur_nr_sectors now. n_end becomes completely unused then and can be
> removed.

Thanks for catching this!

> 
> I wonder why the compiler doesn't complain here, this is uninitialised
> use and a write-only variable at the same time.

n_end is used in the if(). If I remove n_end >... line, gcc complains.

Re: [Qemu-devel] [PATCH v3 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()

[Hu Tao]

On Wed, Jan 22, 2014 at 08:16:44PM +0100, Max Reitz wrote:
> On 22.01.2014 07:57, Hu Tao wrote:
> >When cluster size is big enough it can lead offset overflow
> >in qcow2_alloc_clusters_at(). This patch fixes it.
> >
> >The allocation each time is stopped at L2 table boundary
> >(see handle_alloc()), so the possible maximum bytes could be
> >
> >   2^(cluster_bits - 3 + cluster_bits)
> >
> >so int is safe for cluster_bits<=17, unsafe otherwise.
> >
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >---
> >  block/qcow2-refcount.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks for reviewing!