diff mbox series

[RFC,v3,3/5] file-posix: introduce get_sysfs_long_val for zoned device information.

Message ID 20220627001917.9417-4-faithilikerun@gmail.com
State New
Headers show
Series *** Add support for zoned device *** | expand

Commit Message

Sam Li June 27, 2022, 12:19 a.m. UTC 
Use sysfs attribute files to get the zoned device information in case
that ioctl() commands of zone management interface won't work. It can
return long type of value like chunk_sectors, zoned_append_max_bytes,
max_open_zones, max_active_zones.
---
 block/file-posix.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

Comments

Hannes Reinecke June 27, 2022, 7:31 a.m. UTC | #1 
On 6/27/22 02:19, Sam Li wrote:
> Use sysfs attribute files to get the zoned device information in case
> that ioctl() commands of zone management interface won't work. It can
> return long type of value like chunk_sectors, zoned_append_max_bytes,
> max_open_zones, max_active_zones.
> ---
>   block/file-posix.c | 37 +++++++++++++++++++++++++------------
>   1 file changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 1b8b0d351f..73c2cdfbca 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1216,15 +1216,19 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st)
>   #endif
>   }
>   
> -static int hdev_get_max_segments(int fd, struct stat *st)
> -{
> +/*
> + * Get zoned device information (chunk_sectors, zoned_append_max_bytes,
> + * max_open_zones, max_active_zones) through sysfs attribute files.
> + */
> +static long get_sysfs_long_val(int fd, struct stat *st,
> +                               const char *attribute) {
>   #ifdef CONFIG_LINUX
>       char buf[32];
>       const char *end;
>       char *sysfspath = NULL;
>       int ret;
>       int sysfd = -1;
> -    long max_segments;
> +    long val;
>   
>       if (S_ISCHR(st->st_mode)) {
>           if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
> @@ -1237,8 +1241,9 @@ static int hdev_get_max_segments(int fd, struct stat *st)
>           return -ENOTSUP;
>       }
>   
> -    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
> -                                major(st->st_rdev), minor(st->st_rdev));
> +    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
> +                                major(st->st_rdev), minor(st->st_rdev),
> +                                attribute);
>       sysfd = open(sysfspath, O_RDONLY);
>       if (sysfd == -1) {
>           ret = -errno;
> @@ -1256,9 +1261,9 @@ static int hdev_get_max_segments(int fd, struct stat *st)
>       }
>       buf[ret] = 0;
>       /* The file is ended with '\n', pass 'end' to accept that. */
> -    ret = qemu_strtol(buf, &end, 10, &max_segments);
> +    ret = qemu_strtol(buf, &end, 10, &val);
>       if (ret == 0 && end && *end == '\n') {
> -        ret = max_segments;
> +        ret = val;
>       }
>   
>   out:
> @@ -1272,6 +1277,15 @@ out:
>   #endif
>   }
>   
> +static int hdev_get_max_segments(int fd, struct stat *st) {
> +    int ret;
> +    ret = get_sysfs_long_val(fd, st, "max_segments");
> +    if (ret < 0) {
> +        return -1;
> +    }
> +    return ret;
> +}
> +
>   static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>   {
>       BDRVRawState *s = bs->opaque;
> @@ -1872,6 +1886,7 @@ static int handle_aiocb_zone_report(void *opaque) {
>   
>   static int handle_aiocb_zone_mgmt(void *opaque) {
>       RawPosixAIOData *aiocb = opaque;
> +    BlockDriverState *s = aiocb->bs;
>       int fd = aiocb->aio_fildes;
>       int64_t offset = aiocb->aio_offset;
>       int64_t len = aiocb->aio_nbytes;
> @@ -1884,11 +1899,9 @@ static int handle_aiocb_zone_mgmt(void *opaque) {
>       int64_t zone_size_mask;
>       int ret;
>   
> -    ret = ioctl(fd, BLKGETZONESZ, &zone_size);
> -    if (ret) {
> -        return -1;
> -    }
> -
> +    g_autofree struct stat *file = g_new(struct stat, 1);
> +    stat(s->filename, file);
> +    zone_size = get_sysfs_long_val(fd, file, "chunk_sectors");
>       zone_size_mask = zone_size - 1;
>       if (offset & zone_size_mask) {
>           error_report("offset is not the start of a zone");

Round of applause.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
Sam Li June 27, 2022, 8:32 a.m. UTC | #2 
Hannes Reinecke <hare@suse.de> 于2022年6月27日周一 15:31写道：
>
> On 6/27/22 02:19, Sam Li wrote:
> > Use sysfs attribute files to get the zoned device information in case
> > that ioctl() commands of zone management interface won't work. It can
> > return long type of value like chunk_sectors, zoned_append_max_bytes,
> > max_open_zones, max_active_zones.
> > ---
> >   block/file-posix.c | 37 +++++++++++++++++++++++++------------
> >   1 file changed, 25 insertions(+), 12 deletions(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 1b8b0d351f..73c2cdfbca 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1216,15 +1216,19 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st)
> >   #endif
> >   }
> >
> > -static int hdev_get_max_segments(int fd, struct stat *st)
> > -{
> > +/*
> > + * Get zoned device information (chunk_sectors, zoned_append_max_bytes,
> > + * max_open_zones, max_active_zones) through sysfs attribute files.
> > + */
> > +static long get_sysfs_long_val(int fd, struct stat *st,
> > +                               const char *attribute) {
> >   #ifdef CONFIG_LINUX
> >       char buf[32];
> >       const char *end;
> >       char *sysfspath = NULL;
> >       int ret;
> >       int sysfd = -1;
> > -    long max_segments;
> > +    long val;
> >
> >       if (S_ISCHR(st->st_mode)) {
> >           if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
> > @@ -1237,8 +1241,9 @@ static int hdev_get_max_segments(int fd, struct stat *st)
> >           return -ENOTSUP;
> >       }
> >
> > -    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
> > -                                major(st->st_rdev), minor(st->st_rdev));
> > +    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
> > +                                major(st->st_rdev), minor(st->st_rdev),
> > +                                attribute);
> >       sysfd = open(sysfspath, O_RDONLY);
> >       if (sysfd == -1) {
> >           ret = -errno;
> > @@ -1256,9 +1261,9 @@ static int hdev_get_max_segments(int fd, struct stat *st)
> >       }
> >       buf[ret] = 0;
> >       /* The file is ended with '\n', pass 'end' to accept that. */
> > -    ret = qemu_strtol(buf, &end, 10, &max_segments);
> > +    ret = qemu_strtol(buf, &end, 10, &val);
> >       if (ret == 0 && end && *end == '\n') {
> > -        ret = max_segments;
> > +        ret = val;
> >       }
> >
> >   out:
> > @@ -1272,6 +1277,15 @@ out:
> >   #endif
> >   }
> >
> > +static int hdev_get_max_segments(int fd, struct stat *st) {
> > +    int ret;
> > +    ret = get_sysfs_long_val(fd, st, "max_segments");
> > +    if (ret < 0) {
> > +        return -1;
> > +    }
> > +    return ret;
> > +}
> > +
> >   static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
> >   {
> >       BDRVRawState *s = bs->opaque;
> > @@ -1872,6 +1886,7 @@ static int handle_aiocb_zone_report(void *opaque) {
> >
> >   static int handle_aiocb_zone_mgmt(void *opaque) {
> >       RawPosixAIOData *aiocb = opaque;
> > +    BlockDriverState *s = aiocb->bs;
> >       int fd = aiocb->aio_fildes;
> >       int64_t offset = aiocb->aio_offset;
> >       int64_t len = aiocb->aio_nbytes;
> > @@ -1884,11 +1899,9 @@ static int handle_aiocb_zone_mgmt(void *opaque) {
> >       int64_t zone_size_mask;
> >       int ret;
> >
> > -    ret = ioctl(fd, BLKGETZONESZ, &zone_size);
> > -    if (ret) {
> > -        return -1;
> > -    }
> > -
> > +    g_autofree struct stat *file = g_new(struct stat, 1);
> > +    stat(s->filename, file);
> > +    zone_size = get_sysfs_long_val(fd, file, "chunk_sectors");
> >       zone_size_mask = zone_size - 1;
> >       if (offset & zone_size_mask) {
> >           error_report("offset is not the start of a zone");
>
> Round of applause.
>
Thanks! It was based on Damien's suggestion.

> Reviewed-by: Hannes Reinecke <hare@suse.de>
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                        Kernel Storage Architect
> hare@suse.de                                      +49 911 74053 688
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
Stefan Hajnoczi June 28, 2022, 8:09 a.m. UTC | #3 
On Mon, Jun 27, 2022 at 08:19:15AM +0800, Sam Li wrote:
> Use sysfs attribute files to get the zoned device information in case
> that ioctl() commands of zone management interface won't work. It can
> return long type of value like chunk_sectors, zoned_append_max_bytes,
> max_open_zones, max_active_zones.
> ---
>  block/file-posix.c | 37 +++++++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 1b8b0d351f..73c2cdfbca 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1216,15 +1216,19 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st)
>  #endif
>  }
>  
> -static int hdev_get_max_segments(int fd, struct stat *st)
> -{
> +/*
> + * Get zoned device information (chunk_sectors, zoned_append_max_bytes,
> + * max_open_zones, max_active_zones) through sysfs attribute files.

This function is also used to get max_segments, which is not related to
zoned devices. How about:

  Get a block queue sysfs attribute value.

> + */
> +static long get_sysfs_long_val(int fd, struct stat *st,
> +                               const char *attribute) {
>  #ifdef CONFIG_LINUX
>      char buf[32];
>      const char *end;
>      char *sysfspath = NULL;
>      int ret;
>      int sysfd = -1;
> -    long max_segments;
> +    long val;
>  
>      if (S_ISCHR(st->st_mode)) {
>          if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
> @@ -1237,8 +1241,9 @@ static int hdev_get_max_segments(int fd, struct stat *st)
>          return -ENOTSUP;
>      }
>  
> -    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
> -                                major(st->st_rdev), minor(st->st_rdev));
> +    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
> +                                major(st->st_rdev), minor(st->st_rdev),
> +                                attribute);
>      sysfd = open(sysfspath, O_RDONLY);
>      if (sysfd == -1) {
>          ret = -errno;
> @@ -1256,9 +1261,9 @@ static int hdev_get_max_segments(int fd, struct stat *st)
>      }
>      buf[ret] = 0;
>      /* The file is ended with '\n', pass 'end' to accept that. */
> -    ret = qemu_strtol(buf, &end, 10, &max_segments);
> +    ret = qemu_strtol(buf, &end, 10, &val);
>      if (ret == 0 && end && *end == '\n') {
> -        ret = max_segments;
> +        ret = val;
>      }
>  
>  out:
> @@ -1272,6 +1277,15 @@ out:
>  #endif
>  }
>  
> +static int hdev_get_max_segments(int fd, struct stat *st) {
> +    int ret;
> +    ret = get_sysfs_long_val(fd, st, "max_segments");
> +    if (ret < 0) {
> +        return -1;

This hides the actual error number. The if statement can be dropped and
the function can be simplified to:

  static int hdev_get_max_segments(int fd, struct stat *st) {
      return get_sysfs_long_val(fd, st, "max_segments");
  }

Whether you want to keep the tiny helper function or inline
get_sysfs_long_val(fd, st, "max_segments") into the caller is up to you.

> +    }
> +    return ret;
> +}
> +
>  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
> @@ -1872,6 +1886,7 @@ static int handle_aiocb_zone_report(void *opaque) {
>  
>  static int handle_aiocb_zone_mgmt(void *opaque) {
>      RawPosixAIOData *aiocb = opaque;
> +    BlockDriverState *s = aiocb->bs;
>      int fd = aiocb->aio_fildes;
>      int64_t offset = aiocb->aio_offset;
>      int64_t len = aiocb->aio_nbytes;
> @@ -1884,11 +1899,9 @@ static int handle_aiocb_zone_mgmt(void *opaque) {
>      int64_t zone_size_mask;
>      int ret;
>  
> -    ret = ioctl(fd, BLKGETZONESZ, &zone_size);
> -    if (ret) {
> -        return -1;
> -    }
> -
> +    g_autofree struct stat *file = g_new(struct stat, 1);
> +    stat(s->filename, file);

There is no need to allocate struct stat using g_new(). It's a small
struct that can be on the stack.

Also, fstat(fd, &st) is preferred when the file descriptor is already
open because it avoids race conditions due to file renaming/path
traversal.

Here is how this could be written:

  struct stat st;
  if (fstat(fd, &st) < 0) {
      return -errno;
  }

> +    zone_size = get_sysfs_long_val(fd, file, "chunk_sectors");
>      zone_size_mask = zone_size - 1;
>      if (offset & zone_size_mask) {
>          error_report("offset is not the start of a zone");
> -- 
> 2.35.3
>
Sam Li June 28, 2022, 9:18 a.m. UTC | #4 
Stefan Hajnoczi <stefanha@redhat.com> 于2022年6月28日周二 16:20写道：
>
> On Mon, Jun 27, 2022 at 08:19:15AM +0800, Sam Li wrote:
> > Use sysfs attribute files to get the zoned device information in case
> > that ioctl() commands of zone management interface won't work. It can
> > return long type of value like chunk_sectors, zoned_append_max_bytes,
> > max_open_zones, max_active_zones.
> > ---
> >  block/file-posix.c | 37 +++++++++++++++++++++++++------------
> >  1 file changed, 25 insertions(+), 12 deletions(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 1b8b0d351f..73c2cdfbca 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1216,15 +1216,19 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st)
> >  #endif
> >  }
> >
> > -static int hdev_get_max_segments(int fd, struct stat *st)
> > -{
> > +/*
> > + * Get zoned device information (chunk_sectors, zoned_append_max_bytes,
> > + * max_open_zones, max_active_zones) through sysfs attribute files.
>
> This function is also used to get max_segments, which is not related to
> zoned devices. How about:
>
>   Get a block queue sysfs attribute value.
>
> > + */
> > +static long get_sysfs_long_val(int fd, struct stat *st,
> > +                               const char *attribute) {
> >  #ifdef CONFIG_LINUX
> >      char buf[32];
> >      const char *end;
> >      char *sysfspath = NULL;
> >      int ret;
> >      int sysfd = -1;
> > -    long max_segments;
> > +    long val;
> >
> >      if (S_ISCHR(st->st_mode)) {
> >          if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
> > @@ -1237,8 +1241,9 @@ static int hdev_get_max_segments(int fd, struct stat *st)
> >          return -ENOTSUP;
> >      }
> >
> > -    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
> > -                                major(st->st_rdev), minor(st->st_rdev));
> > +    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
> > +                                major(st->st_rdev), minor(st->st_rdev),
> > +                                attribute);
> >      sysfd = open(sysfspath, O_RDONLY);
> >      if (sysfd == -1) {
> >          ret = -errno;
> > @@ -1256,9 +1261,9 @@ static int hdev_get_max_segments(int fd, struct stat *st)
> >      }
> >      buf[ret] = 0;
> >      /* The file is ended with '\n', pass 'end' to accept that. */
> > -    ret = qemu_strtol(buf, &end, 10, &max_segments);
> > +    ret = qemu_strtol(buf, &end, 10, &val);
> >      if (ret == 0 && end && *end == '\n') {
> > -        ret = max_segments;
> > +        ret = val;
> >      }
> >
> >  out:
> > @@ -1272,6 +1277,15 @@ out:
> >  #endif
> >  }
> >
> > +static int hdev_get_max_segments(int fd, struct stat *st) {
> > +    int ret;
> > +    ret = get_sysfs_long_val(fd, st, "max_segments");
> > +    if (ret < 0) {
> > +        return -1;
>
> This hides the actual error number. The if statement can be dropped and
> the function can be simplified to:
>
>   static int hdev_get_max_segments(int fd, struct stat *st) {
>       return get_sysfs_long_val(fd, st, "max_segments");
>   }
>
> Whether you want to keep the tiny helper function or inline
> get_sysfs_long_val(fd, st, "max_segments") into the caller is up to you.
>
> > +    }
> > +    return ret;
> > +}
> > +
> >  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
> >  {
> >      BDRVRawState *s = bs->opaque;
> > @@ -1872,6 +1886,7 @@ static int handle_aiocb_zone_report(void *opaque) {
> >
> >  static int handle_aiocb_zone_mgmt(void *opaque) {
> >      RawPosixAIOData *aiocb = opaque;
> > +    BlockDriverState *s = aiocb->bs;
> >      int fd = aiocb->aio_fildes;
> >      int64_t offset = aiocb->aio_offset;
> >      int64_t len = aiocb->aio_nbytes;
> > @@ -1884,11 +1899,9 @@ static int handle_aiocb_zone_mgmt(void *opaque) {
> >      int64_t zone_size_mask;
> >      int ret;
> >
> > -    ret = ioctl(fd, BLKGETZONESZ, &zone_size);
> > -    if (ret) {
> > -        return -1;
> > -    }
> > -
> > +    g_autofree struct stat *file = g_new(struct stat, 1);
> > +    stat(s->filename, file);
>
> There is no need to allocate struct stat using g_new(). It's a small
> struct that can be on the stack.
>
> Also, fstat(fd, &st) is preferred when the file descriptor is already
> open because it avoids race conditions due to file renaming/path
> traversal.
>
> Here is how this could be written:
>
>   struct stat st;
>   if (fstat(fd, &st) < 0) {
>       return -errno;
>   }

Thanks for the suggestions!

>
> > +    zone_size = get_sysfs_long_val(fd, file, "chunk_sectors");
> >      zone_size_mask = zone_size - 1;
> >      if (offset & zone_size_mask) {
> >          error_report("offset is not the start of a zone");
> > --
> > 2.35.3
> >
diff mbox series

Patch

diff --git a/block/file-posix.c b/block/file-posix.c
index 1b8b0d351f..73c2cdfbca 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1216,15 +1216,19 @@  static int hdev_get_max_hw_transfer(int fd, struct stat *st)
 #endif
 }
 
-static int hdev_get_max_segments(int fd, struct stat *st)
-{
+/*
+ * Get zoned device information (chunk_sectors, zoned_append_max_bytes,
+ * max_open_zones, max_active_zones) through sysfs attribute files.
+ */
+static long get_sysfs_long_val(int fd, struct stat *st,
+                               const char *attribute) {
 #ifdef CONFIG_LINUX
     char buf[32];
     const char *end;
     char *sysfspath = NULL;
     int ret;
     int sysfd = -1;
-    long max_segments;
+    long val;
 
     if (S_ISCHR(st->st_mode)) {
         if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
@@ -1237,8 +1241,9 @@  static int hdev_get_max_segments(int fd, struct stat *st)
         return -ENOTSUP;
     }
 
-    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
-                                major(st->st_rdev), minor(st->st_rdev));
+    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
+                                major(st->st_rdev), minor(st->st_rdev),
+                                attribute);
     sysfd = open(sysfspath, O_RDONLY);
     if (sysfd == -1) {
         ret = -errno;
@@ -1256,9 +1261,9 @@  static int hdev_get_max_segments(int fd, struct stat *st)
     }
     buf[ret] = 0;
     /* The file is ended with '\n', pass 'end' to accept that. */
-    ret = qemu_strtol(buf, &end, 10, &max_segments);
+    ret = qemu_strtol(buf, &end, 10, &val);
     if (ret == 0 && end && *end == '\n') {
-        ret = max_segments;
+        ret = val;
     }
 
 out:
@@ -1272,6 +1277,15 @@  out:
 #endif
 }
 
+static int hdev_get_max_segments(int fd, struct stat *st) {
+    int ret;
+    ret = get_sysfs_long_val(fd, st, "max_segments");
+    if (ret < 0) {
+        return -1;
+    }
+    return ret;
+}
+
 static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
@@ -1872,6 +1886,7 @@  static int handle_aiocb_zone_report(void *opaque) {
 
 static int handle_aiocb_zone_mgmt(void *opaque) {
     RawPosixAIOData *aiocb = opaque;
+    BlockDriverState *s = aiocb->bs;
     int fd = aiocb->aio_fildes;
     int64_t offset = aiocb->aio_offset;
     int64_t len = aiocb->aio_nbytes;
@@ -1884,11 +1899,9 @@  static int handle_aiocb_zone_mgmt(void *opaque) {
     int64_t zone_size_mask;
     int ret;
 
-    ret = ioctl(fd, BLKGETZONESZ, &zone_size);
-    if (ret) {
-        return -1;
-    }
-
+    g_autofree struct stat *file = g_new(struct stat, 1);
+    stat(s->filename, file);
+    zone_size = get_sysfs_long_val(fd, file, "chunk_sectors");
     zone_size_mask = zone_size - 1;
     if (offset & zone_size_mask) {
         error_report("offset is not the start of a zone");