qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 01/30] qcow2: Make Qcow2AioTask store the full host offset


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v4 01/30] qcow2: Make Qcow2AioTask store the full host offset
Date: Thu, 9 Apr 2020 09:49:44 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

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 <address@hidden>
---
  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 <address@hidden>

--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]