Re: [RFC PATCH v2 12/26] qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER
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
Re: [RFC PATCH v2 12/26] qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER
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 >>> --- >>> 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 signature.asc Description: OpenPGP digital signature
Re: [RFC PATCH v2 12/26] qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER
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 >> --- >> 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
Re: [RFC PATCH v2 12/26] qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER
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 > --- > 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 signature.asc Description: OpenPGP digital signature
[RFC PATCH v2 12/26] qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER
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
---
block/qcow2.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
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) {
--
2.20.1
