| Message ID | d7c9c2d54c7be83eda854db37e54dd7aabb1a1e1.1577014346.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add subcluster allocation to qcow2 | expand |
On 22.12.19 12:37, Alberto Garcia wrote: > Two changes are needed in order to add subcluster support to this > function: deallocated clusters must have their bitmaps cleared, and > expanded clusters must have all the "subcluster allocated" bits set. Not really, to have real subcluster support it would need to be expand_zero_subclusters_in_l1(). Right now it can only deal with full zero clusters, which will actually never happen for images with subclusters. As noted in v2, this function is only called when downgrading qcow2 images to v2. It kind of made sense to just call set_l2_bitmap() in v2, but now with the if () conditional... I suppose it may make more sense to assert that the image does not have subclusters at the beginning of the function and be done with it. OTOH, well, this does make ensuring that we have subcluster “support” everywhere a bit easier because this way all set_l2_entry() calls are accompanied by an “if (subclusters) { set_l2_bitmap() }” part. But it is dead code. Max > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2-cluster.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 207f670c94..ede75138d2 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -2054,6 +2054,9 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, > /* not backed; therefore we can simply deallocate the > * cluster */ > set_l2_entry(s, l2_slice, j, 0); > + if (has_subclusters(s)) { > + set_l2_bitmap(s, l2_slice, j, 0); > + } > l2_dirty = true; > continue; > } > @@ -2120,6 +2123,9 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, > } else { > set_l2_entry(s, l2_slice, j, offset); > } > + if (has_subclusters(s)) { > + set_l2_bitmap(s, l2_slice, j, QCOW_L2_BITMAP_ALL_ALLOC); > + } > l2_dirty = true; > } > >
On Fri 21 Feb 2020 03:57:27 PM CET, Max Reitz wrote: > As noted in v2, this function is only called when downgrading qcow2 > images to v2. It kind of made sense to just call set_l2_bitmap() in > v2, but now with the if () conditional... I suppose it may make more > sense to assert that the image does not have subclusters at the > beginning of the function and be done with it. Hmmm, you're right. > OTOH, well, this does make ensuring that we have subcluster “support” > everywhere a bit easier because this way all set_l2_entry() calls are > accompanied by an “if (subclusters) { set_l2_bitmap() }” part. Another alternative is to assert that the image does not have subcluster but still leave a comment after both set_l2_entry() calls explaining why there's no need to touch the bitmap. I think I'll do that, unless you have a different proposal. Berto
On 26.02.20 18:19, Alberto Garcia wrote: > On Fri 21 Feb 2020 03:57:27 PM CET, Max Reitz wrote: >> As noted in v2, this function is only called when downgrading qcow2 >> images to v2. It kind of made sense to just call set_l2_bitmap() in >> v2, but now with the if () conditional... I suppose it may make more >> sense to assert that the image does not have subclusters at the >> beginning of the function and be done with it. > > Hmmm, you're right. > >> OTOH, well, this does make ensuring that we have subcluster “support” >> everywhere a bit easier because this way all set_l2_entry() calls are >> accompanied by an “if (subclusters) { set_l2_bitmap() }” part. > > Another alternative is to assert that the image does not have subcluster > but still leave a comment after both set_l2_entry() calls explaining why > there's no need to touch the bitmap. Sounds good. Max
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 207f670c94..ede75138d2 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -2054,6 +2054,9 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, /* not backed; therefore we can simply deallocate the * cluster */ set_l2_entry(s, l2_slice, j, 0); + if (has_subclusters(s)) { + set_l2_bitmap(s, l2_slice, j, 0); + } l2_dirty = true; continue; } @@ -2120,6 +2123,9 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, } else { set_l2_entry(s, l2_slice, j, offset); } + if (has_subclusters(s)) { + set_l2_bitmap(s, l2_slice, j, QCOW_L2_BITMAP_ALL_ALLOC); + } l2_dirty = true; }
Two changes are needed in order to add subcluster support to this function: deallocated clusters must have their bitmaps cleared, and expanded clusters must have all the "subcluster allocated" bits set. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qcow2-cluster.c | 6 ++++++ 1 file changed, 6 insertions(+)