[PATCH 0/2] Fix a hard lockup in scsi_remove_target()

[PATCH 0/2] Fix a hard lockup in scsi_remove_target()

[Bart Van Assche]

On October 2, 2015 Johannes Thumshirn reported the following hard lockup 
(see also http://thread.gmane.org/gmane.linux.kernel/2052359):

Kernel panic - not syncing: Watchdog detected hard LOCKUP on cpu 0
[...]
Call Trace:
  [<ffffffff8100471d>] dump_trace+0x7d/0x2d0
  [<ffffffff81004a04>] show_stack_log_lvl+0x94/0x170
  [<ffffffff81005cc1>] show_stack+0x21/0x50
  [<ffffffff8151aa75>] dump_stack+0x41/0x51
  [<ffffffff8151545a>] panic+0xc8/0x1d7
  [<ffffffff810fbdda>] watchdog_overflow_callback+0xba/0xc0
  [<ffffffff811336c8>] __perf_event_overflow+0x88/0x240
  [<ffffffff8101e3aa>] intel_pmu_handle_irq+0x1fa/0x3e0
  [<ffffffff81522836>] perf_event_nmi_handler+0x26/0x40
  [<ffffffff81521fcd>] nmi_handle.isra.2+0x8d/0x180
  [<ffffffff815221e6>] do_nmi+0x126/0x3c0
  [<ffffffff8152159b>] end_repeat_nmi+0x1a/0x1e
  [<ffffffffa00212e8>] scsi_remove_target+0x68/0x240 [scsi_mod]
  [<ffffffff81072742>] process_one_work+0x172/0x420
  [<ffffffff810733ba>] worker_thread+0x11a/0x3c0
  [<ffffffff81079d34>] kthread+0xb4/0xc0
  [<ffffffff81528cd8>] ret_from_fork+0x58/0x90

The following two patches fix this lockup:
0001-Separate-target-visibility-from-reaped-state-informa.patch
0002-Restart-list-search-after-unlock-in-scsi_remove_targ.patch

These patches have been tested with the SRP initiator (ib_srp).

[PATCH 1/2] Separate target visibility from reaped state information

[Bart Van Assche]

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.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: stable <stable@vger.kernel.org>
---
 drivers/scsi/scsi_scan.c   | 31 +++----------------------------
 drivers/scsi/scsi_sysfs.c  |  7 ++++---
 include/scsi/scsi_device.h |  3 ++-
 3 files changed, 9 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f9f3f82..80b8c3f 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->visible) {
+		starget->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 b333389..b9fb61a 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -974,7 +974,7 @@ static int scsi_target_add(struct scsi_target *starget)
 {
 	int error;
 
-	if (starget->state != STARGET_CREATED)
+	if (starget->visible)
 		return 0;
 
 	error = device_add(&starget->dev);
@@ -983,7 +983,7 @@ static int scsi_target_add(struct scsi_target *starget)
 		return error;
 	}
 	transport_add_device(&starget->dev);
-	starget->state = STARGET_RUNNING;
+	starget->visible = true;
 
 	pm_runtime_set_active(&starget->dev);
 	pm_runtime_enable(&starget->dev);
@@ -1166,11 +1166,12 @@ void scsi_remove_target(struct device *dev)
 	 */
 	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) {
 			/* assuming new targets arrive at the end */
 			kref_get(&starget->reap_ref);
+			starget->reaped = true;
 			spin_unlock_irqrestore(shost->host_lock, flags);
 			if (last)
 				scsi_target_reap(last);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index fe89d7c..a734656 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -267,6 +267,8 @@ struct scsi_target {
 	unsigned int		expecting_lun_change:1;	/* A device has reported
 						 * a 3F/0E UA, other devices on
 						 * the same target will also. */
+	unsigned int		visible:1; /* visible in sysfs */
+	unsigned int		reaped:1; /* removed from target list */
 	/* commands actually active on LLD. */
 	atomic_t		target_busy;
 	atomic_t		target_blocked;
@@ -280,7 +282,6 @@ struct scsi_target {
 #define SCSI_DEFAULT_TARGET_BLOCKED	3
 
 	char			scsi_level;
-	enum scsi_target_state	state;
 	void 			*hostdata; /* available to low-level driver */
 	unsigned long		starget_data[0]; /* for the transport */
 	/* starget_data must be the last element!!!! */
-- 
2.6.2.307.g37023ba

Re: [PATCH 1/2] Separate target visibility from reaped state information

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

A few things I noticed when looking over this and the previous iterations,
which are more notes for future work than an actual comment on this patch:

 - what lock protects ->visible and ->reaped (or previously ->state)?
   It seems like the synchronization rules in this code are a little
   fragile.
 - why don't we make the target visible as soon as we allocated it?
   We can still tear it down without a whole more effort even if it is
   visible.

[PATCH 2/2] Restart list search after unlock in scsi_remove_target

[Bart Van Assche]

When dropping a lock while iterating a list we must restart the search
as other threads could have manipulated the list under us. Without this
we can get stuck in an endless loop.

This is a slightly modified version of a patch from Christoph Hellwig
(see also https://www.spinics.net/lists/linux-scsi/msg89416.html).

Reported-by: Johannes Thumshirn <jthumshirn@suse.de>
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: stable <stable@vger.kernel.org>
---
 drivers/scsi/scsi_sysfs.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b9fb61a..5a183d1 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1158,32 +1158,24 @@ static void __scsi_remove_target(struct scsi_target *starget)
 void scsi_remove_target(struct device *dev)
 {
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
-	struct scsi_target *starget, *last = NULL;
+	struct scsi_target *starget;
 	unsigned long flags;
 
-	/* remove targets being careful to lookup next entry before
-	 * deleting the last
-	 */
+restart:
 	spin_lock_irqsave(shost->host_lock, flags);
 	list_for_each_entry(starget, &shost->__targets, siblings) {
 		if (starget->reaped)
 			continue;
 		if (starget->dev.parent == dev || &starget->dev == dev) {
-			/* assuming new targets arrive at the end */
 			kref_get(&starget->reap_ref);
 			starget->reaped = true;
 			spin_unlock_irqrestore(shost->host_lock, flags);
-			if (last)
-				scsi_target_reap(last);
-			last = starget;
 			__scsi_remove_target(starget);
-			spin_lock_irqsave(shost->host_lock, flags);
+			scsi_target_reap(starget);
+			goto restart;
 		}
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
-
-	if (last)
-		scsi_target_reap(last);
 }
 EXPORT_SYMBOL(scsi_remove_target);
 
-- 
2.6.2.307.g37023ba

Re: [PATCH 2/2] Restart list search after unlock in scsi_remove_target

[Bart Van Assche]

On 10/30/2015 03:09 PM, Bart Van Assche wrote:
> When dropping a lock while iterating a list we must restart the search
> as other threads could have manipulated the list under us. Without this
> we can get stuck in an endless loop.
>
> This is a slightly modified version of a patch from Christoph Hellwig
> (see also https://www.spinics.net/lists/linux-scsi/msg89416.html).
>
> Reported-by: Johannes Thumshirn <jthumshirn@suse.de>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: stable <stable@vger.kernel.org>
> ---
>   drivers/scsi/scsi_sysfs.c | 16 ++++------------
>   1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index b9fb61a..5a183d1 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1158,32 +1158,24 @@ static void __scsi_remove_target(struct scsi_target *starget)
>   void scsi_remove_target(struct device *dev)
>   {
>   	struct Scsi_Host *shost = dev_to_shost(dev->parent);
> -	struct scsi_target *starget, *last = NULL;
> +	struct scsi_target *starget;
>   	unsigned long flags;
>
> -	/* remove targets being careful to lookup next entry before
> -	 * deleting the last
> -	 */
> +restart:
>   	spin_lock_irqsave(shost->host_lock, flags);
>   	list_for_each_entry(starget, &shost->__targets, siblings) {
>   		if (starget->reaped)
>   			continue;
>   		if (starget->dev.parent == dev || &starget->dev == dev) {
> -			/* assuming new targets arrive at the end */
>   			kref_get(&starget->reap_ref);
>   			starget->reaped = true;
>   			spin_unlock_irqrestore(shost->host_lock, flags);
> -			if (last)
> -				scsi_target_reap(last);
> -			last = starget;
>   			__scsi_remove_target(starget);
> -			spin_lock_irqsave(shost->host_lock, flags);
> +			scsi_target_reap(starget);
> +			goto restart;
>   		}
>   	}
>   	spin_unlock_irqrestore(shost->host_lock, flags);
> -
> -	if (last)
> -		scsi_target_reap(last);
>   }
>   EXPORT_SYMBOL(scsi_remove_target);

(replying to my own e-mail)

Hello Christoph,

Is it OK for you if I mention you as author of this e-mail ?

Thanks,

Bart.

Re: [PATCH 2/2] Restart list search after unlock in scsi_remove_target

[James Bottomley]

On Wed, 2015-11-04 at 14:35 -0800, Bart Van Assche wrote:
> On 10/30/2015 03:09 PM, Bart Van Assche wrote:
> > When dropping a lock while iterating a list we must restart the search
> > as other threads could have manipulated the list under us. Without this
> > we can get stuck in an endless loop.
> >
> > This is a slightly modified version of a patch from Christoph Hellwig
> > (see also https://www.spinics.net/lists/linux-scsi/msg89416.html).
> >
> > Reported-by: Johannes Thumshirn <jthumshirn@suse.de>
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Cc: Johannes Thumshirn <jthumshirn@suse.de>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: stable <stable@vger.kernel.org>
> > ---
> >   drivers/scsi/scsi_sysfs.c | 16 ++++------------
> >   1 file changed, 4 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > index b9fb61a..5a183d1 100644
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> > @@ -1158,32 +1158,24 @@ static void __scsi_remove_target(struct scsi_target *starget)
> >   void scsi_remove_target(struct device *dev)
> >   {
> >   	struct Scsi_Host *shost = dev_to_shost(dev->parent);
> > -	struct scsi_target *starget, *last = NULL;
> > +	struct scsi_target *starget;
> >   	unsigned long flags;
> >
> > -	/* remove targets being careful to lookup next entry before
> > -	 * deleting the last
> > -	 */
> > +restart:
> >   	spin_lock_irqsave(shost->host_lock, flags);
> >   	list_for_each_entry(starget, &shost->__targets, siblings) {
> >   		if (starget->reaped)
> >   			continue;
> >   		if (starget->dev.parent == dev || &starget->dev == dev) {
> > -			/* assuming new targets arrive at the end */
> >   			kref_get(&starget->reap_ref);
> >   			starget->reaped = true;
> >   			spin_unlock_irqrestore(shost->host_lock, flags);
> > -			if (last)
> > -				scsi_target_reap(last);
> > -			last = starget;
> >   			__scsi_remove_target(starget);
> > -			spin_lock_irqsave(shost->host_lock, flags);
> > +			scsi_target_reap(starget);
> > +			goto restart;
> >   		}
> >   	}
> >   	spin_unlock_irqrestore(shost->host_lock, flags);
> > -
> > -	if (last)
> > -		scsi_target_reap(last);
> >   }
> >   EXPORT_SYMBOL(scsi_remove_target);
> 
> (replying to my own e-mail)
> 
> Hello Christoph,
> 
> Is it OK for you if I mention you as author of this e-mail ?

Could you just both co-operate, especially since there's not much
difference between the patches.

The fundamental problem with this is how have the conditions that caused
us to move away from list restart:

commit bc3f02a795d3b4faa99d37390174be2a75d091bd
Author: Dan Williams <djbw@fb.com>
Date:   Tue Aug 28 22:12:10 2012 -0700

    [SCSI] scsi_remove_target: fix softlockup regression on hot remove

Which was triggered by this bug report

http://thread.gmane.org/gmane.linux.kernel/1348679

been mitigated?

James

Re: [PATCH 2/2] Restart list search after unlock in scsi_remove_target

[Bart Van Assche]

On 11/04/2015 02:44 PM, James Bottomley wrote:
> On Wed, 2015-11-04 at 14:35 -0800, Bart Van Assche wrote:
>> (replying to my own e-mail)
>>
>> Hello Christoph,
>>
>> Is it OK for you if I mention you as author of this e-mail ?
>
> Could you just both co-operate, especially since there's not much
> difference between the patches.

Hello James and Christoph,

Patch [2/2] is identical to the patch Christoph had posted except that 
it has been rebased on top of patch [1/2] of this series. I will be 
happy to step back if Christoph or someone else wants to take the lead 
here. The only reason I started looking into this is that there was no 
further progress on the original e-mail thread about this subject 
(http://thread.gmane.org/gmane.linux.kernel/2052359).

Bart.

Re: [PATCH 2/2] Restart list search after unlock in scsi_remove_target

[Christoph Hellwig]

Bart, can you resend your patch on top of 4.4-rc1?  I think we really
need it so we should get it into 4.4 and backport it to -stable.

Re: [PATCH 2/2] Restart list search after unlock in scsi_remove_target

[Dan Williams]

On Wed, Nov 4, 2015 at 2:44 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
[..]
> The fundamental problem with this is how have the conditions that caused
> us to move away from list restart:
>
> commit bc3f02a795d3b4faa99d37390174be2a75d091bd
> Author: Dan Williams <djbw@fb.com>
> Date:   Tue Aug 28 22:12:10 2012 -0700
>
>     [SCSI] scsi_remove_target: fix softlockup regression on hot remove
>
> Which was triggered by this bug report
>
> http://thread.gmane.org/gmane.linux.kernel/1348679
>
> been mitigated?
>

I think it has because the problem that led to that report was the
fact that scsi_target_destroy() did not advance the target state, but
we changed that in f2495e228fce.

http://marc.info/?l=linux-scsi&m=144527527308725&w=2

Re: [PATCH 2/2] Restart list search after unlock in scsi_remove_target

[James Bottomley]

On Thu, 2015-11-05 at 08:55 -0800, Dan Williams wrote:
> On Wed, Nov 4, 2015 at 2:44 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> [..]
> > The fundamental problem with this is how have the conditions that caused
> > us to move away from list restart:
> >
> > commit bc3f02a795d3b4faa99d37390174be2a75d091bd
> > Author: Dan Williams <djbw@fb.com>
> > Date:   Tue Aug 28 22:12:10 2012 -0700
> >
> >     [SCSI] scsi_remove_target: fix softlockup regression on hot remove
> >
> > Which was triggered by this bug report
> >
> > http://thread.gmane.org/gmane.linux.kernel/1348679
> >
> > been mitigated?
> >
> 
> I think it has because the problem that led to that report was the
> fact that scsi_target_destroy() did not advance the target state, but
> we changed that in f2495e228fce.
> 
> http://marc.info/?l=linux-scsi&m=144527527308725&w=2

Great, thanks, we'll put Christoph's version in then, because it can be
cc'd to stable without problems.  Bart, you can redo your state updates
on top of this because that's inessential to the current problem.

James

Re: [PATCH 2/2] Restart list search after unlock in scsi_remove_target

[Christoph Hellwig]

On Wed, Nov 04, 2015 at 02:35:40PM -0800, Bart Van Assche wrote:
> (replying to my own e-mail)
>
> Hello Christoph,
>
> Is it OK for you if I mention you as author of this e-mail ?

I don't care whom this fix is attributed to, but let's get it in:

Reviewed-by: Christoph Hellwig <hch@lst.de>

or

Signed-off-by: Christoph Hellwig <hch@lst.de>

at your preference :)