Re: [PATCH V2 6/6] block: split .sysfs_lock into two locks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/21/19 2:15 AM, Ming Lei wrote:
diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index 31bbf10d8149..a4cc40ddda86 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -247,7 +247,7 @@ void blk_mq_unregister_dev(struct device *dev, struct request_queue *q)
  	struct blk_mq_hw_ctx *hctx;
  	int i;
- lockdep_assert_held(&q->sysfs_lock);
+	lockdep_assert_held(&q->sysfs_dir_lock);
queue_for_each_hw_ctx(q, hctx, i)
  		blk_mq_unregister_hctx(hctx);
@@ -297,7 +297,7 @@ int __blk_mq_register_dev(struct device *dev, struct request_queue *q)
  	int ret, i;
WARN_ON_ONCE(!q->kobj.parent);
-	lockdep_assert_held(&q->sysfs_lock);
+	lockdep_assert_held(&q->sysfs_dir_lock);
ret = kobject_add(q->mq_kobj, kobject_get(&dev->kobj), "%s", "mq");
  	if (ret < 0)

blk_mq_unregister_dev and __blk_mq_register_dev() are only used by blk_register_queue() and blk_unregister_queue(). It is the responsibility of the callers of these function to serialize request queue registration and unregistration. Is it really necessary to hold a mutex around the blk_mq_unregister_dev and __blk_mq_register_dev() calls? Or in other words, can it ever happen that multiple threads invoke one or both functions concurrently?

@@ -331,7 +331,7 @@ void blk_mq_sysfs_unregister(struct request_queue *q)
  	struct blk_mq_hw_ctx *hctx;
  	int i;
- mutex_lock(&q->sysfs_lock);
+	mutex_lock(&q->sysfs_dir_lock);
  	if (!q->mq_sysfs_init_done)
  		goto unlock;
@@ -339,7 +339,7 @@ void blk_mq_sysfs_unregister(struct request_queue *q)
  		blk_mq_unregister_hctx(hctx);
unlock:
-	mutex_unlock(&q->sysfs_lock);
+	mutex_unlock(&q->sysfs_dir_lock);
  }
int blk_mq_sysfs_register(struct request_queue *q)
@@ -347,7 +347,7 @@ int blk_mq_sysfs_register(struct request_queue *q)
  	struct blk_mq_hw_ctx *hctx;
  	int i, ret = 0;
- mutex_lock(&q->sysfs_lock);
+	mutex_lock(&q->sysfs_dir_lock);
  	if (!q->mq_sysfs_init_done)
  		goto unlock;
@@ -358,7 +358,7 @@ int blk_mq_sysfs_register(struct request_queue *q)
  	}
unlock:
-	mutex_unlock(&q->sysfs_lock);
+	mutex_unlock(&q->sysfs_dir_lock);
return ret;
  }

blk_mq_sysfs_unregister() and blk_mq_sysfs_register() are only used by __blk_mq_update_nr_hw_queues(). Calls to that function are serialized by the tag_list_lock mutex. Is it really necessary to use any locking inside these functions?

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 5b0b5224cfd4..5941a0176f87 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -938,6 +938,7 @@ int blk_register_queue(struct gendisk *disk)
  	int ret;
  	struct device *dev = disk_to_dev(disk);
  	struct request_queue *q = disk->queue;
+	bool has_elevator = false;
if (WARN_ON(!q))
  		return -ENXIO;
@@ -945,7 +946,6 @@ int blk_register_queue(struct gendisk *disk)
  	WARN_ONCE(blk_queue_registered(q),
  		  "%s is registering an already registered queue\n",
  		  kobject_name(&dev->kobj));
-	blk_queue_flag_set(QUEUE_FLAG_REGISTERED, q);
/*
  	 * SCSI probing may synchronously create and destroy a lot of
@@ -966,7 +966,7 @@ int blk_register_queue(struct gendisk *disk)
  		return ret;
/* Prevent changes through sysfs until registration is completed. */
-	mutex_lock(&q->sysfs_lock);
+	mutex_lock(&q->sysfs_dir_lock);
ret = kobject_add(&q->kobj, kobject_get(&dev->kobj), "%s", "queue");
  	if (ret < 0) {
@@ -987,26 +987,37 @@ int blk_register_queue(struct gendisk *disk)
  		blk_mq_debugfs_register(q);
  	}
- kobject_uevent(&q->kobj, KOBJ_ADD);
-
-	wbt_enable_default(q);
-
-	blk_throtl_register_queue(q);
-
+	/*
+	 * The queue's kobject ADD uevent isn't sent out, also the
+	 * flag of QUEUE_FLAG_REGISTERED isn't set yet, so elevator
+	 * switch won't happen at all.
+	 */
  	if (q->elevator) {
-		ret = elv_register_queue(q);
+		ret = elv_register_queue(q, false);
  		if (ret) {
-			mutex_unlock(&q->sysfs_lock);
-			kobject_uevent(&q->kobj, KOBJ_REMOVE);
+			mutex_unlock(&q->sysfs_dir_lock);
  			kobject_del(&q->kobj);
  			blk_trace_remove_sysfs(dev);
  			kobject_put(&dev->kobj);
  			return ret;
  		}
+		has_elevator = true;
  	}
+
+	mutex_lock(&q->sysfs_lock);
+	blk_queue_flag_set(QUEUE_FLAG_REGISTERED, q);
+	wbt_enable_default(q);
+	blk_throtl_register_queue(q);
+	mutex_unlock(&q->sysfs_lock);
+
+	/* Now everything is ready and send out KOBJ_ADD uevent */
+	kobject_uevent(&q->kobj, KOBJ_ADD);
+	if (has_elevator)
+		kobject_uevent(&q->elevator->kobj, KOBJ_ADD);
+
  	ret = 0;
  unlock:
-	mutex_unlock(&q->sysfs_lock);
+	mutex_unlock(&q->sysfs_dir_lock);
  	return ret;
  }

My understanding is that the mutex_lock() / mutex_unlock() calls in this function are necessary today to prevent concurrent changes of the scheduler from this function and from sysfs. If the kobject_uevent(KOBJ_ADD) call is moved, does that mean that all mutex_lock() / mutex_unlock() calls can be left out from this function?

  EXPORT_SYMBOL_GPL(blk_register_queue);
@@ -1021,6 +1032,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,25 +1047,25 @@ 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);
+ mutex_lock(&q->sysfs_dir_lock);
  	/*
  	 * Remove the sysfs attributes before unregistering the queue data
  	 * structures that can be modified through sysfs.
  	 */
  	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);
+	mutex_unlock(&q->sysfs_dir_lock);
kobject_put(&disk_to_dev(disk)->kobj);
  }

If this function would call kobject_del(&q->kobj) before doing anything else, does that mean that all mutex_lock() / mutex_unlock() calls can be left out from this function?

Thanks,

Bart.



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux