Re: [RFC v3 4/5] file-posix: introduce get_sysfs_str_val for device zoned model.

[Sam Li <faithilikerun@gmail.com>]

Hannes Reinecke <hare@suse.de> 于2022年6月27日周一 15:41写道：
>
> On 6/27/22 02:19, Sam Li wrote:
> > ---
> >   block/file-posix.c           | 60 ++++++++++++++++++++++++++++++++++++
> >   include/block/block-common.h |  4 +--
> >   2 files changed, 62 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 73c2cdfbca..74c0245e0f 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1277,6 +1277,66 @@ out:
> >   #endif
> >   }
> >
> > +/*
> > + * Convert the zoned attribute file in sysfs to internal value.
> > + */
> > +static zone_model get_sysfs_str_val(int fd, struct stat *st) {
> > +#ifdef CONFIG_LINUX
> > +    char buf[32];
> > +    char *sysfspath = NULL;
> > +    int ret;
> > +    int sysfd = -1;
> > +
> > +    if (S_ISCHR(st->st_mode)) {
> > +        if (ioctl(fd, SG_GET_SG_TABLESIZE, &ret) == 0) {
> > +            return ret;
> > +        }
> > +        return -ENOTSUP;
> > +    }
> > +
> > +    if (!S_ISBLK(st->st_mode)) {
> > +        return -ENOTSUP;
> > +    }
> > +
> > +    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/zoned",
> > +                                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);
>
> This is wrong.
> read() might return a value smaller than the 'len' argument (sizeof(buf)
> -1 in your case). But in that case it's a short read, and one need to
> call 'read()' again to fetch the remaining bytes.
>
> So the correct code would be something like:
>
> offset = 0;
> do {
>      ret = read(sysfd, buf + offset, sizeof(buf) - 1 + offset);
>      if (ret > 0)
>          offset += ret;
> } while (ret > 0);
>
> Not that you'd actually need it; reads from sysfs are basically never
> interrupted, so you should be able to read from an attribute in one go.
> IE alternatively you can drop the 'while' loop and just call read().
>
Yes, I didn't think it through.

> > +    if (ret < 0) {
> > +        ret = -errno;
> > +        goto out;
> > +    } else if (ret == 0) {
> > +        ret = -EIO;
> > +        goto out;
> > +    }
> > +    buf[ret] = 0;
> > +
> > +    /* The file is ended with '\n' */
>
> I'd rather check if the string ends with an '\n', and overwrite
> it with a '\0'. That way you'd be insulated against any changes
> to sysfs.
>
Right! I was thinking about it but got hasty to hardcode everything.

> > +    if (strcmp(buf, "host-managed\n") == 0) {
> > +        return BLK_Z_HM;
> > +    } else if (strcmp(buf, "host-aware\n") == 0) {
> > +        return BLK_Z_HA;
> > +    } else {
> > +        return -ENOTSUP;
> > +    }
> > +
> > +out:
> > +    if (sysfd != -1) {
> > +        close(sysfd);
> > +    }
> > +    g_free(sysfspath);
> > +    return ret;
> > +#else
> > +    return -ENOTSUP;
> > +#endif
> > +}
> > +
> >   static int hdev_get_max_segments(int fd, struct stat *st) {
> >       int ret;
> >       ret = get_sysfs_long_val(fd, st, "max_segments");
>
> And as you already set a precedent in your previous patch, I'd recommend
> split this in two patches, one introducing a generic function
> 'get_sysfs_str_val()' which returns a string and another function
> (eg hdev_get_zone_model()) which calls this function to fetch the device
> zoned model.
>
Maybe we can just return a str in get_sysfs_str_val and ignore
returning a zone_model for now? I was using zone_model for testing
purpose. Right now, this function is not used by others in
file-posix.c that causes compilation error in QEMU.

Thanks for reviewing!
Sam
> > diff --git a/include/block/block-common.h b/include/block/block-common.h
> > index 78cddeeda5..35e00afe8e 100644
> > --- a/include/block/block-common.h
> > +++ b/include/block/block-common.h
> > @@ -56,8 +56,8 @@ typedef enum zone_op {
> >   } zone_op;
> >
> >   typedef enum zone_model {
> > -    BLK_Z_HM,
> > -    BLK_Z_HA,
> > +    BLK_Z_HM = 0x1,
> > +    BLK_Z_HA = 0x2,
> >   } zone_model;
> >
> >   typedef enum BlkZoneCondition {
>
> 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