On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote: > Tested using:
Hi Nir,
This looks like a good candidate for the qemu-iotests test suite. Adding
it to the automated tests will protect against future regressions.
Please add the script and the expected output to
tests/qemu-iotests/test/write-zeroes-unmap and run it using
`(cd build && tests/qemu-iotests/check write-zeroes-unmap)`.
See the existing test cases in tests/qemu-iotests/ and
tests/qemu-iotests/tests/ for examples. Some are shell scripts and
others are Python. I think shell makes sense for this test case. You can
copy the test framework boilerplate from an existing test case.
Thanks,
Stefan
>
> $ cat test-unmap.sh
> #!/bin/sh
>
> qemu=${1:?Usage: $0 qemu-executable}
> img=/tmp/test.raw
>
> echo
> echo "defaults - write zeroes"
> fallocate -l 1m $img
> echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
> -drive if=none,file=$img,format=raw >/dev/null
> du -sh $img
>
> echo
> echo "defaults - write zeroes unmap"
> fallocate -l 1m $img
> echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
> -drive if=none,file=$img,format=raw >/dev/null
> du -sh $img
>
> echo
> echo "defaults - write actual zeros"
> fallocate -l 1m $img
> echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
> -drive if=none,file=$img,format=raw >/dev/null
> du -sh $img
>
> echo
> echo "discard=off - write zeroes unmap"
> fallocate -l 1m $img
> echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
> -drive if=none,file=$img,format=raw,discard=off >/dev/null
> du -sh $img
>
> echo
> echo "detect-zeros=on - write actual zeros"
> fallocate -l 1m $img
> echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
> -drive if=none,file=$img,format=raw,detect-zeroes=on >/dev/null
> du -sh $img
>
> echo
> echo "detect-zeros=unmap,discard=unmap - write actual zeros"
> fallocate -l 1m $img
> echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
> -drive if=none,file=$img,format=raw,detect-zeroes=unmap,discard=unmap
> >/dev/null
> du -sh $img
>
> echo
> echo "discard=unmap - write zeroes"
> fallocate -l 1m $img
> echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
> -drive if=none,file=$img,format=raw,discard=unmap >/dev/null
> du -sh $img
>
> echo
> echo "discard=unmap - write zeroes unmap"
> fallocate -l 1m $img
> echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
> -drive if=none,file=$img,format=raw,discard=unmap >/dev/null
> du -sh $img
>
> rm $img
>
>
> Before this change:
>
> $ cat before.out
>
> defaults - write zeroes
> 1.0M /tmp/test.raw
>
> defaults - write zeroes unmap
> 0 /tmp/test.raw
>
> defaults - write actual zeros
> 1.0M /tmp/test.raw
>
> discard=off - write zeroes unmap
> 0 /tmp/test.raw
>
> detect-zeros=on - write actual zeros
> 1.0M /tmp/test.raw
>
> detect-zeros=unmap,discard=unmap - write actual zeros
> 0 /tmp/test.raw
>
> discard=unmap - write zeroes
> 1.0M /tmp/test.raw
>
> discard=unmap - write zeroes unmap
> 0 /tmp/test.raw
> [nsoffer build (consider-discard-option)]$
>
>
> After this change:
>
> $ cat after.out
>
> defaults - write zeroes
> 1.0M /tmp/test.raw
>
> defaults - write zeroes unmap
> 1.0M /tmp/test.raw
>
> defaults - write actual zeros
> 1.0M /tmp/test.raw
>
> discard=off - write zeroes unmap
> 1.0M /tmp/test.raw
>
> detect-zeros=on - write actual zeros
> 1.0M /tmp/test.raw
>
> detect-zeros=unmap,discard=unmap - write actual zeros
> 0 /tmp/test.raw
>
> discard=unmap - write zeroes
> 1.0M /tmp/test.raw
>
> discard=unmap - write zeroes unmap
> 0 /tmp/test.raw
>
>
> Differences:
>
> $ diff -u before.out after.out
> --- before.out 2024-06-19 20:24:09.234083713 +0300
> +++ after.out 2024-06-19 20:24:20.526165573 +0300
> @@ -3,13 +3,13 @@
> 1.0M /tmp/test.raw
>
> defaults - write zeroes unmap
> -0 /tmp/test.raw
> +1.0M /tmp/test.raw
>
> defaults - write actual zeros
> 1.0M /tmp/test.raw
>
> discard=off - write zeroes unmap
> -0 /tmp/test.raw
> +1.0M /tmp/test.raw
>
> On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer <nsof...@redhat.com> wrote:
>
> > When opening an image with discard=off, we punch hole in the image when
> > writing zeroes, making the image sparse. This breaks users that want to
> > ensure that writes cannot fail with ENOSPACE by using fully allocated
> > images.
> >
> > bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
> > opened the child without discard=unmap or discard=on. But we don't go
> > through this function when accessing the top node. Move the check down
> > to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.
> >
> > Issues:
> > - We don't punch hole by default, so images are kept allocated. Before
> > this change we punched holes by default. I'm not sure this is a good
> > change in behavior.
> > - Need to run all block tests
> > - Not sure that we have tests covering unmapping, we may need new tests
> > - We may need new tests to cover this change
> >
> > Signed-off-by: Nir Soffer <nsof...@redhat.com>
> > ---
> >
> > Changes since v1:
> > - Replace the incorrect has_discard change with the right fix
> >
> > v1 was here:
> > https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html
> >
> > block/io.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/block/io.c b/block/io.c
> > index 7217cf811b..301514c880 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> > int64_t offset, int64_t bytes,
> > /* By definition there is no user buffer so this flag doesn't make
> > sense */
> > if (flags & BDRV_REQ_REGISTERED_BUF) {
> > return -EINVAL;
> > }
> >
> > + /* If opened with discard=off we should never unmap. */
> > + if (!(bs->open_flags & BDRV_O_UNMAP)) {
> > + flags &= ~BDRV_REQ_MAY_UNMAP;
> > + }
> > +
> > /* Invalidate the cached block-status data range if this write
> > overlaps */
> > bdrv_bsc_invalidate_range(bs, offset, bytes);
> >
> > assert(alignment % bs->bl.request_alignment == 0);
> > head = offset % alignment;
> > @@ -2313,14 +2318,10 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild
> > *child, int64_t offset,
> > {
> > IO_CODE();
> > trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
> > assert_bdrv_graph_readable();
> >
> > - if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
> > - flags &= ~BDRV_REQ_MAY_UNMAP;
> > - }
> > -
> > return bdrv_co_pwritev(child, offset, bytes, NULL,
> > BDRV_REQ_ZERO_WRITE | flags);
> > }
> >
> > /*
> > --
> > 2.45.1
> >
> >
signature.asc
Description: PGP signature
