diff mbox series

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

Message ID 20201104173217.417538-2-mlevitsk@redhat.com (mailing list archive)
State New, archived
Headers show
Series SCSI: fix transfer limits for SCSI passthrough | expand

Commit Message

Maxim Levitsky Nov. 4, 2020, 5:32 p.m. UTC 
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(-)

Comments

Tom Yan Nov. 5, 2020, 5:44 a.m. UTC | #1 
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
>
Maxim Levitsky Nov. 5, 2020, 9:23 a.m. UTC | #2 
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
> >
diff mbox series

Patch

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,