Am 02.05.2012 17:37, schrieb 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.
What lower layers do you mean?
>> 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...
I think the qdev init functions, or whatever they have become with QOM,
are the right place. Looks like they are virtio_blk_init() and
scsi_initfn(). I believe it's possible to fail them.
>> 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.
$ git grep BlockConf
block.h:typedef struct BlockConf {
block.h:} BlockConf;
block.h:static inline unsigned int get_physical_block_exp(BlockConf *conf)
hw/ide/internal.h: BlockConf conf;
hw/s390-virtio-bus.h: BlockConf block;
hw/scsi.h: BlockConf conf;
hw/usb/dev-storage.c: BlockConf conf;
hw/virtio-blk.c: BlockConf *conf;
hw/virtio-blk.c:VirtIODevice *virtio_blk_init(DeviceState *dev,
BlockConf *conf,
hw/virtio-pci.h: BlockConf block;
hw/virtio.h:VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
So there are more users than just virtio-blk.
>> 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?
See above.
>> 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 for the explanation. I wasn't trying to say that your setup is
wrong, I just wasn't aware that such sizes are in use.
Kevin
