Re: [Qemu-devel] [PATCH v2 13/13] block: Enforce non-zero bl.max_transfer

[Kevin Wolf <kwolf@redhat.com>]

Am 15.11.2018 um 03:03 hat Eric Blake geschrieben:
> The raw format driver and the filter drivers default to picking
> up the same limits as what they wrap, and I've audited that they
> are otherwise simple enough in their passthrough to be 64-bit
> clean; it's not worth changing their .bdrv_refresh_limits to
> advertise anything different.  Previous patches changed all
> remaining byte-based format and protocol drivers to supply an
> explicit max_transfer consistent with an audit (or lack thereof)
> of their code.  And for drivers still using sector-based
> callbacks (gluster, iscsi, parallels, qed, replication, sheepdog,
> ssh, vhdx), we can easily supply a 2G default limit while waiting
> for a later per-driver conversion and auditing while moving to
> byte-based callbacks.
> 
> With that, we can assert that max_transfer is now always set (either
> by sector-based default, by merge with a backing layer, or by
> explicit settings), and enforce that it be non-zero by the end
> of bdrv_refresh_limits().  This in turn lets us simplify portions
> of the block layer to use MIN() instead of MIN_NON_ZERO(), while
> still fragmenting things to no larger than 2G chunks.  Also, we no
> longer need to rewrite the driver's bl.max_transfer to a value below
> 2G.  There should be no semantic change from this patch, although
> it does open the door if a future patch wants to let the block layer
> choose a larger value than 2G while still honoring bl.max_transfer
> for fragmentation.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> [hmm - in sending this email, I realize that I didn't specifically
> check whether quorum always picks up a sane limit from its children,
> since it is byte-based but lacks a .bdrv_refresh_limits]
> 
>  include/block/block_int.h | 10 +++++-----
>  block/io.c                | 25 ++++++++++++-------------
>  2 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index b19d94dac5f..410553bb0cc 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -548,11 +548,11 @@ typedef struct BlockLimits {
>      uint32_t opt_transfer;
> 
>      /* Maximal transfer length in bytes.  Need not be power of 2, but
> -     * must be multiple of opt_transfer and bl.request_alignment, or 0
> -     * for no 32-bit limit.  The block layer fragments all actions
> -     * below 2G, so setting this value to anything larger than INT_MAX
> -     * implies that the driver has been audited for 64-bit
> -     * cleanness. */
> +     * must be larger than opt_transfer and request_alignment (the
> +     * block layer rounds it down as needed).  The block layer
> +     * fragments all actions below 2G, so setting this value to
> +     * anything larger than INT_MAX implies that the driver has been
> +     * audited for 64-bit cleanness. */
>      uint64_t max_transfer;
> 
>      /* memory alignment, in bytes so that no bounce buffer is needed */
> diff --git a/block/io.c b/block/io.c
> index 4911a1123eb..0024be0bf31 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -129,6 +129,9 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>      /* Default alignment based on whether driver has byte interface */
>      bs->bl.request_alignment = (drv->bdrv_co_preadv ||
>                                  drv->bdrv_aio_preadv) ? 1 : 512;
> +    /* Default cap at 2G for drivers without byte interface */
> +    if (bs->bl.request_alignment == 1)
> +        bs->bl.max_transfer = BDRV_REQUEST_MAX_BYTES;

Missing braces, and comment and code don't match (the comment suggests
that the condition should be != 1).

Kevin