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

[Max Reitz]

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)”)

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

Max

signature.asc
Description: OpenPGP digital signature