| Message ID | 289ea5edc3f1530787c8fe905b1a524cc48945a9.1572125022.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add subcluster allocation to qcow2 | expand |
On 26.10.19 23:25, Alberto Garcia wrote: > The L2 bitmap needs to be updated after each write to indicate what > new subclusters are now allocated. > > This needs to happen even if the cluster was already allocated and the > L2 entry was otherwise valid. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2-cluster.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index fb6cf8df17..acb7226e03 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -980,6 +980,22 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) > > set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_COPIED | > (cluster_offset + (i << s->cluster_bits))); > + > + /* Update bitmap with the subclusters that were just written */ > + if (has_subclusters(s)) { > + uint64_t written_from = m->cow_start.offset; > + uint64_t written_to = m->cow_end.offset + m->cow_end.nb_bytes; It’s a bit strange to make these uint64_t when all the fields queried are of type unsigned, but more on that below. > + uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i); > + int sc; > + for (sc = 0; sc < s->subclusters_per_cluster; sc++) { > + uint64_t sc_off = i * s->cluster_size + sc * s->subcluster_size; It’s weird to give this a uint64_t type when all the variables in the term are of type int. I’m not sure whether it can overflow. handle_alloc() limits everything to INT_MAX, but I’m not sure about handle_copied(). Speaking of handle_copied(); both elements of Qcow2COWRegion are of type unsigned. handle_copied() doesn’t look like it takes any precautions to limit the range to even UINT_MAX (and it should probably limit it to INT_MAX). Max > + if (sc_off >= written_from && sc_off < written_to) { > + l2_bitmap |= QCOW_OFLAG_SUB_ALLOC(sc); > + l2_bitmap &= ~QCOW_OFLAG_SUB_ZERO(sc); > + } > + } > + set_l2_bitmap(s, l2_slice, l2_index + i, l2_bitmap); > + } > } > > >
On Tue 05 Nov 2019 12:43:16 PM CET, Max Reitz wrote: > Speaking of handle_copied(); both elements of Qcow2COWRegion are of > type unsigned. handle_copied() doesn’t look like it takes any > precautions to limit the range to even UINT_MAX (and it should > probably limit it to INT_MAX). Or rather, both handle_copied() and handle_alloc() should probably limit it to BDRV_REQUEST_MAX_BYTES. Berto
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index fb6cf8df17..acb7226e03 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -980,6 +980,22 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_COPIED | (cluster_offset + (i << s->cluster_bits))); + + /* Update bitmap with the subclusters that were just written */ + if (has_subclusters(s)) { + uint64_t written_from = m->cow_start.offset; + uint64_t written_to = m->cow_end.offset + m->cow_end.nb_bytes; + uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i); + int sc; + for (sc = 0; sc < s->subclusters_per_cluster; sc++) { + uint64_t sc_off = i * s->cluster_size + sc * s->subcluster_size; + if (sc_off >= written_from && sc_off < written_to) { + l2_bitmap |= QCOW_OFLAG_SUB_ALLOC(sc); + l2_bitmap &= ~QCOW_OFLAG_SUB_ZERO(sc); + } + } + set_l2_bitmap(s, l2_slice, l2_index + i, l2_bitmap); + } }
The L2 bitmap needs to be updated after each write to indicate what new subclusters are now allocated. This needs to happen even if the cluster was already allocated and the L2 entry was otherwise valid. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qcow2-cluster.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)