[PATCH 0/3] file-posix: fix refresh_limits for SCSI devices

[PATCH 0/3] file-posix: fix refresh_limits for SCSI devices

[Paolo Bonzini]

refresh_limits is not doing anything for block devices, and is retrieving
the maximum number of s/g list entries incorrectly for character devices.

Patches 2-3 fix these problems, while patch 1 is a small improvement to
avoid making the BlockLimits unnecessarily restrictive when SG_IO is not
in use.

Paolo

Paolo Bonzini (3):
  scsi-generic: pass max_segments via max_iov field in BlockLimits
  file-posix: try BLKSECTGET on block devices too, do not round to power
    of 2
  file-posix: fix max_iov for /dev/sg devices

 block/file-posix.c     | 37 +++++++++++++++++++++++--------------
 hw/scsi/scsi-generic.c |  6 ++++--
 2 files changed, 27 insertions(+), 16 deletions(-)

-- 
2.30.1

[PATCH 1/3] scsi-generic: pass max_segments via max_iov field in BlockLimits

[Paolo Bonzini]

I/O to a disk via read/write is not limited by the number of segments allowed
by the host adapter; the kernel can split requests if needed, and the limit
imposed by the host adapter can be very low (256k or so) to avoid that SG_IO
returns EINVAL if memory is heavily fragmented.

Since this value is only interesting for SG_IO-based I/O, do not include
it in the max_transfer and only take it into account when patching the
block limits VPD page in the scsi-generic device.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/file-posix.c     | 3 +--
 hw/scsi/scsi-generic.c | 6 ++++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 20e14f8e96..9e316a2a97 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1252,8 +1252,7 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 
         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);
+            bs->bl.max_iov = ret;
         }
     }
 
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 98c30c5d5c..82e1e2ee79 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -179,10 +179,12 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s)
         (r->req.cmd.buf[1] & 0x01)) {
         page = r->req.cmd.buf[2];
         if (page == 0xb0) {
-            uint32_t max_transfer =
-                blk_get_max_transfer(s->conf.blk) / s->blocksize;
+            uint32_t max_transfer = blk_get_max_transfer(s->conf.blk);
+            uint32_t max_iov = blk_get_max_iov(s->conf.blk);
 
             assert(max_transfer);
+            max_transfer = MIN_NON_ZERO(max_transfer, max_iov * qemu_real_host_page_size)
+                / s->blocksize;
             stl_be_p(&r->buf[8], max_transfer);
             /* Also take care of the opt xfer len. */
             stl_be_p(&r->buf[12],
-- 
2.30.1

[PATCH 2/3] file-posix: try BLKSECTGET on block devices too, do not round to power of 2

[Paolo Bonzini]

bs->sg is only true for character devices, but block devices can also
be used with scsi-block and scsi-generic.  Unfortunately BLKSECTGET
returns bytes for /dev/sgN devices and sectors for block devices, so
account for that in the code.

The maximum transfer also need not be a power of 2 (for example I have
seen disks with 1280 KiB maximum transfer) so there's no need to pass
the result through pow2floor.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/file-posix.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 9e316a2a97..e37a6bb8de 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1173,13 +1173,13 @@ static void raw_reopen_abort(BDRVReopenState *state)
     s->reopen_state = NULL;
 }
 
-static int sg_get_max_transfer_length(int fd)
+static int sg_get_max_transfer_length(int fd, struct stat *st)
 {
 #ifdef BLKSECTGET
     int max_bytes = 0;
 
     if (ioctl(fd, BLKSECTGET, &max_bytes) == 0) {
-        return max_bytes;
+        return S_ISBLK(st->st_mode) ? max_bytes * 512 : max_bytes;
     } else {
         return -errno;
     }
@@ -1188,7 +1188,7 @@ static int sg_get_max_transfer_length(int fd)
 #endif
 }
 
-static int sg_get_max_segments(int fd)
+static int sg_get_max_segments(int fd, struct stat *st)
 {
 #ifdef CONFIG_LINUX
     char buf[32];
@@ -1197,15 +1197,9 @@ static int sg_get_max_segments(int fd)
     int ret;
     int sysfd = -1;
     long max_segments;
-    struct stat st;
-
-    if (fstat(fd, &st)) {
-        ret = -errno;
-        goto out;
-    }
 
     sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
-                                major(st.st_rdev), minor(st.st_rdev));
+                                major(st->st_rdev), minor(st->st_rdev));
     sysfd = open(sysfspath, O_RDONLY);
     if (sysfd == -1) {
         ret = -errno;
@@ -1242,15 +1236,20 @@ out:
 static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
+    struct stat st;
+
+    if (fstat(s->fd, &st)) {
+        return;
+    }
 
-    if (bs->sg) {
-        int ret = sg_get_max_transfer_length(s->fd);
+    if (bs->sg || S_ISBLK(st.st_mode)) {
+        int ret = sg_get_max_transfer_length(s->fd, &st);
 
         if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
-            bs->bl.max_transfer = pow2floor(ret);
+            bs->bl.max_transfer = ret;
         }
 
-        ret = sg_get_max_segments(s->fd);
+        ret = sg_get_max_segments(s->fd, &st);
         if (ret > 0) {
             bs->bl.max_iov = ret;
         }
-- 
2.30.1

[PATCH 3/3] file-posix: fix max_iov for /dev/sg devices

[Paolo Bonzini]

Even though it was only called for devices that have bs->sg set (which
must be character devices),
sg_get_max_segments looked at /sys/dev/block which only works for
block devices.

On Linux the sg driver has its own way to provide the maximum number of
iovecs in a scatter/gather list.
---
 block/file-posix.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index e37a6bb8de..b348b37ccb 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1198,6 +1198,17 @@ static int sg_get_max_segments(int fd, struct stat *st)
     int sysfd = -1;
     long max_segments;
 
+    if (S_ISCHR(st->st_mode)) {
+        if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
+            return ret;
+        }
+        return -EIO;
+    }
+
+    if (!S_ISBLK(st->st_mode)) {
+        return -ENOTSUP;
+    }
+
     sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
                                 major(st->st_rdev), minor(st->st_rdev));
     sysfd = open(sysfspath, O_RDONLY);
-- 
2.30.1

Re: [PATCH 0/3] file-posix: fix refresh_limits for SCSI devices

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210415124307.428203-1-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210415124307.428203-1-pbonzini@redhat.com
Subject: [PATCH 0/3] file-posix: fix refresh_limits for SCSI devices

=== 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/20210415124307.428203-1-pbonzini@redhat.com -> patchew/20210415124307.428203-1-pbonzini@redhat.com
Switched to a new branch 'test'
37feb25 file-posix: fix max_iov for /dev/sg devices
eec0521 file-posix: try BLKSECTGET on block devices too, do not round to power of 2
c44bc38 scsi-generic: pass max_segments via max_iov field in BlockLimits

=== OUTPUT BEGIN ===
1/3 Checking commit c44bc386ba20 (scsi-generic: pass max_segments via max_iov field in BlockLimits)
WARNING: line over 80 characters
#51: FILE: hw/scsi/scsi-generic.c:186:
+            max_transfer = MIN_NON_ZERO(max_transfer, max_iov * qemu_real_host_page_size)

total: 0 errors, 1 warnings, 23 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/3 Checking commit eec052142154 (file-posix: try BLKSECTGET on block devices too, do not round to power of 2)
3/3 Checking commit 37feb259bbc4 (file-posix: fix max_iov for /dev/sg devices)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 17 lines checked

Patch 3/3 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/20210415124307.428203-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com