diff mbox series

[RFC,v2,12/26] qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER

Message ID 2a6b34635cac78e76150a72c69669b3d9ec0fb8c.1572125022.git.berto@igalia.com (mailing list archive)
State New, archived
Headers show
Series Add subcluster allocation to qcow2 | expand

Commit Message

Alberto Garcia Oct. 26, 2019, 9:25 p.m. UTC
In the previous patch we added a new QCow2ClusterType named
QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER. There is a couple of places
where this new value needs to be handled, and that is what this patch
does.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Max Reitz Nov. 4, 2019, 12:57 p.m. UTC | #1
On 26.10.19 23:25, Alberto Garcia wrote:
> In the previous patch we added a new QCow2ClusterType named
> QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER. There is a couple of places
> where this new value needs to be handled, and that is what this patch
> does.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
This patch deals with everything in qcow2.c.  There are more places that
reference QCOW2_CLUSTER_* constants elsewhere, and I suppose most of
them are handled by the following patches.

But I wonder what the criterion is on where it needs to be handled and
where it’s OK not to.  Right now it looks to me like it’s a bit
arbitrary maybe?  But I suppose I’ll just have to wait until after the
next patches.

Max
Alberto Garcia Nov. 4, 2019, 1:03 p.m. UTC | #2
On Mon 04 Nov 2019 01:57:42 PM CET, Max Reitz wrote:
> On 26.10.19 23:25, Alberto Garcia wrote:
>> In the previous patch we added a new QCow2ClusterType named
>> QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER. There is a couple of places
>> where this new value needs to be handled, and that is what this patch
>> does.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/qcow2.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
> This patch deals with everything in qcow2.c.  There are more places that
> reference QCOW2_CLUSTER_* constants elsewhere, and I suppose most of
> them are handled by the following patches.
>
> But I wonder what the criterion is on where it needs to be handled and
> where it’s OK not to.  Right now it looks to me like it’s a bit
> arbitrary maybe?  But I suppose I’ll just have to wait until after the
> next patches.

This is the part of the series that I'm the least happy about, because
the existing qcow2_get_cluster_type() can never return this new value, I
only updated the cases where this can actually happen.

I'm still considering a different approach for this.

Berto
Max Reitz Nov. 4, 2019, 1:10 p.m. UTC | #3
On 04.11.19 14:03, Alberto Garcia wrote:
> On Mon 04 Nov 2019 01:57:42 PM CET, Max Reitz wrote:
>> On 26.10.19 23:25, Alberto Garcia wrote:
>>> In the previous patch we added a new QCow2ClusterType named
>>> QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER. There is a couple of places
>>> where this new value needs to be handled, and that is what this patch
>>> does.
>>>
>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>>  block/qcow2.c | 13 +++++++++----
>>>  1 file changed, 9 insertions(+), 4 deletions(-)
>> This patch deals with everything in qcow2.c.  There are more places that
>> reference QCOW2_CLUSTER_* constants elsewhere, and I suppose most of
>> them are handled by the following patches.
>>
>> But I wonder what the criterion is on where it needs to be handled and
>> where it’s OK not to.  Right now it looks to me like it’s a bit
>> arbitrary maybe?  But I suppose I’ll just have to wait until after the
>> next patches.
> 
> This is the part of the series that I'm the least happy about, because
> the existing qcow2_get_cluster_type() can never return this new value, I
> only updated the cases where this can actually happen.
> 
> I'm still considering a different approach for this.
I still don’t know what you’re doing in the later patches, but to me it
looks a bit like you don’t dare breaking up the existing structure that
just deals with clusters.

If that is so, I think it will help to make a clear cut between what
concerns subclusters and what concerns clusters at a whole.
QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER shouldn’t be in QCow2ClusterType;
there should be a separate QCow2SubclusterType.

OTOH, that would require more modifications, but (naïvely) I believe
that would make for the cleaner interface in the end.

Max
Alberto Garcia Nov. 7, 2019, 2:44 p.m. UTC | #4
On Mon 04 Nov 2019 02:10:37 PM CET, Max Reitz wrote:

   [QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER]
> I still don’t know what you’re doing in the later patches, but to me
> it looks a bit like you don’t dare breaking up the existing structure
> that just deals with clusters.

Yeah, I decided to extend the existing type to make the changes less
invasive, but I'm also leaning towards creating a different type
now. I'll give it a try.

Berto
diff mbox series

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index ab40ae36ea..0261e87709 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1938,8 +1938,8 @@  static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
 
     *pnum = bytes;
 
-    if ((ret == QCOW2_CLUSTER_NORMAL || ret == QCOW2_CLUSTER_ZERO_ALLOC) &&
-        !s->crypto) {
+    if ((ret == QCOW2_CLUSTER_NORMAL || ret == QCOW2_CLUSTER_ZERO_ALLOC ||
+         ret == QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER) && !s->crypto) {
         index_in_cluster = offset & (s->cluster_size - 1);
         *map = cluster_offset | index_in_cluster;
         *file = s->data_file->bs;
@@ -1947,7 +1947,8 @@  static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
     }
     if (ret == QCOW2_CLUSTER_ZERO_PLAIN || ret == QCOW2_CLUSTER_ZERO_ALLOC) {
         status |= BDRV_BLOCK_ZERO;
-    } else if (ret != QCOW2_CLUSTER_UNALLOCATED) {
+    } else if (ret != QCOW2_CLUSTER_UNALLOCATED &&
+               ret != QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER) {
         status |= BDRV_BLOCK_DATA;
     }
     if (s->metadata_preallocation && (status & BDRV_BLOCK_DATA) &&
@@ -2117,6 +2118,7 @@  static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
         g_assert_not_reached();
 
     case QCOW2_CLUSTER_UNALLOCATED:
+    case QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER:
         assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
 
         BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
@@ -2187,7 +2189,8 @@  static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
 
         if (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
             ret == QCOW2_CLUSTER_ZERO_ALLOC ||
-            (ret == QCOW2_CLUSTER_UNALLOCATED && !bs->backing))
+            (ret == QCOW2_CLUSTER_UNALLOCATED && !bs->backing) ||
+            (ret == QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER && !bs->backing))
         {
             qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes);
         } else {
@@ -3701,6 +3704,7 @@  static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
         nr = s->cluster_size;
         ret = qcow2_get_cluster_offset(bs, offset, &nr, &off);
         if (ret != QCOW2_CLUSTER_UNALLOCATED &&
+            ret != QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER &&
             ret != QCOW2_CLUSTER_ZERO_PLAIN &&
             ret != QCOW2_CLUSTER_ZERO_ALLOC) {
             qemu_co_mutex_unlock(&s->lock);
@@ -3771,6 +3775,7 @@  qcow2_co_copy_range_from(BlockDriverState *bs,
 
         switch (ret) {
         case QCOW2_CLUSTER_UNALLOCATED:
+        case QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER:
             if (bs->backing && bs->backing->bs) {
                 int64_t backing_length = bdrv_getlength(bs->backing->bs);
                 if (src_offset >= backing_length) {