* [PATCH 0/2] qcow2: queue discards when discard-no-unref enabled @ 2025-04-29 10:31 Jean-Louis Dupond 2025-04-29 10:31 ` [PATCH 1/2] qcow2: rename update_refcount_discard to queue_discard Jean-Louis Dupond 2025-04-29 10:31 ` [PATCH 2/2] qcow2: put discards in discard queue when discard-no-unref is enabled Jean-Louis Dupond 0 siblings, 2 replies; 5+ messages in thread From: Jean-Louis Dupond @ 2025-04-29 10:31 UTC (permalink / raw) To: qemu-devel Cc: Alexander Ivanov, Kevin Wolf, Andrey Drobyshev, Hanna Reitz, qemu-block, Jean-Louis Dupond Partially based on the proposal of Andrey in https://patchew.org/QEMU/20240913163942.423050-1-andrey.drobyshev@virtuozzo.com/ Split up this from the rest might get it merged a bit quicker hopefully :) Since the implementation of discard-no-unref, we did not queue the discards correctly when discard-no-unref was enabled. Jean-Louis Dupond (2): qcow2: rename update_refcount_discard to queue_discard qcow2: put discards in discard queue when discard-no-unref is enabled block/qcow2-cluster.c | 16 ++++++---------- block/qcow2-refcount.c | 25 +++++++++++++++++++++---- block/qcow2.h | 4 ++++ 3 files changed, 31 insertions(+), 14 deletions(-) -- 2.49.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] qcow2: rename update_refcount_discard to queue_discard 2025-04-29 10:31 [PATCH 0/2] qcow2: queue discards when discard-no-unref enabled Jean-Louis Dupond @ 2025-04-29 10:31 ` Jean-Louis Dupond 2025-04-29 10:31 ` [PATCH 2/2] qcow2: put discards in discard queue when discard-no-unref is enabled Jean-Louis Dupond 1 sibling, 0 replies; 5+ messages in thread From: Jean-Louis Dupond @ 2025-04-29 10:31 UTC (permalink / raw) To: qemu-devel Cc: Alexander Ivanov, Kevin Wolf, Andrey Drobyshev, Hanna Reitz, qemu-block, Jean-Louis Dupond The function just queues discards, and doesn't do any refcount change. So let's change the function name to align with its function. Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be> --- block/qcow2-refcount.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 0266542cee..d796018970 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -754,8 +754,8 @@ void qcow2_process_discards(BlockDriverState *bs, int ret) } } -static void update_refcount_discard(BlockDriverState *bs, - uint64_t offset, uint64_t length) +static void queue_discard(BlockDriverState *bs, + uint64_t offset, uint64_t length) { BDRVQcow2State *s = bs->opaque; Qcow2DiscardRegion *d, *p, *next; @@ -902,7 +902,7 @@ update_refcount(BlockDriverState *bs, int64_t offset, int64_t length, } if (s->discard_passthrough[type]) { - update_refcount_discard(bs, cluster_offset, s->cluster_size); + queue_discard(bs, cluster_offset, s->cluster_size); } } } -- 2.49.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] qcow2: put discards in discard queue when discard-no-unref is enabled 2025-04-29 10:31 [PATCH 0/2] qcow2: queue discards when discard-no-unref enabled Jean-Louis Dupond 2025-04-29 10:31 ` [PATCH 1/2] qcow2: rename update_refcount_discard to queue_discard Jean-Louis Dupond @ 2025-04-29 10:31 ` Jean-Louis Dupond 2025-05-12 12:28 ` Hanna Czenczek 1 sibling, 1 reply; 5+ messages in thread From: Jean-Louis Dupond @ 2025-04-29 10:31 UTC (permalink / raw) To: qemu-devel Cc: Alexander Ivanov, Kevin Wolf, Andrey Drobyshev, Hanna Reitz, qemu-block, Jean-Louis Dupond When discard-no-unref is enabled, discards are not queued like it should. This was broken since discard-no-unref was added. Add some helper function qcow2_discard_cluster which handles some common checks and calls the queue_discards function if needed to add the discard request to the queue. Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be> --- block/qcow2-cluster.c | 16 ++++++---------- block/qcow2-refcount.c | 19 ++++++++++++++++++- block/qcow2.h | 4 ++++ 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index ce8c0076b3..c655bf6df4 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1978,12 +1978,10 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters, if (!keep_reference) { /* Then decrease the refcount */ qcow2_free_any_cluster(bs, old_l2_entry, type); - } else if (s->discard_passthrough[type] && - (cluster_type == QCOW2_CLUSTER_NORMAL || - cluster_type == QCOW2_CLUSTER_ZERO_ALLOC)) { + } else { /* If we keep the reference, pass on the discard still */ - bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK, - s->cluster_size); + qcow2_discard_cluster(bs, old_l2_entry & L2E_OFFSET_MASK, + s->cluster_size, cluster_type, type); } } @@ -2092,12 +2090,10 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset, if (!keep_reference) { /* Then decrease the refcount */ qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST); - } else if (s->discard_passthrough[QCOW2_DISCARD_REQUEST] && - (type == QCOW2_CLUSTER_NORMAL || - type == QCOW2_CLUSTER_ZERO_ALLOC)) { + } else { /* If we keep the reference, pass on the discard still */ - bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK, - s->cluster_size); + qcow2_discard_cluster(bs, old_l2_entry & L2E_OFFSET_MASK, + s->cluster_size, type, QCOW2_DISCARD_REQUEST); } } } diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index d796018970..e1f830504d 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1205,6 +1205,23 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry, } } +void qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset, + uint64_t length, QCow2ClusterType ctype, + enum qcow2_discard_type dtype) { + + BDRVQcow2State *s = bs->opaque; + + if (s->discard_passthrough[dtype] && + (ctype == QCOW2_CLUSTER_NORMAL || + ctype == QCOW2_CLUSTER_ZERO_ALLOC)) { + if (has_data_file(bs)) { + bdrv_pdiscard(s->data_file, offset, length); + } else { + queue_discard(bs, offset, length); + } + } +} + int qcow2_write_caches(BlockDriverState *bs) { BDRVQcow2State *s = bs->opaque; @@ -3619,7 +3636,7 @@ qcow2_discard_refcount_block(BlockDriverState *bs, uint64_t discard_block_offs) /* discard refblock from the cache if refblock is cached */ qcow2_cache_discard(s->refcount_block_cache, refblock); } - update_refcount_discard(bs, discard_block_offs, s->cluster_size); + queue_discard(bs, discard_block_offs, s->cluster_size); return 0; } diff --git a/block/qcow2.h b/block/qcow2.h index a9e3481c6e..547bb2b814 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -880,6 +880,10 @@ void GRAPH_RDLOCK qcow2_free_clusters(BlockDriverState *bs, void GRAPH_RDLOCK qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry, enum qcow2_discard_type type); +void GRAPH_RDLOCK +qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset, + uint64_t length, QCow2ClusterType ctype, + enum qcow2_discard_type dtype); int GRAPH_RDLOCK qcow2_update_snapshot_refcount(BlockDriverState *bs, int64_t l1_table_offset, -- 2.49.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] qcow2: put discards in discard queue when discard-no-unref is enabled 2025-04-29 10:31 ` [PATCH 2/2] qcow2: put discards in discard queue when discard-no-unref is enabled Jean-Louis Dupond @ 2025-05-12 12:28 ` Hanna Czenczek 2025-05-13 13:27 ` Jean-Louis Dupond 0 siblings, 1 reply; 5+ messages in thread From: Hanna Czenczek @ 2025-05-12 12:28 UTC (permalink / raw) To: Jean-Louis Dupond, qemu-devel Cc: Alexander Ivanov, Kevin Wolf, Andrey Drobyshev, qemu-block On 29.04.25 12:31, Jean-Louis Dupond wrote: > When discard-no-unref is enabled, discards are not queued like it > should. > This was broken since discard-no-unref was added. > > Add some helper function qcow2_discard_cluster which handles some common > checks and calls the queue_discards function if needed to add the > discard request to the queue. > > Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be> > --- > block/qcow2-cluster.c | 16 ++++++---------- > block/qcow2-refcount.c | 19 ++++++++++++++++++- > block/qcow2.h | 4 ++++ > 3 files changed, 28 insertions(+), 11 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index ce8c0076b3..c655bf6df4 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1978,12 +1978,10 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters, > if (!keep_reference) { > /* Then decrease the refcount */ > qcow2_free_any_cluster(bs, old_l2_entry, type); > - } else if (s->discard_passthrough[type] && > - (cluster_type == QCOW2_CLUSTER_NORMAL || > - cluster_type == QCOW2_CLUSTER_ZERO_ALLOC)) { > + } else { > /* If we keep the reference, pass on the discard still */ > - bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK, > - s->cluster_size); > + qcow2_discard_cluster(bs, old_l2_entry & L2E_OFFSET_MASK, > + s->cluster_size, cluster_type, type); > } > } > > @@ -2092,12 +2090,10 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset, > if (!keep_reference) { > /* Then decrease the refcount */ > qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST); > - } else if (s->discard_passthrough[QCOW2_DISCARD_REQUEST] && > - (type == QCOW2_CLUSTER_NORMAL || > - type == QCOW2_CLUSTER_ZERO_ALLOC)) { > + } else { > /* If we keep the reference, pass on the discard still */ > - bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK, > - s->cluster_size); > + qcow2_discard_cluster(bs, old_l2_entry & L2E_OFFSET_MASK, > + s->cluster_size, type, QCOW2_DISCARD_REQUEST); > } > } > } > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index d796018970..e1f830504d 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1205,6 +1205,23 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry, > } > } > > +void qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset, > + uint64_t length, QCow2ClusterType ctype, > + enum qcow2_discard_type dtype) { > + QEMU coding style requires putting the { on a separate line. > + BDRVQcow2State *s = bs->opaque; > + > + if (s->discard_passthrough[dtype] && > + (ctype == QCOW2_CLUSTER_NORMAL || > + ctype == QCOW2_CLUSTER_ZERO_ALLOC)) { > + if (has_data_file(bs)) { > + bdrv_pdiscard(s->data_file, offset, length); > + } else { > + queue_discard(bs, offset, length); > + } > + } > +} > + > int qcow2_write_caches(BlockDriverState *bs) > { > BDRVQcow2State *s = bs->opaque; > @@ -3619,7 +3636,7 @@ qcow2_discard_refcount_block(BlockDriverState *bs, uint64_t discard_block_offs) > /* discard refblock from the cache if refblock is cached */ > qcow2_cache_discard(s->refcount_block_cache, refblock); > } > - update_refcount_discard(bs, discard_block_offs, s->cluster_size); > + queue_discard(bs, discard_block_offs, s->cluster_size); > > return 0; > } This hunk should go into the previous patch. > diff --git a/block/qcow2.h b/block/qcow2.h > index a9e3481c6e..547bb2b814 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -880,6 +880,10 @@ void GRAPH_RDLOCK qcow2_free_clusters(BlockDriverState *bs, > void GRAPH_RDLOCK > qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry, > enum qcow2_discard_type type); > +void GRAPH_RDLOCK > +qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset, > + uint64_t length, QCow2ClusterType ctype, > + enum qcow2_discard_type dtype); > > int GRAPH_RDLOCK > qcow2_update_snapshot_refcount(BlockDriverState *bs, int64_t l1_table_offset, With the above done, for both patch 1 and 2: Reviewed-by: Hanna Czenczek <hreitz@redhat.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] qcow2: put discards in discard queue when discard-no-unref is enabled 2025-05-12 12:28 ` Hanna Czenczek @ 2025-05-13 13:27 ` Jean-Louis Dupond 0 siblings, 0 replies; 5+ messages in thread From: Jean-Louis Dupond @ 2025-05-13 13:27 UTC (permalink / raw) To: Hanna Czenczek, qemu-devel Cc: Alexander Ivanov, Kevin Wolf, Andrey Drobyshev, qemu-block On 5/12/25 14:28, Hanna Czenczek wrote: > On 29.04.25 12:31, Jean-Louis Dupond wrote: >> When discard-no-unref is enabled, discards are not queued like it >> should. >> This was broken since discard-no-unref was added. >> >> Add some helper function qcow2_discard_cluster which handles some common >> checks and calls the queue_discards function if needed to add the >> discard request to the queue. >> >> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be> >> --- >> block/qcow2-cluster.c | 16 ++++++---------- >> block/qcow2-refcount.c | 19 ++++++++++++++++++- >> block/qcow2.h | 4 ++++ >> 3 files changed, 28 insertions(+), 11 deletions(-) >> >> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> index ce8c0076b3..c655bf6df4 100644 >> --- a/block/qcow2-cluster.c >> +++ b/block/qcow2-cluster.c >> @@ -1978,12 +1978,10 @@ discard_in_l2_slice(BlockDriverState *bs, >> uint64_t offset, uint64_t nb_clusters, >> if (!keep_reference) { >> /* Then decrease the refcount */ >> qcow2_free_any_cluster(bs, old_l2_entry, type); >> - } else if (s->discard_passthrough[type] && >> - (cluster_type == QCOW2_CLUSTER_NORMAL || >> - cluster_type == QCOW2_CLUSTER_ZERO_ALLOC)) { >> + } else { >> /* If we keep the reference, pass on the discard still */ >> - bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK, >> - s->cluster_size); >> + qcow2_discard_cluster(bs, old_l2_entry & L2E_OFFSET_MASK, >> + s->cluster_size, cluster_type, type); >> } >> } >> @@ -2092,12 +2090,10 @@ zero_in_l2_slice(BlockDriverState *bs, >> uint64_t offset, >> if (!keep_reference) { >> /* Then decrease the refcount */ >> qcow2_free_any_cluster(bs, old_l2_entry, >> QCOW2_DISCARD_REQUEST); >> - } else if (s->discard_passthrough[QCOW2_DISCARD_REQUEST] && >> - (type == QCOW2_CLUSTER_NORMAL || >> - type == QCOW2_CLUSTER_ZERO_ALLOC)) { >> + } else { >> /* If we keep the reference, pass on the discard >> still */ >> - bdrv_pdiscard(s->data_file, old_l2_entry & >> L2E_OFFSET_MASK, >> - s->cluster_size); >> + qcow2_discard_cluster(bs, old_l2_entry & >> L2E_OFFSET_MASK, >> + s->cluster_size, type, >> QCOW2_DISCARD_REQUEST); >> } >> } >> } >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index d796018970..e1f830504d 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1205,6 +1205,23 @@ void qcow2_free_any_cluster(BlockDriverState >> *bs, uint64_t l2_entry, >> } >> } >> +void qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset, >> + uint64_t length, QCow2ClusterType ctype, >> + enum qcow2_discard_type dtype) { >> + > > QEMU coding style requires putting the { on a separate line. > Fixed >> + BDRVQcow2State *s = bs->opaque; >> + >> + if (s->discard_passthrough[dtype] && >> + (ctype == QCOW2_CLUSTER_NORMAL || >> + ctype == QCOW2_CLUSTER_ZERO_ALLOC)) { >> + if (has_data_file(bs)) { >> + bdrv_pdiscard(s->data_file, offset, length); >> + } else { >> + queue_discard(bs, offset, length); >> + } >> + } >> +} >> + >> int qcow2_write_caches(BlockDriverState *bs) >> { >> BDRVQcow2State *s = bs->opaque; >> @@ -3619,7 +3636,7 @@ qcow2_discard_refcount_block(BlockDriverState >> *bs, uint64_t discard_block_offs) >> /* discard refblock from the cache if refblock is cached */ >> qcow2_cache_discard(s->refcount_block_cache, refblock); >> } >> - update_refcount_discard(bs, discard_block_offs, s->cluster_size); >> + queue_discard(bs, discard_block_offs, s->cluster_size); >> return 0; >> } > > This hunk should go into the previous patch. > Fixed >> diff --git a/block/qcow2.h b/block/qcow2.h >> index a9e3481c6e..547bb2b814 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -880,6 +880,10 @@ void GRAPH_RDLOCK >> qcow2_free_clusters(BlockDriverState *bs, >> void GRAPH_RDLOCK >> qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry, >> enum qcow2_discard_type type); >> +void GRAPH_RDLOCK >> +qcow2_discard_cluster(BlockDriverState *bs, uint64_t offset, >> + uint64_t length, QCow2ClusterType ctype, >> + enum qcow2_discard_type dtype); >> int GRAPH_RDLOCK >> qcow2_update_snapshot_refcount(BlockDriverState *bs, int64_t >> l1_table_offset, > > With the above done, for both patch 1 and 2: > > Reviewed-by: Hanna Czenczek <hreitz@redhat.com> > Send v2 patch with comments addressed. Thanks Jean-Louis ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-05-13 13:28 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-29 10:31 [PATCH 0/2] qcow2: queue discards when discard-no-unref enabled Jean-Louis Dupond 2025-04-29 10:31 ` [PATCH 1/2] qcow2: rename update_refcount_discard to queue_discard Jean-Louis Dupond 2025-04-29 10:31 ` [PATCH 2/2] qcow2: put discards in discard queue when discard-no-unref is enabled Jean-Louis Dupond 2025-05-12 12:28 ` Hanna Czenczek 2025-05-13 13:27 ` Jean-Louis Dupond
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).