| Message ID | 20231020215622.789260-6-andrey.drobyshev@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: make subclusters discardable | expand |
On 20.10.23 23:56, Andrey Drobyshev wrote: > When zeroizing subclusters within single cluster, detect usage of the > BDRV_REQ_MAY_UNMAP flag and fall through to the subcluster-based discard > operation, much like it's done with the cluster-based discards. That > way subcluster-aligned operations "qemu-io -c 'write -z -u ...'" will > lead to actual unmap. Ever since the introduction of discard-no-unref, I wonder whether that is actually an advantage or not. I can’t think of a reason why it’d be advantageous to drop the allocation. On the other hand, zero_in_l2_slice() does it indeed, so consistency is a reasonable argument. > Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> > --- > block/qcow2-cluster.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index cf40f2dc12..040251f2c3 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -2242,7 +2242,7 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset, > unsigned nb_subclusters, int flags) > { > BDRVQcow2State *s = bs->opaque; > - uint64_t new_l2_bitmap; > + uint64_t new_l2_bitmap, l2_bitmap_mask; > int ret, sc = offset_to_sc_index(s, offset); > SubClusterRangeInfo scri = { 0 }; > > @@ -2251,9 +2251,10 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset, > goto out; > } > > + l2_bitmap_mask = QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters); “l2_bitmap_mask” already wasn’t a great name in patch 4 because it doesn’t say what mask it is, but in patch 4, there was at least only one relevant mask. Here, we have two (ALLOC_RANGE and ZERO_RANGE), so the name should reflect what kind of mask it is. > new_l2_bitmap = scri.l2_bitmap; > - new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters); > - new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters); > + new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters); > + new_l2_bitmap &= ~l2_bitmap_mask; > > /* > * If there're no non-zero subclusters left, we might as well zeroize > @@ -2266,6 +2267,16 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset, > 1, flags); > } > > + /* > + * If the request allows discarding subclusters and they're actually > + * allocated, we go down the discard path since after the discard > + * operation the subclusters are going to be read as zeroes anyway. > + */ > + if ((flags & BDRV_REQ_MAY_UNMAP) && (scri.l2_bitmap & l2_bitmap_mask)) { > + return discard_l2_subclusters(bs, offset, nb_subclusters, > + QCOW2_DISCARD_REQUEST, false, &scri); > + } I wonder why it matters whether any subclusters are allocated, i.e. why can’t we just immediately use discard_l2_subclusters() whenever BDRV_REQ_MAY_UNMAP is set, without even fetching SCRI (that would also save us passing SCRI here, which I’ve already said on patch 4, I don’t find great). Doesn’t discard_l2_subclusters() guarantee the clusters read as zero when full_discard=false? There is this case where it won’t mark the subclusters as zero if there is no backing file, and none of the subclusters is allocated. But still, (A) those subclusters will read as zero, and (B) if there is a problem there, why don’t we just always mark those subclusters as zero? This optimization only has effect when the guest discards/zeroes subclusters (not whole clusters) that were already discarded, so sounds miniscule. Hanna > + > if (new_l2_bitmap != scri.l2_bitmap) { > set_l2_bitmap(s, scri.l2_slice, scri.l2_index, new_l2_bitmap); > qcow2_cache_entry_mark_dirty(s->l2_table_cache, scri.l2_slice);
On 11/3/23 17:19, Hanna Czenczek wrote: > On 20.10.23 23:56, Andrey Drobyshev wrote: >> When zeroizing subclusters within single cluster, detect usage of the >> BDRV_REQ_MAY_UNMAP flag and fall through to the subcluster-based discard >> operation, much like it's done with the cluster-based discards. That >> way subcluster-aligned operations "qemu-io -c 'write -z -u ...'" will >> lead to actual unmap. > > Ever since the introduction of discard-no-unref, I wonder whether that > is actually an advantage or not. I can’t think of a reason why it’d be > advantageous to drop the allocation. You mean omitting the UNallocation on the discard path? > > On the other hand, zero_in_l2_slice() does it indeed, so consistency is > a reasonable argument. > >> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> >> --- >> block/qcow2-cluster.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> index cf40f2dc12..040251f2c3 100644 >> --- a/block/qcow2-cluster.c >> +++ b/block/qcow2-cluster.c >> @@ -2242,7 +2242,7 @@ zero_l2_subclusters(BlockDriverState *bs, >> uint64_t offset, >> unsigned nb_subclusters, int flags) >> { >> BDRVQcow2State *s = bs->opaque; >> - uint64_t new_l2_bitmap; >> + uint64_t new_l2_bitmap, l2_bitmap_mask; >> int ret, sc = offset_to_sc_index(s, offset); >> SubClusterRangeInfo scri = { 0 }; >> @@ -2251,9 +2251,10 @@ zero_l2_subclusters(BlockDriverState *bs, >> uint64_t offset, >> goto out; >> } >> + l2_bitmap_mask = QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + >> nb_subclusters); > > “l2_bitmap_mask” already wasn’t a great name in patch 4 because it > doesn’t say what mask it is, but in patch 4, there was at least only one > relevant mask. Here, we have two (ALLOC_RANGE and ZERO_RANGE), so the > name should reflect what kind of mask it is. > Noted. >> new_l2_bitmap = scri.l2_bitmap; >> - new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + >> nb_subclusters); >> - new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + >> nb_subclusters); >> + new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters); >> + new_l2_bitmap &= ~l2_bitmap_mask; >> /* >> * If there're no non-zero subclusters left, we might as well >> zeroize >> @@ -2266,6 +2267,16 @@ zero_l2_subclusters(BlockDriverState *bs, >> uint64_t offset, >> 1, flags); >> } >> + /* >> + * If the request allows discarding subclusters and they're actually >> + * allocated, we go down the discard path since after the discard >> + * operation the subclusters are going to be read as zeroes anyway. >> + */ >> + if ((flags & BDRV_REQ_MAY_UNMAP) && (scri.l2_bitmap & >> l2_bitmap_mask)) { >> + return discard_l2_subclusters(bs, offset, nb_subclusters, >> + QCOW2_DISCARD_REQUEST, false, >> &scri); >> + } > > I wonder why it matters whether any subclusters are allocated, i.e. why > can’t we just immediately use discard_l2_subclusters() whenever > BDRV_REQ_MAY_UNMAP is set, without even fetching SCRI (that would also > save us passing SCRI here, which I’ve already said on patch 4, I don’t > find great). > Yes, this way we'd indeed be able to get rid of passing an additional argument. > Doesn’t discard_l2_subclusters() guarantee the clusters read as zero > when full_discard=false? There is this case where it won’t mark the > subclusters as zero if there is no backing file, and none of the > subclusters is allocated. But still, (A) those subclusters will read as > zero, and (B) if there is a problem there, why don’t we just always mark > those subclusters as zero? This optimization only has effect when the > guest discards/zeroes subclusters (not whole clusters) that were already > discarded, so sounds miniscule. > > Hanna > Good question. I also can't think of any issues when zeroizing/discarding already unallocated clusters. Essentially you're saying that zeroizing subclusters is equivalent to discard_l2_subclusters(full_discard=false). Then in discard_l2_subclusters() implementation we may mark the subclusters as zeroes regardless of their allocation status in case of full_discard=false. But when dealing with the whole clusters this logic isn't quite applicable cause if the l2 entry isn't allocated at all there's no point marking it as zero. Correct? > [...]
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index cf40f2dc12..040251f2c3 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -2242,7 +2242,7 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset, unsigned nb_subclusters, int flags) { BDRVQcow2State *s = bs->opaque; - uint64_t new_l2_bitmap; + uint64_t new_l2_bitmap, l2_bitmap_mask; int ret, sc = offset_to_sc_index(s, offset); SubClusterRangeInfo scri = { 0 }; @@ -2251,9 +2251,10 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset, goto out; } + l2_bitmap_mask = QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters); new_l2_bitmap = scri.l2_bitmap; - new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters); - new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters); + new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters); + new_l2_bitmap &= ~l2_bitmap_mask; /* * If there're no non-zero subclusters left, we might as well zeroize @@ -2266,6 +2267,16 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t offset, 1, flags); } + /* + * If the request allows discarding subclusters and they're actually + * allocated, we go down the discard path since after the discard + * operation the subclusters are going to be read as zeroes anyway. + */ + if ((flags & BDRV_REQ_MAY_UNMAP) && (scri.l2_bitmap & l2_bitmap_mask)) { + return discard_l2_subclusters(bs, offset, nb_subclusters, + QCOW2_DISCARD_REQUEST, false, &scri); + } + if (new_l2_bitmap != scri.l2_bitmap) { set_l2_bitmap(s, scri.l2_slice, scri.l2_index, new_l2_bitmap); qcow2_cache_entry_mark_dirty(s->l2_table_cache, scri.l2_slice);
When zeroizing subclusters within single cluster, detect usage of the BDRV_REQ_MAY_UNMAP flag and fall through to the subcluster-based discard operation, much like it's done with the cluster-based discards. That way subcluster-aligned operations "qemu-io -c 'write -z -u ...'" will lead to actual unmap. Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> --- block/qcow2-cluster.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)