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

[Michael Tokarev]

On 02.05.2012 18:35, Kevin Wolf wrote:
[]
>> 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.
> 
> Even if you create a new version of the protocol (introduce a new
> feature flag or whatever), newer qemus will still have to deal with
> older guests and vice versa.

Sure.  And for these, the checks indeed should be done in lower layers.

[]
> But now that you're extending the property value to 32 bits, but only 25
> bits can be really used, the property type doesn't even theoretically
> suffice as a check.

So, what do you propose?  To add a check into virtio-blk.c (and into
whatever else place is using this variable) too?  If yes, and indeed
it appears to be the right thing to do, care to show me the right
place please, I'm not very familiar with that code...

[]
> But I can't see which structure is only used by virtio-blk, though. The
> min_io_size property is contained in DEFINE_BLOCK_PROPERTIES(), which is
> used by every qdevified block device. Most of them ignore min_io_size,
> but virtio-blk and scsi-disk don't.

I mean the BlockConf struct.  It isn't used anywhere but in virtio-blk.c.

But again, since I'm not familiar with the code, I might be wrong.

[]

> That wouldn't be a good interface. Let's just take a 32 bit number and
> add the checks in the devices.

That's fine.  The only thing left to do is to find the proper places for
the checks.  Help?

[]
> Just curious... What values do you want to use? The 32 MB minimum I/O
> size that are possible with 16 bits and 512 byte sectors already sounds
> insanely large.

I don't "use" any values.  I merely pass whatever is defined on my systems
down to the guest.

md layer in kernel - raid4, raid5 and raid6 implementation - sets min_io_size
to the chunk size, and opt_io_size to "stripe size".  It is not uncommon at
all to have chunk size = 1Mb or more.   I've no idea how useful this information
is, but at least with it present (my small raid5 array has 256Kb chunk size),
xfs created in the guest performs much faster than without this information
(which means usual 512 there).

This is how I discovered the issue - I wondered why xfs created in the guest
is so much slower than the same xfs but created on host.  I/O sizes immediately
come to min, so I added these, but it was still slow.  So I noticed min_io_size
isn't passed correctly, increased the size of this type, and voila, xfs
created in guest now behaves as fast as created on host.  Something like that
anyway.

There's an obvious bug in there, but it is not obvious for me where/how it 
should
be fixed.  Maybe the sizes used by md raid5 are insane, that's arguable 
ofcourse,
but this is what is in use now (and since the day the i/o sizes were added to
md to start with), and this is what makes things fly.

Thanks,

/mjt