Re: [Qemu-devel] [PATCH 1/3] hw/block: Introduce rotational qdev property

[Kevin Wolf]

Am 06.05.2017 um 14:43 hat Aurelien Jarno geschrieben:
> The Linux kernel uses different I/O scheduler depending if the block
> device is a rotational device or not. Also it uses rotational devices
> to add entropy to the random pool.
> 
> This patch add a rotational qdev property so that the block device can
> be configured as a rotational device or a non-rotational device. Default
> to true to not change the default behavior.
> 
> Signed-off-by: Aurelien Jarno <address@hidden>
> ---
>  blockdev.c               | 4 ++++
>  include/hw/block/block.h | 4 +++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 4d8cdedd54..c914420641 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -4037,6 +4037,10 @@ QemuOptsList qemu_common_drive_opts = {
>              .name = BDRV_OPT_READ_ONLY,
>              .type = QEMU_OPT_BOOL,
>              .help = "open drive file as read-only",
> +        },{
> +            .name = "rotational",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "rotational drive (off, on)",
>          },
>  

This hunk is wrong (and unnecessary). It makes the -drive parser parse
rotational=... options without doing anything with them, so instead of
correctly producing an error that this isn't a block driver option, it
would be silently ignored.

-drive has a few legacy options that affect qdev devices, but we don't
want to add anything new there; people can just use -device. So the fix
for your patch is simply dropping this hunk.

> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index f3f6e8ef02..304a579eca 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -27,6 +27,7 @@ typedef struct BlockConf {
>      uint32_t cyls, heads, secs;
>      OnOffAuto wce;
>      bool share_rw;
> +    bool rotational;
>      BlockdevOnError rerror;
>      BlockdevOnError werror;
>  } BlockConf;
> @@ -56,7 +57,8 @@ static inline unsigned int get_physical_block_exp(BlockConf 
> *conf)
>                         _conf.discard_granularity, -1), \
>      DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \
>                              ON_OFF_AUTO_AUTO), \
> -    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
> +    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false), \
> +    DEFINE_PROP_BOOL("rotational", _state, _conf.rotational, true)
>  
>  #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
>      DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \

Also not sure about this one. It adds the property to all block devices,
but only IDE and SCSI will actually look at it. Maybe it would be better
to add this property only to the individual devices that actually
support it.

Kevin