Re: [PATCH for-5.1 1/2] block: Require aligned image size to avoid assertion failure

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4840 bytes --]

Am 14.07.2020 um 18:22 hat Max Reitz geschrieben:
> On 14.07.20 13:08, Kevin Wolf wrote:
> > Am 14.07.2020 um 11:56 hat Max Reitz geschrieben:
> >> On 13.07.20 16:29, Kevin Wolf wrote:
> >>> Am 13.07.2020 um 13:19 hat Max Reitz geschrieben:
> >>>> On 10.07.20 16:21, Kevin Wolf wrote:
> >>>>> Unaligned requests will automatically be aligned to bl.request_alignment
> >>>>> and we don't want to extend requests to access space beyond the end of
> >>>>> the image, so it's required that the image size is aligned.
> >>>>>
> >>>>> With write requests, this could cause assertion failures like this if
> >>>>> RESIZE permissions weren't requested:
> >>>>>
> >>>>> qemu-img: block/io.c:1910: bdrv_co_write_req_prepare: Assertion `end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE' failed.
> >>>>>
> >>>>> This was e.g. triggered by qemu-img converting to a target image with 4k
> >>>>> request alignment when the image was only aligned to 512 bytes, but not
> >>>>> to 4k.
> >>>>>
> >>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>>>> ---
> >>>>>  block.c | 10 ++++++++++
> >>>>>  1 file changed, 10 insertions(+)
> >>>>
> >>>> (I think we had some proposal like this before, but I can’t find it,
> >>>> unfortunately...)
> >>>>
> >>>> I can’t see how with this patch you could create qcow2 images and then
> >>>> use them with direct I/O, because AFAICS, qemu-img create doesn’t allow
> >>>> specifying caching options, so AFAIU you’re stuck with:
> >>>>
> >>>> $ ./qemu-img create -f qcow2 /mnt/tmp/foo.qcow2 1M
> >>>> Formatting '/mnt/tmp/foo.qcow2', fmt=qcow2 cluster_size=65536
> >>>> compression_type=zlib size=1048576 lazy_refcounts=off refcount_bits=16
> >>>>
> >>>> $ sudo ./qemu-io -t none /mnt/tmp/foo.qcow2
> >>>> qemu-io: can't open device /mnt/tmp/foo.qcow2: Image size is not a
> >>>> multiple of request alignment
> >>>>
> >>>> (/mnt/tmp is a filesystem on a “losetup -b 4096” device.)
> >>>
> >>> Hm, that looks like some regrettable collateral damage...
> >>>
> >>> Well, you could argue that we should be writing full L1 tables with zero
> >>> padding instead of just the used part. I thought we had fixed this long
> >>> ago. But looks like we haven't.
> >>
> >> That would help for the standard case.  It wouldn’t when the cluster
> >> size is smaller than the request alignment, which, while maybe not
> >> important, would still be a shame.
> > 
> > I don't think it would be unreasonable to require a cluster size that is
> > a multiple of the logical block size of your host storage if you want to
> > use O_DIRECT.
> 
> True.
> 
> > But we have unaligned images in practice, so this is pure theory anyway.
> 
> Hm.  Maybe it would help to just adjust the error message to instruct
> the user to resize the image to fit the request alignment?  (e.g. “is
> not a multiple of the request alignment %u (try resizing the image to
> %llu bytes)”)

This would require management tools to automatically do this or we would
break any users that don't manually invoke QEMU. I don't think this is a
realistic option, especially since "management tools" must probably
include all those one-off shell scripts that people use.

> >>> But we should still avoid crashing in other cases, so what is the
> >>> difference between both? Is it just that qcow2 has the RESIZE permission
> >>> anyway so it doesn't matter?
> >>
> >> I assume so.
> >>
> >>> If so, maybe attaching to a block node with WRITE, but not RESIZE is
> >>> what needs to fail when the image size is unaligned?
> >>
> >> That sounds reasonable.
> >>
> >> The obvious question is what happens when the RESIZE capability is
> >> removed.  Dropping capabilities may never fail – I suppose we could
> >> force-keep the RESIZE capability for such nodes?
> > 
> > It's not nice, but I think we already have this kind of behaviour for
> > unlocking failures. So yes, that sounds like an option.
> > 
> >> Or we could immediately align such files to the block size once they
> >> are opened (with the RESIZE capability).
> > 
> > Automatically resizing the image file is obviously harmless for qcow2
> > images, but it would be a guest-visible change for raw images. It might
> > be better to avoid this.
> 
> Well, it seems to be what already happens if the guest device has taken
> the RESIZE capability (i.e., whenever there’s no failing assertion).
> The only difference that appears to me is just that it happens only when
> writing to the end of the image instead of unconditionally when opening it.

I would have considered this as part of the bug rather than a desirable
future behaviour. blk_check_byte_request() tries to catch any request
going past EOF, it just doesn't know anything about request_alignment.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]