| Message ID | 6ffd3ee7ca1e53272705ad34d2ea7ac10b50ade0.1584468723.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add subcluster allocation to qcow2 | expand |
On 3/17/20 1:16 PM, 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> > Reviewed-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2-cluster.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index ceacd91ea3..dfd8b66958 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1006,6 +1006,23 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) > assert((offset & L2E_OFFSET_MASK) == offset); > > set_l2_entry(s, l2_slice, l2_index + i, offset | QCOW_OFLAG_COPIED); > + > + /* Update bitmap with the subclusters that were just written */ > + if (has_subclusters(s)) { > + unsigned written_from = m->cow_start.offset; > + unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes ?: > + m->nb_clusters << s->cluster_bits; > + uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i); > + int sc; > + for (sc = 0; sc < s->subclusters_per_cluster; sc++) { > + int 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); > + } > + } Are there more efficient ways to set this series of bits than iterating one bit at a time, while still remaining legible? For example, what if we had something like: l2_bitmap = get_l2_bitmap(...); int sc_from = OFFSET_TO_SC(written_from); int sc_to = OFFSET_TO_SC(written_to - 1); l2_bitmap |= QCOW_OFLAG_SUB_ALLOC_RANGE(sc_from, sc_to); l2_bitmap &= ~QCOW_OFLAG_SUB_ZERO_RANGE(sc_from, sc_to); which would require macros: #define OFFSET_TO_SC(offset) (offset >> (s->cluster_bits - 6)) #define QCOW_OFLAG_SUB_ALLOC_RANGE(from, to) \ deposit64(0, (from), (len) - (from), -1) #define QCOW_OFLAG_SUB_ZERO_RANGE(from, to) \ deposit64(0, (from) + 32, (len) - (from) + 32, -1) > + set_l2_bitmap(s, l2_slice, l2_index + i, l2_bitmap); I'm hoping this function doesn't cause redundant I/O if the L2 entry didn't actually change. But that's not the concern for this patch.
On Fri 24 Apr 2020 09:39:25 PM CEST, Eric Blake wrote: >> + /* Update bitmap with the subclusters that were just written */ >> + if (has_subclusters(s)) { >> + unsigned written_from = m->cow_start.offset; >> + unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes ?: >> + m->nb_clusters << s->cluster_bits; >> + uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i); >> + int sc; >> + for (sc = 0; sc < s->subclusters_per_cluster; sc++) { >> + int 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); >> + } >> + } > > Are there more efficient ways to set this series of bits than iterating > one bit at a time, while still remaining legible? For example, what if > we had something like: > > l2_bitmap = get_l2_bitmap(...); > int sc_from = OFFSET_TO_SC(written_from); > int sc_to = OFFSET_TO_SC(written_to - 1); > l2_bitmap |= QCOW_OFLAG_SUB_ALLOC_RANGE(sc_from, sc_to); > l2_bitmap &= ~QCOW_OFLAG_SUB_ZERO_RANGE(sc_from, sc_to); That's a very good suggestion, thanks! Berto
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index ceacd91ea3..dfd8b66958 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1006,6 +1006,23 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) assert((offset & L2E_OFFSET_MASK) == offset); set_l2_entry(s, l2_slice, l2_index + i, offset | QCOW_OFLAG_COPIED); + + /* Update bitmap with the subclusters that were just written */ + if (has_subclusters(s)) { + unsigned written_from = m->cow_start.offset; + unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes ?: + m->nb_clusters << s->cluster_bits; + uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i); + int sc; + for (sc = 0; sc < s->subclusters_per_cluster; sc++) { + int 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); + } }