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

[Kevin Wolf]

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.

But we have unaligned images in practice, so this is pure theory anyway.

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

Kevin

signature.asc
Description: PGP signature