On 5/30/20 12:14 AM, Douglas Gilbert wrote:
This patch leaves me a bit uneasy but it does cure the crash
that occurs in this function. xarray iterators are pretty safe
_unless_ something deletes the parent node holding the
collection. The problem seems to be these nested loops do not
_explicitly_ remove the starget object. That is done magically
at the sdev level on the removal of the last sdev in a starget.
And that is half an iteration too soon! Hence the shuffle which
isn't a great solution. The magical starget removal is wrong IMO
and this will burn us elsewhere, I suspect.
Thes patch is on top of Hannes Reinecke's "[PATCHv4 0/5] scsi:
use xarray for devices and targets" patchset.
Signed-off-by: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
---
drivers/scsi/scsi_scan.c | 47 +++++++++++++++++++++++++++++++---------
1 file changed, 37 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 0a344653487d..e378f03d0297 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1858,25 +1858,52 @@ void scsi_scan_host(struct Scsi_Host *shost)
}
EXPORT_SYMBOL(scsi_scan_host);
+static void scsi_forget_host_inner(struct Scsi_Host *shost,
+ struct scsi_target *starg,
+ unsigned long *flagsp)
+{
+ struct scsi_device *sdev;
+ struct scsi_device *prev_sdev = NULL;
+ unsigned long lun_id;
+
+ xa_for_each(&starg->__devices, lun_id, sdev) {
+ if (sdev->sdev_state == SDEV_DEL)
+ continue;
+ if (!prev_sdev) {
+ prev_sdev = sdev;
+ continue;
+ }
+ spin_unlock_irqrestore(shost->host_lock, *flagsp);
+ __scsi_remove_device(prev_sdev);
+ spin_lock_irqsave(shost->host_lock, *flagsp);
+ prev_sdev = sdev;
+ }
+ if (prev_sdev) {
+ spin_unlock_irqrestore(shost->host_lock, *flagsp);
+ __scsi_remove_device(prev_sdev);
+ spin_lock_irqsave(shost->host_lock, *flagsp);
+ }
+}
+
+/* N.B. Keeping iteration one step ahead of destruction point */
void scsi_forget_host(struct Scsi_Host *shost)
{
struct scsi_target *starget;
- struct scsi_device *sdev;
+ struct scsi_target *prev_starget = NULL;
unsigned long flags;
- unsigned long tid = 0;
+ unsigned long tid;
spin_lock_irqsave(shost->host_lock, flags);
xa_for_each(&shost->__targets, tid, starget) {
- unsigned long lun_id = 0;
-
- xa_for_each(&starget->__devices, lun_id, sdev) {
- if (sdev->sdev_state == SDEV_DEL)
- continue;
- spin_unlock_irqrestore(shost->host_lock, flags);
- __scsi_remove_device(sdev);
- spin_lock_irqsave(shost->host_lock, flags);
+ if (!prev_starget) {
+ prev_starget = starget;
+ continue;
}
+ scsi_forget_host_inner(shost, prev_starget, &flags);
+ prev_starget = starget;
}
+ if (prev_starget)
+ scsi_forget_host_inner(shost, prev_starget, &flags);
spin_unlock_irqrestore(shost->host_lock, flags);
}
This is probably easier:
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 0a344653487d..fb0b6710b138 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1868,7 +1868,7 @@ void scsi_forget_host(struct Scsi_Host *shost)
spin_lock_irqsave(shost->host_lock, flags);
xa_for_each(&shost->__targets, tid, starget) {
unsigned long lun_id = 0;
-
+ kref_get(&starget->reap_ref);
xa_for_each(&starget->__devices, lun_id, sdev) {
if (sdev->sdev_state == SDEV_DEL)
continue;
@@ -1876,6 +1876,7 @@ void scsi_forget_host(struct Scsi_Host *shost)
__scsi_remove_device(sdev);
spin_lock_irqsave(shost->host_lock, flags);
}
+ scsi_target_reap(starget);
}
spin_unlock_irqrestore(shost->host_lock, flags);
}
But yeah, this 'implicit target destroy' is an abomination.
Actually, I'm planning on using xa_cmpxchg when deleting elements to
eliminate any confusion on who's responsible for removing it from the
xarray; the current code has a design flaw in that it does the iteration
first, and then after various layers and indirections it does the
removal from the iteration.
What _should_ have happened is that you do an iteration which does
implicitly removes it from the list, and _then_ go through the various
hoops to actually remove the device.
But after the xarray series has stabilized :-)
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
