From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50734) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SPaen-0000Zc-FS for qemu-devel@nongnu.org; Wed, 02 May 2012 10:35:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SPaeg-0004gS-I9 for qemu-devel@nongnu.org; Wed, 02 May 2012 10:35:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54980) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SPaeg-0004gM-9L for qemu-devel@nongnu.org; Wed, 02 May 2012 10:35:46 -0400 Message-ID: <4FA1462D.7020108@redhat.com> Date: Wed, 02 May 2012 16:35:25 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1335801160-321-1-git-send-email-mjt@msgid.tls.msk.ru> <4FA104F2.6020106@redhat.com> <4FA1079C.80105@msgid.tls.msk.ru> In-Reply-To: <4FA1079C.80105@msgid.tls.msk.ru> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] increase BlockConf.min_io_size type from uint16_t to uint32_t List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: qemu-devel@nongnu.org, Anthony Liguori , Stefan Hajnoczi Am 02.05.2012 12:08, schrieb Michael Tokarev: > 02.05.2012 13:57, Kevin Wolf =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >> 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<=3D>user interface >>> (in virtio_blk.h, struct virtio_blk_config). But the problem is >>> that in kernel<=3D>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<=3D>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 >>> --- >>> 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. >=20 > That probably should be added too, but I'm not sure I agree these shoul= d be > added into virtio-blk. >=20 > As I already mentioned, the virtio protocol has the same defect (but th= ere > 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 kno= w > 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. > Note that now, we don't have ANY checks of this theme whatsoever, at al= l: > 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 thes= e > DEFINE_PROP_UINT* defines. Looks like a bug in the qdev property parser to me. 128k is obviously an invalid value for a 16 bit property. 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. > I'm not sure I can fix all of this in one go, so I went on and fixed th= e > most specific bug first, without any additional bad side effects. >=20 > 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. Yes, same as for virtio-blk. 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. > 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. That wouldn't be a good interface. Let's just take a 32 bit number and add the checks in the devices. > 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. 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. Kevin