Message ID | 20190107125215.GA5039@192.168.3.9 (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | block: don't show io_timeout if driver has no timeout handler | expand |
On 1/7/19 5:52 AM, Weiping Zhang wrote: > If the low level driver has no timerout handler, the > /sys/block/<disk>/queue/io_timeout will not be displayed. The alternative would be to make it return an error when read/written instead of having this separate group code.
On Mon, 2019-01-07 at 08:43 -0700, Jens Axboe wrote: > On 1/7/19 5:52 AM, Weiping Zhang wrote: > > If the low level driver has no timerout handler, the > > /sys/block/<disk>/queue/io_timeout will not be displayed. > > The alternative would be to make it return an error when read/written > instead of having this separate group code. Hi Jens, Christoph and I had asked Weiping to use a sysfs attribute group. See also https://www.mail-archive.com/linux-block@vger.kernel.org/msg25841.html Best regards, Bart.
On 1/7/19 9:04 AM, Bart Van Assche wrote: > On Mon, 2019-01-07 at 08:43 -0700, Jens Axboe wrote: >> On 1/7/19 5:52 AM, Weiping Zhang wrote: >>> If the low level driver has no timerout handler, the >>> /sys/block/<disk>/queue/io_timeout will not be displayed. >> >> The alternative would be to make it return an error when read/written >> instead of having this separate group code. > > Hi Jens, > > Christoph and I had asked Weiping to use a sysfs attribute group. See also > https://www.mail-archive.com/linux-block@vger.kernel.org/msg25841.html I think both solutions have parts that suck. If the file is only there for some devices, that's a bit of an API wart. But at the same time, if the file is there and not readable/writeable on those same devices, that's also kind of ugly (as the wbt exercise has shown).
On Mon, 2019-01-07 at 20:52 +0800, Weiping Zhang wrote: > If the low level driver has no timerout handler, the ^^^^^^^^ timeout? > +static umode_t queue_attr_visible(struct kobject *kobj, struct attribute *attr, > + int n) > +{ > + struct request_queue *q = > + container_of(kobj, struct request_queue, kobj); > + > + if (attr == &queue_io_timeout_entry.attr) { > + if (!q->mq_ops || !q->mq_ops->timeout) > + return 0; > + } Are there any legacy block drivers left? Do we really need the mq_ops test? Additionally, please combine the two nested if-statements into a single if-statement. > @@ -942,6 +961,14 @@ int blk_register_queue(struct gendisk *disk) > goto unlock; > } > > + ret = sysfs_create_group(&q->kobj, &queue_attr_group); > + if (ret) { > + kobject_del(&q->kobj); > + blk_trace_remove_sysfs(dev); > + kobject_put(&dev->kobj); > + goto unlock; > + } Are you sure the "goto unlock" is OK here? Shouldn't kobject_del() be called to undo the kobject_add() call if sysfs_create_group() fails? > if (queue_is_mq(q)) { > __blk_mq_register_dev(dev, q); > blk_mq_debugfs_register(q); > @@ -958,6 +985,7 @@ int blk_register_queue(struct gendisk *disk) > if (ret) { > mutex_unlock(&q->sysfs_lock); > kobject_uevent(&q->kobj, KOBJ_REMOVE); > + sysfs_remove_group(&q->kobj, &queue_attr_group); > kobject_del(&q->kobj); > blk_trace_remove_sysfs(dev); > kobject_put(&dev->kobj); > @@ -1006,6 +1034,7 @@ void blk_unregister_queue(struct gendisk *disk) > blk_mq_unregister_dev(disk_to_dev(disk), q); > mutex_unlock(&q->sysfs_lock); > > + sysfs_remove_group(&q->kobj, &queue_attr_group); > kobject_uevent(&q->kobj, KOBJ_REMOVE); > kobject_del(&q->kobj); > blk_trace_remove_sysfs(disk_to_dev(disk)); Is it necessary to call sysfs_remove_group() explicitly? Isn't this something kobject_del() does automatically? Thanks, Bart.
On Tue, Jan 08, 2019 at 04:57:40PM -0800, Bart Van Assche wrote: Hi Bart, Sorry to reply so late. > On Mon, 2019-01-07 at 20:52 +0800, Weiping Zhang wrote: > > If the low level driver has no timerout handler, the > ^^^^^^^^ > timeout? > OK, I'll fix this spelling mistake. > > +static umode_t queue_attr_visible(struct kobject *kobj, struct attribute *attr, > > + int n) > > +{ > > + struct request_queue *q = > > + container_of(kobj, struct request_queue, kobj); > > + > > + if (attr == &queue_io_timeout_entry.attr) { > > + if (!q->mq_ops || !q->mq_ops->timeout) > > + return 0; > > + } > > Are there any legacy block drivers left? Do we really need the mq_ops test? As request_fn removed, now we can use mq_ops->timeout directly. > > Additionally, please combine the two nested if-statements into a single > if-statement. OK, it's more clear. > > > @@ -942,6 +961,14 @@ int blk_register_queue(struct gendisk *disk) > > goto unlock; > > } > > > > + ret = sysfs_create_group(&q->kobj, &queue_attr_group); > > + if (ret) { > > + kobject_del(&q->kobj); > > + blk_trace_remove_sysfs(dev); > > + kobject_put(&dev->kobj); > > + goto unlock; > > + } > > Are you sure the "goto unlock" is OK here? Shouldn't kobject_del() be called > to undo the kobject_add() call if sysfs_create_group() fails? Sorry, can you tell me why it's may be not safe, if goto unlock here, if failed to call sysfs_create_group, I think we should call kobject_del. > > > if (queue_is_mq(q)) { > > __blk_mq_register_dev(dev, q); > > blk_mq_debugfs_register(q); > > @@ -958,6 +985,7 @@ int blk_register_queue(struct gendisk *disk) > > if (ret) { > > mutex_unlock(&q->sysfs_lock); > > kobject_uevent(&q->kobj, KOBJ_REMOVE); > > + sysfs_remove_group(&q->kobj, &queue_attr_group); > > kobject_del(&q->kobj); > > blk_trace_remove_sysfs(dev); > > kobject_put(&dev->kobj); > > @@ -1006,6 +1034,7 @@ void blk_unregister_queue(struct gendisk *disk) > > blk_mq_unregister_dev(disk_to_dev(disk), q); > > mutex_unlock(&q->sysfs_lock); > > > > + sysfs_remove_group(&q->kobj, &queue_attr_group); > > kobject_uevent(&q->kobj, KOBJ_REMOVE); > > kobject_del(&q->kobj); > > blk_trace_remove_sysfs(disk_to_dev(disk)); > > Is it necessary to call sysfs_remove_group() explicitly? Isn't this something > kobject_del() does automatically? I check kobject_del, it's same as you said, kobject_del->sysfs_remove_dir->kernfs_remove, it will remove all files and subdirectories belong this kobject directory. > > Thanks, > Thanks Weiping > Bart. >
On Sun, 2019-03-31 at 23:28 +0800, weiping zhang wrote: > On Tue, Jan 08, 2019 at 04:57:40PM -0800, Bart Van Assche wrote: > > > @@ -942,6 +961,14 @@ int blk_register_queue(struct gendisk *disk) > > > goto unlock; > > > } > > > > > > + ret = sysfs_create_group(&q->kobj, &queue_attr_group); > > > + if (ret) { > > > + kobject_del(&q->kobj); > > > + blk_trace_remove_sysfs(dev); > > > + kobject_put(&dev->kobj); > > > + goto unlock; > > > + } > > > > Are you sure the "goto unlock" is OK here? Shouldn't kobject_del() be called > > to undo the kobject_add() call if sysfs_create_group() fails? > > Sorry, can you tell me why it's may be not safe, if goto unlock here, > if failed to call sysfs_create_group, I think we should call > kobject_del. Can you address the other comments and repost your patch? I may have misread your patch when I wrote the above comment. Thanks, Bart.
Bart Van Assche <bvanassche@acm.org> 于2019年4月2日周二 上午6:41写道: > > On Sun, 2019-03-31 at 23:28 +0800, weiping zhang wrote: > > On Tue, Jan 08, 2019 at 04:57:40PM -0800, Bart Van Assche wrote: > > > > @@ -942,6 +961,14 @@ int blk_register_queue(struct gendisk *disk) > > > > goto unlock; > > > > } > > > > > > > > + ret = sysfs_create_group(&q->kobj, &queue_attr_group); > > > > + if (ret) { > > > > + kobject_del(&q->kobj); > > > > + blk_trace_remove_sysfs(dev); > > > > + kobject_put(&dev->kobj); > > > > + goto unlock; > > > > + } > > > > > > Are you sure the "goto unlock" is OK here? Shouldn't kobject_del() be called > > > to undo the kobject_add() call if sysfs_create_group() fails? > > > > Sorry, can you tell me why it's may be not safe, if goto unlock here, > > if failed to call sysfs_create_group, I think we should call > > kobject_del. > > Can you address the other comments and repost your patch? I may have misread > your patch when I wrote the above comment. > Thanks Bart, I post v2 later. > Thanks, > > Bart.
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index 0619c8922893..e9ee81c43d36 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -726,7 +726,7 @@ static struct queue_sysfs_entry throtl_sample_time_entry = { }; #endif -static struct attribute *default_attrs[] = { +static struct attribute *queue_attrs[] = { &queue_requests_entry.attr, &queue_ra_entry.attr, &queue_max_hw_sectors_entry.attr, @@ -768,6 +768,26 @@ static struct attribute *default_attrs[] = { NULL, }; +static umode_t queue_attr_visible(struct kobject *kobj, struct attribute *attr, + int n) +{ + struct request_queue *q = + container_of(kobj, struct request_queue, kobj); + + if (attr == &queue_io_timeout_entry.attr) { + if (!q->mq_ops || !q->mq_ops->timeout) + return 0; + } + + return attr->mode; +} + +static struct attribute_group queue_attr_group = { + .attrs = queue_attrs, + .is_visible = queue_attr_visible, +}; + + #define to_queue(atr) container_of((atr), struct queue_sysfs_entry, attr) static ssize_t @@ -893,7 +913,6 @@ static const struct sysfs_ops queue_sysfs_ops = { struct kobj_type blk_queue_ktype = { .sysfs_ops = &queue_sysfs_ops, - .default_attrs = default_attrs, .release = blk_release_queue, }; @@ -942,6 +961,14 @@ int blk_register_queue(struct gendisk *disk) goto unlock; } + ret = sysfs_create_group(&q->kobj, &queue_attr_group); + if (ret) { + kobject_del(&q->kobj); + blk_trace_remove_sysfs(dev); + kobject_put(&dev->kobj); + goto unlock; + } + if (queue_is_mq(q)) { __blk_mq_register_dev(dev, q); blk_mq_debugfs_register(q); @@ -958,6 +985,7 @@ int blk_register_queue(struct gendisk *disk) if (ret) { mutex_unlock(&q->sysfs_lock); kobject_uevent(&q->kobj, KOBJ_REMOVE); + sysfs_remove_group(&q->kobj, &queue_attr_group); kobject_del(&q->kobj); blk_trace_remove_sysfs(dev); kobject_put(&dev->kobj); @@ -1006,6 +1034,7 @@ void blk_unregister_queue(struct gendisk *disk) blk_mq_unregister_dev(disk_to_dev(disk), q); mutex_unlock(&q->sysfs_lock); + sysfs_remove_group(&q->kobj, &queue_attr_group); kobject_uevent(&q->kobj, KOBJ_REMOVE); kobject_del(&q->kobj); blk_trace_remove_sysfs(disk_to_dev(disk));
If the low level driver has no timerout handler, the /sys/block/<disk>/queue/io_timeout will not be displayed. Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com> --- block/blk-sysfs.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)