| Message ID | b04e7e26cea16892a7f209b37d931c489ef17bd9.1577014346.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add subcluster allocation to qcow2 | expand |
On 12/22/19 5:36 AM, Alberto Garcia wrote: > This patch adds the following new fields to BDRVQcow2State: > > - subclusters_per_cluster: Number of subclusters in a cluster > - subcluster_size: The size of each subcluster, in bytes > - subcluster_bits: No. of bits so 1 << subcluster_bits = subcluster_size > > Images without subclusters are treated as if they had exactly one, > with subcluster_size = cluster_size. The qcow2 spec changes earlier in the series made it sound like your choices are exactly 1 or 32, > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2.c | 5 +++++ > block/qcow2.h | 5 +++++ > 2 files changed, 10 insertions(+) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 3866b47946..cbd857e9c7 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1378,6 +1378,11 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, > } > } > > + s->subclusters_per_cluster = > + has_subclusters(s) ? QCOW_MAX_SUBCLUSTERS_PER_CLUSTER : 1; which matches your code here (other than the name of the constant)... > + s->subcluster_size = s->cluster_size / s->subclusters_per_cluster; > + s->subcluster_bits = ctz32(s->subcluster_size); > + > /* Check support for various header values */ > if (header.refcount_order > 6) { > error_setg(errp, "Reference count entry width too large; may not " > diff --git a/block/qcow2.h b/block/qcow2.h > index 1db3fc5dbc..941330cfc9 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -78,6 +78,8 @@ > /* The cluster reads as all zeros */ > #define QCOW_OFLAG_ZERO (1ULL << 0) > > +#define QCOW_MAX_SUBCLUSTERS_PER_CLUSTER 32 > + ...but this name sounds like other values (2, 4, 8, 16) might be possible? Is this just leftovers from earlier spins of the series before we decided to mandate that clusters must be at least 16k if subclusters are enabled (so that subclusters are at least 512 bytes)? Once we get the right name for the constant, the rest of the patch makes sense.
On 22.12.19 12:36, Alberto Garcia wrote: > This patch adds the following new fields to BDRVQcow2State: > > - subclusters_per_cluster: Number of subclusters in a cluster > - subcluster_size: The size of each subcluster, in bytes > - subcluster_bits: No. of bits so 1 << subcluster_bits = subcluster_size > > Images without subclusters are treated as if they had exactly one, > with subcluster_size = cluster_size. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2.c | 5 +++++ > block/qcow2.h | 5 +++++ > 2 files changed, 10 insertions(+) Reviewed-by: Max Reitz <mreitz@redhat.com>
On Thu 20 Feb 2020 04:28:07 PM CET, Eric Blake wrote: >> Images without subclusters are treated as if they had exactly one, >> with subcluster_size = cluster_size. > > The qcow2 spec changes earlier in the series made it sound like your > choices are exactly 1 or 32, >> +#define QCOW_MAX_SUBCLUSTERS_PER_CLUSTER 32 >> + > > ...but this name sounds like other values (2, 4, 8, 16) might be > possible? I guess I didn't want to call it QCOW_SUBCLUSTERS_PER_CLUSTER because there's already BDRVQcow2State.subclusters_per_cluster. And that one can have two possible values (1 and 32) so 32 would be the maximum. I get your point, however, and I'm open to suggestions. Berto
On 2/20/20 10:34 AM, Alberto Garcia wrote: > On Thu 20 Feb 2020 04:28:07 PM CET, Eric Blake wrote: >>> Images without subclusters are treated as if they had exactly one, >>> with subcluster_size = cluster_size. >> >> The qcow2 spec changes earlier in the series made it sound like your >> choices are exactly 1 or 32, > >>> +#define QCOW_MAX_SUBCLUSTERS_PER_CLUSTER 32 >>> + >> >> ...but this name sounds like other values (2, 4, 8, 16) might be >> possible? > > I guess I didn't want to call it QCOW_SUBCLUSTERS_PER_CLUSTER because > there's already BDRVQcow2State.subclusters_per_cluster. And that one can > have two possible values (1 and 32) so 32 would be the maximum. > > I get your point, however, and I'm open to suggestions. Maybe QCOW_EXTL2_SUBCLUSTERS_PER_CLUSTER since it is a hard-coded property of the EXTL2 feature.
On Thu 20 Feb 2020 05:48:25 PM CET, Eric Blake wrote: >>> The qcow2 spec changes earlier in the series made it sound like your >>> choices are exactly 1 or 32, >> >>>> +#define QCOW_MAX_SUBCLUSTERS_PER_CLUSTER 32 >>>> + >>> >>> ...but this name sounds like other values (2, 4, 8, 16) might be >>> possible? >> >> I guess I didn't want to call it QCOW_SUBCLUSTERS_PER_CLUSTER because >> there's already BDRVQcow2State.subclusters_per_cluster. And that one can >> have two possible values (1 and 32) so 32 would be the maximum. >> >> I get your point, however, and I'm open to suggestions. > > Maybe QCOW_EXTL2_SUBCLUSTERS_PER_CLUSTER > > since it is a hard-coded property of the EXTL2 feature. Sounds good. Berto
diff --git a/block/qcow2.c b/block/qcow2.c index 3866b47946..cbd857e9c7 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1378,6 +1378,11 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, } } + s->subclusters_per_cluster = + has_subclusters(s) ? QCOW_MAX_SUBCLUSTERS_PER_CLUSTER : 1; + s->subcluster_size = s->cluster_size / s->subclusters_per_cluster; + s->subcluster_bits = ctz32(s->subcluster_size); + /* Check support for various header values */ if (header.refcount_order > 6) { error_setg(errp, "Reference count entry width too large; may not " diff --git a/block/qcow2.h b/block/qcow2.h index 1db3fc5dbc..941330cfc9 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -78,6 +78,8 @@ /* The cluster reads as all zeros */ #define QCOW_OFLAG_ZERO (1ULL << 0) +#define QCOW_MAX_SUBCLUSTERS_PER_CLUSTER 32 + #define MIN_CLUSTER_BITS 9 #define MAX_CLUSTER_BITS 21 @@ -284,6 +286,9 @@ typedef struct BDRVQcow2State { int cluster_bits; int cluster_size; int l2_slice_size; + int subcluster_bits; + int subcluster_size; + int subclusters_per_cluster; int l2_bits; int l2_size; int l1_size;
This patch adds the following new fields to BDRVQcow2State: - subclusters_per_cluster: Number of subclusters in a cluster - subcluster_size: The size of each subcluster, in bytes - subcluster_bits: No. of bits so 1 << subcluster_bits = subcluster_size Images without subclusters are treated as if they had exactly one, with subcluster_size = cluster_size. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qcow2.c | 5 +++++ block/qcow2.h | 5 +++++ 2 files changed, 10 insertions(+)