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

[Bart Van Assche]

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.

@@ -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?

Thanks,

Bart.