* [PATCH 1/1] scsi: scsi_forget_host() shuffle
@ 2020-05-29 22:14 Douglas Gilbert
2020-05-30 9:30 ` Hannes Reinecke
0 siblings, 1 reply; 2+ messages in thread
From: Douglas Gilbert @ 2020-05-29 22:14 UTC (permalink / raw
To: linux-scsi; +Cc: martin.petersen, jejb, hare, bvanassche
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@interlog.com>
---
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);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH 1/1] scsi: scsi_forget_host() shuffle
2020-05-29 22:14 [PATCH 1/1] scsi: scsi_forget_host() shuffle Douglas Gilbert
@ 2020-05-30 9:30 ` Hannes Reinecke
0 siblings, 0 replies; 2+ messages in thread
From: Hannes Reinecke @ 2020-05-30 9:30 UTC (permalink / raw
To: Douglas Gilbert, linux-scsi; +Cc: martin.petersen, jejb, bvanassche
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@interlog.com>
> ---
> 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@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-05-30 9:30 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-29 22:14 [PATCH 1/1] scsi: scsi_forget_host() shuffle Douglas Gilbert
2020-05-30 9:30 ` Hannes Reinecke
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.