| Message ID | 1459764020-126038-7-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hannes, >Add a sysfs queue attribute 'zoned' to display the zone layout >for zoned devices. > >Signed-off-by: Hannes Reinecke <hare@suse.de> >--- > block/blk-sysfs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > >diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c >index ff97091..748bb27 100644 >--- a/block/blk-sysfs.c >+++ b/block/blk-sysfs.c >@@ -244,6 +244,43 @@ static ssize_t queue_max_hw_sectors_show(struct request_queue *q, char *page) > return queue_var_show(max_hw_sectors_kb, (page)); > } > >+#ifdef CONFIG_BLK_DEV_ZONED >+static ssize_t queue_zoned_show(struct request_queue *q, char *page) >+{ >+ struct rb_node *node; >+ struct blk_zone *zone; >+ ssize_t offset = 0, end = 0; >+ size_t size = 0, num = 0; >+ enum blk_zone_type type = BLK_ZONE_TYPE_UNKNOWN; >+ >+ for (node = rb_first(&q->zones); node; node = rb_next(node)) { >+ zone = rb_entry(node, struct blk_zone, node); >+ if (zone->type != type || >+ zone->len != size || >+ end != zone->start) { >+ if (size != 0) >+ offset += sprintf(page + offset, "%zu\n", num); >+ /* We can only store one page ... */ >+ if (offset + 42 > PAGE_SIZE) { >+ offset += sprintf(page + offset, "...\n"); >+ return offset; >+ } >+ size = zone->len; >+ type = zone->type; >+ offset += sprintf(page + offset, "%zu %zu %d ", >+ zone->start, size, type); >+ num = 0; >+ end = zone->start + size; >+ } else >+ end += zone->len; >+ num++; >+ } >+ if (num > 0) >+ offset += sprintf(page + offset, "%zu\n", num); >+ return offset > 0 ? offset : -EINVAL; >+} >+#endif With this, an application reading the “zoned” file for a non-SMR disk will get a -EINVAL error. Not really super nice. Could we just have the zoned files contain a single “0” for non-SMR disks ? Or at least have the file empty and read return 0 Bytes. That would allow applications to easily and cleanly detect if they are dealing with a SMR disk (or not) instead of assuming that “-EINVAL” means “not SMR”, which seems very ugly to me. Best. -- Damien Le Moal, Ph.D. Sr Manager, System Software Group, WW Research, HGST, a Western Digital company Damien.LeMoal@hgst.com Tel: (+81) 0466-98-3593 (Ext. 51-3593) 1 kirihara-cho, Fujisawa, Kanagawa, 252-0888 Japan www.hgst.com <http://www.hgst.com> Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer: This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.
On 04/07/2016 03:56 AM, Damien Le Moal wrote: > > Hannes, > >> Add a sysfs queue attribute 'zoned' to display the zone layout >> for zoned devices. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> block/blk-sysfs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 47 insertions(+) >> >> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c >> index ff97091..748bb27 100644 >> --- a/block/blk-sysfs.c >> +++ b/block/blk-sysfs.c >> @@ -244,6 +244,43 @@ static ssize_t queue_max_hw_sectors_show(struct request_queue *q, char *page) >> return queue_var_show(max_hw_sectors_kb, (page)); >> } >> >> +#ifdef CONFIG_BLK_DEV_ZONED >> +static ssize_t queue_zoned_show(struct request_queue *q, char *page) >> +{ >> + struct rb_node *node; >> + struct blk_zone *zone; >> + ssize_t offset = 0, end = 0; >> + size_t size = 0, num = 0; >> + enum blk_zone_type type = BLK_ZONE_TYPE_UNKNOWN; >> + >> + for (node = rb_first(&q->zones); node; node = rb_next(node)) { >> + zone = rb_entry(node, struct blk_zone, node); >> + if (zone->type != type || >> + zone->len != size || >> + end != zone->start) { >> + if (size != 0) >> + offset += sprintf(page + offset, "%zu\n", num); >> + /* We can only store one page ... */ >> + if (offset + 42 > PAGE_SIZE) { >> + offset += sprintf(page + offset, "...\n"); >> + return offset; >> + } >> + size = zone->len; >> + type = zone->type; >> + offset += sprintf(page + offset, "%zu %zu %d ", >> + zone->start, size, type); >> + num = 0; >> + end = zone->start + size; >> + } else >> + end += zone->len; >> + num++; >> + } >> + if (num > 0) >> + offset += sprintf(page + offset, "%zu\n", num); >> + return offset > 0 ? offset : -EINVAL; >> +} >> +#endif > > With this, an application reading the “zoned” file for a non-SMR disk > will get a -EINVAL error. Not really super nice. Could we just have the > zoned files contain a single “0” for non-SMR disks ? Or at least have the > file empty and read return 0 Bytes. That would allow applications to easily > and cleanly detect if they are dealing with a SMR disk (or not) instead of > assuming that “-EINVAL” means “not SMR”, which seems very ugly to me. > Sure. Actually I was looking in blanking out this attribute entirely like I did with the SCSI sysfs attributes, but as the 'queue' sysfs directory is a blank kobj it's not easily done. But yes, '0' seems like a reasonable value here. Cheers, Hannes
On 04/04/2016 03:00 AM, Hannes Reinecke wrote: > Add a sysfs queue attribute 'zoned' to display the zone layout > for zoned devices. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > block/blk-sysfs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c > index ff97091..748bb27 100644 > --- a/block/blk-sysfs.c > +++ b/block/blk-sysfs.c > @@ -244,6 +244,43 @@ static ssize_t queue_max_hw_sectors_show(struct request_queue *q, char *page) > return queue_var_show(max_hw_sectors_kb, (page)); > } > > +#ifdef CONFIG_BLK_DEV_ZONED > +static ssize_t queue_zoned_show(struct request_queue *q, char *page) > +{ > + struct rb_node *node; > + struct blk_zone *zone; > + ssize_t offset = 0, end = 0; > + size_t size = 0, num = 0; > + enum blk_zone_type type = BLK_ZONE_TYPE_UNKNOWN; > + > + for (node = rb_first(&q->zones); node; node = rb_next(node)) { > + zone = rb_entry(node, struct blk_zone, node); > + if (zone->type != type || > + zone->len != size || > + end != zone->start) { > + if (size != 0) > + offset += sprintf(page + offset, "%zu\n", num); > + /* We can only store one page ... */ > + if (offset + 42 > PAGE_SIZE) { > + offset += sprintf(page + offset, "...\n"); > + return offset; > + } > + size = zone->len; > + type = zone->type; > + offset += sprintf(page + offset, "%zu %zu %d ", > + zone->start, size, type); > + num = 0; > + end = zone->start + size; > + } else > + end += zone->len; > + num++; > + } > + if (num > 0) > + offset += sprintf(page + offset, "%zu\n", num); > + return offset > 0 ? offset : -EINVAL; > +} > +#endif > + > #define QUEUE_SYSFS_BIT_FNS(name, flag, neg) \ > static ssize_t \ > queue_show_##name(struct request_queue *q, char *page) \ > @@ -468,6 +505,13 @@ static struct queue_sysfs_entry queue_write_same_max_entry = { > .show = queue_write_same_max_show, > }; > > +#ifdef CONFIG_BLK_DEV_ZONED > +static struct queue_sysfs_entry queue_zoned_entry = { > + .attr = {.name = "zoned", .mode = S_IRUGO }, > + .show = queue_zoned_show, > +}; > +#endif > + Hello Hannes, Have you considered to move the above definitions into a new file? That would allow to avoid two #ifdefs and to move the code that decides whether or not the above code gets built into block/Makefile. Additionally, have you considered to create one sysfs directory per zone instead of one sysfs attribute with all zone information? From Documentation/filesystems/sysfs.txt: "Attributes should be ASCII text files, preferably with only one value per file." Thanks, Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/15/2016 07:45 PM, Bart Van Assche wrote: > On 04/04/2016 03:00 AM, Hannes Reinecke wrote: >> Add a sysfs queue attribute 'zoned' to display the zone layout >> for zoned devices. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> block/blk-sysfs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 47 insertions(+) >> >> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c >> index ff97091..748bb27 100644 >> --- a/block/blk-sysfs.c >> +++ b/block/blk-sysfs.c >> @@ -244,6 +244,43 @@ static ssize_t queue_max_hw_sectors_show(struct >> request_queue *q, char *page) >> return queue_var_show(max_hw_sectors_kb, (page)); >> } >> >> +#ifdef CONFIG_BLK_DEV_ZONED >> +static ssize_t queue_zoned_show(struct request_queue *q, char *page) >> +{ >> + struct rb_node *node; >> + struct blk_zone *zone; >> + ssize_t offset = 0, end = 0; >> + size_t size = 0, num = 0; >> + enum blk_zone_type type = BLK_ZONE_TYPE_UNKNOWN; >> + >> + for (node = rb_first(&q->zones); node; node = rb_next(node)) { >> + zone = rb_entry(node, struct blk_zone, node); >> + if (zone->type != type || >> + zone->len != size || >> + end != zone->start) { >> + if (size != 0) >> + offset += sprintf(page + offset, "%zu\n", num); >> + /* We can only store one page ... */ >> + if (offset + 42 > PAGE_SIZE) { >> + offset += sprintf(page + offset, "...\n"); >> + return offset; >> + } >> + size = zone->len; >> + type = zone->type; >> + offset += sprintf(page + offset, "%zu %zu %d ", >> + zone->start, size, type); >> + num = 0; >> + end = zone->start + size; >> + } else >> + end += zone->len; >> + num++; >> + } >> + if (num > 0) >> + offset += sprintf(page + offset, "%zu\n", num); >> + return offset > 0 ? offset : -EINVAL; >> +} >> +#endif >> + >> #define QUEUE_SYSFS_BIT_FNS(name, flag, neg) \ >> static ssize_t \ >> queue_show_##name(struct request_queue *q, char *page) \ >> @@ -468,6 +505,13 @@ static struct queue_sysfs_entry >> queue_write_same_max_entry = { >> .show = queue_write_same_max_show, >> }; >> >> +#ifdef CONFIG_BLK_DEV_ZONED >> +static struct queue_sysfs_entry queue_zoned_entry = { >> + .attr = {.name = "zoned", .mode = S_IRUGO }, >> + .show = queue_zoned_show, >> +}; >> +#endif >> + > > Hello Hannes, > > Have you considered to move the above definitions into a new file? That > would allow to avoid two #ifdefs and to move the code that decides > whether or not the above code gets built into block/Makefile. > Well, it's just these few lines, so I thought it easier to put it into the existing file. But sure I can add a separate files for that. > Additionally, have you considered to create one sysfs directory per zone > instead of one sysfs attribute with all zone information? From > Documentation/filesystems/sysfs.txt: "Attributes should be ASCII text > files, preferably with only one value per file." > Yes, I have considered it. But doing so would require me to add about 20k sysfs attributes. For no apparent gain. So I've settled on this condensed approach here. Cheers, Hannes
On 04/15/2016 11:03 AM, Hannes Reinecke wrote: > On 04/15/2016 07:45 PM, Bart Van Assche wrote: >> Additionally, have you considered to create one sysfs directory per zone >> instead of one sysfs attribute with all zone information? From >> Documentation/filesystems/sysfs.txt: "Attributes should be ASCII text >> files, preferably with only one value per file." > > Yes, I have considered it. > But doing so would require me to add about 20k sysfs attributes. > For no apparent gain. > So I've settled on this condensed approach here. Hello Hannes, My understanding is that the one-value-per-file rule was introduced for sysfs to make it easy for software, e.g. shell scripts, to process that information. If multiple values occur in the same sysfs file sometimes complicated code is needed in shell scripts to parse these values. If one value is present in each sysfs file it becomes much easier for software to process that sysfs information. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index ff97091..748bb27 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -244,6 +244,43 @@ static ssize_t queue_max_hw_sectors_show(struct request_queue *q, char *page) return queue_var_show(max_hw_sectors_kb, (page)); } +#ifdef CONFIG_BLK_DEV_ZONED +static ssize_t queue_zoned_show(struct request_queue *q, char *page) +{ + struct rb_node *node; + struct blk_zone *zone; + ssize_t offset = 0, end = 0; + size_t size = 0, num = 0; + enum blk_zone_type type = BLK_ZONE_TYPE_UNKNOWN; + + for (node = rb_first(&q->zones); node; node = rb_next(node)) { + zone = rb_entry(node, struct blk_zone, node); + if (zone->type != type || + zone->len != size || + end != zone->start) { + if (size != 0) + offset += sprintf(page + offset, "%zu\n", num); + /* We can only store one page ... */ + if (offset + 42 > PAGE_SIZE) { + offset += sprintf(page + offset, "...\n"); + return offset; + } + size = zone->len; + type = zone->type; + offset += sprintf(page + offset, "%zu %zu %d ", + zone->start, size, type); + num = 0; + end = zone->start + size; + } else + end += zone->len; + num++; + } + if (num > 0) + offset += sprintf(page + offset, "%zu\n", num); + return offset > 0 ? offset : -EINVAL; +} +#endif + #define QUEUE_SYSFS_BIT_FNS(name, flag, neg) \ static ssize_t \ queue_show_##name(struct request_queue *q, char *page) \ @@ -468,6 +505,13 @@ static struct queue_sysfs_entry queue_write_same_max_entry = { .show = queue_write_same_max_show, }; +#ifdef CONFIG_BLK_DEV_ZONED +static struct queue_sysfs_entry queue_zoned_entry = { + .attr = {.name = "zoned", .mode = S_IRUGO }, + .show = queue_zoned_show, +}; +#endif + static struct queue_sysfs_entry queue_nonrot_entry = { .attr = {.name = "rotational", .mode = S_IRUGO | S_IWUSR }, .show = queue_show_nonrot, @@ -525,6 +569,9 @@ static struct attribute *default_attrs[] = { &queue_discard_zeroes_data_entry.attr, &queue_write_same_max_entry.attr, &queue_nonrot_entry.attr, +#ifdef CONFIG_BLK_DEV_ZONED + &queue_zoned_entry.attr, +#endif &queue_nomerges_entry.attr, &queue_rq_affinity_entry.attr, &queue_iostats_entry.attr,
Add a sysfs queue attribute 'zoned' to display the zone layout for zoned devices. Signed-off-by: Hannes Reinecke <hare@suse.de> --- block/blk-sysfs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)