[PATCH 0/2] block;scsi-generic: Fix max transfer size calculation

[PATCH 0/2] block;scsi-generic: Fix max transfer size calculation

[Dmitry Fomichev]

When a host-managed zoned device is passed through to the
guest system using scsi-generic driver, the maximum i/o size for the
drive at the guest may end up being larger than at the host, causing
i/o errors while accessing the backing zoned drive at the host system.

Two problems prevent correct setting of the maximum i/o transfer size
at the guest in this configuration. One issue is specific to
host-managed zone devices - scsi-generic driver doesn't recognize the
SCSI type of HM-zoned devices. The other problem is that file-posix
code for finding max_segments system value doesn't correctly handle
SG nodes.

The following two patches fix these problems.

Based-on: <20200424084338.26803-16-armbru@redhat.com>

Dmitry Fomichev (2):
  file-posix: Correctly read max_segments of SG nodes
  scsi-generic: Fix HM-zoned device scan

 block/file-posix.c       | 55 +++++++++++++++++++++++-----------------
 hw/scsi/scsi-generic.c   | 10 +++++---
 include/scsi/constants.h |  1 +
 3 files changed, 39 insertions(+), 27 deletions(-)

-- 
2.21.0

[PATCH 1/2] file-posix: Correctly read max_segments of SG nodes

[Dmitry Fomichev]

If scsi-generic driver is in use, an SG node can be specified in
the command line in place of a regular SCSI device. In this case,
sg_get_max_segments() fails to open max_segments entry in sysfs
because /dev/sgX is a character device. As the result, the maximum
transfer size for the device may be calculated incorrectly, causing
I/O errors if the maximum transfer size at the guest ends up to be
larger compared to the host.

Check system device type in sg_get_max_segments() and read the
max_segments value differently if it is a character device.

Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc
Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 block/file-posix.c | 55 +++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 094e3b0212..f9e2424e8f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1108,6 +1108,7 @@ static int sg_get_max_segments(int fd)
     int ret;
     int sysfd = -1;
     long max_segments;
+    unsigned int max_segs;
     struct stat st;
 
     if (fstat(fd, &st)) {
@@ -1115,30 +1116,38 @@ static int sg_get_max_segments(int fd)
         goto out;
     }
 
-    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);
-    if (sysfd == -1) {
-        ret = -errno;
-        goto out;
+    if (S_ISBLK(st.st_mode)) {
+        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);
+        if (sysfd == -1) {
+            ret = -errno;
+            goto out;
+        }
+        do {
+            ret = read(sysfd, buf, sizeof(buf) - 1);
+        } while (ret == -1 && errno == EINTR);
+        if (ret < 0) {
+            ret = -errno;
+            goto out;
+        } else if (ret == 0) {
+            ret = -EIO;
+            goto out;
+        }
+        buf[ret] = 0;
+        /* The file is ended with '\n', pass 'end' to accept that. */
+        ret = qemu_strtol(buf, &end, 10, &max_segments);
+        if (ret == 0 && end && *end == '\n') {
+            ret = max_segments;
+        }
+    } else {
+	ret = ioctl(fd, SG_GET_SG_TABLESIZE, &max_segs);
+        if (ret != 0) {
+            ret = -errno;
+            goto out;
+        }
+        ret = max_segs;
     }
-    do {
-        ret = read(sysfd, buf, sizeof(buf) - 1);
-    } while (ret == -1 && errno == EINTR);
-    if (ret < 0) {
-        ret = -errno;
-        goto out;
-    } else if (ret == 0) {
-        ret = -EIO;
-        goto out;
-    }
-    buf[ret] = 0;
-    /* The file is ended with '\n', pass 'end' to accept that. */
-    ret = qemu_strtol(buf, &end, 10, &max_segments);
-    if (ret == 0 && end && *end == '\n') {
-        ret = max_segments;
-    }
-
 out:
     if (sysfd != -1) {
         close(sysfd);
-- 
2.21.0

[PATCH 2/2] scsi-generic: Fix HM-zoned device scan

[Dmitry Fomichev]

Several important steps during device scan depend on SCSI type of the
device. For example, max_transfer property is only determined and
assigned if the device has the type of TYPE_DISK.

Host-managed ZBC disks retain most of the properties of regular SCSI
drives, but they have their own SCSI device type, 0x14. This prevents
the proper assignment of max_transfer property for HM-zoned devices in
scsi-generic driver leading to I/O errors if the maximum i/o size
calculated at the guest exceeds the host value.

To fix this, define TYPE_ZBC to have the standard value from SCSI ZBC
standard spec. Several scan steps that were previously done only for
TYPE_DISK devices, are now performed for the SCSI devices having
TYPE_ZBC too.

Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 hw/scsi/scsi-generic.c   | 10 ++++++----
 include/scsi/constants.h |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 86ed0a3822..2cb23ca891 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -162,7 +162,8 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s)
         }
     }
 
