qemu-devel
[Top][All Lists]
Advanced

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

Re: [PATCH v4 02/30] qcow2: Convert qcow2_get_cluster_offset() into qcow


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v4 02/30] qcow2: Convert qcow2_get_cluster_offset() into qcow2_get_host_offset()
Date: Thu, 9 Apr 2020 10:50:52 +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:
qcow2_get_cluster_offset() takes an (unaligned) guest offset and
returns the (aligned) offset of the corresponding cluster in the qcow2
image.

In practice none of the callers need to know where the cluster starts
so this patch makes the function calculate and return the final host
offset directly. The function is also renamed accordingly.

Great that you rename functions and variables which change their behavior, it 
simplifies reviewing!


There is a pre-existing exception with compressed clusters: in this
case the function returns the complete cluster descriptor (containing
the offset and size of the compressed data). This does not change with
this patch but it is now documented.

Signed-off-by: Alberto Garcia <address@hidden>

[..]

-int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
-                             unsigned int *bytes, uint64_t *cluster_offset)
+int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
+                          unsigned int *bytes, uint64_t *host_offset)
  {
      BDRVQcow2State *s = bs->opaque;
      unsigned int l2_index;
-    uint64_t l1_index, l2_offset, *l2_slice;
+    uint64_t l1_index, l2_offset, *l2_slice, l2_entry;
      int c;
      unsigned int offset_in_cluster;
      uint64_t bytes_available, bytes_needed, nb_clusters;
@@ -537,7 +542,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
          bytes_needed = bytes_available;
      }
- *cluster_offset = 0;
+    *host_offset = 0;
/* seek to the l2 offset in the l1 table */ @@ -570,7 +575,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
      /* find the cluster offset for the given disk offset */
l2_index = offset_to_l2_slice_index(s, offset);
-    *cluster_offset = be64_to_cpu(l2_slice[l2_index]);
+    l2_entry = be64_to_cpu(l2_slice[l2_index]);
nb_clusters = size_to_clusters(s, bytes_needed);
      /* bytes_needed <= *bytes + offset_in_cluster, both of which are unsigned
@@ -578,7 +583,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
       * true */
      assert(nb_clusters <= INT_MAX);
- type = qcow2_get_cluster_type(bs, *cluster_offset);
+    type = qcow2_get_cluster_type(bs, l2_entry);
      if (s->qcow_version < 3 && (type == QCOW2_CLUSTER_ZERO_PLAIN ||
                                  type == QCOW2_CLUSTER_ZERO_ALLOC)) {
          qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry found"
@@ -599,41 +604,42 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, 
uint64_t offset,
          }
          /* Compressed clusters can only be processed one by one */
          c = 1;
-        *cluster_offset &= L2E_COMPRESSED_OFFSET_SIZE_MASK;
+        *host_offset = l2_entry & L2E_COMPRESSED_OFFSET_SIZE_MASK;
          break;
      case QCOW2_CLUSTER_ZERO_PLAIN:
      case QCOW2_CLUSTER_UNALLOCATED:
          /* how many empty clusters ? */
          c = count_contiguous_clusters_unallocated(bs, nb_clusters,
                                                    &l2_slice[l2_index], type);
-        *cluster_offset = 0;
+        *host_offset = 0;

Actually, dead assignment now.. But I feel that better to keep it.

Hmm. May be, drop the first assignment of zero to host_offset? We actually 
don't need it, user should not rely on host_offset if we return an error.

          break;
      case QCOW2_CLUSTER_ZERO_ALLOC:
      case QCOW2_CLUSTER_NORMAL:
          /* how many allocated clusters ? */
          c = count_contiguous_clusters(bs, nb_clusters, s->cluster_size,
                                        &l2_slice[l2_index], QCOW_OFLAG_ZERO);
-        *cluster_offset &= L2E_OFFSET_MASK;
-        if (offset_into_cluster(s, *cluster_offset)) {
+        *host_offset = l2_entry & L2E_OFFSET_MASK;
+        if (offset_into_cluster(s, *host_offset)) {
              qcow2_signal_corruption(bs, true, -1, -1,
                                      "Cluster allocation offset %#"
                                      PRIx64 " unaligned (L2 offset: %#" PRIx64
-                                    ", L2 index: %#x)", *cluster_offset,
+                                    ", L2 index: %#x)", *host_offset,
                                      l2_offset, l2_index);
              ret = -EIO;
              goto fail;
          }
-        if (has_data_file(bs) && *cluster_offset != offset - offset_in_cluster)

[..]

@@ -3735,7 +3726,7 @@ static coroutine_fn int 
qcow2_co_pwrite_zeroes(BlockDriverState *bs,
          offset = QEMU_ALIGN_DOWN(offset, s->cluster_size);
          bytes = s->cluster_size;

Unrelated to the patch, but.. Why we change bytes?? So, we can finish with 
success, but zero-out only first cluster?

Ah, found, generic block-layer take care of it and never issue unaligned 
requests crossing cluster boundary.

          nr = s->cluster_size;
-        ret = qcow2_get_cluster_offset(bs, offset, &nr, &off);
+        ret = qcow2_get_host_offset(bs, offset, &nr, &off);
          if (ret != QCOW2_CLUSTER_UNALLOCATED &&
              ret != QCOW2_CLUSTER_ZERO_PLAIN &&
              ret != QCOW2_CLUSTER_ZERO_ALLOC) {
@@ -3800,7 +3791,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
          cur_bytes = MIN(bytes, INT_MAX);
          cur_write_flags = write_flags;
- ret = qcow2_get_cluster_offset(bs, src_offset, &cur_bytes, &copy_offset);
+        ret = qcow2_get_host_offset(bs, src_offset, &cur_bytes, &copy_offset);
          if (ret < 0) {
              goto out;
          }
@@ -3832,7 +3823,6 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
case QCOW2_CLUSTER_NORMAL:
              child = s->data_file;
-            copy_offset += offset_into_cluster(s, src_offset);
              break;
default:


with or without first assignment dropped:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

--
Best regards,
Vladimir



reply via email to

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