[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 for-6.2] file-posix: Fix alignment after reopen changing O
From: |
Eric Blake |
Subject: |
Re: [PATCH v2 for-6.2] file-posix: Fix alignment after reopen changing O_DIRECT |
Date: |
Tue, 16 Nov 2021 08:30:33 -0600 |
User-agent: |
NeoMutt/20211029-16-b680fe |
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