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

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

[Weiping Zhang]

If the low level driver has no timeout handler, the
/sys/block/<disk>/queue/io_timeout will not be displayed.

Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
---
Changes since v1:
 * don't call sysfs_remove_group explicitly
 * also check q->mq_ops otherwise bug at md driver
[    2.094516]  internal_create_group+0xd8/0x3a0
[    2.095238]  blk_register_queue+0xb8/0x1e0
[    2.095912]  __device_add_disk+0x2b5/0x4a0
[    2.096573]  md_alloc+0x1ab/0x330

 block/blk-sysfs.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 422327089e0f..a16a02c52a85 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -728,7 +728,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,
@@ -770,6 +770,25 @@ 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 &&
+		(!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
@@ -890,7 +909,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,
 };
 
@@ -939,6 +957,14 @@ int blk_register_queue(struct gendisk *disk)
 		goto unlock;
 	}
 
+	ret = sysfs_create_group(&q->kobj, &queue_attr_group);
+	if (ret) {
+		blk_trace_remove_sysfs(dev);
+		kobject_del(&q->kobj);
+		kobject_put(&dev->kobj);
+		goto unlock;
+	}
+
 	if (queue_is_mq(q)) {
 		__blk_mq_register_dev(dev, q);
 		blk_mq_debugfs_register(q);
-- 
2.14.1

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

[Bart Van Assche]

On Tue, 2019-04-02 at 21:14 +0800, Weiping Zhang wrote:
> [ ... ]
> -static struct attribute *default_attrs[] = {
> +static struct attribute *queue_attrs[] = {
>  	&queue_requests_entry.attr,
>  	&queue_ra_entry.attr,
>  	&queue_max_hw_sectors_entry.attr,
> @@ -770,6 +770,25 @@ static struct attribute *default_attrs[] = {
>  	NULL,
>  };
> 
> [ ... ]
>
>  static ssize_t
> @@ -890,7 +909,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,
>  };
>  
> @@ -939,6 +957,14 @@ int blk_register_queue(struct gendisk *disk)
>  		goto unlock;
>  	}
>  
> +	ret = sysfs_create_group(&q->kobj, &queue_attr_group);
> +	if (ret) {
> +		blk_trace_remove_sysfs(dev);
> +		kobject_del(&q->kobj);
> +		kobject_put(&dev->kobj);
> +		goto unlock;
> +	}
> +
>  	if (queue_is_mq(q)) {
>  		__blk_mq_register_dev(dev, q);
>  		blk_mq_debugfs_register(q);

This kind of change introduces an unacceptable race against udev. I think we
should wait with making this change until the patch series "kobject: Add
default group support to kobj_type and replace subsystem uses" is upstream.

Bart.

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

[Weiping Zhang]

Bart Van Assche <bvanassche@acm.org> 于2019年4月3日周三 上午1:10写道：
>
> On Tue, 2019-04-02 at 21:14 +0800, Weiping Zhang wrote:
> > [ ... ]
> > -static struct attribute *default_attrs[] = {
> > +static struct attribute *queue_attrs[] = {
> >       &queue_requests_entry.attr,
> >       &queue_ra_entry.attr,
> >       &queue_max_hw_sectors_entry.attr,
> > @@ -770,6 +770,25 @@ static struct attribute *default_attrs[] = {
> >       NULL,
> >  };
> >
> > [ ... ]
> >
> >  static ssize_t
> > @@ -890,7 +909,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,
> >  };
> >
> > @@ -939,6 +957,14 @@ int blk_register_queue(struct gendisk *disk)
> >               goto unlock;
> >       }
> >
> > +     ret = sysfs_create_group(&q->kobj, &queue_attr_group);
> > +     if (ret) {
> > +             blk_trace_remove_sysfs(dev);
> > +             kobject_del(&q->kobj);
> > +             kobject_put(&dev->kobj);
> > +             goto unlock;
> > +     }
> > +
> >       if (queue_is_mq(q)) {
> >               __blk_mq_register_dev(dev, q);
> >               blk_mq_debugfs_register(q);
>
> This kind of change introduces an unacceptable race against udev. I think we
> should wait with making this change until the patch series "kobject: Add
> default group support to kobj_type and replace subsystem uses" is upstream.
>
It's good patch, I'll send new patch after it was merged.
Could you point out the race case for udev?
> Bart.

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

[Bart Van Assche]

On Wed, 2019-04-03 at 18:48 +0800, Weiping Zhang wrote:
> Could you point out the race case for udev?

Hi Weiping,

A quote from Documentation/kobject.txt:

"Use the KOBJ_ADD action for when the kobject is first added to the kernel.
This should be done only after any attributes or children of the kobject
have been initialized properly, as userspace will instantly start to look
for them when this call happens."

You may want to have a look at commit 33b14f67a4e1 ("nvme: register ns_id
attributes as default sysfs groups") as an example of a patch that fixes a
race condition between sysfs attribute creation and udev.

Regarding the block layer timeout attribute: I just noticed that for the
request queue kobj attribute KOBJ_ADD is triggered explicitly from
blk_register_queue():

	kobject_uevent(&q->kobj, KOBJ_ADD);

Your patch adds sysfs attributes before that code is encountered so it should
be fine.

Bart.

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

[Bart Van Assche]

On Tue, 2019-04-02 at 21:14 +0800, Weiping Zhang wrote:
> If the low level driver has no timeout handler, the
> /sys/block/<disk>/queue/io_timeout will not be displayed.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

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

[Weiping Zhang]

Hi Jens,

Ping

Bart Van Assche <bvanassche@acm.org> 于2019年4月4日周四 上午12:54写道：
>
> On Tue, 2019-04-02 at 21:14 +0800, Weiping Zhang wrote:
> > If the low level driver has no timeout handler, the
> > /sys/block/<disk>/queue/io_timeout will not be displayed.
>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
>
>

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

[Jens Axboe]

On 4/2/19 7:14 AM, Weiping Zhang wrote:
> If the low level driver has no timeout handler, the
> /sys/block/<disk>/queue/io_timeout will not be displayed.

Applied, thanks.

-- 
Jens Axboe