1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] file-posix: Incoporate max_segments in block limit

Fam Zheng posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170307021748.7743-1-famz@redhat.com
Test checkpatch passed
Test docker passed
There is a newer version of this series
block/file-posix.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
[Qemu-devel] [PATCH] file-posix: Incoporate max_segments in block limit
Posted by Fam Zheng 7 years, 1 month ago 
Linux exposes a separate limit, /sys/block/.../queue/max_segments, which
in the worst case can be more restrictive than BLKSECTGET (as they are
two different things). Similar to the BLKSECTGET story, guests don't see
this limit and send big requests will get -EINVAL error on SG_IO.

Lean on the safer side to clamp max_transfer according to max_segments
and page size, because in the end what host HBA gets is the mapped host
pages rather than a guest buffer.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/file-posix.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 4de1abd..b615262 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -668,6 +668,59 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
 #endif
 }
 
+static int hdev_get_max_segments(BlockDriverState *bs)
+{
+#ifdef CONFIG_LINUX
+    char buf[32];
+    const char *end;
+    char *sysfspath, *fullpath;
+    int ret;
+    int fd = -1;
+    long max_segments;
+
+    fullpath = realpath(bs->exact_filename, NULL);
+    if (!fullpath) {
+        return -errno;
+    }
+    if (strncmp(fullpath, "/dev/", 5) || !fullpath[5]) {
+        ret = -ENOTSUP;
+        goto out;
+    }
+    sysfspath = g_strdup_printf("/sys/block/%s/queue/max_segments",
+                                &fullpath[5]);
+    fd = open(sysfspath, O_RDONLY);
+    if (fd == -1) {
+        ret = -errno;
+        goto out;
+    }
+    do {
+        ret = read(fd, buf, sizeof(buf));
+    } 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 (fd >= 0) {
+        close(fd);
+    }
+    free(fullpath);
+    return ret;
+#else
+    return -ENOTSUP;
+#endif
+}
+
 static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
@@ -679,6 +732,11 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
             if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
                 bs->bl.max_transfer = pow2floor(ret);
             }
+            ret = hdev_get_max_segments(bs);
+            if (ret > 0) {
+                bs->bl.max_transfer = MIN(bs->bl.max_transfer,
+                                          ret * getpagesize());
+            }
         }
     }
 
-- 
2.9.3
Re: [Qemu-devel] [PATCH] file-posix: Incoporate max_segments in block limit
Posted by Paolo Bonzini 7 years, 1 month ago 

On 07/03/2017 03:17, Fam Zheng wrote:
> Linux exposes a separate limit, /sys/block/.../queue/max_segments, which
> in the worst case can be more restrictive than BLKSECTGET (as they are
> two different things). Similar to the BLKSECTGET story, guests don't see
> this limit and send big requests will get -EINVAL error on SG_IO.
> 
> Lean on the safer side to clamp max_transfer according to max_segments
> and page size, because in the end what host HBA gets is the mapped host
> pages rather than a guest buffer.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 4de1abd..b615262 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -668,6 +668,59 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
>  #endif
>  }
>  
> +static int hdev_get_max_segments(BlockDriverState *bs)
> +{
> +#ifdef CONFIG_LINUX
> +    char buf[32];
> +    const char *end;
> +    char *sysfspath, *fullpath;
> +    int ret;
> +    int fd = -1;
> +    long max_segments;
> +
> +    fullpath = realpath(bs->exact_filename, NULL);
> +    if (!fullpath) {
> +        return -errno;
> +    }
> +    if (strncmp(fullpath, "/dev/", 5) || !fullpath[5]) {
> +        ret = -ENOTSUP;
> +        goto out;
> +    }
> +    sysfspath = g_strdup_printf("/sys/block/%s/queue/max_segments",
> +                                &fullpath[5]);

I think you cannot rely on the /dev/... path.  Luckily, there is an
alternative path to "queue" via /sys/dev/block/MAJOR:MINOR/queue, where
MAJOR:MINOR can be retrieved via fstat.  This also avoids any possible
TOC-TOU races.

Thanks,

Paolo

> +    fd = open(sysfspath, O_RDONLY);
> +    if (fd == -1) {
> +        ret = -errno;
> +        goto out;
> +    }
> +    do {
> +        ret = read(fd, buf, sizeof(buf));
> +    } 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 (fd >= 0) {
> +        close(fd);
> +    }
> +    free(fullpath);
> +    return ret;
> +#else
> +    return -ENOTSUP;
> +#endif
> +}
> +
>  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
> @@ -679,6 +732,11 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>              if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
>                  bs->bl.max_transfer = pow2floor(ret);
>              }
> +            ret = hdev_get_max_segments(bs);
> +            if (ret > 0) {
> +                bs->bl.max_transfer = MIN(bs->bl.max_transfer,
> +                                          ret * getpagesize());
> +            }
>          }
>      }
>  
>
Re: [Qemu-devel] [PATCH] file-posix: Incoporate max_segments in block limit
Posted by Fam Zheng 7 years, 1 month ago 
On Tue, 03/07 11:58, Paolo Bonzini wrote:
> 
> 
> On 07/03/2017 03:17, Fam Zheng wrote:
> > Linux exposes a separate limit, /sys/block/.../queue/max_segments, which
> > in the worst case can be more restrictive than BLKSECTGET (as they are
> > two different things). Similar to the BLKSECTGET story, guests don't see
> > this limit and send big requests will get -EINVAL error on SG_IO.
> > 
> > Lean on the safer side to clamp max_transfer according to max_segments
> > and page size, because in the end what host HBA gets is the mapped host
> > pages rather than a guest buffer.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/file-posix.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 4de1abd..b615262 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -668,6 +668,59 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
> >  #endif
> >  }
> >  
> > +static int hdev_get_max_segments(BlockDriverState *bs)
> > +{
> > +#ifdef CONFIG_LINUX
> > +    char buf[32];
> > +    const char *end;
> > +    char *sysfspath, *fullpath;
> > +    int ret;
> > +    int fd = -1;
> > +    long max_segments;
> > +
> > +    fullpath = realpath(bs->exact_filename, NULL);
> > +    if (!fullpath) {
> > +        return -errno;
> > +    }
> > +    if (strncmp(fullpath, "/dev/", 5) || !fullpath[5]) {
> > +        ret = -ENOTSUP;
> > +        goto out;
> > +    }
> > +    sysfspath = g_strdup_printf("/sys/block/%s/queue/max_segments",
> > +                                &fullpath[5]);
> 
> I think you cannot rely on the /dev/... path.  Luckily, there is an
> alternative path to "queue" via /sys/dev/block/MAJOR:MINOR/queue, where
> MAJOR:MINOR can be retrieved via fstat.  This also avoids any possible
> TOC-TOU races.

Sounds good, I'll send another version.

Fam

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.