* [PATCH v6 01/12] iotests: add qcow2-discard-during-rewrite
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 02/12] qcow2: fix cache discarding in update_refcount() Vladimir Sementsov-Ogievskiy
` (12 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
Simple test:
- start writing to allocated cluster A
- discard this cluster
- write to another unallocated cluster B (it's allocated in same place
where A was allocated)
- continue writing to A
For now last action pollutes cluster B which is a bug fixed by the
following commit.
For now, add test to "disabled" group, so that it doesn't run
automatically.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
.../tests/qcow2-discard-during-rewrite | 72 +++++++++++++++++++
.../tests/qcow2-discard-during-rewrite.out | 21 ++++++
2 files changed, 93 insertions(+)
create mode 100755 tests/qemu-iotests/tests/qcow2-discard-during-rewrite
create mode 100644 tests/qemu-iotests/tests/qcow2-discard-during-rewrite.out
diff --git a/tests/qemu-iotests/tests/qcow2-discard-during-rewrite b/tests/qemu-iotests/tests/qcow2-discard-during-rewrite
new file mode 100755
index 0000000000..7f0d8a107a
--- /dev/null
+++ b/tests/qemu-iotests/tests/qcow2-discard-during-rewrite
@@ -0,0 +1,72 @@
+#!/usr/bin/env bash
+# group: quick disabled
+#
+# Test discarding (and reusing) host cluster during writing data to it.
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# 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=vsementsov@virtuozzo.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+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 fuse
+_supported_os Linux
+
+size=1M
+_make_test_img $size
+
+(
+cat <<EOF
+write -P 1 0 64K
+
+break pwritev A
+aio_write -P 2 0 64K
+wait_break A
+
+discard 0 64K
+write -P 3 128K 64K
+read -P 3 128K 64K
+
+break pwritev_done B
+resume A
+wait_break B
+resume B
+
+read -P 0 0 64K
+read -P 3 128K 64K
+EOF
+) | $QEMU_IO blkdebug::$TEST_IMG | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/tests/qcow2-discard-during-rewrite.out b/tests/qemu-iotests/tests/qcow2-discard-during-rewrite.out
new file mode 100644
index 0000000000..8e75b2fbff
--- /dev/null
+++ b/tests/qemu-iotests/tests/qcow2-discard-during-rewrite.out
@@ -0,0 +1,21 @@
+QA output created by qcow2-discard-during-rewrite
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+blkdebug: Suspended request 'A'
+discard 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 131072
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 131072
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+blkdebug: Resuming request 'A'
+blkdebug: Suspended request 'B'
+blkdebug: Resuming request 'B'
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 131072
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 02/12] qcow2: fix cache discarding in update_refcount()
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 01/12] iotests: add qcow2-discard-during-rewrite Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 03/12] block/qcow2-cluster: assert no data_file on compressed write path Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den, Alberto Garcia
Here refcount of cluster at @cluster_offset reached 0, so we "free"
that cluster. Not a cluster at @offset. The thing that save us from the
bug is that L2 tables and refblocks are discarded one by one. Still,
let's be precise.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2-refcount.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 8e649b008e..543fcf289c 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -887,14 +887,15 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
void *table;
table = qcow2_cache_is_table_offset(s->refcount_block_cache,
- offset);
+ cluster_offset);
if (table != NULL) {
qcow2_cache_put(s->refcount_block_cache, &refcount_block);
old_table_index = -1;
qcow2_cache_discard(s->refcount_block_cache, table);
}
- table = qcow2_cache_is_table_offset(s->l2_table_cache, offset);
+ table = qcow2_cache_is_table_offset(s->l2_table_cache,
+ cluster_offset);
if (table != NULL) {
qcow2_cache_discard(s->l2_table_cache, table);
}
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 03/12] block/qcow2-cluster: assert no data_file on compressed write path
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 01/12] iotests: add qcow2-discard-during-rewrite Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 02/12] qcow2: fix cache discarding in update_refcount() Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 04/12] block/qcow2-refcount: rename and publish update_refcount_discard() Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
First, we return 0 here, but it's not a success, we didn't set
host_offset out parameter.
Next, it can't happen, as has_data_file() is checked in
qcow2_co_pwritev_compressed_part(). So, let's just assert it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-cluster.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index bd0597842f..6105d4e7e0 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -821,9 +821,7 @@ int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
int64_t cluster_offset;
int nb_csectors;
- if (has_data_file(bs)) {
- return 0;
- }
+ assert(!has_data_file(bs));
ret = get_cluster_table(bs, offset, &l2_slice, &l2_index);
if (ret < 0) {
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 04/12] block/qcow2-refcount: rename and publish update_refcount_discard()
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2021-04-22 16:30 ` [PATCH v6 03/12] block/qcow2-cluster: assert no data_file on compressed write path Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 05/12] block/qcow2: introduce qcow2_parse_compressed_cluster_descriptor() Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
Make the function public to be reused later. Make it's name to show
what function does instead of where it is called.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.h | 3 +++
block/qcow2-refcount.c | 8 ++++----
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 0fe5f74ed3..95119876e1 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -896,6 +896,9 @@ int qcow2_shrink_reftable(BlockDriverState *bs);
int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
int qcow2_detect_metadata_preallocation(BlockDriverState *bs);
+void qcow2_cache_host_discard(BlockDriverState *bs,
+ uint64_t offset, uint64_t length);
+
/* qcow2-cluster.c functions */
int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
bool exact_size);
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 543fcf289c..ad021aab7a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -749,8 +749,8 @@ void qcow2_process_discards(BlockDriverState *bs, int ret)
}
}
-static void update_refcount_discard(BlockDriverState *bs,
- uint64_t offset, uint64_t length)
+void qcow2_cache_host_discard(BlockDriverState *bs,
+ uint64_t offset, uint64_t length)
{
BDRVQcow2State *s = bs->opaque;
Qcow2DiscardRegion *d, *p, *next;
@@ -901,7 +901,7 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
}
if (s->discard_passthrough[type]) {
- update_refcount_discard(bs, cluster_offset, s->cluster_size);
+ qcow2_cache_host_discard(bs, cluster_offset, s->cluster_size);
}
}
}
@@ -3369,7 +3369,7 @@ static int qcow2_discard_refcount_block(BlockDriverState *bs,
/* discard refblock from the cache if refblock is cached */
qcow2_cache_discard(s->refcount_block_cache, refblock);
}
- update_refcount_discard(bs, discard_block_offs, s->cluster_size);
+ qcow2_cache_host_discard(bs, discard_block_offs, s->cluster_size);
return 0;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 05/12] block/qcow2: introduce qcow2_parse_compressed_cluster_descriptor()
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2021-04-22 16:30 ` [PATCH v6 04/12] block/qcow2-refcount: rename and publish update_refcount_discard() Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 06/12] block/qcow2: refactor qcow2_co_preadv_task() to have one return Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
This functionality will be reused later. Let's make a separate function
now.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.h | 4 ++++
block/qcow2.c | 21 ++++++++++++++++-----
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 95119876e1..511db948ec 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -917,6 +917,10 @@ int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
uint64_t offset,
int compressed_size,
uint64_t *host_offset);
+void qcow2_parse_compressed_cluster_descriptor(BDRVQcow2State *s,
+ uint64_t cluster_descriptor,
+ uint64_t *coffset,
+ int *csize);
int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m);
diff --git a/block/qcow2.c b/block/qcow2.c
index 9727ae8fe3..7049f7fc3e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4663,6 +4663,19 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
return ret;
}
+void qcow2_parse_compressed_cluster_descriptor(BDRVQcow2State *s,
+ uint64_t cluster_descriptor,
+ uint64_t *coffset,
+ int *csize)
+{
+ int nb_csectors;
+
+ *coffset = cluster_descriptor & s->cluster_offset_mask;
+ nb_csectors = ((cluster_descriptor >> s->csize_shift) & s->csize_mask) + 1;
+ *csize = nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE -
+ (*coffset & ~QCOW2_COMPRESSED_SECTOR_MASK);
+}
+
static int coroutine_fn
qcow2_co_preadv_compressed(BlockDriverState *bs,
uint64_t cluster_descriptor,
@@ -4672,15 +4685,13 @@ qcow2_co_preadv_compressed(BlockDriverState *bs,
size_t qiov_offset)
{
BDRVQcow2State *s = bs->opaque;
- int ret = 0, csize, nb_csectors;
+ int ret = 0, csize;
uint64_t coffset;
uint8_t *buf, *out_buf;
int offset_in_cluster = offset_into_cluster(s, offset);
- coffset = cluster_descriptor & s->cluster_offset_mask;
- nb_csectors = ((cluster_descriptor >> s->csize_shift) & s->csize_mask) + 1;
- csize = nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE -
- (coffset & ~QCOW2_COMPRESSED_SECTOR_MASK);
+ qcow2_parse_compressed_cluster_descriptor(s, cluster_descriptor, &coffset,
+ &csize);
buf = g_try_malloc(csize);
if (!buf) {
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 06/12] block/qcow2: refactor qcow2_co_preadv_task() to have one return
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2021-04-22 16:30 ` [PATCH v6 05/12] block/qcow2: introduce qcow2_parse_compressed_cluster_descriptor() Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 07/12] block/qcow2: qcow2_co_pwrite_zeroes: use QEMU_LOCK_GUARD Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
We are going to add an action before return for this function. Refactor
function now to make further patch simpler.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 7049f7fc3e..066d44e2be 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2234,6 +2234,7 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
size_t qiov_offset)
{
BDRVQcow2State *s = bs->opaque;
+ int ret;
switch (subc_type) {
case QCOW2_SUBCLUSTER_ZERO_PLAIN:
@@ -2246,28 +2247,31 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
- return bdrv_co_preadv_part(bs->backing, offset, bytes,
- qiov, qiov_offset, 0);
+ ret = bdrv_co_preadv_part(bs->backing, offset, bytes,
+ qiov, qiov_offset, 0);
+ break;
case QCOW2_SUBCLUSTER_COMPRESSED:
- return qcow2_co_preadv_compressed(bs, host_offset,
- offset, bytes, qiov, qiov_offset);
+ ret = qcow2_co_preadv_compressed(bs, host_offset,
+ offset, bytes, qiov, qiov_offset);
+ break;
case QCOW2_SUBCLUSTER_NORMAL:
if (bs->encrypted) {
- return qcow2_co_preadv_encrypted(bs, host_offset,
- offset, bytes, qiov, qiov_offset);
+ ret = qcow2_co_preadv_encrypted(bs, host_offset,
+ offset, bytes, qiov, qiov_offset);
+ } else {
+ BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
+ ret = bdrv_co_preadv_part(s->data_file, host_offset,
+ bytes, qiov, qiov_offset, 0);
}
-
- BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
- return bdrv_co_preadv_part(s->data_file, host_offset,
- bytes, qiov, qiov_offset, 0);
+ break;
default:
g_assert_not_reached();
}
- g_assert_not_reached();
+ return ret;
}
static coroutine_fn int qcow2_co_preadv_task_entry(AioTask *task)
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 07/12] block/qcow2: qcow2_co_pwrite_zeroes: use QEMU_LOCK_GUARD
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2021-04-22 16:30 ` [PATCH v6 06/12] block/qcow2: refactor qcow2_co_preadv_task() to have one return Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 08/12] qcow2: introduce is_cluster_free() helper Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
We'll need to handle qcow2_get_host_offset() success and failure in
separate in future patch. Still, let's go a bit further and refactor
the function to use QEMU_LOCK_GUARD.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.c | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 066d44e2be..be62585e03 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3931,10 +3931,6 @@ static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
}
if (head || tail) {
- uint64_t off;
- unsigned int nr;
- QCow2SubclusterType type;
-
assert(head + bytes + tail <= s->subcluster_size);
/* check whether remainder of cluster already reads as zero */
@@ -3942,32 +3938,37 @@ static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
is_zero(bs, offset + bytes, tail))) {
return -ENOTSUP;
}
+ }
+
+ QEMU_LOCK_GUARD(&s->lock);
+
+ if (head || tail) {
+ uint64_t off;
+ unsigned int nr;
+ QCow2SubclusterType type;
- qemu_co_mutex_lock(&s->lock);
/* We can have new write after previous check */
offset -= head;
bytes = s->subcluster_size;
nr = s->subcluster_size;
ret = qcow2_get_host_offset(bs, offset, &nr, &off, &type);
- if (ret < 0 ||
- (type != QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN &&
- type != QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC &&
- type != QCOW2_SUBCLUSTER_ZERO_PLAIN &&
- type != QCOW2_SUBCLUSTER_ZERO_ALLOC)) {
- qemu_co_mutex_unlock(&s->lock);
- return ret < 0 ? ret : -ENOTSUP;
+ if (ret < 0) {
+ return ret;
+ }
+
+ if (type != QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN &&
+ type != QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC &&
+ type != QCOW2_SUBCLUSTER_ZERO_PLAIN &&
+ type != QCOW2_SUBCLUSTER_ZERO_ALLOC)
+ {
+ return -ENOTSUP;
}
- } else {
- qemu_co_mutex_lock(&s->lock);
}
trace_qcow2_pwrite_zeroes(qemu_coroutine_self(), offset, bytes);
/* Whatever is left can use real zero subclusters */
- ret = qcow2_subcluster_zeroize(bs, offset, bytes, flags);
- qemu_co_mutex_unlock(&s->lock);
-
- return ret;
+ return qcow2_subcluster_zeroize(bs, offset, bytes, flags);
}
static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 08/12] qcow2: introduce is_cluster_free() helper
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2021-04-22 16:30 ` [PATCH v6 07/12] block/qcow2: qcow2_co_pwrite_zeroes: use QEMU_LOCK_GUARD Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 09/12] qcow2: introduce host-range-refs Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
We are going to change the concept of "free host cluster", so let's
clarify it now and add a helper, which we will modify later.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index ad021aab7a..72e6d1efd7 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -962,13 +962,32 @@ int qcow2_update_cluster_refcount(BlockDriverState *bs,
/* cluster allocation functions */
+/*
+ * Cluster is free when its refcount is 0
+ *
+ * Return < 0 if failed to get refcount
+ * 0 if cluster is not free
+ * 1 if cluster is free
+ */
+static int is_cluster_free(BlockDriverState *bs, int64_t cluster_index)
+{
+ int ret;
+ uint64_t refcount;
+
+ ret = qcow2_get_refcount(bs, cluster_index, &refcount);
+ if (ret < 0) {
+ return ret;
+ }
+
+ return refcount == 0;
+}
/* return < 0 if error */
static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size,
uint64_t max)
{
BDRVQcow2State *s = bs->opaque;
- uint64_t i, nb_clusters, refcount;
+ uint64_t i, nb_clusters;
int ret;
/* We can't allocate clusters if they may still be queued for discard. */
@@ -980,11 +999,11 @@ static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size,
retry:
for(i = 0; i < nb_clusters; i++) {
uint64_t next_cluster_index = s->free_cluster_index++;
- ret = qcow2_get_refcount(bs, next_cluster_index, &refcount);
+ ret = is_cluster_free(bs, next_cluster_index);
if (ret < 0) {
return ret;
- } else if (refcount != 0) {
+ } else if (!ret) {
goto retry;
}
}
@@ -1031,7 +1050,7 @@ int64_t qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
int64_t nb_clusters)
{
BDRVQcow2State *s = bs->opaque;
- uint64_t cluster_index, refcount;
+ uint64_t cluster_index;
uint64_t i;
int ret;
@@ -1044,10 +1063,10 @@ int64_t qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
/* Check how many clusters there are free */
cluster_index = offset >> s->cluster_bits;
for(i = 0; i < nb_clusters; i++) {
- ret = qcow2_get_refcount(bs, cluster_index++, &refcount);
+ ret = is_cluster_free(bs, cluster_index++);
if (ret < 0) {
return ret;
- } else if (refcount != 0) {
+ } else if (!ret) {
break;
}
}
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 09/12] qcow2: introduce host-range-refs
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2021-04-22 16:30 ` [PATCH v6 08/12] qcow2: introduce is_cluster_free() helper Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 10/12] qcow2: introduce qcow2_host_cluster_postponed_discard() Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
We have a bug in qcow2: assume we've started data write into host
cluster A. s->lock is unlocked. During the write the refcount of
cluster A may become zero, cluster may be reallocated for other needs,
and our in-flight write become a use-after-free. More details will be
in the further commit which actually fixes the bug.
For now, let's prepare infrastructure for the following fix. We are
going to track these in-flight data writes and other operations. So, we
create a hash map
cluster_index -> HostCluster
And for each HostCluster we calculate number of in-flight operations on
it (which does qcow2_host_range_ref() of course).
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.h | 12 ++++
block/qcow2-host-range-refs.c | 127 ++++++++++++++++++++++++++++++++++
block/qcow2.c | 3 +
block/meson.build | 1 +
4 files changed, 143 insertions(+)
create mode 100644 block/qcow2-host-range-refs.c
diff --git a/block/qcow2.h b/block/qcow2.h
index 511db948ec..d6de9543c4 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -420,6 +420,9 @@ typedef struct BDRVQcow2State {
* is to convert the image with the desired compression type set.
*/
Qcow2CompressionType compression_type;
+
+ /* For qcow2-host-range-refs.c */
+ GHashTable *host_range_refs;
} BDRVQcow2State;
typedef struct Qcow2COWRegion {
@@ -899,6 +902,15 @@ int qcow2_detect_metadata_preallocation(BlockDriverState *bs);
void qcow2_cache_host_discard(BlockDriverState *bs,
uint64_t offset, uint64_t length);
+void qcow2_init_host_range_refs(BDRVQcow2State *s);
+void qcow2_release_host_range_refs(BDRVQcow2State *s);
+void qcow2_host_range_ref(BlockDriverState *bs, int64_t offset,
+ int64_t length);
+void qcow2_host_range_unref(BlockDriverState *bs, int64_t offset,
+ int64_t length);
+uint64_t qcow2_get_host_range_refcnt(BlockDriverState *bs,
+ int64_t cluster_index);
+
/* qcow2-cluster.c functions */
int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
bool exact_size);
diff --git a/block/qcow2-host-range-refs.c b/block/qcow2-host-range-refs.c
new file mode 100644
index 0000000000..54f0be27a4
--- /dev/null
+++ b/block/qcow2-host-range-refs.c
@@ -0,0 +1,127 @@
+/*
+ * Block driver for the QCOW version 2 format
+ *
+ * Copyright (c) 2021 Virtuozzo International GmbH.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qcow2.h"
+
+typedef struct HostCluster {
+ uint64_t host_range_refcnt;
+
+ /* For convenience, keep cluster_index here */
+ int64_t cluster_index;
+} HostCluster;
+
+void qcow2_init_host_range_refs(BDRVQcow2State *s)
+{
+ s->host_range_refs =
+ g_hash_table_new_full(g_int64_hash, g_int64_equal, g_free, g_free);
+}
+
+void qcow2_release_host_range_refs(BDRVQcow2State *s)
+{
+ assert(g_hash_table_size(s->host_range_refs) == 0);
+ g_hash_table_unref(s->host_range_refs);
+}
+
+static HostCluster *find_host_cluster(BDRVQcow2State *s, int64_t cluster_index)
+{
+ HostCluster *cl;
+
+ if (!s->host_range_refs) {
+ return NULL;
+ }
+
+ cl = g_hash_table_lookup(s->host_range_refs, &cluster_index);
+
+ if (cl) {
+ assert(cl->host_range_refcnt > 0);
+ }
+
+ return cl;
+}
+
+uint64_t qcow2_get_host_range_refcnt(BlockDriverState *bs,
+ int64_t cluster_index)
+{
+ BDRVQcow2State *s = bs->opaque;
+ HostCluster *cl = find_host_cluster(s, cluster_index);
+
+ if (!cl) {
+ return 0;
+ }
+
+ return cl->host_range_refcnt;
+}
+
+/* Inrease host_range_refcnt of clusters intersecting with range */
+void coroutine_fn
+qcow2_host_range_ref(BlockDriverState *bs, int64_t offset, int64_t length)
+{
+ BDRVQcow2State *s = bs->opaque;
+ int64_t start, last, cluster_index;
+
+ start = start_of_cluster(s, offset) >> s->cluster_bits;
+ last = start_of_cluster(s, offset + length - 1) >> s->cluster_bits;
+ for (cluster_index = start; cluster_index <= last; cluster_index++) {
+ HostCluster *cl = find_host_cluster(s, cluster_index);
+
+ if (!cl) {
+ cl = g_new(HostCluster, 1);
+ *cl = (HostCluster) {
+ .cluster_index = cluster_index,
+ .host_range_refcnt = 1,
+ };
+ g_hash_table_insert(s->host_range_refs,
+ g_memdup(&cluster_index,
+ sizeof(cluster_index)), cl);
+ } else {
+ cl->host_range_refcnt++;
+ }
+ continue;
+ }
+}
+
+/* Decrease host_range_refcnt of clusters intersecting with range */
+void coroutine_fn
+qcow2_host_range_unref(BlockDriverState *bs, int64_t offset, int64_t length)
+{
+ BDRVQcow2State *s = bs->opaque;
+ int64_t start, last, cluster_index;
+
+ start = start_of_cluster(s, offset) >> s->cluster_bits;
+ last = start_of_cluster(s, offset + length - 1) >> s->cluster_bits;
+ for (cluster_index = start; cluster_index <= last; cluster_index++) {
+ HostCluster *cl = find_host_cluster(s, cluster_index);
+
+ assert(cl);
+ assert(cl->host_range_refcnt >= 1);
+
+ if (cl->host_range_refcnt > 1) {
+ cl->host_range_refcnt--;
+ continue;
+ }
+
+ g_hash_table_remove(s->host_range_refs, &cluster_index);
+ }
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index be62585e03..aa298c9e42 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1834,6 +1834,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
#endif
qemu_co_queue_init(&s->thread_task_queue);
+ qcow2_init_host_range_refs(s);
return ret;
@@ -2714,6 +2715,8 @@ static void qcow2_close(BlockDriverState *bs)
g_free(s->image_backing_file);
g_free(s->image_backing_format);
+ qcow2_release_host_range_refs(s);
+
if (has_data_file(bs)) {
bdrv_unref_child(bs, s->data_file);
s->data_file = NULL;
diff --git a/block/meson.build b/block/meson.build
index d21990ec95..a9bf6fde0c 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -25,6 +25,7 @@ block_ss.add(files(
'qcow2-bitmap.c',
'qcow2-cache.c',
'qcow2-cluster.c',
+ 'qcow2-host-range-refs.c',
'qcow2-refcount.c',
'qcow2-snapshot.c',
'qcow2-threads.c',
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 10/12] qcow2: introduce qcow2_host_cluster_postponed_discard()
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2021-04-22 16:30 ` [PATCH v6 09/12] qcow2: introduce host-range-refs Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 11/12] qcow2: protect data writing by host range reference Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
We have a bug in qcow2: assume we've started data write into host
cluster A. s->lock is unlocked. During the write the refcount of
cluster A may become zero, cluster may be reallocated for other needs,
and our in-flight write become a use-after-free.
To fix the bug let's do the following. Or better, let's start from what
we have now:
Now we consider cluster "free", when its refcount is 0. When cluster
becomes "free" we also update s->free_cluster_index and optionally
discard it on bs->file level. These two operations are done in same
s->lock critical section where refcount becomes 0 (and this all is in
update_refcount()). Calling update_refcount() wirthout s->lock held is
wrong. It's ofcourse done sometimes, as not everything is moved to
coroutine for now..
Still, it's out of our topic.
Later, we can reallocate "free" cluster in alloc_clusters_noref() and
qcow2_alloc_clusters_at(), where is_cluster_free() is used.
OK, to correctly handle in-flight writes, let's modify a concept of
"free" cluster, so that cluster is "free" when its refcount is 0 and
there no inflight writes. We are going to track in-flight writes (and
other operations with host data clusters) with help of
host-range-references recently implemented. So cluster would be "free"
when its refcount is 0 and host-range-refcnt is 0 too.
So, we discard the cluster at bs->file level, update
s->free_cluster_index and allow reallocation only when both refcount
and inflight-write-cnt becomes both zero. It may happen either in
update_refcount() or in qcow2_host_range_unref().
In update_refcount() we just discard if host-range-refcnt is 0 and
register postponded discard if it isnt.
We implement postponed discard functionality so that
qcow2_host_range_unref() doesn't have to load refcounts.
So in qcow2_host_range_unref() we just do postcponed discard if it is
registered in HostCluster struct.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.h | 4 +++
block/qcow2-host-range-refs.c | 47 +++++++++++++++++++++++++++++++++++
block/qcow2-refcount.c | 23 +++++++++++------
3 files changed, 67 insertions(+), 7 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index d6de9543c4..c40548c4fb 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -911,6 +911,10 @@ void qcow2_host_range_unref(BlockDriverState *bs, int64_t offset,
uint64_t qcow2_get_host_range_refcnt(BlockDriverState *bs,
int64_t cluster_index);
+bool qcow2_host_cluster_postponed_discard(BlockDriverState *bs,
+ int64_t cluster_index,
+ enum qcow2_discard_type type);
+
/* qcow2-cluster.c functions */
int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
bool exact_size);
diff --git a/block/qcow2-host-range-refs.c b/block/qcow2-host-range-refs.c
index 54f0be27a4..e07cb06184 100644
--- a/block/qcow2-host-range-refs.c
+++ b/block/qcow2-host-range-refs.c
@@ -30,6 +30,13 @@ typedef struct HostCluster {
/* For convenience, keep cluster_index here */
int64_t cluster_index;
+
+ /*
+ * Qcow2 refcount of this host cluster is zero. So, when all dynamic users
+ * put their references back, we should discard the cluster.
+ */
+ bool postponed_discard;
+ enum qcow2_discard_type postponed_discard_type;
} HostCluster;
void qcow2_init_host_range_refs(BDRVQcow2State *s)
@@ -122,6 +129,46 @@ qcow2_host_range_unref(BlockDriverState *bs, int64_t offset, int64_t length)
continue;
}
+ if (!cl->postponed_discard) {
+ g_hash_table_remove(s->host_range_refs, &cluster_index);
+ continue;
+ }
+
+ /*
+ * OK. refcnt become 0 and we should do postponed discard. Let's keep
+ * host_range_refcnt = 1 during this final IO operation.
+ */
+ if (s->discard_passthrough[cl->postponed_discard_type]) {
+ int64_t cluster_offset = cluster_index << s->cluster_bits;
+ if (s->cache_discards) {
+ qcow2_cache_host_discard(bs, cluster_offset, s->cluster_size);
+ } else {
+ /* Discard is optional, ignore the return value */
+ bdrv_pdiscard(bs->file, cluster_offset, s->cluster_size);
+ }
+ }
+
g_hash_table_remove(s->host_range_refs, &cluster_index);
+
+ if (cluster_index < s->free_cluster_index) {
+ s->free_cluster_index = cluster_index;
+ }
+ }
+}
+
+bool qcow2_host_cluster_postponed_discard(BlockDriverState *bs,
+ int64_t cluster_index,
+ enum qcow2_discard_type type)
+{
+ BDRVQcow2State *s = bs->opaque;
+ HostCluster *cl = find_host_cluster(s, cluster_index);
+
+ if (!cl) {
+ return false;
}
+
+ cl->postponed_discard = true;
+ cl->postponed_discard_type = type;
+
+ return true;
}
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 72e6d1efd7..7f238649db 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -878,9 +878,6 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
} else {
refcount += addend;
}
- if (refcount == 0 && cluster_index < s->free_cluster_index) {
- s->free_cluster_index = cluster_index;
- }
s->set_refcount(refcount_block, block_index, refcount);
if (refcount == 0) {
@@ -900,8 +897,20 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
qcow2_cache_discard(s->l2_table_cache, table);
}
- if (s->discard_passthrough[type]) {
- qcow2_cache_host_discard(bs, cluster_offset, s->cluster_size);
+ if (!qcow2_host_cluster_postponed_discard(bs, cluster_index,
+ type))
+ {
+ /*
+ * Refcount is zero as well as host-range-refcnt. Cluster is
+ * free.
+ */
+ if (cluster_index < s->free_cluster_index) {
+ s->free_cluster_index = cluster_index;
+ }
+ if (s->discard_passthrough[type]) {
+ qcow2_cache_host_discard(bs, cluster_offset,
+ s->cluster_size);
+ }
}
}
}
@@ -963,7 +972,7 @@ int qcow2_update_cluster_refcount(BlockDriverState *bs,
/*
- * Cluster is free when its refcount is 0
+ * Cluster is free when its refcount is 0 and there is no in-flight writes
*
* Return < 0 if failed to get refcount
* 0 if cluster is not free
@@ -979,7 +988,7 @@ static int is_cluster_free(BlockDriverState *bs, int64_t cluster_index)
return ret;
}
- return refcount == 0;
+ return refcount == 0 && qcow2_get_host_range_refcnt(bs, cluster_index) == 0;
}
/* return < 0 if error */
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 11/12] qcow2: protect data writing by host range reference
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2021-04-22 16:30 ` [PATCH v6 10/12] qcow2: introduce qcow2_host_cluster_postponed_discard() Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-22 16:30 ` [PATCH v6 12/12] qcow2: protect data reading " Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
We have the following bug:
1. Start write to qcow2. Assume guest cluster G and corresponding host
cluster is H.
2. The write requests come to the point of data writing to .file. The
write to .file is started and qcow2 mutex is unlocked.
3. At this time refcount of H becomes 0. For example, it may be due to
discard operation on qcow2 node, or rewriting compressed data by
normal write, or some operation with snapshots..
4. Next, some operations occurs and leads to allocation of H for some
other needs: it may be another write-to-qcow2-node operation, or
allocation of L2 table or some other data or metadata cluster
allocation.
5. So, at this point H is used for something other. Assume, L2 table is
written into H.
6. And now, our write from [2] finishes. And pollutes L2 table in H.
That's a bug.
To fix the bug we now have host-range-refs, which work in a
way that cluster is not "free" (and therefore will not be reused
and we don't fall into use-after-free described above) until both
refcount and host-range-ref are zero for this cluster.
Let's call qcow2_host_range_ref() in cluster allocation functions:
qcow2_alloc_host_offset() and qcow2_alloc_compressed_cluster_offset()
used on when writing host clusters. So that now these functions returns
"referenced" range, which caller should finally unref.
Iotest qcow2-discard-during-rewrite is enabled, as it works now.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-cluster.c | 13 +++++++++++++
block/qcow2.c | 16 ++++++++++++++++
.../tests/qcow2-discard-during-rewrite | 2 +-
3 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 6105d4e7e0..999a739024 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -809,6 +809,10 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
* already allocated at the offset, return an error.
*
* Return 0 on success and -errno in error cases
+ *
+ * On success the host range [*host_offset, *host_offset + compressed_size) is
+ * referenced. Caller is responsible to unref it by qcow2_host_range_unref()
+ * after finishing IO operation with this range.
*/
int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
uint64_t offset,
@@ -866,6 +870,9 @@ int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
*host_offset = cluster_offset & s->cluster_offset_mask;
+
+ qcow2_host_range_ref(bs, *host_offset, compressed_size);
+
return 0;
}
@@ -1738,6 +1745,10 @@ out:
* is queued and will be reentered when the dependency has completed.
*
* Return 0 on success and -errno in error cases
+ *
+ * On success the host range [*host_offset, *host_offset + *bytes) is
+ * referenced. Caller is responsible to unref it by qcow2_host_range_unref()
+ * after finishing IO operation with this range.
*/
int qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset,
unsigned int *bytes, uint64_t *host_offset,
@@ -1848,6 +1859,8 @@ again:
assert(offset_into_cluster(s, *host_offset) ==
offset_into_cluster(s, offset));
+ qcow2_host_range_ref(bs, *host_offset, *bytes);
+
return 0;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index aa298c9e42..d0d2eaa914 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2489,6 +2489,8 @@ static int handle_alloc_space(BlockDriverState *bs, QCowL2Meta *l2meta)
* Called with s->lock unlocked
* l2meta - if not NULL, qcow2_co_pwritev_task() will consume it. Caller must
* not use it somehow after qcow2_co_pwritev_task() call
+ *
+ * Function consumes range reference both on success and failure.
*/
static coroutine_fn int qcow2_co_pwritev_task(BlockDriverState *bs,
uint64_t host_offset,
@@ -2554,6 +2556,9 @@ out_unlocked:
out_locked:
qcow2_handle_l2meta(bs, &l2meta, false);
+
+ qcow2_host_range_unref(bs, host_offset, bytes);
+
qemu_co_mutex_unlock(&s->lock);
qemu_vfree(crypt_buf);
@@ -2610,6 +2615,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
ret = qcow2_pre_write_overlap_check(bs, 0, host_offset,
cur_bytes, true);
if (ret < 0) {
+ qcow2_host_range_unref(bs, host_offset, cur_bytes);
goto out_locked;
}
@@ -3151,6 +3157,9 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
goto out;
}
+ /* We do truncate under mutex, don't bother with host range refs */
+ qcow2_host_range_unref(bs, host_offset, cur_bytes);
+
for (m = meta; m != NULL; m = m->next) {
m->prealloc = true;
}
@@ -4122,12 +4131,14 @@ qcow2_co_copy_range_to(BlockDriverState *bs,
ret = qcow2_pre_write_overlap_check(bs, 0, host_offset, cur_bytes,
true);
if (ret < 0) {
+ qcow2_host_range_unref(bs, host_offset, cur_bytes);
goto fail;
}
qemu_co_mutex_unlock(&s->lock);
ret = bdrv_co_copy_range_to(src, src_offset, s->data_file, host_offset,
cur_bytes, read_flags, write_flags);
+ qcow2_host_range_unref(bs, host_offset, cur_bytes);
qemu_co_mutex_lock(&s->lock);
if (ret < 0) {
goto fail;
@@ -4540,6 +4551,7 @@ qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
ssize_t out_len;
uint8_t *buf, *out_buf;
uint64_t cluster_offset;
+ bool unref_range = false;
assert(bytes == s->cluster_size || (bytes < s->cluster_size &&
(offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS)));
@@ -4574,6 +4586,7 @@ qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
qemu_co_mutex_unlock(&s->lock);
goto fail;
}
+ unref_range = true;
ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len, true);
qemu_co_mutex_unlock(&s->lock);
@@ -4589,6 +4602,9 @@ qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
success:
ret = 0;
fail:
+ if (unref_range) {
+ qcow2_host_range_unref(bs, cluster_offset, out_len);
+ }
qemu_vfree(buf);
g_free(out_buf);
return ret;
diff --git a/tests/qemu-iotests/tests/qcow2-discard-during-rewrite b/tests/qemu-iotests/tests/qcow2-discard-during-rewrite
index 7f0d8a107a..2e2e0d2cb0 100755
--- a/tests/qemu-iotests/tests/qcow2-discard-during-rewrite
+++ b/tests/qemu-iotests/tests/qcow2-discard-during-rewrite
@@ -1,5 +1,5 @@
#!/usr/bin/env bash
-# group: quick disabled
+# group: quick
#
# Test discarding (and reusing) host cluster during writing data to it.
#
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 12/12] qcow2: protect data reading by host range reference
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (10 preceding siblings ...)
2021-04-22 16:30 ` [PATCH v6 11/12] qcow2: protect data writing by host range reference Vladimir Sementsov-Ogievskiy
@ 2021-04-22 16:30 ` Vladimir Sementsov-Ogievskiy
2021-04-26 12:15 ` [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
2021-05-10 13:07 ` Vladimir Sementsov-Ogievskiy
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-22 16:30 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, vsementsov, den
Similarly to previous commit: host cluster may be discarded and reused
for another cluster or metadata during data read.
This is not as dangerous as write path, we will not corrupt data or
metadata. Still it's bad: guest will probably see data or metadata
which it should not see, so it's a kind of security hole. Let's fix it
too.
Data reading goes through qcow2_get_host_offset(). Let's reference
range returned by this function. Read path differs from write, as we
have to handle compressed cluster descriptor. Also, we should handle
ZERO and UNALLOCATED clusters, for which we have nothing to ref. So, to
keep the whole logic in one place, create qcow2_put_host_offset(),
which should be always called after qcow2_get_host_offset().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.h | 3 +++
block/qcow2-cluster.c | 38 ++++++++++++++++++++++++++++++++++++++
block/qcow2.c | 15 +++++++++++++++
3 files changed, 56 insertions(+)
diff --git a/block/qcow2.h b/block/qcow2.h
index c40548c4fb..2ac61eccc5 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -926,6 +926,9 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
unsigned int *bytes, uint64_t *host_offset,
QCow2SubclusterType *subcluster_type);
+void qcow2_put_host_offset(BlockDriverState *bs,
+ unsigned int bytes, uint64_t host_offset,
+ QCow2SubclusterType subcluster_type);
int qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset,
unsigned int *bytes, uint64_t *host_offset,
QCowL2Meta **m);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 999a739024..126d95b062 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -568,6 +568,10 @@ static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
* Compressed clusters are always processed one by one.
*
* Returns 0 on success, -errno in error cases.
+ *
+ * The returned range is referenced, so that it can't be discarded in parallel.
+ * Caller is responsible to unref by qcow2_put_host_offset() after finishing IO
+ * operations with the range.
*/
int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
unsigned int *bytes, uint64_t *host_offset,
@@ -721,6 +725,17 @@ out:
*subcluster_type = type;
+ if (type == QCOW2_SUBCLUSTER_COMPRESSED) {
+ uint64_t coffset;
+ int csize;
+
+ qcow2_parse_compressed_cluster_descriptor(s, *host_offset, &coffset,
+ &csize);
+ qcow2_host_range_ref(bs, coffset, csize);
+ } else if (*host_offset) {
+ qcow2_host_range_ref(bs, *host_offset, *bytes);
+ }
+
return 0;
fail:
@@ -728,6 +743,29 @@ fail:
return ret;
}
+/*
+ * Caller of qcow2_get_host_offset() must call qcow2_put_host_offset() with
+ * returned parameters of qcow2_get_host_offset() when caller don't need them
+ * anymore.
+ */
+void qcow2_put_host_offset(BlockDriverState *bs,
+ unsigned int bytes, uint64_t host_offset,
+ QCow2SubclusterType subcluster_type)
+{
+ BDRVQcow2State *s = bs->opaque;
+
+ if (subcluster_type == QCOW2_SUBCLUSTER_COMPRESSED) {
+ uint64_t coffset;
+ int csize;
+
+ qcow2_parse_compressed_cluster_descriptor(s, host_offset, &coffset,
+ &csize);
+ qcow2_host_range_unref(bs, coffset, csize);
+ } else if (host_offset) {
+ qcow2_host_range_unref(bs, host_offset, bytes);
+ }
+}
+
/*
* get_cluster_table
*
diff --git a/block/qcow2.c b/block/qcow2.c
index d0d2eaa914..d3461b7243 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2069,6 +2069,8 @@ static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
return ret;
}
+ qcow2_put_host_offset(bs, bytes, host_offset, type);
+
*pnum = bytes;
if ((type == QCOW2_SUBCLUSTER_NORMAL ||
@@ -2227,6 +2229,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
return 0;
}
+/* Function consumes host range reference if needed */
static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
QCow2SubclusterType subc_type,
uint64_t host_offset,
@@ -2272,6 +2275,8 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
g_assert_not_reached();
}
+ qcow2_put_host_offset(bs, bytes, host_offset, subc_type);
+
return ret;
}
@@ -2320,6 +2325,7 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
(type == QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC && !bs->backing))
{
qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes);
+ qcow2_put_host_offset(bs, cur_bytes, host_offset, type);
} else {
if (!aio && cur_bytes != bytes) {
aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
@@ -3968,6 +3974,12 @@ static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
return ret;
}
+ /*
+ * We do the whole thing under s->lock, so we are safe in modifying
+ * metadata. We don't need the reference.
+ */
+ qcow2_put_host_offset(bs, nr, off, type);
+
if (type != QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN &&
type != QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC &&
type != QCOW2_SUBCLUSTER_ZERO_PLAIN &&
@@ -4064,6 +4076,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
break;
case QCOW2_SUBCLUSTER_COMPRESSED:
+ qcow2_put_host_offset(bs, cur_bytes, copy_offset, type);
ret = -ENOTSUP;
goto out;
@@ -4079,6 +4092,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
copy_offset,
dst, dst_offset,
cur_bytes, read_flags, cur_write_flags);
+ qcow2_put_host_offset(bs, cur_bytes, copy_offset, type);
qemu_co_mutex_lock(&s->lock);
if (ret < 0) {
goto out;
@@ -4700,6 +4714,7 @@ void qcow2_parse_compressed_cluster_descriptor(BDRVQcow2State *s,
(*coffset & ~QCOW2_COMPRESSED_SECTOR_MASK);
}
+/* Function consumes host range reference */
static int coroutine_fn
qcow2_co_preadv_compressed(BlockDriverState *bs,
uint64_t cluster_descriptor,
--
2.29.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless)
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (11 preceding siblings ...)
2021-04-22 16:30 ` [PATCH v6 12/12] qcow2: protect data reading " Vladimir Sementsov-Ogievskiy
@ 2021-04-26 12:15 ` Vladimir Sementsov-Ogievskiy
2021-05-10 13:07 ` Vladimir Sementsov-Ogievskiy
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-26 12:15 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, den
22.04.2021 19:30, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> It's an alternative lock-less solution to
> [PATCH v4 0/3] qcow2: fix parallel rewrite and discard (rw-lock)
>
> In v6 a lot of things are rewritten.
>
> What is changed:
>
> 1. rename the feature to host_range_refcnt, move it to separate file
> 2. better naming for everything (I hope)
> 3. cover reads, not only writes
> 4. do "ref" in qcow2_get_host_offset(), qcow2_alloc_host_offset(),
> qcow2_alloc_compressed_cluster_offset().
> and callers do "unref" appropriately.
>
About performance. With these series we do extra allocations and hash-map operations.. Still testing by
./build/qemu-img bench -c 1000000 -s 4K --image-opts driver=null-co,size=5G
and
./build/qemu-img bench -c 1000000 -s 4K -w --image-opts driver=null-co,size=5G
I see difference less than 1%.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless)
2021-04-22 16:30 [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
` (12 preceding siblings ...)
2021-04-26 12:15 ` [PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless) Vladimir Sementsov-Ogievskiy
@ 2021-05-10 13:07 ` Vladimir Sementsov-Ogievskiy
13 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-10 13:07 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, mreitz, kwolf, den
Hi!
Kevin, what do think on this now? Do you think "[PATCH v4 0/3] qcow2: fix parallel rewrite and discard (rw-lock)" still worth to update to cover reads and resend? (consider also my replies to "[PATCH v4 0/3] qcow2: fix parallel rewrite and discard (rw-lock)" branch)
22.04.2021 19:30, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> It's an alternative lock-less solution to
> [PATCH v4 0/3] qcow2: fix parallel rewrite and discard (rw-lock)
>
> In v6 a lot of things are rewritten.
>
> What is changed:
>
> 1. rename the feature to host_range_refcnt, move it to separate file
> 2. better naming for everything (I hope)
> 3. cover reads, not only writes
> 4. do "ref" in qcow2_get_host_offset(), qcow2_alloc_host_offset(),
> qcow2_alloc_compressed_cluster_offset().
> and callers do "unref" appropriately.
>
> Vladimir Sementsov-Ogievskiy (12):
> iotests: add qcow2-discard-during-rewrite
> qcow2: fix cache discarding in update_refcount()
> block/qcow2-cluster: assert no data_file on compressed write path
> block/qcow2-refcount: rename and publish update_refcount_discard()
> block/qcow2: introduce qcow2_parse_compressed_cluster_descriptor()
> block/qcow2: refactor qcow2_co_preadv_task() to have one return
> block/qcow2: qcow2_co_pwrite_zeroes: use QEMU_LOCK_GUARD
> qcow2: introduce is_cluster_free() helper
> qcow2: introduce host-range-refs
> qcow2: introduce qcow2_host_cluster_postponed_discard()
> qcow2: protect data writing by host range reference
> qcow2: protect data reading by host range reference
>
> block/qcow2.h | 26 +++
> block/qcow2-cluster.c | 55 +++++-
> block/qcow2-host-range-refs.c | 174 ++++++++++++++++++
> block/qcow2-refcount.c | 61 ++++--
> block/qcow2.c | 118 ++++++++----
> block/meson.build | 1 +
> .../tests/qcow2-discard-during-rewrite | 72 ++++++++
> .../tests/qcow2-discard-during-rewrite.out | 21 +++
> 8 files changed, 475 insertions(+), 53 deletions(-)
> create mode 100644 block/qcow2-host-range-refs.c
> create mode 100755 tests/qemu-iotests/tests/qcow2-discard-during-rewrite
> create mode 100644 tests/qemu-iotests/tests/qcow2-discard-during-rewrite.out
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread