Re: [PATCH v2 for-6.2] file-posix: Fix alignment after reopen changing O_DIRECT

[Eric Blake <eblake@redhat.com>]

On Tue, Nov 16, 2021 at 11:14:31AM +0100, Hanna Reitz wrote:
> From: Kevin Wolf <kwolf@redhat.com>
> 
> At the end of a reopen, we already call bdrv_refresh_limits(), which
> should update bs->request_alignment according to the new file
> descriptor. However, raw_probe_alignment() relies on s->needs_alignment
> and just uses 1 if it isn't set. We neglected to update this field, so
> starting with cache=writeback and then reopening with cache=none means
> that we get an incorrect bs->request_alignment == 1 and unaligned
> requests fail instead of being automatically aligned.
> 

> v2:
> Don't continue operating on a qcow2 file with an effectively random
> size, but create a new 1 MB file to run our O_DIRECT reopen read tests
> on.  Otherwise, we'll get into permission trouble because qemu-io does
> not take the RESIZE permission by default, which it would need to
> auto-align the file size to the sector size.
> 
> Note that the qcow2 file is not even aligned to 512 byte sectors before
> the test case (its size is 196616), but the error message doesn't appear
> then because qemu internally calculates file sizes in multiples of 512
> bytes (BDRV_SECTOR_SIZE), so a misalignment can only become visible when
> the physical sector size exceeds 512 bytes.
> (That's "OK" because qemu only counts sizes in multiples of 512, so any
> resize below that granularity is not visible as a resize to qemu, and so
> does not need the RESIZE permission.)
> 
> Another way we could solve this problem is by having qemu-io take the
> RESIZE permission, but I believe it would need to retain it not only
> through the reopen, but until the image size is aligned to the sector
> size; that is, we would need to resize the image anyway to be able to
> drop the permission and not keep it constantly.  So it's simpler to just
> create an aligned image from the start.

I concur that your patch is simpler, and since we are in rc phase,
simpler is better.

> +++ b/tests/qemu-iotests/142
> @@ -350,6 +350,35 @@ info block backing-file"
>  
>  echo "$hmp_cmds" | run_qemu -drive "$files","$ids" | grep "Cache"
>  
> +echo
> +echo "--- Alignment after changing O_DIRECT ---"
> +echo
> +
> +# Directly test the protocol level: Can unaligned requests succeed even if
> +# O_DIRECT was only enabled through a reopen and vice versa?
> +
> +# Ensure image size is a multiple of the sector size (required for O_DIRECT)
> +$QEMU_IMG create -f file "$TEST_IMG" 1M | _filter_img_create
> +
> +# And write some data (not strictly necessary, but it feels better to actually
> +# have something to be read)
> +$QEMU_IO -f file -c 'write 0 4096' "$TEST_IMG" | _filter_qemu_io

Looks nice.

> +
> +$QEMU_IO --cache=writeback -f file $TEST_IMG <<EOF | _filter_qemu_io
> +read 42 42
> +reopen -o cache.direct=on
> +read 42 42

This particular region of the disk is a sub-sector, but completely
contained within one sector.  Is it also worth testing the case of an
unaligned read that crosses a 4096-byte boundary?  But we could do
that on top, your patch is already an improvement and catches the
particular problem you set out to solve.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org