Re: [PATCH v3 3/5] block: add max_ioctl_transfer to BlockLimits

[Max Reitz]

On 17.12.20 17:56, Maxim Levitsky wrote:
> Maximum transfer size when accessing a kernel block device is only relevant
> when using SCSI passthrough (SG_IO ioctl) since only in this case the requests
> are passed directly to underlying hardware with no pre-processing.

So by “with no pre-processing” you mean in particular no pre-processing 
by the kernel?  I.e., that file-posix for non-SG devices actually has no 
max transfer limit, because the kernel will split overly long requests 
by itself?

> Same is true when using /dev/sg* character devices (which only support SG_IO)
>
> Therefore split the block driver's advertized max transfer size by
> the regular max transfer size, and the max transfer size for SCSI passthrough
> (the new max_ioctl_transfer field)
>
> In the next patch, the qemu block drivers that support SCSI passthrough
> will set the max_ioctl_transfer field, and simultaneously, the block devices
> that implement scsi passthrough will switch to 'blk_get_max_ioctl_transfer' to
> query and to pass it to the guest.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>   block/block-backend.c          | 12 ++++++++++++
>   block/io.c                     |  2 ++
>   include/block/block_int.h      |  4 ++++
>   include/sysemu/block-backend.h |  1 +
>   4 files changed, 19 insertions(+)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index ce78d30794..c1d149a755 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1938,6 +1938,18 @@ uint32_t blk_get_max_transfer(BlockBackend *blk)
>       return MIN_NON_ZERO(max, INT_MAX);
>   }
>   
> +/* Returns the maximum transfer length, for SCSI passthrough */
> +uint32_t blk_get_max_ioctl_transfer(BlockBackend *blk)
> +{
> +    BlockDriverState *bs = blk_bs(blk);
> +    uint32_t max = 0;
> +
> +    if (bs) {
> +        max = bs->bl.max_ioctl_transfer;
> +    }
> +    return MIN_NON_ZERO(max, INT_MAX);
> +}
> +
>   int blk_get_max_iov(BlockBackend *blk)
>   {
>       return blk->root->bs->bl.max_iov;
> diff --git a/block/io.c b/block/io.c
> index 24205f5168..ac5aea435e 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -126,6 +126,8 @@ static void bdrv_merge_limits(BlockLimits *dst, const 
> BlockLimits *src)
>   {
>       dst->opt_transfer = MAX(dst->opt_transfer, src->opt_transfer);
>       dst->max_transfer = MIN_NON_ZERO(dst->max_transfer, src->max_transfer);
> +    dst->max_ioctl_transfer = MIN_NON_ZERO(dst->max_ioctl_transfer,
> +                                        src->max_ioctl_transfer);

I’d prefer this to be aligned to the opening parenthesis.

>       dst->opt_mem_alignment = MAX(dst->opt_mem_alignment,
>                                    src->opt_mem_alignment);
>       dst->min_mem_alignment = MAX(dst->min_mem_alignment,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 1eeafc118c..c59b0aefc4 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -686,6 +686,10 @@ typedef struct BlockLimits {
>        * clamped down. */
>       uint32_t max_transfer;
>   
> +    /* Maximal transfer length for SCSI passthrough (ioctl interface) */
> +    uint32_t max_ioctl_transfer;
> +
> +

Is there a specific reason you added a double newline here?  (All other 
fields in this struct are separated by a single newline)

Max

>       /* memory alignment, in bytes so that no bounce buffer is needed */
>       size_t min_mem_alignment;
>   
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 8203d7f6f9..b019a37b7a 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -203,6 +203,7 @@ void blk_eject(BlockBackend *blk, bool eject_flag);
>   int blk_get_flags(BlockBackend *blk);
>   uint32_t blk_get_request_alignment(BlockBackend *blk);
>   uint32_t blk_get_max_transfer(BlockBackend *blk);
> +uint32_t blk_get_max_ioctl_transfer(BlockBackend *blk);
>   int blk_get_max_iov(BlockBackend *blk);
>   void blk_set_guest_block_size(BlockBackend *blk, int align);
>   void *blk_try_blockalign(BlockBackend *blk, size_t size);