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

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

[Maxim Levitsky]

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

[PATCH 1/5] file-posix: split hdev_refresh_limits from raw_refresh_limits

[Maxim Levitsky]

From: Tom Yan <tom.ty89@gmail.com>

We can and should get max transfer length and max segments for all host
devices / cdroms (on Linux).

Also use MIN_NON_ZERO instead when we clamp max transfer length against
max segments.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/file-posix.c | 61 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 16 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index c63926d592..6581f41b2b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1162,6 +1162,12 @@ static void raw_reopen_abort(BDRVReopenState *state)
 
 static int sg_get_max_transfer_length(int fd)
 {
+    /*
+     * BLKSECTGET for /dev/sg* character devices incorrectly returns
+     * the max transfer size in bytes (rather than in blocks).
+     * Also note that /dev/sg* doesn't support BLKSSZGET ioctl.
+     */
+
 #ifdef BLKSECTGET
     int max_bytes = 0;
 
@@ -1175,7 +1181,24 @@ static int sg_get_max_transfer_length(int fd)
 #endif
 }
 
-static int sg_get_max_segments(int fd)
+static int get_max_transfer_length(int fd)
+{
+#if defined(BLKSECTGET) && defined(BLKSSZGET)
+    int sect = 0;
+    int ssz = 0;
+
+    if (ioctl(fd, BLKSECTGET, &sect) == 0 &&
+        ioctl(fd, BLKSSZGET, &ssz) == 0) {
+        return sect * ssz;
+    } else {
+        return -errno;
+    }
+#else
+    return -ENOSYS;
+#endif
+}
+
+static int get_max_segments(int fd)
 {
 #ifdef CONFIG_LINUX
     char buf[32];
@@ -1230,23 +1253,29 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
 
-    if (bs->sg) {
-        int ret = sg_get_max_transfer_length(s->fd);
+    raw_probe_alignment(bs, s->fd, errp);
+    bs->bl.min_mem_alignment = s->buf_align;
+    bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
+}
 
-        if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
-            bs->bl.max_transfer = pow2floor(ret);
-        }
+static void hdev_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    BDRVRawState *s = bs->opaque;
 
-        ret = sg_get_max_segments(s->fd);
-        if (ret > 0) {
-            bs->bl.max_transfer = MIN(bs->bl.max_transfer,
-                                      ret * qemu_real_host_page_size);
-        }
+    int ret = bs->sg ? sg_get_max_transfer_length(s->fd) :
+                       get_max_transfer_length(s->fd);
+
+    if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
+        bs->bl.max_transfer = pow2floor(ret);
     }
 
-    raw_probe_alignment(bs, s->fd, errp);
-    bs->bl.min_mem_alignment = s->buf_align;
-    bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
+    ret = get_max_segments(s->fd);
+    if (ret > 0) {
+        bs->bl.max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
+                                           ret * qemu_real_host_page_size);
+    }
+
+    raw_refresh_limits(bs, errp);
 }
 
 static int check_for_dasd(int fd)
@@ -3600,7 +3629,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_co_pdiscard       = hdev_co_pdiscard,
     .bdrv_co_copy_range_from = raw_co_copy_range_from,
     .bdrv_co_copy_range_to  = raw_co_copy_range_to,
-    .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_refresh_limits = hdev_refresh_limits,
     .bdrv_io_plug = raw_aio_plug,
     .bdrv_io_unplug = raw_aio_unplug,
     .bdrv_attach_aio_context = raw_aio_attach_aio_context,
@@ -3724,7 +3753,7 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_co_preadv         = raw_co_preadv,
     .bdrv_co_pwritev        = raw_co_pwritev,
     .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
-    .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_refresh_limits = hdev_refresh_limits,
     .bdrv_io_plug = raw_aio_plug,
     .bdrv_io_unplug = raw_aio_unplug,
     .bdrv_attach_aio_context = raw_aio_attach_aio_context,
-- 
2.26.2

[PATCH 2/5] file-posix: add sg_get_max_segments that actually works with sg

[Maxim Levitsky]

From: Tom Yan <tom.ty89@gmail.com>

sg devices have different major/minor than their corresponding
block devices. Using sysfs to get max segments never really worked
for them.

