Re: [Qemu-devel] [PATCH] block/iscsi: use 16 byte CDBs only when necessary

[Peter Lieven]

> Am 03.09.2014 um 14:31 schrieb Stefan Hajnoczi <address@hidden>:
> 
>> On Wed, Sep 03, 2014 at 10:09:21AM +0200, Peter Lieven wrote:
>> 
>> 
>>> Am 02.09.2014 um 21:30 schrieb Peter Lieven <address@hidden>:
>>> 
>>> Looking at the code, is it possible that not the guest is causing trouble 
>>> here, but
>>> multiwrite_merge code?
>>> 
>>> From what I see the only limit it has when merging requests is the number 
>>> of IOVs.
>>> 
>>> 
>>> Any thoughts?
>>> 
>>> Mine are:
>>> a) Introducing bs->bl.max_request_size and set merge = 0 if the result 
>>> would be too big. Default
>>> max request size to 32768 sectors (see below).
>>> b) Hardcoding the limit in multiwrite_merge for now limiting the merged 
>>> size to 16MB (32768 sectors).
>>>    Which is the limit we already use in bdrv_co_discard and 
>>> bdrv_co_write_zeroes if we don't know
>>>    better.
>> 
>> or c) disabling multiwrite merge for RAW or only iSCSI completely.
> 
> I think you're right, multiwrite could merge a larger request than the
> storage device can handle.
> 
> Do you want to implement a)?

i can try, but for now it would be limited to report the max xfer len in the 
block limits vpd and avoiding merges that are too big. a full implementation 
would also handle big requests coming in from the OS and slice them in 
bdrv_read/write. but currently i have no time for that.

i also think that the default should be no limit. we should only set a limit if 
we have a hint what the actual limit is. for iscsi it is in the block limits 
vpd and 0xffffff for 10byte CDBs and 0xffffffff for 16byte CDBs.

> 
> b) is okayish.  c) is too hacky and might result in performance
> regressions because it changes the I/O pattern for existing guests.

yes, but is this something we are allowed to do for all protocols? it increases 
latency and isnt it the task of the guest OS to merge things?

Peter