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

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

[Tom Yan]

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>
---
v2: fix get_max_transfer_length for non-sg devices; also add/fix
sg_get_max_segments for sg devices
v3: fix typo in get_max_transfer_length
v4: fix accidental .orig inclusion
 block/file-posix.c | 57 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 9a00d4190a..411a23cf99 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1178,7 +1178,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];
@@ -1233,23 +1250,31 @@ 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);
+}
+
+static void hdev_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    BDRVRawState *s = bs->opaque;
+
+    /* BLKSECTGET has always been implemented wrongly in the sg driver
+       and BLKSSZGET has never been supported by it*/
+    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);
-        }
+    if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
+        bs->bl.max_transfer = pow2floor(ret);
+    }
 
-        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);
-        }
+    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_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);
+    raw_refresh_limits(bs, errp);
 }
 
 static int check_for_dasd(int fd)
@@ -3604,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,
@@ -3728,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.28.0

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

[Tom Yan]

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>
---
 block/file-posix.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 411a23cf99..9e37594145 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1178,6 +1178,21 @@ static int sg_get_max_transfer_length(int fd)
 #endif
 }
 
+static int sg_get_max_segments(int fd)
+{
+#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)
@@ -1268,7 +1283,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.28.0

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

[Tom Yan]

In commit 867eccfed84f96b54f4a432c510a02c2ce03b430, Levitsky appears
to have assumed that the only "SCSI Passthrough" is `-device
scsi-generic`, while the fact is there's also `-device scsi-block`
(passthrough without the sg driver). Unlike `-device scsi-hd`, getting
max_sectors is necessary to it (more precisely, hw_max_sectors might
what matters, but BLKSECTGET reports max_sectors, so).

I'm unsure about how qemu-nbd works, but the commit clearly wasn't the
right approach to fix the original issue it addresses. (It should for
example ignore the max_transfer if it will never matter in to it, or
overrides it in certain cases; when I glimpsed over
https://bugzilla.redhat.com/show_bug.cgi?id=1647104, I don't see how
it could be file-posix problem when it is reporting the right thing,
regardless of whether "removing" the code helps.)

I don't think we want to "mark" `-device scsi-block` as sg either. It
will probably bring even more unexpected problems, because they are
literally different sets of things behind the scene / in the kernel.

On Fri, 4 Sep 2020 at 10:09, Tom Yan <tom.ty89@gmail.com> wrote:
>
> 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>
> ---
>  block/file-posix.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 411a23cf99..9e37594145 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1178,6 +1178,21 @@ static int sg_get_max_transfer_length(int fd)
>  #endif
>  }
>
> +static int sg_get_max_segments(int fd)
> +{
> +#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)
> @@ -1268,7 +1283,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.28.0
>

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

[Tom Yan]

Maybe you want to add some condition for this:
https://github.com/qemu/qemu/blob/v5.1.0/nbd/server.c#L659
Or not clamp it at all.

On Sun, 6 Sep 2020 at 18:58, Tom Yan <tom.ty89@gmail.com> wrote:
>
> In commit 867eccfed84f96b54f4a432c510a02c2ce03b430, Levitsky appears
> to have assumed that the only "SCSI Passthrough" is `-device
> scsi-generic`, while the fact is there's also `-device scsi-block`
> (passthrough without the sg driver). Unlike `-device scsi-hd`, getting
> max_sectors is necessary to it (more precisely, hw_max_sectors might
> what matters, but BLKSECTGET reports max_sectors, so).
>
> I'm unsure about how qemu-nbd works, but the commit clearly wasn't the
> right approach to fix the original issue it addresses. (It should for
> example ignore the max_transfer if it will never matter in to it, or
> overrides it in certain cases; when I glimpsed over
> https://bugzilla.redhat.com/show_bug.cgi?id=1647104, I don't see how
> it could be file-posix problem when it is reporting the right thing,
> regardless of whether "removing" the code helps.)
>
> I don't think we want to "mark" `-device scsi-block` as sg either. It
> will probably bring even more unexpected problems, because they are
> literally different sets of things behind the scene / in the kernel.
>
> On Fri, 4 Sep 2020 at 10:09, Tom Yan <tom.ty89@gmail.com> wrote:
> >
> > 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>
> > ---
> >  block/file-posix.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 411a23cf99..9e37594145 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1178,6 +1178,21 @@ static int sg_get_max_transfer_length(int fd)
> >  #endif
> >  }
> >
> > +static int sg_get_max_segments(int fd)
> > +{
> > +#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)
> > @@ -1268,7 +1283,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.28.0
> >

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

[Maxim Levitsky]

On Sun, 2020-09-06 at 19:04 +0800, Tom Yan wrote:
> Maybe you want to add some condition for this:
> https://github.com/qemu/qemu/blob/v5.1.0/nbd/server.c#L659
> Or not clamp it at all.
> 
> On Sun, 6 Sep 2020 at 18:58, Tom Yan <tom.ty89@gmail.com> wrote:
> > In commit 867eccfed84f96b54f4a432c510a02c2ce03b430, Levitsky appears
> > to have assumed that the only "SCSI Passthrough" is `-device
> > scsi-generic`, while the fact is there's also `-device scsi-block`
> > (passthrough without the sg driver). Unlike `-device scsi-hd`, getting
> > max_sectors is necessary to it (more precisely, hw_max_sectors might
> > what matters, but BLKSECTGET reports max_sectors, so).
> > 
> > I'm unsure about how qemu-nbd works, but the commit clearly wasn't the
> > right approach to fix the original issue it addresses. (It should for
> > example ignore the max_transfer if it will never matter in to it, or
> > overrides it in certain cases; when I glimpsed over
> > https://bugzilla.redhat.com/show_bug.cgi?id=1647104, I don't see how
> > it could be file-posix problem when it is reporting the right thing,
> > regardless of whether "removing" the code helps.)
> > 
> > I don't think we want to "mark" `-device scsi-block` as sg either. It
> > will probably bring even more unexpected problems, because they are
> > literally different sets of things behind the scene / in the kernel.

Yes, I did overlook the fact that scsi-block is kind of hybrid passthough device,
doing SG_IO for some things and regular IO for others.

I don't think that my approach was wrong way to fix the problem, but as you found
out, abusing 'bs->sg' hack (which I would be very happy to remove completely)
wasn't a good idea.
I actualy was aware of scsi-block and that it does SG_IO but it 
was forgotten some where on the way.

So in summary what the problem is and what I think is the right solution:


Each qemu block driver exposes block limits and assumes that they are the same 
for two IO interfaces the block driver can expose:

1. Regular qemu blk_pread/pwrite alike functions
2. blk_ioctl (tiny wrapper against SG_IO), supported by posix-raw on 
   host block devices/sg char devices, and by iscsi

The problem is that these two interfaces can have different block limits.

I don't know about iscsi, but for files, doing regular IO is always unlimited,
since it passes through the kernel block layer and segemented there on 
demand which is faster that doing it in userspace, while SG_IO is passed as is
to the underlying SCSI device and lacks this segmentation.

Regardless of how NBD uses these limits, I think that these limits should be correctly
exposed by the block drivers, and therefore I propose is that each qemu block driver 
would expose a pair of block limits.
One for the regular block IO, and other for SG_IO.

Then block driver clients (like scsi devices that you mention, nbd, etc) 
can choose which limit to use, depending on which IO api they use.
The scsi-generic, and scsi-block can use the SG_IO limits, 
while the rest  can use the normal (unlimited for file I/O) limits, including the NBD.

Best regards,
	Maxim Levitsky

> > 
> > On Fri, 4 Sep 2020 at 10:09, Tom Yan <tom.ty89@gmail.com> wrote:
> > > 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>
> > > ---
> > >  block/file-posix.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > index 411a23cf99..9e37594145 100644
> > > --- a/block/file-posix.c
> > > +++ b/block/file-posix.c
> > > @@ -1178,6 +1178,21 @@ static int sg_get_max_transfer_length(int fd)
> > >  #endif
> > >  }
> > > 
> > > +static int sg_get_max_segments(int fd)
> > > +{
> > > +#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)
> > > @@ -1268,7 +1283,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.28.0
> > > 

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

[Tom Yan]

I don't disagree with your proposal, but the thing is, do we even need
another field/limit for case 1? For example, do we *really* need to
clamp sizes[2] (NBD_INFO_BLOCK_SIZE) in nbd/server.c and
max_io_sectors (SCSIBlockLimits) in hw/scsi/scsi-disk.c to to any kind
of "dynamic" limit?

Either way I can add another field (`max_pwrite`, maybe?) to
BlockLimits, as an infrastructure/hint, but what should be switched to
it, and what value should each block driver set it to, is probably
beyond what I can take.

IMHO my patches should be merged first (they are really just fixing a
regression and some bugs). If anyone finds it necessary and is capable
and willing to fix the insufficiency of BlockLimits, they can do it
afterwards with another patch series as an improvement. I can add a
patch to remove the clamping in nbd/server.c as a perhaps-temporary
measure to address the original issue, if desired.

On Sun, 6 Sep 2020 at 20:53, Maxim Levitsky <mlevitsk@redhat.com> wrote:
>
> On Sun, 2020-09-06 at 19:04 +0800, Tom Yan wrote:
> > Maybe you want to add some condition for this:
> > https://github.com/qemu/qemu/blob/v5.1.0/nbd/server.c#L659
> > Or not clamp it at all.
> >
> > On Sun, 6 Sep 2020 at 18:58, Tom Yan <tom.ty89@gmail.com> wrote:
> > > In commit 867eccfed84f96b54f4a432c510a02c2ce03b430, Levitsky appears
> > > to have assumed that the only "SCSI Passthrough" is `-device
> > > scsi-generic`, while the fact is there's also `-device scsi-block`
> > > (passthrough without the sg driver). Unlike `-device scsi-hd`, getting
> > > max_sectors is necessary to it (more precisely, hw_max_sectors might
> > > what matters, but BLKSECTGET reports max_sectors, so).
> > >
> > > I'm unsure about how qemu-nbd works, but the commit clearly wasn't the
> > > right approach to fix the original issue it addresses. (It should for
> > > example ignore the max_transfer if it will never matter in to it, or
> > > overrides it in certain cases; when I glimpsed over
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1647104, I don't see how
> > > it could be file-posix problem when it is reporting the right thing,
> > > regardless of whether "removing" the code helps.)
> > >
> > > I don't think we want to "mark" `-device scsi-block` as sg either. It
> > > will probably bring even more unexpected problems, because they are
> > > literally different sets of things behind the scene / in the kernel.
>
> Yes, I did overlook the fact that scsi-block is kind of hybrid passthough device,
> doing SG_IO for some things and regular IO for others.
>
> I don't think that my approach was wrong way to fix the problem, but as you found
> out, abusing 'bs->sg' hack (which I would be very happy to remove completely)
> wasn't a good idea.
> I actualy was aware of scsi-block and that it does SG_IO but it
> was forgotten some where on the way.
>
> So in summary what the problem is and what I think is the right solution:
>
>
> Each qemu block driver exposes block limits and assumes that they are the same
> for two IO interfaces the block driver can expose:
>
> 1. Regular qemu blk_pread/pwrite alike functions
> 2. blk_ioctl (tiny wrapper against SG_IO), supported by posix-raw on
>    host block devices/sg char devices, and by iscsi
>
> The problem is that these two interfaces can have different block limits.
>
> I don't know about iscsi, but for files, doing regular IO is always unlimited,
> since it passes through the kernel block layer and segemented there on
> demand which is faster that doing it in userspace, while SG_IO is passed as is
> to the underlying SCSI device and lacks this segmentation.
>
> Regardless of how NBD uses these limits, I think that these limits should be correctly
> exposed by the block drivers, and therefore I propose is that each qemu block driver
> would expose a pair of block limits.
> One for the regular block IO, and other for SG_IO.
>
> Then block driver clients (like scsi devices that you mention, nbd, etc)
> can choose which limit to use, depending on which IO api they use.
> The scsi-generic, and scsi-block can use the SG_IO limits,
> while the rest  can use the normal (unlimited for file I/O) limits, including the NBD.
>
> Best regards,
>         Maxim Levitsky
>
> > >
> > > On Fri, 4 Sep 2020 at 10:09, Tom Yan <tom.ty89@gmail.com> wrote:
> > > > 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>
> > > > ---
> > > >  block/file-posix.c | 17 ++++++++++++++++-
> > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > > index 411a23cf99..9e37594145 100644
> > > > --- a/block/file-posix.c
> > > > +++ b/block/file-posix.c
> > > > @@ -1178,6 +1178,21 @@ static int sg_get_max_transfer_length(int fd)
> > > >  #endif
> > > >  }
> > > >
> > > > +static int sg_get_max_segments(int fd)
> > > > +{
> > > > +#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)
> > > > @@ -1268,7 +1283,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.28.0
> > > >
>
>

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

[Maxim Levitsky]

On Sun, 2020-09-06 at 23:26 +0800, Tom Yan wrote:
> I don't disagree with your proposal, but the thing is, do we even need
> another field/limit for case 1? For example, do we *really* need to
> clamp sizes[2] (NBD_INFO_BLOCK_SIZE) in nbd/server.c and
> max_io_sectors (SCSIBlockLimits) in hw/scsi/scsi-disk.c to to any kind
> of "dynamic" limit?

It depends on if we have other block drivers that have IO segment/max transfer sizes,
that we need to enforce, other than the SG_IO case.

Knowing that the commit that added these limits to file-posix, which I was fixing is relatively
new, I guess there are other cases for these limits.

I'll check this tomorrow.

> 
> Either way I can add another field (`max_pwrite`, maybe?) to
> BlockLimits, as an infrastructure/hint, but what should be switched to
> it, and what value should each block driver set it to, is probably
> beyond what I can take.

Implementation wise, I think I can do this, but I'll wait few days for the
feedback on my proposal.

Thanks for finding this bug!

Best regards,
	Maxim Levitsky

> 
> IMHO my patches should be merged first (they are really just fixing a
> regression and some bugs). If anyone finds it necessary and is capable
> and willing to fix the insufficiency of BlockLimits, they can do it
> afterwards with another patch series as an improvement. I can add a
> patch to remove the clamping in nbd/server.c as a perhaps-temporary
> measure to address the original issue, if desired.
> 
> On Sun, 6 Sep 2020 at 20:53, Maxim Levitsky <mlevitsk@redhat.com> wrote:
> > On Sun, 2020-09-06 at 19:04 +0800, Tom Yan wrote:
> > > Maybe you want to add some condition for this:
> > > https://github.com/qemu/qemu/blob/v5.1.0/nbd/server.c#L659
> > > Or not clamp it at all.
> > > 
> > > On Sun, 6 Sep 2020 at 18:58, Tom Yan <tom.ty89@gmail.com> wrote:
> > > > In commit 867eccfed84f96b54f4a432c510a02c2ce03b430, Levitsky appears
> > > > to have assumed that the only "SCSI Passthrough" is `-device
> > > > scsi-generic`, while the fact is there's also `-device scsi-block`
> > > > (passthrough without the sg driver). Unlike `-device scsi-hd`, getting
> > > > max_sectors is necessary to it (more precisely, hw_max_sectors might
> > > > what matters, but BLKSECTGET reports max_sectors, so).
> > > > 
> > > > I'm unsure about how qemu-nbd works, but the commit clearly wasn't the
> > > > right approach to fix the original issue it addresses. (It should for
> > > > example ignore the max_transfer if it will never matter in to it, or
> > > > overrides it in certain cases; when I glimpsed over
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1647104, I don't see how
> > > > it could be file-posix problem when it is reporting the right thing,
> > > > regardless of whether "removing" the code helps.)
> > > > 
> > > > I don't think we want to "mark" `-device scsi-block` as sg either. It
> > > > will probably bring even more unexpected problems, because they are
> > > > literally different sets of things behind the scene / in the kernel.
> > 
> > Yes, I did overlook the fact that scsi-block is kind of hybrid passthough device,
> > doing SG_IO for some things and regular IO for others.
> > 
> > I don't think that my approach was wrong way to fix the problem, but as you found
> > out, abusing 'bs->sg' hack (which I would be very happy to remove completely)
> > wasn't a good idea.
> > I actualy was aware of scsi-block and that it does SG_IO but it
> > was forgotten some where on the way.
> > 
> > So in summary what the problem is and what I think is the right solution:
> > 
> > 
> > Each qemu block driver exposes block limits and assumes that they are the same
> > for two IO interfaces the block driver can expose:
> > 
> > 1. Regular qemu blk_pread/pwrite alike functions
> > 2. blk_ioctl (tiny wrapper against SG_IO), supported by posix-raw on
> >    host block devices/sg char devices, and by iscsi
> > 
> > The problem is that these two interfaces can have different block limits.
> > 
> > I don't know about iscsi, but for files, doing regular IO is always unlimited,
> > since it passes through the kernel block layer and segemented there on
> > demand which is faster that doing it in userspace, while SG_IO is passed as is
> > to the underlying SCSI device and lacks this segmentation.
> > 
> > Regardless of how NBD uses these limits, I think that these limits should be correctly
> > exposed by the block drivers, and therefore I propose is that each qemu block driver
> > would expose a pair of block limits.
> > One for the regular block IO, and other for SG_IO.
> > 
> > Then block driver clients (like scsi devices that you mention, nbd, etc)
> > can choose which limit to use, depending on which IO api they use.
> > The scsi-generic, and scsi-block can use the SG_IO limits,
> > while the rest  can use the normal (unlimited for file I/O) limits, including the NBD.
> > 
> > Best regards,
> >         Maxim Levitsky
> > 
> > > > On Fri, 4 Sep 2020 at 10:09, Tom Yan <tom.ty89@gmail.com> wrote:
> > > > > 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>
> > > > > ---
> > > > >  block/file-posix.c | 17 ++++++++++++++++-
> > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > > > index 411a23cf99..9e37594145 100644
> > > > > --- a/block/file-posix.c
> > > > > +++ b/block/file-posix.c
> > > > > @@ -1178,6 +1178,21 @@ static int sg_get_max_transfer_length(int fd)
> > > > >  #endif
> > > > >  }
> > > > > 
> > > > > +static int sg_get_max_segments(int fd)
> > > > > +{
> > > > > +#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)
> > > > > @@ -1268,7 +1283,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.28.0
> > > > > 

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

[Dmitry Fomichev]

> -----Original Message-----
> From: Qemu-block <qemu-block-
> bounces+dmitry.fomichev=wdc.com@nongnu.org> On Behalf Of Maxim
> Levitsky
> Sent: Sunday, September 6, 2020 1:06 PM
> To: Tom Yan <tom.ty89@gmail.com>; eblake@redhat.com;
> pbonzini@redhat.com; fam@euphon.net; anielhb@linux.vnet.ibm.com;
> kwolf@redhat.com; mreitz@redhat.com
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org
> Subject: Re: [PATCH v4 2/2] file-posix: add sg_get_max_segments that
> actually works with sg
> 
> On Sun, 2020-09-06 at 23:26 +0800, Tom Yan wrote:
> > I don't disagree with your proposal, but the thing is, do we even need
> > another field/limit for case 1? For example, do we *really* need to
> > clamp sizes[2] (NBD_INFO_BLOCK_SIZE) in nbd/server.c and
> > max_io_sectors (SCSIBlockLimits) in hw/scsi/scsi-disk.c to to any kind
> > of "dynamic" limit?
> 
> It depends on if we have other block drivers that have IO segment/max
> transfer sizes,
> that we need to enforce, other than the SG_IO case.
> 
> Knowing that the commit that added these limits to file-posix, which I was
> fixing is relatively
> new, I guess there are other cases for these limits.
> 
> I'll check this tomorrow.
> 
> >
> > Either way I can add another field (`max_pwrite`, maybe?) to
> > BlockLimits, as an infrastructure/hint, but what should be switched to
> > it, and what value should each block driver set it to, is probably
> > beyond what I can take.
> 
> Implementation wise, I think I can do this, but I'll wait few days for the
> feedback on my proposal.
> 
> Thanks for finding this bug!

There was also https://lore.kernel.org/qemu-devel/20200811225122.17342-2-dmitry.fomichev@wdc.com/
posted three weeks ago, but, seemingly, it was ignored... sorry if this
bug is already being fixed in a different way, but I think the fix presented
in that patch is worth considering.

Very best,
Dmitry

> 
> Best regards,
> 	Maxim Levitsky
> 
> >
> > IMHO my patches should be merged first (they are really just fixing a
> > regression and some bugs). If anyone finds it necessary and is capable
> > and willing to fix the insufficiency of BlockLimits, they can do it
> > afterwards with another patch series as an improvement. I can add a
> > patch to remove the clamping in nbd/server.c as a perhaps-temporary
> > measure to address the original issue, if desired.
> >
> > On Sun, 6 Sep 2020 at 20:53, Maxim Levitsky <mlevitsk@redhat.com>
> wrote:
> > > On Sun, 2020-09-06 at 19:04 +0800, Tom Yan wrote:
> > > > Maybe you want to add some condition for this:
> > > > https://github.com/qemu/qemu/blob/v5.1.0/nbd/server.c#L659
> > > > Or not clamp it at all.
> > > >
> > > > On Sun, 6 Sep 2020 at 18:58, Tom Yan <tom.ty89@gmail.com> wrote:
> > > > > In commit 867eccfed84f96b54f4a432c510a02c2ce03b430, Levitsky
> appears
> > > > > to have assumed that the only "SCSI Passthrough" is `-device
> > > > > scsi-generic`, while the fact is there's also `-device scsi-block`
> > > > > (passthrough without the sg driver). Unlike `-device scsi-hd`, getting
> > > > > max_sectors is necessary to it (more precisely, hw_max_sectors
> might
> > > > > what matters, but BLKSECTGET reports max_sectors, so).
> > > > >
> > > > > I'm unsure about how qemu-nbd works, but the commit clearly
> wasn't the
> > > > > right approach to fix the original issue it addresses. (It should for
> > > > > example ignore the max_transfer if it will never matter in to it, or
> > > > > overrides it in certain cases; when I glimpsed over
> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1647104, I don't see
> how
> > > > > it could be file-posix problem when it is reporting the right thing,
> > > > > regardless of whether "removing" the code helps.)
> > > > >
> > > > > I don't think we want to "mark" `-device scsi-block` as sg either. It
> > > > > will probably bring even more unexpected problems, because they
> are
> > > > > literally different sets of things behind the scene / in the kernel.
> > >
> > > Yes, I did overlook the fact that scsi-block is kind of hybrid passthough
> device,
> > > doing SG_IO for some things and regular IO for others.
> > >
> > > I don't think that my approach was wrong way to fix the problem, but as
> you found
> > > out, abusing 'bs->sg' hack (which I would be very happy to remove
> completely)
> > > wasn't a good idea.
> > > I actualy was aware of scsi-block and that it does SG_IO but it
> > > was forgotten some where on the way.
> > >
> > > So in summary what the problem is and what I think is the right solution:
> > >
> > >
> > > Each qemu block driver exposes block limits and assumes that they are
> the same
> > > for two IO interfaces the block driver can expose:
> > >
> > > 1. Regular qemu blk_pread/pwrite alike functions
> > > 2. blk_ioctl (tiny wrapper against SG_IO), supported by posix-raw on
> > >    host block devices/sg char devices, and by iscsi
> > >
> > > The problem is that these two interfaces can have different block limits.
> > >
> > > I don't know about iscsi, but for files, doing regular IO is always unlimited,
> > > since it passes through the kernel block layer and segemented there on
> > > demand which is faster that doing it in userspace, while SG_IO is passed
> as is
> > > to the underlying SCSI device and lacks this segmentation.
> > >
> > > Regardless of how NBD uses these limits, I think that these limits should
> be correctly
> > > exposed by the block drivers, and therefore I propose is that each qemu
> block driver
> > > would expose a pair of block limits.
> > > One for the regular block IO, and other for SG_IO.
> > >
> > > Then block driver clients (like scsi devices that you mention, nbd, etc)
> > > can choose which limit to use, depending on which IO api they use.
> > > The scsi-generic, and scsi-block can use the SG_IO limits,
> > > while the rest  can use the normal (unlimited for file I/O) limits, including
> the NBD.
> > >
> > > Best regards,
> > >         Maxim Levitsky
> > >
> > > > > On Fri, 4 Sep 2020 at 10:09, Tom Yan <tom.ty89@gmail.com> wrote:
> > > > > > 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>
> > > > > > ---
> > > > > >  block/file-posix.c | 17 ++++++++++++++++-
> > > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > > > > index 411a23cf99..9e37594145 100644
> > > > > > --- a/block/file-posix.c
> > > > > > +++ b/block/file-posix.c
> > > > > > @@ -1178,6 +1178,21 @@ static int
> sg_get_max_transfer_length(int fd)
> > > > > >  #endif
> > > > > >  }
> > > > > >
> > > > > > +static int sg_get_max_segments(int fd)
> > > > > > +{
> > > > > > +#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)
> > > > > > @@ -1268,7 +1283,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.28.0
> > > > > >
> 
> 

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

[Tom Yan]

Your patch only fixed it for sg devices. S_ISBLK() will never be true
when sg_get_max_segments() is only called in raw_refresh_limits() when
bs->sg is true.

With that said, it's not like we cannot ditch the bs->sg check in
raw_refresh_limits() and switch to S_ISBLK()/S_ISCHR(). (Although not
strictly necessary, I would also do S_ISCHR() for
SG_GET_SG_TABLESIZE.)

I don't know if it's a better approach though. Seems repetitive to me
(and bs->sg is not only used/checked in
file-posix/raw_refresh_limits):
https://github.com/qemu/qemu/blob/v5.1.0/block/file-posix.c#L3390
https://github.com/qemu/qemu/blob/v5.1.0/block/file-posix.c#L3504

On Mon, 7 Sep 2020 at 03:50, Dmitry Fomichev <Dmitry.Fomichev@wdc.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Qemu-block <qemu-block-
> > bounces+dmitry.fomichev=wdc.com@nongnu.org> On Behalf Of Maxim
> > Levitsky
> > Sent: Sunday, September 6, 2020 1:06 PM
> > To: Tom Yan <tom.ty89@gmail.com>; eblake@redhat.com;
> > pbonzini@redhat.com; fam@euphon.net; anielhb@linux.vnet.ibm.com;
> > kwolf@redhat.com; mreitz@redhat.com
> > Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org
> > Subject: Re: [PATCH v4 2/2] file-posix: add sg_get_max_segments that
> > actually works with sg
> >
> > On Sun, 2020-09-06 at 23:26 +0800, Tom Yan wrote:
> > > I don't disagree with your proposal, but the thing is, do we even need
> > > another field/limit for case 1? For example, do we *really* need to
> > > clamp sizes[2] (NBD_INFO_BLOCK_SIZE) in nbd/server.c and
> > > max_io_sectors (SCSIBlockLimits) in hw/scsi/scsi-disk.c to to any kind
> > > of "dynamic" limit?
> >
> > It depends on if we have other block drivers that have IO segment/max
> > transfer sizes,
> > that we need to enforce, other than the SG_IO case.
> >
> > Knowing that the commit that added these limits to file-posix, which I was
> > fixing is relatively
> > new, I guess there are other cases for these limits.
> >
> > I'll check this tomorrow.
> >
> > >
> > > Either way I can add another field (`max_pwrite`, maybe?) to
> > > BlockLimits, as an infrastructure/hint, but what should be switched to
> > > it, and what value should each block driver set it to, is probably
> > > beyond what I can take.
> >
> > Implementation wise, I think I can do this, but I'll wait few days for the
> > feedback on my proposal.
> >
> > Thanks for finding this bug!
>
> There was also https://lore.kernel.org/qemu-devel/20200811225122.17342-2-dmitry.fomichev@wdc.com/
> posted three weeks ago, but, seemingly, it was ignored... sorry if this
> bug is already being fixed in a different way, but I think the fix presented
> in that patch is worth considering.
>
> Very best,
> Dmitry
>
> >
> > Best regards,
> >       Maxim Levitsky
> >
> > >
> > > IMHO my patches should be merged first (they are really just fixing a
> > > regression and some bugs). If anyone finds it necessary and is capable
> > > and willing to fix the insufficiency of BlockLimits, they can do it
> > > afterwards with another patch series as an improvement. I can add a
> > > patch to remove the clamping in nbd/server.c as a perhaps-temporary
> > > measure to address the original issue, if desired.
> > >
> > > On Sun, 6 Sep 2020 at 20:53, Maxim Levitsky <mlevitsk@redhat.com>
> > wrote:
> > > > On Sun, 2020-09-06 at 19:04 +0800, Tom Yan wrote:
> > > > > Maybe you want to add some condition for this:
> > > > > https://github.com/qemu/qemu/blob/v5.1.0/nbd/server.c#L659
> > > > > Or not clamp it at all.
> > > > >
> > > > > On Sun, 6 Sep 2020 at 18:58, Tom Yan <tom.ty89@gmail.com> wrote:
> > > > > > In commit 867eccfed84f96b54f4a432c510a02c2ce03b430, Levitsky
> > appears
> > > > > > to have assumed that the only "SCSI Passthrough" is `-device
> > > > > > scsi-generic`, while the fact is there's also `-device scsi-block`
> > > > > > (passthrough without the sg driver). Unlike `-device scsi-hd`, getting
> > > > > > max_sectors is necessary to it (more precisely, hw_max_sectors
> > might
> > > > > > what matters, but BLKSECTGET reports max_sectors, so).
> > > > > >
> > > > > > I'm unsure about how qemu-nbd works, but the commit clearly
> > wasn't the
> > > > > > right approach to fix the original issue it addresses. (It should for
> > > > > > example ignore the max_transfer if it will never matter in to it, or
> > > > > > overrides it in certain cases; when I glimpsed over
> > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1647104, I don't see
> > how
> > > > > > it could be file-posix problem when it is reporting the right thing,
> > > > > > regardless of whether "removing" the code helps.)
> > > > > >
> > > > > > I don't think we want to "mark" `-device scsi-block` as sg either. It
> > > > > > will probably bring even more unexpected problems, because they
> > are
> > > > > > literally different sets of things behind the scene / in the kernel.
> > > >
> > > > Yes, I did overlook the fact that scsi-block is kind of hybrid passthough
> > device,
> > > > doing SG_IO for some things and regular IO for others.
> > > >
> > > > I don't think that my approach was wrong way to fix the problem, but as
> > you found
> > > > out, abusing 'bs->sg' hack (which I would be very happy to remove
> > completely)
> > > > wasn't a good idea.
> > > > I actualy was aware of scsi-block and that it does SG_IO but it
> > > > was forgotten some where on the way.
> > > >
> > > > So in summary what the problem is and what I think is the right solution:
> > > >
> > > >
> > > > Each qemu block driver exposes block limits and assumes that they are
> > the same
> > > > for two IO interfaces the block driver can expose:
> > > >
> > > > 1. Regular qemu blk_pread/pwrite alike functions
> > > > 2. blk_ioctl (tiny wrapper against SG_IO), supported by posix-raw on
> > > >    host block devices/sg char devices, and by iscsi
> > > >
> > > > The problem is that these two interfaces can have different block limits.
> > > >
> > > > I don't know about iscsi, but for files, doing regular IO is always unlimited,
> > > > since it passes through the kernel block layer and segemented there on
> > > > demand which is faster that doing it in userspace, while SG_IO is passed
> > as is
> > > > to the underlying SCSI device and lacks this segmentation.
> > > >
> > > > Regardless of how NBD uses these limits, I think that these limits should
> > be correctly
> > > > exposed by the block drivers, and therefore I propose is that each qemu
> > block driver
> > > > would expose a pair of block limits.
> > > > One for the regular block IO, and other for SG_IO.
> > > >
> > > > Then block driver clients (like scsi devices that you mention, nbd, etc)
> > > > can choose which limit to use, depending on which IO api they use.
> > > > The scsi-generic, and scsi-block can use the SG_IO limits,
> > > > while the rest  can use the normal (unlimited for file I/O) limits, including
> > the NBD.
> > > >
> > > > Best regards,
> > > >         Maxim Levitsky
> > > >
> > > > > > On Fri, 4 Sep 2020 at 10:09, Tom Yan <tom.ty89@gmail.com> wrote:
> > > > > > > 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>
> > > > > > > ---
> > > > > > >  block/file-posix.c | 17 ++++++++++++++++-
> > > > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/block/file-posix.c b/block/file-posix.c
> > > > > > > index 411a23cf99..9e37594145 100644
> > > > > > > --- a/block/file-posix.c
> > > > > > > +++ b/block/file-posix.c
> > > > > > > @@ -1178,6 +1178,21 @@ static int
> > sg_get_max_transfer_length(int fd)
> > > > > > >  #endif
> > > > > > >  }
> > > > > > >
> > > > > > > +static int sg_get_max_segments(int fd)
> > > > > > > +{
> > > > > > > +#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)
> > > > > > > @@ -1268,7 +1283,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.28.0
> > > > > > >
> >
> >
>