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

[Eric Blake <eblake@redhat.com>]

On 7/10/20 9:21 AM, 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.

Yep, that's what I've already done on nbd images.

nbdkit has '--filter=truncate' which rounds an image size up to 
alignment by reading the absent tail as zeros, and permitting writes 
that rewrite zero but failing with EIO any write that would attempt to 
change the tail.  We may eventually want that complexity in qemu's block 
layer for ALL drivers (as part of switching the block layer to 
byte-accurate sizing everywhere), but that's a LOT more effort.  The 
short term of just mandating alignment is much easier and still defensible.

> 
> 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(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block.c b/block.c
> index cc377d7ef3..c635777911 100644
> --- a/block.c
> +++ b/block.c
> @@ -1489,6 +1489,16 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>           return -EINVAL;
>       }
>   
> +    /*
> +     * 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.
> +     */
> +    if ((bs->total_sectors * BDRV_SECTOR_SIZE) % bs->bl.request_alignment) {
> +        error_setg(errp, "Image size is not a multiple of request alignment");
> +        return -EINVAL;
> +    }
> +

Do we have any iotest coverage of this new message?  (If none of our 
existing tests broke, then you should add one...)


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org