-    if (s->type == TYPE_DISK && (r->req.cmd.buf[1] & 0x01)) {
+    if ((s->type == TYPE_DISK || s->type == TYPE_ZBC) &&
+        (r->req.cmd.buf[1] & 0x01)) {
         page = r->req.cmd.buf[2];
         if (page == 0xb0) {
             uint32_t max_transfer =
@@ -299,10 +300,11 @@ static void scsi_read_complete(void * opaque, int ret)
     }
     blk_set_guest_block_size(s->conf.blk, s->blocksize);
 
-    /* Patch MODE SENSE device specific parameters if the BDS is opened
+    /*
+     * Patch MODE SENSE device specific parameters if the BDS is opened
      * readonly.
      */
-    if ((s->type == TYPE_DISK || s->type == TYPE_TAPE) &&
+    if ((s->type == TYPE_DISK || s->type == TYPE_TAPE || s->type == TYPE_ZBC) &&
         blk_is_read_only(s->conf.blk) &&
         (r->req.cmd.buf[0] == MODE_SENSE ||
          r->req.cmd.buf[0] == MODE_SENSE_10) &&
@@ -617,7 +619,7 @@ static void scsi_generic_read_device_identification(SCSIDevice *s)
 void scsi_generic_read_device_inquiry(SCSIDevice *s)
 {
     scsi_generic_read_device_identification(s);
-    if (s->type == TYPE_DISK) {
+    if (s->type == TYPE_DISK || s->type == TYPE_ZBC) {
         scsi_generic_set_vpd_bl_emulation(s);
     } else {
         s->needs_vpd_bl_emulation = false;
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..2a32c08b5e 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -218,6 +218,7 @@
 #define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
 #define TYPE_RBC            0x0e    /* Simplified Direct-Access Device */
 #define TYPE_OSD            0x11    /* Object-storage Device */
+#define TYPE_ZBC            0x14    /* Host-managed Zoned SCSI Device */
 #define TYPE_WLUN           0x1e    /* Well known LUN */
 #define TYPE_NOT_PRESENT    0x1f
 #define TYPE_INACTIVE       0x20
-- 
2.21.0

Re: [PATCH 2/2] scsi-generic: Fix HM-zoned device scan

[Alistair Francis]

On Tue, Aug 11, 2020 at 3:52 PM Dmitry Fomichev <dmitry.fomichev@wdc.com> wrote:
>
> Several important steps during device scan depend on SCSI type of the
> device. For example, max_transfer property is only determined and
> assigned if the device has the type of TYPE_DISK.
>
> Host-managed ZBC disks retain most of the properties of regular SCSI
> drives, but they have their own SCSI device type, 0x14. This prevents
> the proper assignment of max_transfer property for HM-zoned devices in
> scsi-generic driver leading to I/O errors if the maximum i/o size
> calculated at the guest exceeds the host value.
>
> To fix this, define TYPE_ZBC to have the standard value from SCSI ZBC
> standard spec. Several scan steps that were previously done only for
> TYPE_DISK devices, are now performed for the SCSI devices having
> TYPE_ZBC too.
>
> Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/scsi/scsi-generic.c   | 10 ++++++----
>  include/scsi/constants.h |  1 +
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 86ed0a3822..2cb23ca891 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -162,7 +162,8 @@ static void scsi_handle_inquiry_reply(SCSIGenericReq *r, SCSIDevice *s)
>          }
>      }
>
> -    if (s->type == TYPE_DISK && (r->req.cmd.buf[1] & 0x01)) {
> +    if ((s->type == TYPE_DISK || s->type == TYPE_ZBC) &&
> +        (r->req.cmd.buf[1] & 0x01)) {
>          page = r->req.cmd.buf[2];
>          if (page == 0xb0) {
>              uint32_t max_transfer =
> @@ -299,10 +300,11 @@ static void scsi_read_complete(void * opaque, int ret)
>      }
>      blk_set_guest_block_size(s->conf.blk, s->blocksize);
>
> -    /* Patch MODE SENSE device specific parameters if the BDS is opened
> +    /*
> +     * Patch MODE SENSE device specific parameters if the BDS is opened
>       * readonly.
>       */
> -    if ((s->type == TYPE_DISK || s->type == TYPE_TAPE) &&
> +    if ((s->type == TYPE_DISK || s->type == TYPE_TAPE || s->type == TYPE_ZBC) &&
>          blk_is_read_only(s->conf.blk) &&
>          (r->req.cmd.buf[0] == MODE_SENSE ||
>           r->req.cmd.buf[0] == MODE_SENSE_10) &&
> @@ -617,7 +619,7 @@ static void scsi_generic_read_device_identification(SCSIDevice *s)
>  void scsi_generic_read_device_inquiry(SCSIDevice *s)
>  {
>      scsi_generic_read_device_identification(s);
> -    if (s->type == TYPE_DISK) {
> +    if (s->type == TYPE_DISK || s->type == TYPE_ZBC) {
>          scsi_generic_set_vpd_bl_emulation(s);
>      } else {
>          s->needs_vpd_bl_emulation = false;
> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
> index 874176019e..2a32c08b5e 100644
> --- a/include/scsi/constants.h
> +++ b/include/scsi/constants.h
> @@ -218,6 +218,7 @@
>  #define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
>  #define TYPE_RBC            0x0e    /* Simplified Direct-Access Device */
>  #define TYPE_OSD            0x11    /* Object-storage Device */
> +#define TYPE_ZBC            0x14    /* Host-managed Zoned SCSI Device */
>  #define TYPE_WLUN           0x1e    /* Well known LUN */
>  #define TYPE_NOT_PRESENT    0x1f
>  #define TYPE_INACTIVE       0x20
> --
> 2.21.0
>
>

Re: [PATCH 0/2] block; scsi-generic: Fix max transfer size calculation

[Paolo Bonzini]

On 12/08/20 00:51, Dmitry Fomichev wrote:
> When a host-managed zoned device is passed through to the
> guest system using scsi-generic driver, the maximum i/o size for the
> drive at the guest may end up being larger than at the host, causing
> i/o errors while accessing the backing zoned drive at the host system.
> 
> Two problems prevent correct setting of the maximum i/o transfer size
> at the guest in this configuration. One issue is specific to
> host-managed zone devices - scsi-generic driver doesn't recognize the
> SCSI type of HM-zoned devices. The other problem is that file-posix
> code for finding max_segments system value doesn't correctly handle
> SG nodes.
> 
> The following two patches fix these problems.
> 
> Based-on: <20200424084338.26803-16-armbru@redhat.com>
> 
> Dmitry Fomichev (2):
>   file-posix: Correctly read max_segments of SG nodes
>   scsi-generic: Fix HM-zoned device scan
> 
>  block/file-posix.c       | 55 +++++++++++++++++++++++-----------------
>  hw/scsi/scsi-generic.c   | 10 +++++---
>  include/scsi/constants.h |  1 +
>  3 files changed, 39 insertions(+), 27 deletions(-)
> 

The patches are more or less unrelated; I have queued the second, while
the first is outside my maintenance area.

Paolo

Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes

[Max Reitz]

On 12.08.20 00:51, Dmitry Fomichev wrote:
> If scsi-generic driver is in use, an SG node can be specified in
> the command line in place of a regular SCSI device. In this case,
> sg_get_max_segments() fails to open max_segments entry in sysfs
> because /dev/sgX is a character device. As the result, the maximum
> transfer size for the device may be calculated incorrectly, causing
> I/O errors if the maximum transfer size at the guest ends up to be
> larger compared to the host.
> 
> Check system device type in sg_get_max_segments() and read the
> max_segments value differently if it is a character device.
> 
> Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  block/file-posix.c | 55 +++++++++++++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 094e3b0212..f9e2424e8f 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1108,6 +1108,7 @@ static int sg_get_max_segments(int fd)
>      int ret;
>      int sysfd = -1;
>      long max_segments;
> +    unsigned int max_segs;
>      struct stat st;
>  
>      if (fstat(fd, &st)) {
> @@ -1115,30 +1116,38 @@ static int sg_get_max_segments(int fd)
>          goto out;
>      }
>  
> -    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);
> -    if (sysfd == -1) {
> -        ret = -errno;
> -        goto out;
> +    if (S_ISBLK(st.st_mode)) {
> +        sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
> +                                    major(st.st_rdev), minor(st.st_rdev));

Sounds reasonable, but this function is (naturally) only called if
bs->sg is true, which is set by hdev_is_sg(), which returns true only if
the device file is a character device.

So is this path ever taken, or can we just replace it all with the ioctl?

(Before 867eccfed84, this function was used for all host devices, which
might explain why the code even exists.)

Max

Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes

[Maxim Levitsky]

On Thu, 2020-09-17 at 15:16 +0200, Max Reitz wrote:
> On 12.08.20 00:51, Dmitry Fomichev wrote:
> > If scsi-generic driver is in use, an SG node can be specified in
> > the command line in place of a regular SCSI device. In this case,
> > sg_get_max_segments() fails to open max_segments entry in sysfs
> > because /dev/sgX is a character device. As the result, the maximum
> > transfer size for the device may be calculated incorrectly, causing
> > I/O errors if the maximum transfer size at the guest ends up to be
> > larger compared to the host.
> > 
> > Check system device type in sg_get_max_segments() and read the
> > max_segments value differently if it is a character device.
> > 
> > Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc
> > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> >  block/file-posix.c | 55 +++++++++++++++++++++++++++-------------------
> >  1 file changed, 32 insertions(+), 23 deletions(-)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 094e3b0212..f9e2424e8f 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1108,6 +1108,7 @@ static int sg_get_max_segments(int fd)
> >      int ret;
> >      int sysfd = -1;
> >      long max_segments;
> > +    unsigned int max_segs;
> >      struct stat st;
> >  
> >      if (fstat(fd, &st)) {
> > @@ -1115,30 +1116,38 @@ static int sg_get_max_segments(int fd)
> >          goto out;
> >      }
> >  
> > -    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);
> > -    if (sysfd == -1) {
> > -        ret = -errno;
> > -        goto out;
> > +    if (S_ISBLK(st.st_mode)) {
> > +        sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
> > +                                    major(st.st_rdev), minor(st.st_rdev));
> 
> Sounds reasonable, but this function is (naturally) only called if
> bs->sg is true, which is set by hdev_is_sg(), which returns true only if
> the device file is a character device.
> 
> So is this path ever taken, or can we just replace it all with the ioctl?
> 
> (Before 867eccfed84, this function was used for all host devices, which
> might explain why the code even exists.)
> 
> Max

I have another proposal which I am currently evaluating.

How about we drop all the SG_IO limits code alltogher from the raw driver, and
instead just let the scsi drivers (scsi-block and scsi-generic) query
the device directly, since I don't think that the kernel (I will double check this)?


Best regards,
	Maxim Levitsky

Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes

[Maxim Levitsky]

On Thu, 2020-09-17 at 16:22 +0300, Maxim Levitsky wrote:
> On Thu, 2020-09-17 at 15:16 +0200, Max Reitz wrote:
> > On 12.08.20 00:51, Dmitry Fomichev wrote:
> > > If scsi-generic driver is in use, an SG node can be specified in
> > > the command line in place of a regular SCSI device. In this case,
> > > sg_get_max_segments() fails to open max_segments entry in sysfs
> > > because /dev/sgX is a character device. As the result, the maximum
> > > transfer size for the device may be calculated incorrectly, causing
> > > I/O errors if the maximum transfer size at the guest ends up to be
> > > larger compared to the host.
> > > 
> > > Check system device type in sg_get_max_segments() and read the
> > > max_segments value differently if it is a character device.
> > > 
> > > Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > > Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc
> > > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > > ---
> > >  block/file-posix.c | 55 +++++++++++++++++++++++++++-------------------
> > >  1 file changed, 32 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 094e3b0212..f9e2424e8f 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -1108,6 +1108,7 @@ static int sg_get_max_segments(int fd)
> > >      int ret;
> > >      int sysfd = -1;
> > >      long max_segments;
> > > +    unsigned int max_segs;
> > >      struct stat st;
> > >  
> > >      if (fstat(fd, &st)) {
> > > @@ -1115,30 +1116,38 @@ static int sg_get_max_segments(int fd)
> > >          goto out;
> > >      }
> > >  
> > > -    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);
> > > -    if (sysfd == -1) {
> > > -        ret = -errno;
> > > -        goto out;
> > > +    if (S_ISBLK(st.st_mode)) {
> > > +        sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
> > > +                                    major(st.st_rdev), minor(st.st_rdev));
> > 
> > Sounds reasonable, but this function is (naturally) only called if
> > bs->sg is true, which is set by hdev_is_sg(), which returns true only if
> > the device file is a character device.
> > 
> > So is this path ever taken, or can we just replace it all with the ioctl?
> > 
> > (Before 867eccfed84, this function was used for all host devices, which
> > might explain why the code even exists.)
> > 
> > Max
> 
> I have another proposal which I am currently evaluating.
> 
> How about we drop all the SG_IO limits code alltogher from the raw driver, and
> instead just let the scsi drivers (scsi-block and scsi-generic) query
> the device directly, since I don't think that the kernel (I will double check this)?

I hit send too soon. I mean I don't think that the kernel imposes its own limits on SG_IO.

Best regards,
	Maxim Levitsky
> 
> 
> Best regards,
> 	Maxim Levitsky
> 
> 

RE: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes

[Dmitry Fomichev]

> -----Original Message-----
> From: Maxim Levitsky <mlevitsk@redhat.com>
> Sent: Thursday, September 17, 2020 9:24 AM
> To: Max Reitz <mreitz@redhat.com>; Dmitry Fomichev
> <Dmitry.Fomichev@wdc.com>; Kevin Wolf <kwolf@redhat.com>; Paolo
> Bonzini <pbonzini@redhat.com>; Fam Zheng <fam@euphon.net>; Philippe
> Mathieu-Daudé <philmd@redhat.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>; Damien Le Moal
> <Damien.LeMoal@wdc.com>; qemu-block@nongnu.org; qemu-
> devel@nongnu.org
> Subject: Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG
> nodes
> 
> On Thu, 2020-09-17 at 16:22 +0300, Maxim Levitsky wrote:
> > On Thu, 2020-09-17 at 15:16 +0200, Max Reitz wrote:
> > > On 12.08.20 00:51, Dmitry Fomichev wrote:
> > > > If scsi-generic driver is in use, an SG node can be specified in
> > > > the command line in place of a regular SCSI device. In this case,
> > > > sg_get_max_segments() fails to open max_segments entry in sysfs
> > > > because /dev/sgX is a character device. As the result, the maximum
> > > > transfer size for the device may be calculated incorrectly, causing
> > > > I/O errors if the maximum transfer size at the guest ends up to be
> > > > larger compared to the host.
> > > >
> > > > Check system device type in sg_get_max_segments() and read the
> > > > max_segments value differently if it is a character device.
> > > >
> > > > Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > > > Fixes: 9103f1ceb46614b150bcbc3c9a4fbc72b47fedcc
> > > > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > > > ---
> > > >  block/file-posix.c | 55 +++++++++++++++++++++++++++----------------
> ---
> > > >  1 file changed, 32 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > > index 094e3b0212..f9e2424e8f 100644
> > > > --- a/block/file-posix.c
> > > > +++ b/block/file-posix.c
> > > > @@ -1108,6 +1108,7 @@ static int sg_get_max_segments(int fd)
> > > >      int ret;
> > > >      int sysfd = -1;
> > > >      long max_segments;
> > > > +    unsigned int max_segs;
> > > >      struct stat st;
> > > >
> > > >      if (fstat(fd, &st)) {
> > > > @@ -1115,30 +1116,38 @@ static int sg_get_max_segments(int fd)
> > > >          goto out;
> > > >      }
> > > >
> > > > -    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);
> > > > -    if (sysfd == -1) {
> > > > -        ret = -errno;
> > > > -        goto out;
> > > > +    if (S_ISBLK(st.st_mode)) {
> > > > +        sysfspath =
> g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
> > > > +                                    major(st.st_rdev), minor(st.st_rdev));
> > >
> > > Sounds reasonable, but this function is (naturally) only called if
> > > bs->sg is true, which is set by hdev_is_sg(), which returns true only if
> > > the device file is a character device.
> > >
> > > So is this path ever taken, or can we just replace it all with the ioctl?
> > >
> > > (Before 867eccfed84, this function was used for all host devices, which
> > > might explain why the code even exists.)
> > >
> > > Max
> >
> > I have another proposal which I am currently evaluating.
> >
> > How about we drop all the SG_IO limits code alltogher from the raw driver,
> and
> > instead just let the scsi drivers (scsi-block and scsi-generic) query
> > the device directly, since I don't think that the kernel (I will double check
> this)?
> 
> I hit send too soon. I mean I don't think that the kernel imposes its own limits
> on SG_IO.
> 

When I was making the fix, I had a feeling that this code should be moved
someplace else where it could be called both for block devices and sg nodes,
but that would make the patch more intrusive.

Maxim, looks like you are on top of this problem and your approach sounds
sensible too me. Just FYI, it is also possible to avoid using SG_GET_SG_TABLESIZE
ioctl and rely entirely on sysfs, but the code gets a bit more complicated (see below)

Cheers,
Dmitry

--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1110,6 +1110,8 @@ static int sg_get_max_segments(int fd)
     char buf[32];
     const char *end;
     char *sysfspath = NULL;
+    struct dirent *blkdev = NULL;
+    DIR *dir = NULL;
     int ret;
     int sysfd = -1;
     long max_segments;
@@ -1120,8 +1122,30 @@ static int sg_get_max_segments(int fd)
         goto out;
     }
 
-    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
-                                major(st.st_rdev), minor(st.st_rdev));
+    if (S_ISBLK(st.st_mode)) {
+        sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
+                                    major(st.st_rdev), minor(st.st_rdev));
+    } else {
+        sysfspath = g_strdup_printf("/sys/dev/char/%u:%u/device/block",
+                                    major(st.st_rdev), minor(st.st_rdev));
+        dir = opendir(sysfspath);
+        if (!dir) {
+            ret = -errno;
+            goto out;
+        }
+        do {
+            blkdev = readdir(dir);
+            if (!blkdev) {
+                ret = -errno;
+                closedir(dir);
+                goto out;
+            }
+        } while (*blkdev->d_name == '.');
+        g_free(sysfspath);
+        sysfspath = g_strdup_printf("/sys/block/%s/queue/max_segments",
+                                    blkdev->d_name);
+        closedir(dir);
+    }
     sysfd = open(sysfspath, O_RDONLY);
     if (sysfd == -1) {
         ret = -errno;

> Best regards,
> 	Maxim Levitsky
> >
> >
> > Best regards,
> > 	Maxim Levitsky
> >
> >
> 

Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes

[Paolo Bonzini]

On 17/09/20 15:16, Max Reitz wrote:
> So is this path ever taken, or can we just replace it all with the ioctl?
> 
> (Before 867eccfed84, this function was used for all host devices, which
> might explain why the code even exists.)

Because 867eccfed84 is wrong.  If you use /dev/sda* with SG_IO you do
need to take into account the hardware max segment size/max segment count.

Probably ->sg needs to be set by the front-end, not by the back-end.  An
even better way (but for which I'd leave the task to you and Kevin)
could be to have a new permission BLK_PERM_WRITE_BYPASS and to reduce
the limits to the hardware limits if anybody has requested that
permission.  I tried to implement that a couple years ago but I just
couldn't wrap my mind around the permission code.

Paolo

Re: [PATCH 1/2] file-posix: Correctly read max_segments of SG nodes

[Paolo Bonzini]

On 17/09/20 18:44, Dmitry Fomichev wrote:
> 
> Maxim, looks like you are on top of this problem and your approach sounds
> sensible too me. Just FYI, it is also possible to avoid using SG_GET_SG_TABLESIZE
> ioctl and rely entirely on sysfs, but the code gets a bit more complicated (see below)

I would prefer to have the code in block/ but I have no problem if the
hardware limits are placed in a new field of bs->bl.  Then scsi-block
and scsi-generic can consult it instead of bs->bl.max_transfer.

Paolo