On Tue, Nov 16, 2021 at 11:14:31AM +0100, Hanna Reitz wrote: > From: Kevin Wolf <kw...@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 <ebl...@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
