Re: [PATCH v2 0/5] SCSI: fix transfer limits for SCSI passthrough
On Thu, 2020-12-10 at 01:09 +0100, Paolo Bonzini wrote:
> On 09/12/20 15:03, no-re...@patchew.org wrote:
> > Patchew URL:
> > https://patchew.org/QEMU/20201209135355.561745-1-mlevi...@redhat.com/
> >
> >
> >
> > Hi,
> >
> > This series seems to have some coding style problems. See output below for
> > more information:
> >
> > Type: series
> > Message-id: 20201209135355.561745-1-mlevi...@redhat.com
> > Subject: [PATCH v2 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
> > * [new tag] patchew/20201209135355.561745-1-mlevi...@redhat.com
> > -> patchew/20201209135355.561745-1-mlevi...@redhat.com
> > Switched to a new branch 'test'
> > 77c9000 block/scsi: correctly emulate the VPD block limits page
> > 61f49e1 block: use blk_get_max_ioctl_transfer for SCSI passthrough
> > 35c66d6 block: add max_ioctl_transfer to BlockLimits
> > 08ba263 file-posix: add sg_get_max_segments that actually works with sg
> > e9fd749 file-posix: split hdev_refresh_limits from raw_refresh_limits
> >
> > === OUTPUT BEGIN ===
> > 1/5 Checking commit e9fd7498060c (file-posix: split hdev_refresh_limits
> > from raw_refresh_limits)
> > 2/5 Checking commit 08ba263f565d (file-posix: add sg_get_max_segments that
> > actually works with sg)
> > 3/5 Checking commit 35c66d636d83 (block: add max_ioctl_transfer to
> > BlockLimits)
> > 4/5 Checking commit 61f49e1c953b (block: use blk_get_max_ioctl_transfer for
> > SCSI passthrough)
> > 5/5 Checking commit 77c9000b7c30 (block/scsi: correctly emulate the VPD
> > block limits page)
> > ERROR: braces {} are necessary for all arms of this statement
> > #39: FILE: hw/scsi/scsi-generic.c:204:
> > +if (len < r->buflen)
> > [...]
> >
> > total: 1 errors, 0 warnings, 28 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/20201209135355.561745-1-mlevi...@redhat.com/testing.checkpatch/?type=message.
> > ---
> > Email generated automatically by Patchew [https://patchew.org/].
> > Please send your feedback to patchew-de...@redhat.com
> >
>
> Time for v3?
I am waiting a bit to see if anything else pops up,
to avoid doing too much noise.
Best regards,
Maxim Levitsky
>
> Paolo
>
Re: [PATCH v2 0/5] SCSI: fix transfer limits for SCSI passthrough
On 09/12/20 15:03, no-re...@patchew.org wrote:
Patchew URL:
https://patchew.org/QEMU/20201209135355.561745-1-mlevi...@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20201209135355.561745-1-mlevi...@redhat.com
Subject: [PATCH v2 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
* [new tag] patchew/20201209135355.561745-1-mlevi...@redhat.com ->
patchew/20201209135355.561745-1-mlevi...@redhat.com
Switched to a new branch 'test'
77c9000 block/scsi: correctly emulate the VPD block limits page
61f49e1 block: use blk_get_max_ioctl_transfer for SCSI passthrough
35c66d6 block: add max_ioctl_transfer to BlockLimits
08ba263 file-posix: add sg_get_max_segments that actually works with sg
e9fd749 file-posix: split hdev_refresh_limits from raw_refresh_limits
=== OUTPUT BEGIN ===
1/5 Checking commit e9fd7498060c (file-posix: split hdev_refresh_limits from
raw_refresh_limits)
2/5 Checking commit 08ba263f565d (file-posix: add sg_get_max_segments that
actually works with sg)
3/5 Checking commit 35c66d636d83 (block: add max_ioctl_transfer to BlockLimits)
4/5 Checking commit 61f49e1c953b (block: use blk_get_max_ioctl_transfer for
SCSI passthrough)
5/5 Checking commit 77c9000b7c30 (block/scsi: correctly emulate the VPD block
limits page)
ERROR: braces {} are necessary for all arms of this statement
#39: FILE: hw/scsi/scsi-generic.c:204:
+if (len < r->buflen)
[...]
total: 1 errors, 0 warnings, 28 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/20201209135355.561745-1-mlevi...@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
Time for v3?
Paolo
Re: [PATCH v2 0/5] SCSI: fix transfer limits for SCSI passthrough
Patchew URL:
https://patchew.org/QEMU/20201209135355.561745-1-mlevi...@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20201209135355.561745-1-mlevi...@redhat.com
Subject: [PATCH v2 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
* [new tag] patchew/20201209135355.561745-1-mlevi...@redhat.com ->
patchew/20201209135355.561745-1-mlevi...@redhat.com
Switched to a new branch 'test'
77c9000 block/scsi: correctly emulate the VPD block limits page
61f49e1 block: use blk_get_max_ioctl_transfer for SCSI passthrough
35c66d6 block: add max_ioctl_transfer to BlockLimits
08ba263 file-posix: add sg_get_max_segments that actually works with sg
e9fd749 file-posix: split hdev_refresh_limits from raw_refresh_limits
=== OUTPUT BEGIN ===
1/5 Checking commit e9fd7498060c (file-posix: split hdev_refresh_limits from
raw_refresh_limits)
2/5 Checking commit 08ba263f565d (file-posix: add sg_get_max_segments that
actually works with sg)
3/5 Checking commit 35c66d636d83 (block: add max_ioctl_transfer to BlockLimits)
4/5 Checking commit 61f49e1c953b (block: use blk_get_max_ioctl_transfer for
SCSI passthrough)
5/5 Checking commit 77c9000b7c30 (block/scsi: correctly emulate the VPD block
limits page)
ERROR: braces {} are necessary for all arms of this statement
#39: FILE: hw/scsi/scsi-generic.c:204:
+if (len < r->buflen)
[...]
total: 1 errors, 0 warnings, 28 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/20201209135355.561745-1-mlevi...@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v2 0/5] SCSI: fix transfer limits for SCSI passthrough
On Wed, 2020-12-09 at 06:03 -0800, no-re...@patchew.org wrote:
> Patchew URL:
> https://patchew.org/QEMU/20201209135355.561745-1-mlevi...@redhat.com/
>
>
>
> Hi,
>
> This series seems to have some coding style problems. See output below for
> more information:
>
> Type: series
> Message-id: 20201209135355.561745-1-mlevi...@redhat.com
> Subject: [PATCH v2 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
> * [new tag] patchew/20201209135355.561745-1-mlevi...@redhat.com ->
> patchew/20201209135355.561745-1-mlevi...@redhat.com
> Switched to a new branch 'test'
> 77c9000 block/scsi: correctly emulate the VPD block limits page
> 61f49e1 block: use blk_get_max_ioctl_transfer for SCSI passthrough
> 35c66d6 block: add max_ioctl_transfer to BlockLimits
> 08ba263 file-posix: add sg_get_max_segments that actually works with sg
> e9fd749 file-posix: split hdev_refresh_limits from raw_refresh_limits
>
> === OUTPUT BEGIN ===
> 1/5 Checking commit e9fd7498060c (file-posix: split hdev_refresh_limits from
> raw_refresh_limits)
> 2/5 Checking commit 08ba263f565d (file-posix: add sg_get_max_segments that
> actually works with sg)
> 3/5 Checking commit 35c66d636d83 (block: add max_ioctl_transfer to
> BlockLimits)
> 4/5 Checking commit 61f49e1c953b (block: use blk_get_max_ioctl_transfer for
> SCSI passthrough)
> 5/5 Checking commit 77c9000b7c30 (block/scsi: correctly emulate the VPD block
> limits page)
> ERROR: braces {} are necessary for all arms of this statement
> #39: FILE: hw/scsi/scsi-generic.c:204:
> +if (len < r->buflen)
+1 Good bot :-)
Best regards,
Maxim Levitsky
> [...]
>
> total: 1 errors, 0 warnings, 28 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/20201209135355.561745-1-mlevi...@redhat.com/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-de...@redhat.com
[PATCH v2 0/5] SCSI: fix transfer limits for SCSI passthrough
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. V2: fixed an issue in a patch from Tom Yan (thanks), and removed refactoring from last patch according to Paulo's request. 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 | 77 +++--- block/io.c | 2 + block/iscsi.c | 1 + hw/scsi/scsi-generic.c | 12 -- include/block/block_int.h | 4 ++ include/sysemu/block-backend.h | 1 + 7 files changed, 90 insertions(+), 19 deletions(-) -- 2.26.2
