* [Qemu-devel] [PATCH v3 0/4] qemu-img: fix bugs when cluster size is larger than the default value
@ 2014-01-22 6:57 Hu Tao
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset() Hu Tao
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Hu Tao @ 2014-01-22 6:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Max Reitz
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()
2014-01-22 6:57 [Qemu-devel] [PATCH v3 0/4] qemu-img: fix bugs when cluster size is larger than the default value Hu Tao
@ 2014-01-22 6:57 ` Hu Tao
2014-01-22 9:56 ` Kevin Wolf
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at() Hu Tao
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Hu Tao @ 2014-01-22 6:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Max Reitz
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v3 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()
2014-01-22 6:57 [Qemu-devel] [PATCH v3 0/4] qemu-img: fix bugs when cluster size is larger than the default value Hu Tao
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset() Hu Tao
@ 2014-01-22 6:57 ` Hu Tao
2014-01-22 19:16 ` Max Reitz
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 3/4] qcow2: check for NULL l2meta Hu Tao
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes Hu Tao
3 siblings, 1 reply; 11+ messages in thread
From: Hu Tao @ 2014-01-22 6:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Max Reitz
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v3 3/4] qcow2: check for NULL l2meta
2014-01-22 6:57 [Qemu-devel] [PATCH v3 0/4] qemu-img: fix bugs when cluster size is larger than the default value Hu Tao
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset() Hu Tao
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at() Hu Tao
@ 2014-01-22 6:57 ` Hu Tao
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes Hu Tao
3 siblings, 0 replies; 11+ messages in thread
From: Hu Tao @ 2014-01-22 6:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Max Reitz
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v3 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes
2014-01-22 6:57 [Qemu-devel] [PATCH v3 0/4] qemu-img: fix bugs when cluster size is larger than the default value Hu Tao
` (2 preceding siblings ...)
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 3/4] qcow2: check for NULL l2meta Hu Tao
@ 2014-01-22 6:57 ` Hu Tao
2014-01-22 10:02 ` Kevin Wolf
3 siblings, 1 reply; 11+ messages in thread
From: Hu Tao @ 2014-01-22 6:57 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Max Reitz
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset() Hu Tao
@ 2014-01-22 9:56 ` Kevin Wolf
2014-01-23 2:52 ` Hu Tao
0 siblings, 1 reply; 11+ messages in thread
From: Kevin Wolf @ 2014-01-22 9:56 UTC (permalink / raw)
To: Hu Tao; +Cc: qemu-devel, Max Reitz
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes Hu Tao
@ 2014-01-22 10:02 ` Kevin Wolf
2014-01-23 2:48 ` Hu Tao
0 siblings, 1 reply; 11+ messages in thread
From: Kevin Wolf @ 2014-01-22 10:02 UTC (permalink / raw)
To: Hu Tao; +Cc: qemu-devel, Max Reitz
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at() Hu Tao
@ 2014-01-22 19:16 ` Max Reitz
2014-01-23 2:53 ` Hu Tao
0 siblings, 1 reply; 11+ messages in thread
From: Max Reitz @ 2014-01-22 19:16 UTC (permalink / raw)
To: Hu Tao, qemu-devel; +Cc: Kevin Wolf
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes
2014-01-22 10:02 ` Kevin Wolf
@ 2014-01-23 2:48 ` Hu Tao
0 siblings, 0 replies; 11+ messages in thread
From: Hu Tao @ 2014-01-23 2:48 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Max Reitz
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!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()
2014-01-22 9:56 ` Kevin Wolf
@ 2014-01-23 2:52 ` Hu Tao
0 siblings, 0 replies; 11+ messages in thread
From: Hu Tao @ 2014-01-23 2:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Max Reitz
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()
2014-01-22 19:16 ` Max Reitz
@ 2014-01-23 2:53 ` Hu Tao
0 siblings, 0 replies; 11+ messages in thread
From: Hu Tao @ 2014-01-23 2:53 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel
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!
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-01-23 3:07 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-22 6:57 [Qemu-devel] [PATCH v3 0/4] qemu-img: fix bugs when cluster size is larger than the default value Hu Tao
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset() Hu Tao
2014-01-22 9:56 ` Kevin Wolf
2014-01-23 2:52 ` Hu Tao
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at() Hu Tao
2014-01-22 19:16 ` Max Reitz
2014-01-23 2:53 ` Hu Tao
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 3/4] qcow2: check for NULL l2meta Hu Tao
2014-01-22 6:57 ` [Qemu-devel] [PATCH v3 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes Hu Tao
2014-01-22 10:02 ` Kevin Wolf
2014-01-23 2:48 ` Hu Tao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.