Re: [PATCH] qemu-img: Allow target be aligned to sector size

[Hanna Reitz <hreitz@redhat.com>]

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 <hreitz@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.