| Message ID | 20201104173217.417538-1-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | SCSI: fix transfer limits for SCSI passthrough | expand |
Patchew URL: https://patchew.org/QEMU/20201104173217.417538-1-mlevitsk@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20201104173217.417538-1-mlevitsk@redhat.com
Subject: [PATCH 0/5] SCSI: fix transfer limits for SCSI passthrough
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
b1266b6..3c8c36c master -> master
- [tag update] patchew/20201104160021.2342108-1-ehabkost@redhat.com -> patchew/20201104160021.2342108-1-ehabkost@redhat.com
* [new tag] patchew/20201104173217.417538-1-mlevitsk@redhat.com -> patchew/20201104173217.417538-1-mlevitsk@redhat.com
Switched to a new branch 'test'
bde6371 block/scsi: correctly emulate the VPD block limits page
c4180d6 block: use blk_get_max_ioctl_transfer for SCSI passthrough
9ff7edc block: add max_ioctl_transfer to BlockLimits
dd2f1f7 file-posix: add sg_get_max_segments that actually works with sg
f9ad940 file-posix: split hdev_refresh_limits from raw_refresh_limits
=== OUTPUT BEGIN ===
1/5 Checking commit f9ad9400e011 (file-posix: split hdev_refresh_limits from raw_refresh_limits)
2/5 Checking commit dd2f1f77a5d2 (file-posix: add sg_get_max_segments that actually works with sg)
3/5 Checking commit 9ff7edc31002 (block: add max_ioctl_transfer to BlockLimits)
4/5 Checking commit c4180d6accff (block: use blk_get_max_ioctl_transfer for SCSI passthrough)
5/5 Checking commit bde637139536 (block/scsi: correctly emulate the VPD block limits page)
ERROR: braces {} are necessary for all arms of this statement
#51: FILE: hw/scsi/scsi-generic.c:196:
+ if (page_idx >= r->buflen)
[...]
total: 1 errors, 0 warnings, 53 lines checked
Patch 5/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20201104173217.417538-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Wed, 2020-11-04 at 09:48 -0800, no-reply@patchew.org wrote: > Patchew URL: https://patchew.org/QEMU/20201104173217.417538-1-mlevitsk@redhat.com/ > > > > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > Type: series > Message-id: 20201104173217.417538-1-mlevitsk@redhat.com > Subject: [PATCH 0/5] SCSI: fix transfer limits for SCSI passthrough > > === TEST SCRIPT BEGIN === > #!/bin/bash > git rev-parse base > /dev/null || exit 0 > git config --local diff.renamelimit 0 > git config --local diff.renames True > git config --local diff.algorithm histogram > ./scripts/checkpatch.pl --mailback base.. > === TEST SCRIPT END === > > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 > From https://github.com/patchew-project/qemu > b1266b6..3c8c36c master -> master > - [tag update] patchew/20201104160021.2342108-1-ehabkost@redhat.com -> patchew/20201104160021.2342108-1-ehabkost@redhat.com > * [new tag] patchew/20201104173217.417538-1-mlevitsk@redhat.com -> patchew/20201104173217.417538-1-mlevitsk@redhat.com > Switched to a new branch 'test' > bde6371 block/scsi: correctly emulate the VPD block limits page > c4180d6 block: use blk_get_max_ioctl_transfer for SCSI passthrough > 9ff7edc block: add max_ioctl_transfer to BlockLimits > dd2f1f7 file-posix: add sg_get_max_segments that actually works with sg > f9ad940 file-posix: split hdev_refresh_limits from raw_refresh_limits > > === OUTPUT BEGIN === > 1/5 Checking commit f9ad9400e011 (file-posix: split hdev_refresh_limits from raw_refresh_limits) > 2/5 Checking commit dd2f1f77a5d2 (file-posix: add sg_get_max_segments that actually works with sg) > 3/5 Checking commit 9ff7edc31002 (block: add max_ioctl_transfer to BlockLimits) > 4/5 Checking commit c4180d6accff (block: use blk_get_max_ioctl_transfer for SCSI passthrough) > 5/5 Checking commit bde637139536 (block/scsi: correctly emulate the VPD block limits page) > ERROR: braces {} are necessary for all arms of this statement > #51: FILE: hw/scsi/scsi-generic.c:196: > + if (page_idx >= r->buflen) Sorry about that. Triple checked this code for correctness, but didn't run checkpatch on the last revision :-( Best regards, Maxim Levitsky > [...] > > total: 1 errors, 0 warnings, 53 lines checked > > Patch 5/5 has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. > > === OUTPUT END === > > Test command exited with code: 1 > > > The full log is available at > http://patchew.org/logs/20201104173217.417538-1-mlevitsk@redhat.com/testing.checkpatch/?type=message. > --- > Email generated automatically by Patchew [https://patchew.org/]. > Please send your feedback to patchew-devel@redhat.com
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. Best regards, Maxim Levitsky
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