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

[weiping zhang <zhangweiping@didichuxing.com>]

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