[PATCH v2, RESEND] Separate target visibility from reaped state information
Instead of representing the states "visible in sysfs" and
"has been removed from the target list" by a single state
variable, use two variables to represent this information.
This patch avoids that SCSI device removal can trigger a
soft lockup.
See also:
* "scsi: restart list search after unlock in scsi_remove_target"
(commit 40998193560d).
* "scsi_remove_target: fix softlockup regression on hot remove"
(commit bc3f02a795d3).
Reported-by: Sebastian Herbszt
Tested-by: Sebastian Herbszt
Signed-off-by: Bart Van Assche
Cc: Christoph Hellwig
Cc: Johannes Thumshirn
Cc: Dan Williams
Cc: stable
---
drivers/scsi/scsi_scan.c | 31 +++
drivers/scsi/scsi_sysfs.c | 7 ---
include/scsi/scsi_device.h | 9 ++---
3 files changed, 9 insertions(+), 38 deletions(-)
See also:
- http://thread.gmane.org/gmane.linux.scsi/107245.
- http://thread.gmane.org/gmane.linux.scsi/108614.
Changes compared to v1:
- Renamed "visible" into "is_visible" for consistency with the rest of the
SCSI initiator code.
- Removed the scsi_target_state since it is no longer used.
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 8324539..6accec3 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -314,7 +314,6 @@ static void scsi_target_destroy(struct scsi_target *starget)
struct Scsi_Host *shost = dev_to_shost(dev->parent);
unsigned long flags;
- starget->state = STARGET_DEL;
transport_destroy_device(dev);
spin_lock_irqsave(shost->host_lock, flags);
if (shost->hostt->target_destroy)
@@ -379,19 +378,15 @@ static void scsi_target_reap_ref_release(struct kref
*kref)
struct scsi_target *starget
= container_of(kref, struct scsi_target, reap_ref);
- /*
-* if we get here and the target is still in the CREATED state that
-* means it was allocated but never made visible (because a scan
-* turned up no LUNs), so don't call device_del() on it.
-*/
- if (starget->state != STARGET_CREATED) {
+ if (starget->is_visible) {
+ starget->is_visible = false;
transport_remove_device(&starget->dev);
device_del(&starget->dev);
}
scsi_target_destroy(starget);
}
-static void scsi_target_reap_ref_put(struct scsi_target *starget)
+void scsi_target_reap(struct scsi_target *starget)
{
kref_put(&starget->reap_ref, scsi_target_reap_ref_release);
}
@@ -437,7 +432,6 @@ static struct scsi_target *scsi_alloc_target(struct device
*parent,
starget->can_queue = 0;
INIT_LIST_HEAD(&starget->siblings);
INIT_LIST_HEAD(&starget->devices);
- starget->state = STARGET_CREATED;
starget->scsi_level = SCSI_2;
starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
retry:
@@ -498,25 +492,6 @@ static struct scsi_target *scsi_alloc_target(struct device
*parent,
}
/**
- * scsi_target_reap - check to see if target is in use and destroy if not
- * @starget: target to be checked
- *
- * This is used after removing a LUN or doing a last put of the target
- * it checks atomically that nothing is using the target and removes
- * it if so.
- */
-void scsi_target_reap(struct scsi_target *starget)
-{
- /*
-* serious problem if this triggers: STARGET_DEL is only set in the if
-* the reap_ref drops to zero, so we're trying to do another final put
-* on an already released kref
-*/
- BUG_ON(starget->state == STARGET_DEL);
- scsi_target_reap_ref_put(starget);
-}
-
-/**
* sanitize_inquiry_string - remove non-graphical chars from an INQUIRY result
string
* @s: INQUIRY result string to sanitize
* @len: length of the string
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 8d23122..c5ea634 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1000,7 +1000,7 @@ static int scsi_target_add(struct scsi_target *starget)
{
int error;
- if (starget->state != STARGET_CREATED)
+ if (starget->is_visible)
return 0;
error = device_add(&starget->dev);
@@ -1009,7 +1009,7 @@ static int scsi_target_add(struct scsi_target *starget)
return error;
}
transport_add_device(&starget->dev);
- starget->state = STARGET_RUNNING;
+ starget->is_visible = true;
pm_runtime_set_active(&starget->dev);
pm_runtime_enable(&starget->dev);
@@ -1198,10 +1198,11 @@ void scsi_remove_target(struct device *dev)
restart:
spin_lock_irqsave(shost->host_lock, flags);
list_for_each_entry(starget, &shost->__targets, siblings) {
- if (starget->state == STARGET_DEL)
+ if (starget->reaped)
continue;
if (starget->dev.parent == dev || &starget->dev == dev) {
kref_get(&starget->reap_ref);
+
Re: [PATCH v2, RESEND] Separate target visibility from reaped state information
Looks good, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2, RESEND] Separate target visibility from reaped state information
On Wed, 2016-01-06 at 09:24 +0100, Bart Van Assche wrote:
> Instead of representing the states "visible in sysfs" and
> "has been removed from the target list" by a single state
> variable, use two variables to represent this information.
>
> This patch avoids that SCSI device removal can trigger a
> soft lockup.
It does? When I asked you this the last time, you said the soft lockup
was fixed by a prior patch:
http://thread.gmane.org/gmane.linux.scsi/107248
If you've actually caught a problem, can we have details because the
distro people will want to know what gets fixed by this.
Thanks,
James
> See also:
> * "scsi: restart list search after unlock in scsi_remove_target"
> (commit 40998193560d).
> * "scsi_remove_target: fix softlockup regression on hot remove"
> (commit bc3f02a795d3).
>
> Reported-by: Sebastian Herbszt
> Tested-by: Sebastian Herbszt
> Signed-off-by: Bart Van Assche
> Cc: Christoph Hellwig
> Cc: Johannes Thumshirn
> Cc: Dan Williams
> Cc: stable
> ---
> drivers/scsi/scsi_scan.c | 31 +++
> drivers/scsi/scsi_sysfs.c | 7 ---
> include/scsi/scsi_device.h | 9 ++---
> 3 files changed, 9 insertions(+), 38 deletions(-)
>
> See also:
> - http://thread.gmane.org/gmane.linux.scsi/107245.
> - http://thread.gmane.org/gmane.linux.scsi/108614.
>
> Changes compared to v1:
> - Renamed "visible" into "is_visible" for consistency with the rest
> of the
> SCSI initiator code.
> - Removed the scsi_target_state since it is no longer used.
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 8324539..6accec3 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -314,7 +314,6 @@ static void scsi_target_destroy(struct
> scsi_target *starget)
> struct Scsi_Host *shost = dev_to_shost(dev->parent);
> unsigned long flags;
>
> - starget->state = STARGET_DEL;
> transport_destroy_device(dev);
> spin_lock_irqsave(shost->host_lock, flags);
> if (shost->hostt->target_destroy)
> @@ -379,19 +378,15 @@ static void scsi_target_reap_ref_release(struct
> kref *kref)
> struct scsi_target *starget
> = container_of(kref, struct scsi_target, reap_ref);
>
> - /*
> - * if we get here and the target is still in the CREATED
> state that
> - * means it was allocated but never made visible (because a
> scan
> - * turned up no LUNs), so don't call device_del() on it.
> - */
> - if (starget->state != STARGET_CREATED) {
> + if (starget->is_visible) {
> + starget->is_visible = false;
> transport_remove_device(&starget->dev);
> device_del(&starget->dev);
> }
> scsi_target_destroy(starget);
> }
>
> -static void scsi_target_reap_ref_put(struct scsi_target *starget)
> +void scsi_target_reap(struct scsi_target *starget)
> {
> kref_put(&starget->reap_ref, scsi_target_reap_ref_release);
> }
> @@ -437,7 +432,6 @@ static struct scsi_target
> *scsi_alloc_target(struct device *parent,
> starget->can_queue = 0;
> INIT_LIST_HEAD(&starget->siblings);
> INIT_LIST_HEAD(&starget->devices);
> - starget->state = STARGET_CREATED;
> starget->scsi_level = SCSI_2;
> starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
> retry:
> @@ -498,25 +492,6 @@ static struct scsi_target
> *scsi_alloc_target(struct device *parent,
> }
>
> /**
> - * scsi_target_reap - check to see if target is in use and destroy
> if not
> - * @starget: target to be checked
> - *
> - * This is used after removing a LUN or doing a last put of the
> target
> - * it checks atomically that nothing is using the target and removes
> - * it if so.
> - */
> -void scsi_target_reap(struct scsi_target *starget)
> -{
> - /*
> - * serious problem if this triggers: STARGET_DEL is only set
> in the if
> - * the reap_ref drops to zero, so we're trying to do another
> final put
> - * on an already released kref
> - */
> - BUG_ON(starget->state == STARGET_DEL);
> - scsi_target_reap_ref_put(starget);
> -}
> -
> -/**
> * sanitize_inquiry_string - remove non-graphical chars from an
> INQUIRY result string
> * @s: INQUIRY result string to sanitize
> * @len: length of the string
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 8d23122..c5ea634 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1000,7 +1000,7 @@ static int scsi_target_add(struct scsi_target
> *starget)
> {
> int error;
>
> - if (starget->state != STARGET_CREATED)
> + if (starget->is_visible)
> return 0;
>
> error = device_add(&starget->dev);
> @@ -1009,7 +1009,7 @@ static int scsi_target_add(struct scsi_target
> *starget)
> return error;
> }
> transport_add_device(&starget->dev);
> - starget->state = STARGET_RUNNING;
> + starget->is_visible = true;
>
> pm_runtime_set_
Re: [PATCH v2, RESEND] Separate target visibility from reaped state information
James Bottomley wrote: > On Wed, 2016-01-06 at 09:24 +0100, Bart Van Assche wrote: > > Instead of representing the states "visible in sysfs" and > > "has been removed from the target list" by a single state > > variable, use two variables to represent this information. > > > > This patch avoids that SCSI device removal can trigger a > > soft lockup. > > It does? When I asked you this the last time, you said the soft lockup > was fixed by a prior patch: > > http://thread.gmane.org/gmane.linux.scsi/107248 > > If you've actually caught a problem, can we have details because the > distro people will want to know what gets fixed by this. > > Thanks, > > James Which details do you need? > > See also: > > * "scsi: restart list search after unlock in scsi_remove_target" > > (commit 40998193560d). > > * "scsi_remove_target: fix softlockup regression on hot remove" > > (commit bc3f02a795d3). > > > > Reported-by: Sebastian Herbszt > > Tested-by: Sebastian Herbszt > > Signed-off-by: Bart Van Assche > > Cc: Christoph Hellwig > > Cc: Johannes Thumshirn > > Cc: Dan Williams > > Cc: stable > > --- > > drivers/scsi/scsi_scan.c | 31 +++ > > drivers/scsi/scsi_sysfs.c | 7 --- > > include/scsi/scsi_device.h | 9 ++--- > > 3 files changed, 9 insertions(+), 38 deletions(-) > > > > See also: > > - http://thread.gmane.org/gmane.linux.scsi/107245. I was able to hit a soft lockup and reported it here: > > - http://thread.gmane.org/gmane.linux.scsi/108614. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
