Re: [PATCH 0/5] SCSI: fix transfer limits for SCSI passthrough

[Paolo Bonzini <pbonzini@redhat.com>]

On 03/12/20 13:49, Maxim Levitsky wrote:
> On Wed, 2020-11-04 at 19:32 +0200, Maxim Levitsky wrote:
>> This patch series attempts to provide a solution to the problem of the transfer
>> limits of the raw file driver (host_device/file-posix), some of which I
>> already tried to fix in the past.
>>
>> I included 2 patches from Tom Yan which fix two issues with reading the limits
>> correctly from the */dev/sg* character devices in the first place.
>>
>> The only change to these patches is that I tweaked a bit the comments in the
>> source to better document the /dev/sg quirks.
>>
>> The other two patches in this series split the max transfer limits that qemu
>> block devices expose in two:
>> One limit is for the regular IO, and another is for the SG_IO (aka bdrv_*_ioctl),
>> and the two device drivers (scsi-block and scsi-generic) that use the later
>> are switched to the new interface.
>>
>> This should ensure that the raw driver can still advertise the unlimited
>> transfer  length, unless it is used for SG_IO, because that yields the highest
>> performance.
>>
>> Also I include a somewhat unrelated fix to a bug I found in qemu's
>> SCSI passthrough while testing this:
>> When qemu emulates the VPD block limit page, for a SCSI device that doesn't
>> implement it, it doesn't really advertise the emulated page to the guest.
>>
>> I tested this by doing both regular and SG_IO passthrough of my
>> USB SD card reader.
>>
>> That device turned out to be a perfect device for the task, since it has max
>> transfer size of 1024 blocks (512K), and it enforces it.
>>
>> Also it didn't implement the VPD block limits page,
>> (transfer size limit probably comes from something USB related) which triggered
>> the unrelated bug.
>>
>> I was able to see IO errors without the patches, and the wrong max transfer
>> size in the guest, and with patches both issues were gone.
>>
>> I also found an unrelated issue in /dev/sg passthrough in the kernel.
>> It turns out that in-kernel driver has a limitation of 16 requests in flight,
>> regardless of what underlying device supports.
>>
>> With a large multi-threaded fio job  and a debug print in qemu, it is easy to
>> see it, although the errors don't do much harm to the guest as it retries the
>> IO, and eventually succeed.
>> It is an open question if this should be solved.
>>
>> Maxim Levitsky (3):
>>    block: add max_ioctl_transfer to BlockLimits
>>    block: use blk_get_max_ioctl_transfer for SCSI passthrough
>>    block/scsi: correctly emulate the VPD block limits page
>>
>> Tom Yan (2):
>>    file-posix: split hdev_refresh_limits from raw_refresh_limits
>>    file-posix: add sg_get_max_segments that actually works with sg
>>
>>   block/block-backend.c          | 12 ++++++
>>   block/file-posix.c             | 79 +++++++++++++++++++++++++++-------
>>   block/io.c                     |  2 +
>>   block/iscsi.c                  |  1 +
>>   hw/scsi/scsi-generic.c         | 32 ++++++++------
>>   include/block/block_int.h      |  4 ++
>>   include/sysemu/block-backend.h |  1 +
>>   7 files changed, 103 insertions(+), 28 deletions(-)
>>
>> -- 
>> 2.26.2
>>
> Very gentle ping on this path series.

Hi, I was waiting for a respin followin Tom Yan's remark to patch 1.

For patch 5, however, I prefer a minimal change because the existing 
code is trying to update the contents of the page even if the outcome is 
then truncated.

Paolo
Paolo