Tested using: $ 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 > >