Re: [PATCHv3 0/4] scsi: use xarray ... scsi_alloc_target()

[Douglas Gilbert <dgilbert@interlog.com>]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

Hannes,
I believe scsi_alloc_target() is broken in the found_target case.
For starters starget is created, built and _not_ put in the xarray,
hence it is leaked?  Seems to me that the code shouldn't go
as far as it does before it detects the found_target case.

What I'm seeing in testing (script attached) after applying my
patches outlined in earlier posts (second attachment) is that
LUN 0 is missing on all targets within a host apart from
target_id==0 . For example:

# modprobe scsi_debug ptype=0x9 no_uld=1 max_luns=2 num_tgts=2
# lsscsi -gs
[0:0:0:0]    comms   Linux    scsi_debug       0189  -  /dev/sg0        -
[0:0:0:1]    comms   Linux    scsi_debug       0189  -  /dev/sg1        -
[0:0:1:1]    comms   Linux    scsi_debug       0189  -  /dev/sg2        -
[N:0:1:1]    disk    INTEL SSDPEKKF256G7L__1     /dev/nvme0n1  -      256GB

# sg_luns /dev/sg2
Lun list length = 16 which imples 2 lun entries
Report luns [select_report=0x0]:
     0000000000000000
     0001000000000000

So where did [0:0:1:0] go? The response to REPORT LUNS says it should
be there.

I thought about jumping into scsi_alloc_target() but it is horribly
complicated, so I'll let you deal with it :-)

Doug Gilbert

[-- Attachment #2: tst_sdebug_modpr_rmmod.sh --]
[-- Type: application/x-shellscript, Size: 808 bytes --]

[-- Attachment #3: hr2_fixes.patch --]
[-- Type: text/x-patch, Size: 4382 bytes --]

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 004a50a95560..7dff8ac850ab 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -345,6 +345,7 @@ static void scsi_host_dev_release(struct device *dev)
 
 	if (parent)
 		put_device(parent);
+	xa_destroy(&shost->__targets);
 	kfree(shost);
 }
 
@@ -382,7 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->host_lock = &shost->default_lock;
 	spin_lock_init(shost->host_lock);
 	shost->shost_state = SHOST_CREATED;
-	xa_init(&shost->__targets);
+	xa_init_flags(&shost->__targets, XA_FLAGS_LOCK_IRQ);
 	INIT_LIST_HEAD(&shost->eh_cmd_q);
 	INIT_LIST_HEAD(&shost->starved_list);
 	init_waitqueue_head(&shost->host_wait);
@@ -501,6 +502,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
  fail_index_remove:
 	ida_simple_remove(&host_index_ida, shost->host_no);
  fail_kfree:
+	xa_destroy(&shost->__targets);
 	kfree(shost);
 	return NULL;
 }
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e75e7f93f8f6..e146c36adcf3 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -309,6 +309,7 @@ static void scsi_target_destroy(struct scsi_target *starget)
 {
 	struct device *dev = &starget->dev;
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
+	struct scsi_target *e_starg;
 	unsigned long flags;
 	unsigned long tid = scsi_target_index(starget);
 
@@ -318,8 +319,10 @@ static void scsi_target_destroy(struct scsi_target *starget)
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (shost->hostt->target_destroy)
 		shost->hostt->target_destroy(starget);
-	xa_erase(&shost->__targets, tid);
+	e_starg = xa_erase(&shost->__targets, tid);
 	spin_unlock_irqrestore(shost->host_lock, flags);
+	if (e_starg != starget)
+		pr_err("%s: bad xa_erase()\n", __func__);
 	put_device(dev);
 }
 
@@ -328,6 +331,7 @@ static void scsi_target_dev_release(struct device *dev)
 	struct device *parent = dev->parent;
 	struct scsi_target *starget = to_scsi_target(dev);
 
+	xa_destroy(&starget->devices);
 	kfree(starget);
 	put_device(parent);
 }
@@ -415,7 +419,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	starget->id = id;
 	starget->channel = channel;
 	starget->can_queue = 0;
-	xa_init(&starget->devices);
+	xa_init_flags(&starget->devices, XA_FLAGS_LOCK_IRQ);
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
 	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
@@ -428,7 +432,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 		get_device(&found_target->dev);
 		goto found;
 	}
-	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
+	if (xa_insert(&shost->__targets, tid, starget, GFP_ATOMIC)) {
 		dev_printk(KERN_ERR, dev, "target index busy\n");
 		kfree(starget);
 		return NULL;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 63fa57684782..8a5e6c0a4bed 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -434,6 +434,7 @@ static void scsi_device_cls_release(struct device *class_dev)
 static void scsi_device_dev_release_usercontext(struct work_struct *work)
 {
 	struct scsi_device *sdev;
+	struct scsi_device *e_sdev;
 	struct scsi_target *starget;
 	struct device *parent;
 	struct list_head *this, *tmp;
@@ -449,9 +450,11 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	parent = sdev->sdev_gendev.parent;
 
 	spin_lock_irqsave(sdev->host->host_lock, flags);
-	xa_erase(&starget->devices, sdev->lun_idx);
+	e_sdev = xa_erase(&starget->devices, sdev->lun_idx);
 	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
+	if (e_sdev != sdev)
+		pr_err("%s: bad sdev erase\n", __func__);
 
 	cancel_work_sync(&sdev->event_work);
 
@@ -1621,14 +1624,14 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 	if (sdev->lun < 256) {
 		sdev->lun_idx = sdev->lun;
 		WARN_ON(xa_insert(&starget->devices, sdev->lun_idx,
-				   sdev, GFP_KERNEL) < 0);
+				   sdev, GFP_ATOMIC) < 0);
 	} else {
 		struct xa_limit scsi_lun_limit = {
 			.min = 256,
 			.max = UINT_MAX,
 		};
 		WARN_ON(xa_alloc(&starget->devices, &sdev->lun_idx,
-				  sdev, scsi_lun_limit, GFP_KERNEL) < 0);
+				  sdev, scsi_lun_limit, GFP_ATOMIC) < 0);
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	/*