* [PATCH] block: don't show io_timeout if driver has no timeout handler
@ 2019-01-07 12:52 Weiping Zhang
2019-01-07 15:43 ` Jens Axboe
2019-01-09 0:57 ` Bart Van Assche
0 siblings, 2 replies; 8+ messages in thread
From: Weiping Zhang @ 2019-01-07 12:52 UTC (permalink / raw)
To: axboe; +Cc: bvanassche, hch, linux-block
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(-)
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));
--
2.14.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
2019-01-07 12:52 [PATCH] block: don't show io_timeout if driver has no timeout handler Weiping Zhang
@ 2019-01-07 15:43 ` Jens Axboe
2019-01-07 16:04 ` Bart Van Assche
2019-01-09 0:57 ` Bart Van Assche
1 sibling, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2019-01-07 15:43 UTC (permalink / raw)
To: bvanassche, hch, linux-block
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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
2019-01-07 15:43 ` Jens Axboe
@ 2019-01-07 16:04 ` Bart Van Assche
2019-01-07 16:06 ` Jens Axboe
0 siblings, 1 reply; 8+ messages in thread
From: Bart Van Assche @ 2019-01-07 16:04 UTC (permalink / raw)
To: Jens Axboe, hch, linux-block
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
2019-01-07 16:04 ` Bart Van Assche
@ 2019-01-07 16:06 ` Jens Axboe
0 siblings, 0 replies; 8+ messages in thread
From: Jens Axboe @ 2019-01-07 16:06 UTC (permalink / raw)
To: Bart Van Assche, hch, linux-block
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).
--
Jens Axboe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
2019-01-07 12:52 [PATCH] block: don't show io_timeout if driver has no timeout handler Weiping Zhang
2019-01-07 15:43 ` Jens Axboe
@ 2019-01-09 0:57 ` Bart Van Assche
2019-03-31 15:28 ` weiping zhang
1 sibling, 1 reply; 8+ messages in thread
From: Bart Van Assche @ 2019-01-09 0:57 UTC (permalink / raw)
To: Weiping Zhang, axboe; +Cc: hch, linux-block
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
2019-01-09 0:57 ` Bart Van Assche
@ 2019-03-31 15:28 ` weiping zhang
2019-04-01 22:40 ` Bart Van Assche
0 siblings, 1 reply; 8+ messages in thread
From: weiping zhang @ 2019-03-31 15:28 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Weiping Zhang, axboe, hch, linux-block
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.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
2019-03-31 15:28 ` weiping zhang
@ 2019-04-01 22:40 ` Bart Van Assche
2019-04-02 12:43 ` Weiping Zhang
0 siblings, 1 reply; 8+ messages in thread
From: Bart Van Assche @ 2019-04-01 22:40 UTC (permalink / raw)
To: weiping zhang; +Cc: Weiping Zhang, axboe, hch, linux-block
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] block: don't show io_timeout if driver has no timeout handler
2019-04-01 22:40 ` Bart Van Assche
@ 2019-04-02 12:43 ` Weiping Zhang
0 siblings, 0 replies; 8+ messages in thread
From: Weiping Zhang @ 2019-04-02 12:43 UTC (permalink / raw)
To: Bart Van Assche
Cc: weiping zhang, Weiping Zhang, Jens Axboe, hch, linux-block
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-02 12:43 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-07 12:52 [PATCH] block: don't show io_timeout if driver has no timeout handler Weiping Zhang
2019-01-07 15:43 ` Jens Axboe
2019-01-07 16:04 ` Bart Van Assche
2019-01-07 16:06 ` Jens Axboe
2019-01-09 0:57 ` Bart Van Assche
2019-03-31 15:28 ` weiping zhang
2019-04-01 22:40 ` Bart Van Assche
2019-04-02 12:43 ` Weiping Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).