Fortunately the sg driver provides an ioctl to get sg_tablesize,
which is apparently equivalent to max segments.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/file-posix.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 6581f41b2b..c4df757504 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1181,6 +1181,26 @@ static int sg_get_max_transfer_length(int fd)
 #endif
 }
 
+static int sg_get_max_segments(int fd)
+{
+    /*
+     * /dev/sg* character devices report 'max_segments' via
+     * SG_GET_SG_TABLESIZE ioctl
+     */
+
+#ifdef SG_GET_SG_TABLESIZE
+    long max_segments = 0;
+
+    if (ioctl(fd, SG_GET_SG_TABLESIZE, &max_segments) == 0) {
+        return max_segments;
+    } else {
+        return -errno;
+    }
+#else
+    return -ENOSYS;
+#endif
+}
+
 static int get_max_transfer_length(int fd)
 {
 #if defined(BLKSECTGET) && defined(BLKSSZGET)
@@ -1269,7 +1289,7 @@ static void hdev_refresh_limits(BlockDriverState *bs, Error **errp)
         bs->bl.max_transfer = pow2floor(ret);
     }
 
-    ret = get_max_segments(s->fd);
+    ret = bs->sg ? sg_get_max_segments(s->fd) : get_max_segments(s->fd);
     if (ret > 0) {
         bs->bl.max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
                                            ret * qemu_real_host_page_size);
-- 
2.26.2

[PATCH 3/5] block: add max_ioctl_transfer to BlockLimits

[Maxim Levitsky]

Maximum transfer size when accessing a kernel block device is only relevant
when using SCSI passthrough (SG_IO ioctl) since only in this case the requests
are passed directly to underlying hardware with no pre-processing.
Same is true when using /dev/sg* character devices (which only support SG_IO)

Therefore split the block driver's advertized max transfer size by
the regular max transfer size, and the max transfer size for SCSI passthrough
(the new max_ioctl_transfer field)

In the next patch, the qemu block drivers that support SCSI passthrough
will set the max_ioctl_transfer field, and simultaneously, the block devices
that implement scsi passthrough will switch to 'blk_get_max_ioctl_transfer' to
query and to pass it to the guest.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/block-backend.c          | 12 ++++++++++++
 block/io.c                     |  2 ++
 include/block/block_int.h      |  4 ++++
 include/sysemu/block-backend.h |  1 +
 4 files changed, 19 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index ce78d30794..c1d149a755 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1938,6 +1938,18 @@ uint32_t blk_get_max_transfer(BlockBackend *blk)
     return MIN_NON_ZERO(max, INT_MAX);
 }
 
