* [PATCH 1/2] qcow2: handle discard-no-unref in measure @ 2024-06-05 9:06 Jean-Louis Dupond 2024-06-05 9:06 ` [PATCH 2/2] qcow2: don't allow discard-no-unref when discard is not enabled Jean-Louis Dupond 2024-06-05 10:59 ` [PATCH 1/2] qcow2: handle discard-no-unref in measure Jean-Louis Dupond 0 siblings, 2 replies; 3+ messages in thread From: Jean-Louis Dupond @ 2024-06-05 9:06 UTC (permalink / raw) To: qemu-devel, kwolf, hreitz, andrey.drobyshev; +Cc: Jean-Louis Dupond When doing a measure on an image with a backing file and discard-no-unref is enabled, the code should take this into account. If for example you have a snapshot image with a base, and you do a discard within the snapshot, it will be ZERO and ALLOCATED, but without host offset. Now if we commit this snapshot, and the clusters in the base image were allocated, the clusters will only be set to ZERO, but the host offset will not be cleared. Therefor ZERO & ALLOCATED clusters in the top image need to check the base to see if space will be freed or not, to have a correct measure output. Bug-Url: https://gitlab.com/qemu-project/qemu/-/issues/2369 Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be> --- block/qcow2.c | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 956128b409..1ce7ebbab4 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -5163,9 +5163,16 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs, } else { int64_t offset; int64_t pnum = 0; + BlockDriverState *parent = bdrv_filter_or_cow_bs(in_bs); + BDRVQcow2State *s = NULL; + + if (parent) { + s = parent->opaque; + } for (offset = 0; offset < ssize; offset += pnum) { int ret; + int retp = 0; ret = bdrv_block_status_above(in_bs, NULL, offset, ssize - offset, &pnum, NULL, @@ -5176,10 +5183,33 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs, goto err; } - if (ret & BDRV_BLOCK_ZERO) { + /* If we have a parent in the chain and the current block is zero but allocated, + * then we want to check the allocation state of the parent block. + * If it was allocated and now zero, we want + * to include it into the calculation, cause it will not free space when + * committing the top into base with discard-no-unref enabled. + */ + if (parent && + ((ret & (BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED)) == + (BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED)) && + s->discard_no_unref) { + int64_t pnum_parent = 0; + retp = bdrv_block_status_above(parent, NULL, offset, + ssize - offset, &pnum_parent, NULL, + NULL); + // Check if parent block has an offset + if (retp & BDRV_BLOCK_OFFSET_VALID) { + pnum = retp; + } + } + if (ret & BDRV_BLOCK_ZERO && !retp) { /* Skip zero regions (safe with no backing file) */ - } else if ((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) == - (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) { + } else if (((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) == + (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) || + (((ret & (BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED)) == + (BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED)) && + s && s->discard_no_unref && + retp & BDRV_BLOCK_OFFSET_VALID)) { /* Extend pnum to end of cluster for next iteration */ pnum = ROUND_UP(offset + pnum, cluster_size) - offset; -- 2.45.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 2/2] qcow2: don't allow discard-no-unref when discard is not enabled 2024-06-05 9:06 [PATCH 1/2] qcow2: handle discard-no-unref in measure Jean-Louis Dupond @ 2024-06-05 9:06 ` Jean-Louis Dupond 2024-06-05 10:59 ` [PATCH 1/2] qcow2: handle discard-no-unref in measure Jean-Louis Dupond 1 sibling, 0 replies; 3+ messages in thread From: Jean-Louis Dupond @ 2024-06-05 9:06 UTC (permalink / raw) To: qemu-devel, kwolf, hreitz, andrey.drobyshev; +Cc: Jean-Louis Dupond When discard is not set to unmap/on, we should not allow setting discard-no-unref. Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be> --- block/qcow2.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/block/qcow2.c b/block/qcow2.c index 1ce7ebbab4..54c6b041b1 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1156,6 +1156,12 @@ qcow2_update_options_prepare(BlockDriverState *bs, Qcow2ReopenState *r, ret = -EINVAL; goto fail; } + if (r->discard_no_unref && !(flags & BDRV_O_UNMAP)) { + error_setg(errp, + "discard-no-unref is only valid with discard=unmap/on"); + ret = -EINVAL; + goto fail; + } switch (s->crypt_method_header) { case QCOW_CRYPT_NONE: -- 2.45.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] qcow2: handle discard-no-unref in measure 2024-06-05 9:06 [PATCH 1/2] qcow2: handle discard-no-unref in measure Jean-Louis Dupond 2024-06-05 9:06 ` [PATCH 2/2] qcow2: don't allow discard-no-unref when discard is not enabled Jean-Louis Dupond @ 2024-06-05 10:59 ` Jean-Louis Dupond 1 sibling, 0 replies; 3+ messages in thread From: Jean-Louis Dupond @ 2024-06-05 10:59 UTC (permalink / raw) To: qemu-devel, kwolf, hreitz, andrey.drobyshev On 5/06/2024 11:06, Jean-Louis Dupond wrote: > When doing a measure on an image with a backing file and > discard-no-unref is enabled, the code should take this into account. > > If for example you have a snapshot image with a base, and you do a > discard within the snapshot, it will be ZERO and ALLOCATED, but without > host offset. > Now if we commit this snapshot, and the clusters in the base image were > allocated, the clusters will only be set to ZERO, but the host offset > will not be cleared. > Therefor ZERO & ALLOCATED clusters in the top image need to check the > base to see if space will be freed or not, to have a correct measure > output. > > Bug-Url: https://gitlab.com/qemu-project/qemu/-/issues/2369 > Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be> > --- > block/qcow2.c | 36 +++++++++++++++++++++++++++++++++--- > 1 file changed, 33 insertions(+), 3 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 956128b409..1ce7ebbab4 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -5163,9 +5163,16 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs, > } else { > int64_t offset; > int64_t pnum = 0; > + BlockDriverState *parent = bdrv_filter_or_cow_bs(in_bs); > + BDRVQcow2State *s = NULL; > + > + if (parent) { > + s = parent->opaque; > + } > > for (offset = 0; offset < ssize; offset += pnum) { > int ret; > + int retp = 0; > > ret = bdrv_block_status_above(in_bs, NULL, offset, > ssize - offset, &pnum, NULL, > @@ -5176,10 +5183,33 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs, > goto err; > } > > - if (ret & BDRV_BLOCK_ZERO) { > + /* If we have a parent in the chain and the current block is zero but allocated, > + * then we want to check the allocation state of the parent block. > + * If it was allocated and now zero, we want > + * to include it into the calculation, cause it will not free space when > + * committing the top into base with discard-no-unref enabled. > + */ > + if (parent && > + ((ret & (BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED)) == > + (BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED)) && > + s->discard_no_unref) { > + int64_t pnum_parent = 0; > + retp = bdrv_block_status_above(parent, NULL, offset, > + ssize - offset, &pnum_parent, NULL, > + NULL); > + // Check if parent block has an offset > + if (retp & BDRV_BLOCK_OFFSET_VALID) { > + pnum = retp; This should be `pnum = pnum_parent` of course :) > + } > + } > + if (ret & BDRV_BLOCK_ZERO && !retp) { > /* Skip zero regions (safe with no backing file) */ > - } else if ((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) == > - (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) { > + } else if (((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) == > + (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) || > + (((ret & (BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED)) == > + (BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED)) && > + s && s->discard_no_unref && > + retp & BDRV_BLOCK_OFFSET_VALID)) { > /* Extend pnum to end of cluster for next iteration */ > pnum = ROUND_UP(offset + pnum, cluster_size) - offset; > This seems to work fine in my tests with the following commands: ./build/qemu-img create -f qcow2 /tmp/test.qcow2 128M ./build/qemu-io -c 'open /tmp/test.qcow2' -c 'write 0 8M' -c 'write 56M 20M' -c 'write 10M 8M' -c 'write 24M 32M' ./build/qemu-img create -f qcow2 -b /tmp/test.qcow2 -F qcow2 /tmp/test_snap.qcow2 ./build/qemu-io -c 'open -o discard=unmap,discard-no-unref=on /tmp/test_snap.qcow2' -c 'write 16M 8M' -c 'discard 60M 20M' -c 'write 84M 10M' ./build/qemu-img measure --output json -O qcow2 'json:{"file": {"driver": "file", "filename": "/tmp/test_snap.qcow2"}, "driver": "qcow2", "backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "/tmp/test.qcow2"}, "backing": null}}' ./build/qemu-img measure --output json -O qcow2 'json:{"file": {"driver": "file", "filename": "/tmp/test_snap.qcow2"}, "driver": "qcow2", "discard":"unmap", "discard-no-unref":true, "backing": {"driver": "qcow2", "discard-no-unref":true, "file": {"driver": "file", "filename": "/tmp/test.qcow2"}, "backing": null}}' But it does not seem to work when the base image has ZERO ALLOCATED clusters that overlap with the ZERO ALLOCATED clusters in the snapshot. As its then seen as a single zero cluster by the bdrv_block_status_above function. This happens for example when the base vm was initially running without discard-no-unref and enabled it only later. Any idea's on how to handle that ? Thanks Jean-Louis ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-06-05 11:00 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-06-05 9:06 [PATCH 1/2] qcow2: handle discard-no-unref in measure Jean-Louis Dupond 2024-06-05 9:06 ` [PATCH 2/2] qcow2: don't allow discard-no-unref when discard is not enabled Jean-Louis Dupond 2024-06-05 10:59 ` [PATCH 1/2] qcow2: handle discard-no-unref in measure 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).