Re: [PATCH v3 1/5] file-posix: split hdev_refresh_limits from raw_refresh_limits

[Max Reitz <mreitz@redhat.com>]

On 17.12.20 17:56, Maxim Levitsky wrote:
> From: Tom Yan <tom.ty89@gmail.com>
> 
> We can and should get max transfer length and max segments for all host
> devices / cdroms (on Linux).
> 
> Also use MIN_NON_ZERO instead when we clamp max transfer length against
> max segments.
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>   block/file-posix.c | 57 +++++++++++++++++++++++++++++++++-------------
>   1 file changed, 41 insertions(+), 16 deletions(-)

I’m aware that most of my remarks below apply to the pre-patch state 
just as well, but I feel like now is a good time to raise them, so, here 
goes:

> diff --git a/block/file-posix.c b/block/file-posix.c
> index 9804681d5c..cbf1271773 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1166,6 +1166,10 @@ static int sg_get_max_transfer_length(int fd)
>       int max_bytes = 0;
>   
>       if (ioctl(fd, BLKSECTGET, &max_bytes) == 0) {
> +        /*
> +         * BLKSECTGET for /dev/sg* character devices incorrectly returns
> +         * the max transfer size in bytes (rather than in blocks).
> +         */
>           return max_bytes;
>       } else {
>           return -errno;
> @@ -1175,7 +1179,22 @@ static int sg_get_max_transfer_length(int fd)
>   #endif
>   }
>   
> -static int sg_get_max_segments(int fd)
> +static int get_max_transfer_length(int fd)
> +{
> +#if defined(BLKSECTGET)
> +    int sect = 0;
> +
> +    if (ioctl(fd, BLKSECTGET, &sect) == 0) {
> +        return sect << 9;

Can this overflow?

(I mean, technically it would still be safe, because either the limit is 
set too low or it isn’t set at all, which would be correct on overflow. 
  But still.)

> +    } else {
> +        return -errno;
> +    }
> +#else
> +    return -ENOSYS;
> +#endif
> +}
> +
> +static int get_max_segments(int fd)
>   {
>   #ifdef CONFIG_LINUX
>       char buf[32];

This function stores max_segments (a long) in ret (an int) and returns 
the latter.  Should we guard against overflows?

> @@ -1230,23 +1249,29 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>   {
>       BDRVRawState *s = bs->opaque;
>   
> -    if (bs->sg) {
> -        int ret = sg_get_max_transfer_length(s->fd);
> +    raw_probe_alignment(bs, s->fd, errp);
> +    bs->bl.min_mem_alignment = s->buf_align;
> +    bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
> +}
> +
> +static void hdev_refresh_limits(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVRawState *s = bs->opaque;
> +
> +    int ret = bs->sg ? sg_get_max_transfer_length(s->fd) :
> +                       get_max_transfer_length(s->fd);
>   
> -        if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
> -            bs->bl.max_transfer = pow2floor(ret);
> -        }
> +    if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
> +        bs->bl.max_transfer = pow2floor(ret);
> +    }
>   
> -        ret = sg_get_max_segments(s->fd);
> -        if (ret > 0) {
> -            bs->bl.max_transfer = MIN(bs->bl.max_transfer,
> -                                      ret * qemu_real_host_page_size);

(1) Can this overflow?  (Which I suppose could result in a 
non-power-of-two result)

(2) Even disregarding overflows, is ret * qemu_real_host_page_size 
guaranteed to be a power of two?

Max

> -        }
> +    ret = get_max_segments(s->fd);
> +    if (ret > 0) {
> +        bs->bl.max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
> +                                           ret * qemu_real_host_page_size);
>       }
>   
> -    raw_probe_alignment(bs, s->fd, errp);
> -    bs->bl.min_mem_alignment = s->buf_align;
> -    bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
> +    raw_refresh_limits(bs, errp);
>   }
>   
>   static int check_for_dasd(int fd)
> @@ -3600,7 +3625,7 @@ static BlockDriver bdrv_host_device = {
>       .bdrv_co_pdiscard       = hdev_co_pdiscard,
>       .bdrv_co_copy_range_from = raw_co_copy_range_from,
>       .bdrv_co_copy_range_to  = raw_co_copy_range_to,
> -    .bdrv_refresh_limits = raw_refresh_limits,
> +    .bdrv_refresh_limits = hdev_refresh_limits,
>       .bdrv_io_plug = raw_aio_plug,
>       .bdrv_io_unplug = raw_aio_unplug,
>       .bdrv_attach_aio_context = raw_aio_attach_aio_context,
> @@ -3724,7 +3749,7 @@ static BlockDriver bdrv_host_cdrom = {
>       .bdrv_co_preadv         = raw_co_preadv,
>       .bdrv_co_pwritev        = raw_co_pwritev,
>       .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
> -    .bdrv_refresh_limits = raw_refresh_limits,
> +    .bdrv_refresh_limits = hdev_refresh_limits,
>       .bdrv_io_plug = raw_aio_plug,
>       .bdrv_io_unplug = raw_aio_unplug,
>       .bdrv_attach_aio_context = raw_aio_attach_aio_context,
>