On Thu, Aug 19, 2021 at 05:14:30PM +0200, Hanna Reitz wrote: > On 19.08.21 16:31, Jose R. Ziviani wrote: > > Hello Hanna, > > > > On Thu, Aug 19, 2021 at 12:12:00PM +0200, Hanna Reitz wrote: > > > We cannot write to images opened with O_DIRECT unless we allow them to > > > be resized so they are aligned to the sector size: Since 9c60a5d1978, > > > bdrv_node_refresh_perm() ensures that for nodes whose length is not > > > aligned to the request alignment and where someone has taken a WRITE > > > permission, the RESIZE permission is taken, too). > > > > > > Let qemu-img convert pass the BDRV_O_RESIZE flag (which causes > > > blk_new_open() to take the RESIZE permission) when using cache=none for > > > the target, so that when writing to it, it can be aligned to the target > > > sector size. > > > > > > Without this patch, an error is returned: > > > > > > $ qemu-img convert -f raw -O raw -t none foo.img /mnt/tmp/foo.img > > > qemu-img: Could not open '/mnt/tmp/foo.img': Cannot get 'write' > > > permission without 'resize': Image size is not a multiple of request > > > alignment > > > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1994266 > > > Signed-off-by: Hanna Reitz <hre...@redhat.com> > > > --- > > > As I have written in the BZ linked above, I am not sure what behavior we > > > want. It can be argued that the current behavior is perfectly OK > > > because we want the target to have the same size as the source, so if > > > this cannot be done, we should just print the above error (which I think > > > explains the problem well enough that users can figure out they need to > > > resize the source image). > > > > > > OTOH, it is difficult for me to imagine a case where the user would > > > prefer the above error to just having qemu-img align the target image's > > > length. > > This is timely convenient because I'm currently investigating an issue > > detected > > by a libvirt-tck test: > > > > https://gitlab.com/libvirt/libvirt-tck/-/blob/master/scripts/qemu/200-qcow2-single-backing-file.t > > > > It fails with the same message: > > qemu-system-x86_64: -device > > virtio-blk-pci,bus=pci.0,addr=0x3,drive=libvirt-1-format,id=virtio-disk1,write-cache=on: > > Cannot get 'write' permission without 'resize': Image size is not a > > multiple of request alignment > > > > Libvirt-tck basically wants to make sure that libvirt won't pass a 'backing' > > argument if we force a QCOW2 image to be interpreted as a RAW image. But, > > the > > actual size of a (not preallocated) QCOW2 is usually unaligned so we ended > > up > > failing at that requirement. > > > > I crafted a reproducer (tck-main is a QCOW2 image): > > $ ./qemu-system-x86_64 \ > > -machine pc-i440fx-6.0,accel=kvm -m 1024 -display none -no-user-config > > -nodefaults \ > > -kernel vmlinuz -initrd initrd \ > > -blockdev > > driver=file,filename=/var/cache/libvirt-tck/storage-fs/tck/tck-main,node-name=a,cache.direct=on > > \ > > -blockdev node-name=name,driver=raw,file=a \ > > -device virtio-blk-pci,drive=name > > > > And if I remove 'cache.direct=on' OR if I remove the device virtio-blk-pci I > > don't hit the failure. > > > > In order to fix the libvirt-tck test case, I think that creating a > > preallocated > > QCOW2 image is the best approach, considering their test case goal. But, if > > you > > don't mind, could you explain why cache.direct=on doesn't set resize > > permission > > with virtio-blk-pci? > > Well, users only take the permissions they need. Because the device doesn’t > actively want to resize the disk, it doesn’t take the permission (because > other simultaneous users might not share that permission, and so taking more > permissions than you need may cause problems). > > So the decision whether to take the permission or not is a tradeoff. I > mean, there’s always a work-around: The error message tells you that the > image is not aligned to the request alignment, so you can align the image > size manually. The question is whether taking the permissions may be > problematic, and whether the user can be expected to align the image size. > > (And we also need to keep in mind that this case is extremely rare. I don’t > think that users in practice will ever have images that are not 4k-aligned. > What the test is doing is of course something that would never happen in a > practical set-up, in fact, I believe attaching a qcow2 image as a raw image > to a VM is something that would usually be considered dangerous from a > security perspective.[1]) > > For qemu-img convert (i.e. for this patch), I decided that it is extremely > unlikely there are concurrent users for the target, so I can’t imagine > taking more permissions would cause problems. The only downside I could see > is that the target image would be larger than the source image, but I think > that is still the outcome that users want. > > On the other side, applying the work-around may be problematic for users: > The source image of qemu-img convert shouldn’t have to be writable. So > resizing it (just so it can be converted to be stored on a medium with 4k > sector size) may actually be impossible for the user. > > Now, for the virtio-blk case, that is different. If you’re willing to apply > the work-around, then you don’t have to do so on an “innocent bystander” > image. You have to resize the very image you want to use, i.e. one that > must be writable anyway. So resizing the image outside of qemu to match the > alignment is feasible. > > OTOH, because this is a live and complete image, it’s entirely possible that > there are concurrent users that would block virtio-blk from taking the > RESIZE permission, so I don’t think we should take it lightly. > > So I think for the virtio-blk case the weight of pro and contra is very > different than for qemu-img. I’m not sure we should take the RESIZE > permission in that case, especially considering that the example is not a > real-world one. > > I think if we really want to improve something for the virtio-blk case, it > would be to have the error message tell what the request alignment is, and > to add an error hint like > > “Try resizing the image using `qemu-img resize -f {bs->drv->format_name} > +{ROUND_UP(length, aligment) - length}`.” > > Hanna > > [1] Just to have it mentioned: Attaching a qcow2 image as a qcow2 image > should work perfectly fine, because the qcow2 driver takes the RESIZE > permission. >
Yep, it works perfectly fine. I only get the alignment error because (I think) the RAW driver sees a QCOW2 containing only a few kB of metadata and assumes it's the whole disk size. Even after some debugging I was not understanding much about the requirements that apply to that RESIZE permission, which is clear now. Thank you SO much for such detailed explanation! Jose
signature.asc
Description: Digital signature
