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:
tst_sdebug_modpr_rmmod.sh
Description: application/shellscript
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);
/*
