| Message ID | 2a6b34635cac78e76150a72c69669b3d9ec0fb8c.1572125022.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add subcluster allocation to qcow2 | expand |
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
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
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
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 --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) {
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(-)