+/* Returns the maximum transfer length, for SCSI passthrough */
+uint32_t blk_get_max_ioctl_transfer(BlockBackend *blk)
+{
+    BlockDriverState *bs = blk_bs(blk);
+    uint32_t max = 0;
+
+    if (bs) {
+        max = bs->bl.max_ioctl_transfer;
+    }
+    return MIN_NON_ZERO(max, INT_MAX);
+}
+
 int blk_get_max_iov(BlockBackend *blk)
 {
     return blk->root->bs->bl.max_iov;
diff --git a/block/io.c b/block/io.c
index ec5e152bb7..3eae176992 100644
--- a/block/io.c
+++ b/block/io.c
@@ -126,6 +126,8 @@ static void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src)
 {
     dst->opt_transfer = MAX(dst->opt_transfer, src->opt_transfer);
     dst->max_transfer = MIN_NON_ZERO(dst->max_transfer, src->max_transfer);
+    dst->max_ioctl_transfer = MIN_NON_ZERO(dst->max_ioctl_transfer,
+                                        src->max_ioctl_transfer);
     dst->opt_mem_alignment = MAX(dst->opt_mem_alignment,
                                  src->opt_mem_alignment);
     dst->min_mem_alignment = MAX(dst->min_mem_alignment,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 38cad9d15c..b198165114 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -678,6 +678,10 @@ typedef struct BlockLimits {
      * clamped down. */
     uint32_t max_transfer;
 
+    /* Maximal transfer length for SCSI passthrough (ioctl interface) */
+    uint32_t max_ioctl_transfer;
+
+
     /* memory alignment, in bytes so that no bounce buffer is needed */
     size_t min_mem_alignment;
 
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8203d7f6f9..b019a37b7a 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -203,6 +203,7 @@ void blk_eject(BlockBackend *blk, bool eject_flag);
 int blk_get_flags(BlockBackend *blk);
 uint32_t blk_get_request_alignment(BlockBackend *blk);
 uint32_t blk_get_max_transfer(BlockBackend *blk);
+uint32_t blk_get_max_ioctl_transfer(BlockBackend *blk);
 int blk_get_max_iov(BlockBackend *blk);
 void blk_set_guest_block_size(BlockBackend *blk, int align);
 void *blk_try_blockalign(BlockBackend *blk, size_t size);
-- 
2.26.2

[PATCH 4/5] block: use blk_get_max_ioctl_transfer for SCSI passthrough

[Maxim Levitsky]

Switch file-posix to expose only the max_ioctl_transfer limit.

Let the iscsi driver work as it did before since it is bound by the transfer
limit in both regular read/write and in SCSI passthrough case.

Switch the scsi-disk and scsi-block drivers to read the SG max transfer limits
using the new blk_get_max_ioctl_transfer interface.


Fixes: 867eccfed8 ("file-posix: Use max transfer length/segment count only for SCSI passthrough")
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/file-posix.c     | 4 ++--
 block/iscsi.c          | 1 +
 hw/scsi/scsi-generic.c | 4 ++--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index c4df757504..edba8fc86d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1286,12 +1286,12 @@ static void hdev_refresh_limits(BlockDriverState *bs, Error **errp)
                        get_max_transfer_length(s->fd);
 
     if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
-        bs->bl.max_transfer = pow2floor(ret);
+        bs->bl.max_ioctl_transfer = pow2floor(ret);
     }
 
     ret = bs->sg ? sg_get_max_segments(s->fd) : get_max_segments(s->fd);
     if (ret > 0) {
-        bs->bl.max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
+        bs->bl.max_ioctl_transfer = MIN_NON_ZERO(bs->bl.max_ioctl_transfer,
                                            ret * qemu_real_host_page_size);
     }
 
diff --git a/block/iscsi.c b/block/iscsi.c
index e30a7e3606..3685da2971 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2065,6 +2065,7 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
 
     if (max_xfer_len * block_size < INT_MAX) {
         bs->bl.max_transfer = max_xfer_len * iscsilun->block_size;
+        bs->bl.max_ioctl_transfer = bs->bl.max_transfer;
     }
 
     if (iscsilun->lbp.lbpu) {
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 2cb23ca891..6df67bf889 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -167,7 +167,7 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s)
         page = r->req.cmd.buf[2];
         if (page == 0xb0) {
             uint32_t max_transfer =
-                blk_get_max_transfer(s->conf.blk) / s->blocksize;
+                blk_get_max_ioctl_transfer(s->conf.blk) / s->blocksize;
 
             assert(max_transfer);
             stl_be_p(&r->buf[8], max_transfer);
@@ -210,7 +210,7 @@ static int scsi_generic_emulate_block_limits(SCSIGenericReq *r, SCSIDevice *s)
     uint8_t buf[64];
 
     SCSIBlockLimits bl = {
-        .max_io_sectors = blk_get_max_transfer(s->conf.blk) / s->blocksize
+        .max_io_sectors = blk_get_max_ioctl_transfer(s->conf.blk) / s->blocksize
     };
 
     memset(r->buf, 0, r->buflen);
-- 
2.26.2

[PATCH 5/5] block/scsi: correctly emulate the VPD block limits page

[Maxim Levitsky]

When the device doesn't support the VPD block limits page, we emulate it even
for SCSI passthrough.

As a part of the emulation we need to add it to the 'Supported VPD Pages'

The code that does this adds it to the page, but it doesn't increase the length
of the data to be copied to the guest, thus the guest never sees the VPD block
limits page as supported.

Bump the transfer size by 1 in this case.

I also refactored the code a bit, and I hopefully didn't introduce
another buffer overflow to this code...

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/scsi/scsi-generic.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 6df67bf889..387d885aee 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -134,9 +134,9 @@ static int execute_command(BlockBackend *blk,
     return 0;
 }
 
-static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s)
+static int scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s, int len)
 {
-    uint8_t page, page_idx;
+    uint8_t page;
 
     /*
      *  EVPD set to zero returns the standard INQUIRY data.
@@ -188,20 +188,26 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s)
              * right place with an in-place insert.  When the while loop
              * begins the device response is at r[0] to r[page_idx - 1].
              */
-            page_idx = lduw_be_p(r->buf + 2) + 4;
-            page_idx = MIN(page_idx, r->buflen);
+            uint16_t page_len = lduw_be_p(r->buf + 2) + 4;
+
+            /* pointer to first byte after the page that device gave us */
+            uint16_t page_idx = page_len;
+
+            if (page_idx >= r->buflen)
+                return len;
+
             while (page_idx > 4 && r->buf[page_idx - 1] >= 0xb0) {
-                if (page_idx < r->buflen) {
-                    r->buf[page_idx] = r->buf[page_idx - 1];
-                }
+                r->buf[page_idx] = r->buf[page_idx - 1];
                 page_idx--;
             }
-            if (page_idx < r->buflen) {
-                r->buf[page_idx] = 0xb0;
-            }
+            r->buf[page_idx] = 0xb0;
+
+            /* increase the page len field */
             stw_be_p(r->buf + 2, lduw_be_p(r->buf + 2) + 1);
+            len++;
         }
     }
