[RFC] scsi_debug: add hosts initialization --> worker

* [RFC] scsi_debug: add hosts initialization --> worker
@ 2021-03-19 15:05 Douglas Gilbert
  2021-03-19 19:48 ` Luis Chamberlain
  2021-03-23 13:26   ` kernel test robot
  0 siblings, 2 replies; 6+ messages in thread
From: Douglas Gilbert @ 2021-03-19 15:05 UTC (permalink / raw)
  To: linux-scsi; +Cc: martin.petersen, mcgrof, hare

Adding (pseudo) SCSI hosts has been done in the scsi_debug_init()
function. Each added SCSI host triggers an (async) scan and for
every LUN found, the host environment can trigger a lot of work.
On a recent Ubuntu/Debian distribution a lot of this "work" seems
to involve udev and its scripts. The result of this work can be
seconds elapsed before scsi_debug_init() returns.

This experimental code places the function to add those SCSI hosts
in its own thread. This allows scsi_debug_init() to complete a lot
faster. To impede malevolent user space code that might try to send
a 'rmmod scsi_debug', an extra module_get() reference is taken
before the worker thread starts and that worker gives back that
reference when it completes.

This patch is against MKP's repository and its 5.13/scsi-queue
branch which is sitting at lk 5.12.0-rc1. It should apply to later
release candidates in that series.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---

This RFC is in response to
   https://bugzilla.kernel.org/show_bug.cgi?id=212337
titled: "[Bug 212337] scsi_debug: race at module load and module
unload" raised by Luis Chamberlain. It has been lightly tested and
seems to work and make the 'modprobe scsi_debug ...' command 
complete almost immediately. And this pair of commands causes no
crash: 'modprobe scsi_debug ... ; rmmod scsi_debug' just a warning:
   rmmod: ERROR: Module scsi_debug is in use

 drivers/scsi/scsi_debug.c | 81 ++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 35 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 70165be10f00..aa74bf59e827 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -361,6 +361,8 @@ static atomic_t sdebug_a_tsf;	     /* 'almost task set full' counter */
 static atomic_t sdeb_inject_pending;
 static atomic_t sdeb_mq_poll_count;  /* bumped when mq_poll returns > 0 */
 
+static struct execute_work ew_init;
+
 struct opcode_info_t {
 	u8 num_attached;	/* 0 if this is it (i.e. a leaf); use 0xff */
 				/* for terminating element */
@@ -6651,14 +6653,51 @@ static struct attribute *sdebug_drv_attrs[] = {
 };
 ATTRIBUTE_GROUPS(sdebug_drv);
 
+static void sdeb_add_hosts_thr(struct work_struct *work)
+{
+	bool want_store = (sdebug_fake_rw == 0);
+	int k, ret, hosts_to_add;
+	int idx = -1;
+
+	if (want_store) {
+		idx = sdebug_add_store();
+		if (idx < 0) {
+			pr_warn("%s: unable to allocate any store\n", __func__);
+			goto fini;
+		}
+	}
+	hosts_to_add = READ_ONCE(sdebug_add_host);
+	WRITE_ONCE(sdebug_add_host, 0);
+
+	for (k = 0; k < hosts_to_add; k++) {
+		if (want_store && k == 0) {
+			ret = sdebug_add_host_helper(idx);
+			if (ret < 0) {
+				pr_err("add_host_helper k=%d, error=%d\n",
+				       k, -ret);
+				break;
+			}
+		} else {
+			ret = sdebug_do_add_host(want_store &&
+						 sdebug_per_host_store);
+			if (ret < 0) {
+				pr_err("add_host k=%d error=%d\n", k, -ret);
+				break;
+			}
+		}
+	}
+	if (sdebug_verbose)
+		pr_info("built %d host(s)\n", sdebug_num_hosts);
+fini:
+	module_put(THIS_MODULE);
+}
+
 static struct device *pseudo_primary;
 
 static int __init scsi_debug_init(void)
 {
-	bool want_store = (sdebug_fake_rw == 0);
 	unsigned long sz;
-	int k, ret, hosts_to_add;
-	int idx = -1;
+	int k, ret;
 
 	ramdisk_lck_a[0] = &atomic_rw;
 	ramdisk_lck_a[1] = &atomic_rw2;
@@ -6841,19 +6880,12 @@ static int __init scsi_debug_init(void)
 		}
 	}
 	xa_init_flags(per_store_ap, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
-	if (want_store) {
-		idx = sdebug_add_store();
-		if (idx < 0) {
-			ret = idx;
-			goto free_q_arr;
-		}
-	}
 
 	pseudo_primary = root_device_register("pseudo_0");
 	if (IS_ERR(pseudo_primary)) {
 		pr_warn("root_device_register() error\n");
 		ret = PTR_ERR(pseudo_primary);
-		goto free_vm;
+		goto free_q_arr;
 	}
 	ret = bus_register(&pseudo_lld_bus);
 	if (ret < 0) {
@@ -6866,28 +6898,9 @@ static int __init scsi_debug_init(void)
 		goto bus_unreg;
 	}
 
-	hosts_to_add = sdebug_add_host;
-	sdebug_add_host = 0;
-
-	for (k = 0; k < hosts_to_add; k++) {
-		if (want_store && k == 0) {
-			ret = sdebug_add_host_helper(idx);
-			if (ret < 0) {
-				pr_err("add_host_helper k=%d, error=%d\n",
-				       k, -ret);
-				break;
-			}
-		} else {
-			ret = sdebug_do_add_host(want_store &&
-						 sdebug_per_host_store);
-			if (ret < 0) {
-				pr_err("add_host k=%d error=%d\n", k, -ret);
-				break;
-			}
-		}
-	}
-	if (sdebug_verbose)
-		pr_info("built %d host(s)\n", sdebug_num_hosts);
+	__module_get(THIS_MODULE);	/* _put() at end of sdeb_add_hosts_thr() */
+	INIT_WORK(&ew_init.work, sdeb_add_hosts_thr);
+	schedule_work(&ew_init.work);
 
 	return 0;
 
@@ -6895,8 +6908,6 @@ static int __init scsi_debug_init(void)
 	bus_unregister(&pseudo_lld_bus);
 dev_unreg:
 	root_device_unregister(pseudo_primary);
-free_vm:
-	sdebug_erase_store(idx, NULL);
 free_q_arr:
 	kfree(sdebug_q_arr);
 	return ret;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread