Re: [PATCH v4 3/3] block: make BlockConf.*_size properties 32-bit

[Kevin Wolf <kwolf@redhat.com>]

Am 20.05.2020 um 10:06 hat Roman Kagan geschrieben:
> Devices (virtio-blk, scsi, etc.) and the block layer are happy to use
> 32-bit for logical_block_size, physical_block_size, and min_io_size.
> However, the properties in BlockConf are defined as uint16_t limiting
> the values to 32768.
> 
> This appears unnecessary tight, and we've seen bigger block sizes handy
> at times.
> 
> Make them 32 bit instead and lift the limitation up to 2 MiB which
> appears to be good enough for everybody, and matches the qcow2 cluster
> size limit.
> 
> As the values can now be fairly big and awkward to type, make the
> property setter accept common size suffixes (k, m).
> 
> Also as the devices which use min_io_size (virtio-blk and scsi) pass its
> value to the guest in units of logical blocks in a 16bit field, to
> prevent its silent truncation add a corresponding check to
> blkconf_blocksizes.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> ---
> v3 -> v4:
> - check min_io_size against truncation [Kevin]
> 
> v2 -> v3:
> - mention qcow2 cluster size limit in the log and comment [Eric]
> 
> v1 -> v2:
> - cap the property at 2 MiB [Eric]
> - accept size suffixes
> 
>  include/hw/block/block.h     |  8 ++++----
>  include/hw/qdev-properties.h |  2 +-
>  hw/block/block.c             | 11 +++++++++++
>  hw/core/qdev-properties.c    | 34 ++++++++++++++++++++++++----------
>  4 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 784953a237..2fa09aa0b1 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -18,9 +18,9 @@
>  
>  typedef struct BlockConf {
>      BlockBackend *blk;
> -    uint16_t physical_block_size;
> -    uint16_t logical_block_size;
> -    uint16_t min_io_size;
> +    uint32_t physical_block_size;
> +    uint32_t logical_block_size;
> +    uint32_t min_io_size;
>      uint32_t opt_io_size;
>      int32_t bootindex;
>      uint32_t discard_granularity;
> @@ -51,7 +51,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>                            _conf.logical_block_size),                    \
>      DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
>                            _conf.physical_block_size),                   \
> -    DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),    \
> +    DEFINE_PROP_UINT32("min_io_size", _state, _conf.min_io_size, 0),    \
>      DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
>      DEFINE_PROP_UINT32("discard_granularity", _state,                   \
>                         _conf.discard_granularity, -1),                  \
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index f161604fb6..f9e0f8c041 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -197,7 +197,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>  #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
>  #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
> -    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
> +    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
>  #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
>      DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
>  #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
> diff --git a/hw/block/block.c b/hw/block/block.c
> index 5f8ebff59c..cd95e7e38f 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -96,6 +96,17 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
>          return false;
>      }
>  
> +    /*
> +     * all devices which support min_io_size (scsi and virtio-blk) expose it to
> +     * the guest as a uint16_t in units of logical blocks
> +     */
> +    if ((conf->min_io_size / conf->logical_block_size) > UINT16_MAX) {
> +        error_setg(errp,
> +                   "min_io_size must be no more than " stringify(UINT16_MAX)
> +                   " of logical_block_size");

I'm not a native speaker, but "no more than 65536 of logical_block_size"
sounds odd to me. Maybe "65536 times the logical_block_size"?

> +        return false;
> +    }
> +
>      if (conf->opt_io_size % conf->logical_block_size) {
>          error_setg(errp,
>                     "opt_io_size must be a multple of logical_block_size");

Kevin