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

[Jose R. Ziviani]

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

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