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