+    return len;
 }
 
 static int scsi_generic_emulate_block_limits(SCSIGenericReq *r, SCSIDevice *s)
@@ -316,7 +322,7 @@ static void scsi_read_complete(void * opaque, int ret)
         }
     }
     if (r->req.cmd.buf[0] == INQUIRY) {
-        scsi_handle_inquiry_reply(r, s);
+        len = scsi_handle_inquiry_reply(r, s, len);
     }
 
 req_complete:
-- 
2.26.2

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

[no-reply]

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

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

[Maxim Levitsky]

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

Re: [PATCH 1/5] file-posix: split hdev_refresh_limits from raw_refresh_limits

[Tom Yan]

Actually I made a mistake in this. BLKSECTGET (the one in the block
layer) returns the number of "sectors", which is "defined" as 512-byte
block. So we shouldn't use BLKSSZGET here, but simply 512 (1 << 9).
See logical_to_sectors() in sd.h of the kernel.

On Thu, 5 Nov 2020 at 01:32, Maxim Levitsky <mlevitsk@redhat.com> wrote:
>
> From: Tom Yan <tom.ty89@gmail.com>
>
> We can and should get max transfer length and max segments for all host
> devices / cdroms (on Linux).
>
> Also use MIN_NON_ZERO instead when we clamp max transfer length against
> max segments.
>
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/file-posix.c | 61 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 16 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index c63926d592..6581f41b2b 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1162,6 +1162,12 @@ static void raw_reopen_abort(BDRVReopenState *state)
>
>  static int sg_get_max_transfer_length(int fd)
>  {
> +    /*
> +     * BLKSECTGET for /dev/sg* character devices incorrectly returns
> +     * the max transfer size in bytes (rather than in blocks).
> +     * Also note that /dev/sg* doesn't support BLKSSZGET ioctl.
> +     */
> +
>  #ifdef BLKSECTGET
>      int max_bytes = 0;
>
> @@ -1175,7 +1181,24 @@ static int sg_get_max_transfer_length(int fd)
>  #endif
>  }
>
> -static int sg_get_max_segments(int fd)
> +static int get_max_transfer_length(int fd)
> +{
> +#if defined(BLKSECTGET) && defined(BLKSSZGET)
> +    int sect = 0;
> +    int ssz = 0;
> +
> +    if (ioctl(fd, BLKSECTGET, &sect) == 0 &&
> +        ioctl(fd, BLKSSZGET, &ssz) == 0) {
> +        return sect * ssz;
> +    } else {
> +        return -errno;
> +    }
> +#else
> +    return -ENOSYS;
> +#endif
> +}
> +
> +static int get_max_segments(int fd)
>  {
>  #ifdef CONFIG_LINUX
>      char buf[32];
> @@ -1230,23 +1253,29 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
>
> -    if (bs->sg) {
> -        int ret = sg_get_max_transfer_length(s->fd);
> +    raw_probe_alignment(bs, s->fd, errp);
> +    bs->bl.min_mem_alignment = s->buf_align;
> +    bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
> +}
>
> -        if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
> -            bs->bl.max_transfer = pow2floor(ret);
> -        }
> +static void hdev_refresh_limits(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVRawState *s = bs->opaque;
>
> -        ret = sg_get_max_segments(s->fd);
> -        if (ret > 0) {
> -            bs->bl.max_transfer = MIN(bs->bl.max_transfer,
> -                                      ret * qemu_real_host_page_size);
> -        }
> +    int ret = bs->sg ? sg_get_max_transfer_length(s->fd) :
> +                       get_max_transfer_length(s->fd);
> +
> +    if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
> +        bs->bl.max_transfer = pow2floor(ret);
>      }
>
> -    raw_probe_alignment(bs, s->fd, errp);
> -    bs->bl.min_mem_alignment = s->buf_align;
> -    bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
> +    ret = get_max_segments(s->fd);
> +    if (ret > 0) {
> +        bs->bl.max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
> +                                           ret * qemu_real_host_page_size);
> +    }
> +
> +    raw_refresh_limits(bs, errp);
>  }
>
>  static int check_for_dasd(int fd)
> @@ -3600,7 +3629,7 @@ static BlockDriver bdrv_host_device = {
>      .bdrv_co_pdiscard       = hdev_co_pdiscard,
>      .bdrv_co_copy_range_from = raw_co_copy_range_from,
>      .bdrv_co_copy_range_to  = raw_co_copy_range_to,
> -    .bdrv_refresh_limits = raw_refresh_limits,
> +    .bdrv_refresh_limits = hdev_refresh_limits,
>      .bdrv_io_plug = raw_aio_plug,
>      .bdrv_io_unplug = raw_aio_unplug,
>      .bdrv_attach_aio_context = raw_aio_attach_aio_context,
> @@ -3724,7 +3753,7 @@ static BlockDriver bdrv_host_cdrom = {
>      .bdrv_co_preadv         = raw_co_preadv,
>      .bdrv_co_pwritev        = raw_co_pwritev,
>      .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
> -    .bdrv_refresh_limits = raw_refresh_limits,
> +    .bdrv_refresh_limits = hdev_refresh_limits,
>      .bdrv_io_plug = raw_aio_plug,
>      .bdrv_io_unplug = raw_aio_unplug,
>      .bdrv_attach_aio_context = raw_aio_attach_aio_context,
> --
> 2.26.2
>

Re: [PATCH 1/5] file-posix: split hdev_refresh_limits from raw_refresh_limits

[Maxim Levitsky]

On Thu, 2020-11-05 at 13:44 +0800, Tom Yan wrote:
> Actually I made a mistake in this. BLKSECTGET (the one in the block
> layer) returns the number of "sectors", which is "defined" as 512-byte
> block. So we shouldn't use BLKSSZGET here, but simply 512 (1 << 9).
> See logical_to_sectors() in sd.h of the kernel.
I see it now. I'll respin the patches in a few days I guess after the rest
of the patches are reviewed.

Best regards,
	Maxim Levitsky

> 
> On Thu, 5 Nov 2020 at 01:32, Maxim Levitsky <mlevitsk@redhat.com> wrote:
> > From: Tom Yan <tom.ty89@gmail.com>
> > 
> > We can and should get max transfer length and max segments for all host
> > devices / cdroms (on Linux).
> > 
> > Also use MIN_NON_ZERO instead when we clamp max transfer length against
> > max segments.
> > 
> > Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/file-posix.c | 61 ++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 45 insertions(+), 16 deletions(-)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index c63926d592..6581f41b2b 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1162,6 +1162,12 @@ static void raw_reopen_abort(BDRVReopenState *state)
> > 
> >  static int sg_get_max_transfer_length(int fd)
> >  {
> > +    /*
> > +     * BLKSECTGET for /dev/sg* character devices incorrectly returns
> > +     * the max transfer size in bytes (rather than in blocks).
> > +     * Also note that /dev/sg* doesn't support BLKSSZGET ioctl.
> > +     */
> > +
> >  #ifdef BLKSECTGET
> >      int max_bytes = 0;
> > 
> > @@ -1175,7 +1181,24 @@ static int sg_get_max_transfer_length(int fd)
> >  #endif
> >  }
> > 
> > -static int sg_get_max_segments(int fd)
> > +static int get_max_transfer_length(int fd)
> > +{
> > +#if defined(BLKSECTGET) && defined(BLKSSZGET)
> > +    int sect = 0;
> > +    int ssz = 0;
> > +
> > +    if (ioctl(fd, BLKSECTGET, &sect) == 0 &&
> > +        ioctl(fd, BLKSSZGET, &ssz) == 0) {
> > +        return sect * ssz;
> > +    } else {
> > +        return -errno;
> > +    }
> > +#else
> > +    return -ENOSYS;
> > +#endif
> > +}
> > +
> > +static int get_max_segments(int fd)
> >  {
> >  #ifdef CONFIG_LINUX
> >      char buf[32];
> > @@ -1230,23 +1253,29 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
> >  {
> >      BDRVRawState *s = bs->opaque;
> > 
> > -    if (bs->sg) {
> > -        int ret = sg_get_max_transfer_length(s->fd);
> > +    raw_probe_alignment(bs, s->fd, errp);
> > +    bs->bl.min_mem_alignment = s->buf_align;
> > +    bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
> > +}
> > 
> > -        if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
> > -            bs->bl.max_transfer = pow2floor(ret);
> > -        }
> > +static void hdev_refresh_limits(BlockDriverState *bs, Error **errp)
> > +{
> > +    BDRVRawState *s = bs->opaque;
> > 
> > -        ret = sg_get_max_segments(s->fd);
> > -        if (ret > 0) {
> > -            bs->bl.max_transfer = MIN(bs->bl.max_transfer,
> > -                                      ret * qemu_real_host_page_size);
> > -        }
> > +    int ret = bs->sg ? sg_get_max_transfer_length(s->fd) :
> > +                       get_max_transfer_length(s->fd);
> > +
> > +    if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
> > +        bs->bl.max_transfer = pow2floor(ret);
> >      }
> > 
> > -    raw_probe_alignment(bs, s->fd, errp);
> > -    bs->bl.min_mem_alignment = s->buf_align;
> > -    bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
> > +    ret = get_max_segments(s->fd);
> > +    if (ret > 0) {
> > +        bs->bl.max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
> > +                                           ret * qemu_real_host_page_size);
> > +    }
> > +
> > +    raw_refresh_limits(bs, errp);
> >  }
> > 
> >  static int check_for_dasd(int fd)
> > @@ -3600,7 +3629,7 @@ static BlockDriver bdrv_host_device = {
> >      .bdrv_co_pdiscard       = hdev_co_pdiscard,
> >      .bdrv_co_copy_range_from = raw_co_copy_range_from,
> >      .bdrv_co_copy_range_to  = raw_co_copy_range_to,
> > -    .bdrv_refresh_limits = raw_refresh_limits,
> > +    .bdrv_refresh_limits = hdev_refresh_limits,
> >      .bdrv_io_plug = raw_aio_plug,
> >      .bdrv_io_unplug = raw_aio_unplug,
> >      .bdrv_attach_aio_context = raw_aio_attach_aio_context,
> > @@ -3724,7 +3753,7 @@ static BlockDriver bdrv_host_cdrom = {
> >      .bdrv_co_preadv         = raw_co_preadv,
> >      .bdrv_co_pwritev        = raw_co_pwritev,
> >      .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
> > -    .bdrv_refresh_limits = raw_refresh_limits,
> > +    .bdrv_refresh_limits = hdev_refresh_limits,
> >      .bdrv_io_plug = raw_aio_plug,
> >      .bdrv_io_unplug = raw_aio_unplug,
> >      .bdrv_attach_aio_context = raw_aio_attach_aio_context,
> > --
> > 2.26.2
> > 

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

[Maxim Levitsky]

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

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

[Paolo Bonzini]

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