[v5,13/31] qcow2: Update get/set_l2_entry() and add get/set_l2_bitmap()

Commit Message

Alberto Garcia May 5, 2020, 5:38 p.m. UTC

Extended L2 entries are 128-bit wide: 64 bits for the entry itself and
64 bits for the subcluster allocation bitmap.

In order to support them correctly get/set_l2_entry() need to be
updated so they take the entry width into account in order to
calculate the correct offset.

This patch also adds the get/set_l2_bitmap() functions that are
used to access the bitmaps. For convenience we allow calling
get_l2_bitmap() on images without subclusters. In this case the
returned value is always 0 and has no meaning.

Signed-off-by: Alberto Garcia <[e-mail]>
---
 block/qcow2.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Eric Blake May 5, 2020, 8:04 p.m. UTC | #1

On 5/5/20 12:38 PM, Alberto Garcia wrote:
> Extended L2 entries are 128-bit wide: 64 bits for the entry itself and
> 64 bits for the subcluster allocation bitmap.
> 
> In order to support them correctly get/set_l2_entry() need to be
> updated so they take the entry width into account in order to
> calculate the correct offset.
> 
> This patch also adds the get/set_l2_bitmap() functions that are
> used to access the bitmaps. For convenience we allow calling
> get_l2_bitmap() on images without subclusters. In this case the
> returned value is always 0 and has no meaning.
> 
> Signed-off-by: Alberto Garcia <[e-mail]>
> ---
>   block/qcow2.h | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 

> +static inline void set_l2_bitmap(BDRVQcow2State *s, uint64_t *l2_slice,
> +                                 int idx, uint64_t bitmap)
> +{
> +    assert(has_subclusters(s));
> +    idx *= l2_entry_size(s) / sizeof(uint64_t);
> +    l2_slice[idx + 1] = cpu_to_be64(bitmap);
> +}

Unrelated to this patch, but I just thought of it:

What happens for an image whose size is not cluster-aligned?  Must the 
bits corresponding to subclusters not present in the final cluster 
always be zero, or are they instead ignored regardless of value?

But for this patch:

Reviewed-by: Eric Blake <[e-mail]>

Alberto Garcia May 6, 2020, 12:06 p.m. UTC | #2

On Tue 05 May 2020 10:04:32 PM CEST, Eric Blake wrote:
> What happens for an image whose size is not cluster-aligned?  Must the
> bits corresponding to subclusters not present in the final cluster
> always be zero, or are they instead ignored regardless of value?

Attempting to read or write beyond the end of the image returns an error
(see blk_check_byte_request()).

But let's say we have a 32k image (only 1/2 of the first cluster is
used) and we manipulate the bitmap to mark all subclusters as allocated.

In this case if you try 'read 0 32k' then count_contiguous_subclusters()
would indeed say that there are 64k of data available. However the
caller knows that we only want 32k and

    if (bytes_available > bytes_needed) {
        bytes_available = bytes_needed;
    }

so in the end it doesn't really matter.

I think in practice it's the same as with traditional qcow2 images: once
we reach qcow2_get_host_offset() the code does not really know or care
about the size of the image or how much of the last cluster contains
actual data. It only cares about how much data the caller needs. The
limits have already been checked before.

But we could document it: "if the image size is not a multiple of the
cluster size then the bits corresponding to the subclusters beyond the
end of the image are ignored and should be set to zero", or something
like that.

Berto

Patch

diff --git a/block/qcow2.h b/block/qcow2.h
index 80ceb352c9..4ad93772b9 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -515,15 +515,36 @@  static inline size_t l2_entry_size(BDRVQcow2State *s)
 static inline uint64_t get_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice,
                                     int idx)
 {
+    idx *= l2_entry_size(s) / sizeof(uint64_t);
     return be64_to_cpu(l2_slice[idx]);
 }
 
+static inline uint64_t get_l2_bitmap(BDRVQcow2State *s, uint64_t *l2_slice,
+                                     int idx)
+{
+    if (has_subclusters(s)) {
+        idx *= l2_entry_size(s) / sizeof(uint64_t);
+        return be64_to_cpu(l2_slice[idx + 1]);
+    } else {
+        return 0; /* For convenience only; this value has no meaning. */
+    }
+}
+
 static inline void set_l2_entry(BDRVQcow2State *s, uint64_t *l2_slice,
                                 int idx, uint64_t entry)
 {
+    idx *= l2_entry_size(s) / sizeof(uint64_t);
     l2_slice[idx] = cpu_to_be64(entry);
 }
 
+static inline void set_l2_bitmap(BDRVQcow2State *s, uint64_t *l2_slice,
+                                 int idx, uint64_t bitmap)
+{
+    assert(has_subclusters(s));
+    idx *= l2_entry_size(s) / sizeof(uint64_t);
+    l2_slice[idx + 1] = cpu_to_be64(bitmap);
+}
+
 static inline bool has_data_file(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;