Re: [Qemu-devel] [PATCH] increase BlockConf.min_io_size type from uint16_t to uint32_t

[Michael Tokarev]

02.05.2012 13:57, Kevin Wolf пишет:
> Am 30.04.2012 17:52, schrieb Michael Tokarev:
>> This value is used currently for virtio-blk only.  It was defined
>> as uint16_t before, which is the same as in kernel<=>user interface
>> (in virtio_blk.h, struct virtio_blk_config).  But the problem is
>> that in kernel<=>user interface the units are sectors (which is
>> usually 512 bytes or more), while in qemu it is in bytes.  However,
>> for, say, md raid5 arrays, it is typical to have actual min_io_size
>> of a host device to be large.  For example, for raid5 device of
>> 3 drives with 64Kb chunk size, that value will be 128Kb, which does
>> not fit in a uint16_t anymore.
>>
>> Increase the value size from 16bits to 32bits for now.
>>
>> But apparently, the kernel<=>user interface needs to be fixed too
>> (while it is much more difficult due to compatibility issues),
>> because even with 512byte units, the 16bit value there will overflow
>> too with quite normal MD RAID configuration.
>>
>> Signed-off-by: Michael Tokarev <address@hidden>
>> ---
>>  block.h |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.h b/block.h
>> index f163e54..cd5ae79 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -425,7 +425,7 @@ typedef struct BlockConf {
>>      BlockDriverState *bs;
>>      uint16_t physical_block_size;
>>      uint16_t logical_block_size;
>> -    uint16_t min_io_size;
>> +    uint32_t min_io_size;
>>      uint32_t opt_io_size;
>>      int32_t bootindex;
>>      uint32_t discard_granularity;
>> @@ -450,7 +450,7 @@ static inline unsigned int 
>> get_physical_block_exp(BlockConf *conf)
>>                            _conf.logical_block_size, 512),               \
>>      DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
>>                            _conf.physical_block_size, 512),              \
>> -    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_INT32("bootindex", _state, _conf.bootindex, -1),        \
>>      DEFINE_PROP_UINT32("discard_granularity", _state, \
> 
> Don't you need an additional check in virtio-blk now so that you don't
> overflow the 16 bit field in the virtio protocol? And I guess the same
> for SCSI, where INQUIRY reports only a 16 bits sector count as well.

That probably should be added too, but I'm not sure I agree these should be
added into virtio-blk.

As I already mentioned, the virtio protocol has the same defect (but there
it is less serious due to usage of larger units).  And that's where the
additional overflow needs to be ELIMINATED, not just checked.  Ie, the
protocol should be changed somehow - the only issue is that I don't know
how to do that now, once it has been in use for quite some time.

Note that now, we don't have ANY checks of this theme whatsoever, at all:
I tried using 128K as min_io_size, and the guest sees it as 0 currently, --
the limits (at least the fact that the value fits in the defined number
of bits) should be checked in the common function which implements these
DEFINE_PROP_UINT* defines.

I'm not sure I can fix all of this in one go, so I went on and fixed the
most specific bug first, without any additional bad side effects.

Besides, as I also already mentioned, this whole structure is used in
virtio-blk *only*, so only virtio driver passes this information into
guest, scsi does not use it.  And again, in case of scsi, the units are
*sectors*, not bytes.

So maybe the solution is to keep this as 16bits but switch to sectors.
But for this, DEFINE_PROP_UINT can't be used anymore, unless we agree
to change user interface TOO and require the user to specify these
values in sectors to start with.

And yes, if SCSI uses 16bit value here, it will have the same issue
as virtio currently have -- pretty normal MD RAID array can't be
expressed using 16bit number of sectors already...  Oh well, but we
at least have a (small) chance to fix virtio.

Thanks,

/mjt