The file_cluster_offset field of Qcow2AioTask stores a cluster-aligned
host offset. In practice this is not very useful because all users(*)
of this structure need the final host offset into the cluster, which
they calculate using
host_offset = file_cluster_offset + offset_into_cluster(s, offset)
There is no reason why Qcow2AioTask cannot store host_offset directly
and that is what this patch does.
(*) compressed clusters are the exception: in this case what
file_cluster_offset was storing was the full compressed cluster
descriptor (offset + size). This does not change with this patch
but it is documented now.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2.c | 68 +++++++++++++++++++++++++--------------------------
1 file changed, 33 insertions(+), 35 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index d44b45633d..a00b0c8e45 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -74,7 +74,7 @@ typedef struct {
static int coroutine_fn
qcow2_co_preadv_compressed(BlockDriverState *bs,
- uint64_t file_cluster_offset,
+ uint64_t cluster_descriptor,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -2041,7 +2041,7 @@ out:
static coroutine_fn int
qcow2_co_preadv_encrypted(BlockDriverState *bs,
- uint64_t file_cluster_offset,
+ uint64_t host_offset,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -2068,16 +2068,12 @@ qcow2_co_preadv_encrypted(BlockDriverState *bs,
}
BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
- ret = bdrv_co_pread(s->data_file,
- file_cluster_offset + offset_into_cluster(s, offset),
- bytes, buf, 0);
+ ret = bdrv_co_pread(s->data_file, host_offset, bytes, buf, 0);
if (ret < 0) {
goto fail;
}
- if (qcow2_co_decrypt(bs,
- file_cluster_offset + offset_into_cluster(s, offset),
- offset, buf, bytes) < 0)
+ if (qcow2_co_decrypt(bs, host_offset, offset, buf, bytes) < 0)
{
ret = -EIO;
goto fail;
@@ -2095,7 +2091,7 @@ typedef struct Qcow2AioTask {
BlockDriverState *bs;
QCow2ClusterType cluster_type; /* only for read */
- uint64_t file_cluster_offset;
+ uint64_t host_offset; /* or full descriptor in compressed clusters */
uint64_t offset;
uint64_t bytes;
QEMUIOVector *qiov;
@@ -2108,7 +2104,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
AioTaskPool *pool,
AioTaskFunc func,
QCow2ClusterType cluster_type,
- uint64_t file_cluster_offset,
+ uint64_t host_offset,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -2123,7 +2119,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
.bs = bs,
.cluster_type = cluster_type,
.qiov = qiov,
- .file_cluster_offset = file_cluster_offset,
+ .host_offset = host_offset,
.offset = offset,
.bytes = bytes,
.qiov_offset = qiov_offset,
@@ -2132,7 +2128,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
trace_qcow2_add_task(qemu_coroutine_self(), bs, pool,
func == qcow2_co_preadv_task_entry ? "read" : "write",
- cluster_type, file_cluster_offset, offset, bytes,
+ cluster_type, host_offset, offset, bytes,
qiov, qiov_offset);
if (!pool) {
@@ -2146,13 +2142,12 @@ static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
QCow2ClusterType cluster_type,
- uint64_t file_cluster_offset,
+ uint64_t host_offset,
uint64_t offset, uint64_t bytes,
QEMUIOVector *qiov,
size_t qiov_offset)
{
BDRVQcow2State *s = bs->opaque;
- int offset_in_cluster = offset_into_cluster(s, offset);
switch (cluster_type) {
case QCOW2_CLUSTER_ZERO_PLAIN:
@@ -2168,19 +2163,17 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
qiov, qiov_offset, 0);
case QCOW2_CLUSTER_COMPRESSED:
- return qcow2_co_preadv_compressed(bs, file_cluster_offset,
+ return qcow2_co_preadv_compressed(bs, host_offset,
offset, bytes, qiov, qiov_offset);
case QCOW2_CLUSTER_NORMAL:
- assert(offset_into_cluster(s, file_cluster_offset) == 0);
if (bs->encrypted) {
- return qcow2_co_preadv_encrypted(bs, file_cluster_offset,
+ return qcow2_co_preadv_encrypted(bs, host_offset,
offset, bytes, qiov, qiov_offset);
}
BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
- return bdrv_co_preadv_part(s->data_file,
- file_cluster_offset + offset_in_cluster,
+ return bdrv_co_preadv_part(s->data_file, host_offset,
bytes, qiov, qiov_offset, 0);
default:
@@ -2196,7 +2189,7 @@ static coroutine_fn int qcow2_co_preadv_task_entry(AioTask *task)
assert(!t->l2meta);
- return qcow2_co_preadv_task(t->bs, t->cluster_type, t->file_cluster_offset,
+ return qcow2_co_preadv_task(t->bs, t->cluster_type, t->host_offset,
t->offset, t->bytes, t->qiov, t->qiov_offset);
}
@@ -2232,11 +2225,20 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
{
qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes);
} else {
+ /*
+ * For compressed clusters the variable cluster_offset
+ * does not actually store the offset but the full
+ * descriptor. We need to leave it unchanged because
+ * that's what qcow2_co_preadv_compressed() expects.
+ */
+ uint64_t host_offset = (ret == QCOW2_CLUSTER_COMPRESSED) ?
+ cluster_offset :
+ cluster_offset + offset_into_cluster(s, offset);
if (!aio && cur_bytes != bytes) {
aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
}
ret = qcow2_add_task(bs, aio, qcow2_co_preadv_task_entry, ret,
- cluster_offset, offset, cur_bytes,
+ host_offset, offset, cur_bytes,
qiov, qiov_offset, NULL);
if (ret < 0) {
goto out;
@@ -2387,7 +2389,7 @@ static int handle_alloc_space(BlockDriverState *bs, QCowL2Meta *l2meta)
* not use it somehow after qcow2_co_pwritev_task() call
*/
static coroutine_fn int qcow2_co_pwritev_task(BlockDriverState *bs,
- uint64_t file_cluster_offset,
+ uint64_t host_offset,
uint64_t offset, uint64_t bytes,
QEMUIOVector *qiov,
uint64_t qiov_offset,
@@ -2396,7 +2398,6 @@ static coroutine_fn int qcow2_co_pwritev_task(BlockDriverState *bs,
int ret;
BDRVQcow2State *s = bs->opaque;
void *crypt_buf = NULL;
- int offset_in_cluster = offset_into_cluster(s, offset);
QEMUIOVector encrypted_qiov;
if (bs->encrypted) {
@@ -2409,8 +2410,7 @@ static coroutine_fn int qcow2_co_pwritev_task(BlockDriverState *bs,
}
qemu_iovec_to_buf(qiov, qiov_offset, crypt_buf, bytes);
- if (qcow2_co_encrypt(bs, file_cluster_offset + offset_in_cluster,
- offset, crypt_buf, bytes) < 0)
+ if (qcow2_co_encrypt(bs, host_offset, offset, crypt_buf, bytes) < 0)
{
ret = -EIO;
goto out_unlocked;
@@ -2435,10 +2435,8 @@ static coroutine_fn int qcow2_co_pwritev_task(BlockDriverState *bs,
*/
if (!merge_cow(offset, bytes, qiov, qiov_offset, l2meta)) {
BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
- trace_qcow2_writev_data(qemu_coroutine_self(),
- file_cluster_offset + offset_in_cluster);
- ret = bdrv_co_pwritev_part(s->data_file,
- file_cluster_offset + offset_in_cluster,
+ trace_qcow2_writev_data(qemu_coroutine_self(), host_offset);
+ ret = bdrv_co_pwritev_part(s->data_file, host_offset,
bytes, qiov, qiov_offset, 0);
if (ret < 0) {
goto out_unlocked;
@@ -2468,7 +2466,7 @@ static coroutine_fn int qcow2_co_pwritev_task_entry(AioTask *task)
assert(!t->cluster_type);
- return qcow2_co_pwritev_task(t->bs, t->file_cluster_offset,
+ return qcow2_co_pwritev_task(t->bs, t->host_offset,
t->offset, t->bytes, t->qiov, t->qiov_offset,
t->l2meta);
}
@@ -2523,8 +2521,8 @@ static coroutine_fn int qcow2_co_pwritev_part(
aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
}
ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_task_entry, 0,
- cluster_offset, offset, cur_bytes,
- qiov, qiov_offset, l2meta);
+ cluster_offset + offset_in_cluster, offset,
+ cur_bytes, qiov, qiov_offset, l2meta);
l2meta = NULL; /* l2meta is consumed by qcow2_co_pwritev_task() */
if (ret < 0) {
goto fail_nometa;
@@ -4358,7 +4356,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
static int coroutine_fn
qcow2_co_preadv_compressed(BlockDriverState *bs,
- uint64_t file_cluster_offset,
+ uint64_t cluster_descriptor,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -4370,8 +4368,8 @@ qcow2_co_preadv_compressed(BlockDriverState *bs,
uint8_t *buf, *out_buf;
int offset_in_cluster = offset_into_cluster(s, offset);
- coffset = file_cluster_offset & s->cluster_offset_mask;
- nb_csectors = ((file_cluster_offset >> s->csize_shift) & s->csize_mask) + 1;
+ 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);
--
2.20.1
On 3/17/20 1:15 PM, Alberto Garcia wrote: > The file_cluster_offset field of Qcow2AioTask stores a cluster-aligned > host offset. In practice this is not very useful because all users(*) > of this structure need the final host offset into the cluster, which > they calculate using > > host_offset = file_cluster_offset + offset_into_cluster(s, offset) > > There is no reason why Qcow2AioTask cannot store host_offset directly > and that is what this patch does. > > (*) compressed clusters are the exception: in this case what > file_cluster_offset was storing was the full compressed cluster > descriptor (offset + size). This does not change with this patch > but it is documented now. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2.c | 68 +++++++++++++++++++++++++-------------------------- > 1 file changed, 33 insertions(+), 35 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 17.03.20 19:15, Alberto Garcia wrote:
> The file_cluster_offset field of Qcow2AioTask stores a cluster-aligned
> host offset. In practice this is not very useful because all users(*)
> of this structure need the final host offset into the cluster, which
> they calculate using
>
> host_offset = file_cluster_offset + offset_into_cluster(s, offset)
>
> There is no reason why Qcow2AioTask cannot store host_offset directly
> and that is what this patch does.
>
> (*) compressed clusters are the exception: in this case what
> file_cluster_offset was storing was the full compressed cluster
> descriptor (offset + size). This does not change with this patch
> but it is documented now.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block/qcow2.c | 68 +++++++++++++++++++++++++--------------------------
> 1 file changed, 33 insertions(+), 35 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d44b45633d..a00b0c8e45 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
[...]
> @@ -2409,8 +2410,7 @@ static coroutine_fn int qcow2_co_pwritev_task(BlockDriverState *bs,
> }
> qemu_iovec_to_buf(qiov, qiov_offset, crypt_buf, bytes);
>
> - if (qcow2_co_encrypt(bs, file_cluster_offset + offset_in_cluster,
> - offset, crypt_buf, bytes) < 0)
> + if (qcow2_co_encrypt(bs, host_offset, offset, crypt_buf, bytes) < 0)
> {
This { should now go on the preceding line; with that fixed:
Reviewed-by: Max Reitz <mreitz@redhat.com>
> ret = -EIO;
> goto out_unlocked;
17.03.2020 21:15, Alberto Garcia wrote:
> The file_cluster_offset field of Qcow2AioTask stores a cluster-aligned
> host offset. In practice this is not very useful because all users(*)
> of this structure need the final host offset into the cluster, which
> they calculate using
>
> host_offset = file_cluster_offset + offset_into_cluster(s, offset)
>
> There is no reason why Qcow2AioTask cannot store host_offset directly
> and that is what this patch does.
>
> (*) compressed clusters are the exception: in this case what
> file_cluster_offset was storing was the full compressed cluster
> descriptor (offset + size). This does not change with this patch
> but it is documented now.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block/qcow2.c | 68 +++++++++++++++++++++++++--------------------------
> 1 file changed, 33 insertions(+), 35 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d44b45633d..a00b0c8e45 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -74,7 +74,7 @@ typedef struct {
>
> static int coroutine_fn
> qcow2_co_preadv_compressed(BlockDriverState *bs,
> - uint64_t file_cluster_offset,
> + uint64_t cluster_descriptor,
> uint64_t offset,
> uint64_t bytes,
> QEMUIOVector *qiov,
> @@ -2041,7 +2041,7 @@ out:
>
> static coroutine_fn int
> qcow2_co_preadv_encrypted(BlockDriverState *bs,
> - uint64_t file_cluster_offset,
> + uint64_t host_offset,
> uint64_t offset,
> uint64_t bytes,
> QEMUIOVector *qiov,
> @@ -2068,16 +2068,12 @@ qcow2_co_preadv_encrypted(BlockDriverState *bs,
> }
>
> BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
> - ret = bdrv_co_pread(s->data_file,
> - file_cluster_offset + offset_into_cluster(s, offset),
> - bytes, buf, 0);
> + ret = bdrv_co_pread(s->data_file, host_offset, bytes, buf, 0);
> if (ret < 0) {
> goto fail;
> }
>
> - if (qcow2_co_decrypt(bs,
> - file_cluster_offset + offset_into_cluster(s, offset),
> - offset, buf, bytes) < 0)
> + if (qcow2_co_decrypt(bs, host_offset, offset, buf, bytes) < 0)
> {
> ret = -EIO;
> goto fail;
> @@ -2095,7 +2091,7 @@ typedef struct Qcow2AioTask {
>
> BlockDriverState *bs;
> QCow2ClusterType cluster_type; /* only for read */
> - uint64_t file_cluster_offset;
> + uint64_t host_offset; /* or full descriptor in compressed clusters */
> uint64_t offset;
> uint64_t bytes;
> QEMUIOVector *qiov;
> @@ -2108,7 +2104,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
> AioTaskPool *pool,
> AioTaskFunc func,
> QCow2ClusterType cluster_type,
> - uint64_t file_cluster_offset,
> + uint64_t host_offset,
> uint64_t offset,
> uint64_t bytes,
> QEMUIOVector *qiov,
> @@ -2123,7 +2119,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
> .bs = bs,
> .cluster_type = cluster_type,
> .qiov = qiov,
> - .file_cluster_offset = file_cluster_offset,
> + .host_offset = host_offset,
> .offset = offset,
> .bytes = bytes,
> .qiov_offset = qiov_offset,
> @@ -2132,7 +2128,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
>
> trace_qcow2_add_task(qemu_coroutine_self(), bs, pool,
> func == qcow2_co_preadv_task_entry ? "read" : "write",
> - cluster_type, file_cluster_offset, offset, bytes,
> + cluster_type, host_offset, offset, bytes,
Please, update also the trace-point in block/trace-events
> qiov, qiov_offset);
>
> if (!pool) {
> @@ -2146,13 +2142,12 @@ static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
>
Maybe, add comment
/* host_offset: host offset, or cluster descriptor for compressed cluster */
> static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
> QCow2ClusterType cluster_type,
> - uint64_t file_cluster_offset,
> + uint64_t host_offset,
> uint64_t offset, uint64_t bytes,
> QEMUIOVector *qiov,
> size_t qiov_offset)
> {
> BDRVQcow2State *s = bs->opaque;
> - int offset_in_cluster = offset_into_cluster(s, offset);
>
> switch (cluster_type) {
> case QCOW2_CLUSTER_ZERO_PLAIN:
> @@ -2168,19 +2163,17 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
> qiov, qiov_offset, 0);
>
> case QCOW2_CLUSTER_COMPRESSED:
> - return qcow2_co_preadv_compressed(bs, file_cluster_offset,
> + return qcow2_co_preadv_compressed(bs, host_offset,
> offset, bytes, qiov, qiov_offset);
>
> case QCOW2_CLUSTER_NORMAL:
> - assert(offset_into_cluster(s, file_cluster_offset) == 0);
> if (bs->encrypted) {
> - return qcow2_co_preadv_encrypted(bs, file_cluster_offset,
> + return qcow2_co_preadv_encrypted(bs, host_offset,
> offset, bytes, qiov, qiov_offset);
> }
>
> BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
> - return bdrv_co_preadv_part(s->data_file,
> - file_cluster_offset + offset_in_cluster,
> + return bdrv_co_preadv_part(s->data_file, host_offset,
> bytes, qiov, qiov_offset, 0);
>
> default:
> @@ -2196,7 +2189,7 @@ static coroutine_fn int qcow2_co_preadv_task_entry(AioTask *task)
>
> assert(!t->l2meta);
>
> - return qcow2_co_preadv_task(t->bs, t->cluster_type, t->file_cluster_offset,
> + return qcow2_co_preadv_task(t->bs, t->cluster_type, t->host_offset,
> t->offset, t->bytes, t->qiov, t->qiov_offset);
> }
>
> @@ -2232,11 +2225,20 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
> {
> qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes);
> } else {
> + /*
> + * For compressed clusters the variable cluster_offset
> + * does not actually store the offset but the full
> + * descriptor. We need to leave it unchanged because
> + * that's what qcow2_co_preadv_compressed() expects.
> + */
Hmm, good to document it for qcow2_get_cluster_offset function. May be you did it in next patch.
With at least updated trace-event:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.