[PATCH] block: don't show io_timeout if driver has no timeout handler

[PATCH] block: don't show io_timeout if driver has no timeout handler

[Weiping Zhang]

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

Re: [PATCH] block: don't show io_timeout if driver has no timeout handler

[Jens Axboe]

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

Re: [PATCH] block: don't show io_timeout if driver has no timeout handler

[Bart Van Assche]

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.

Re: [PATCH] block: don't show io_timeout if driver has no timeout handler

[Jens Axboe]

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

Re: [PATCH] block: don't show io_timeout if driver has no timeout handler

[Bart Van Assche]

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.

Re: [PATCH] block: don't show io_timeout if driver has no timeout handler

[weiping zhang]

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.
> 

Re: [PATCH] block: don't show io_timeout if driver has no timeout handler

[Bart Van Assche]

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.

Re: [PATCH] block: don't show io_timeout if driver has no timeout handler

[Weiping Zhang]

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.