Re: [PATCH] block: don't acquire .sysfs_lock before removing mq & iosched kobjects

[Ming Lei <ming.lei@redhat.com>]

On Fri, Aug 16, 2019 at 08:31:06AM -0700, Bart Van Assche wrote:
> On 8/16/19 6:55 AM, Ming Lei wrote:
> > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > index 977c659dcd18..46f033b48917 100644
> > --- a/block/blk-sysfs.c
> > +++ b/block/blk-sysfs.c
> > @@ -1021,6 +1021,7 @@ EXPORT_SYMBOL_GPL(blk_register_queue);
> >   void blk_unregister_queue(struct gendisk *disk)
> >   {
> >   	struct request_queue *q = disk->queue;
> > +	bool has_elevator;
> >   	if (WARN_ON(!q))
> >   		return;
> > @@ -1035,8 +1036,9 @@ void blk_unregister_queue(struct gendisk *disk)
> >   	 * concurrent elv_iosched_store() calls.
> >   	 */
> >   	mutex_lock(&q->sysfs_lock);
> > -
> >   	blk_queue_flag_clear(QUEUE_FLAG_REGISTERED, q);
> > +	has_elevator = q->elevator;
> > +	mutex_unlock(&q->sysfs_lock);
> 
> blk_queue_flag_clear() modifies queue flags atomically so no need to hold
> sysfs_lock around calls of that function.

If you take a look at the above comment, you will see why the sysfs lock
is needed.

> 
> > @@ -1044,16 +1046,13 @@ void blk_unregister_queue(struct gendisk *disk)
> >   	 */
> >   	if (queue_is_mq(q))
> >   		blk_mq_unregister_dev(disk_to_dev(disk), q);
> > -	mutex_unlock(&q->sysfs_lock);
> >   	kobject_uevent(&q->kobj, KOBJ_REMOVE);
> >   	kobject_del(&q->kobj);
> >   	blk_trace_remove_sysfs(disk_to_dev(disk));
> > -	mutex_lock(&q->sysfs_lock);
> > -	if (q->elevator)
> > +	if (has_elevator)
> >   		elv_unregister_queue(q);
> > -	mutex_unlock(&q->sysfs_lock);
> 
> Have you considered to move the q->elevator check into
> elv_unregister_queue() such that no new 'has_elevator' variable has to be
> introduced in this function?

No, I'd keep to read 'q->elevator' with .sysfs_lock.

Thanks,
Ming