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 the following soft lockup:

NMI watchdog: BUG: soft lockup - CPU#1 stuck for 22s! [kworker/1:1:29]

CPU: 1 PID: 29 Comm: kworker/1:1 Tainted: G           O    4.4.0-rc5-2.g1e923a3-default #1
Workqueue: fc_wq_4 fc_rport_final_delete [scsi_transport_fc]
Call Trace:
 [<c066b0f7>] scsi_remove_target+0x167/0x1c0
 [<f8f0a4ed>] fc_rport_final_delete+0x9d/0x1e0 [scsi_transport_fc]
 [<c026cb25>] process_one_work+0x155/0x3e0
 [<c026cde7>] worker_thread+0x37/0x490
 [<c027214b>] kthread+0x9b/0xb0
 [<c07e72c1>] ret_from_kernel_thread+0x21/0x40

See also commit bc3f02a795d3 ("scsi_remove_target: fix softlockup regression on hot remove").

Reported-by Sebastian Herbszt <[email protected]>

Tested-by Sebastian Herbszt <[email protected]>

Fixes commit 40998193560d ("scsi: restart list search after unlock in scsi_remove_target")

Signed-off-by Bart Van Assche <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Johannes Thumshirn <[email protected]>

Cc Dan Williams <[email protected]>

Cc stable <[email protected]>

---
 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(-)

drivers/scsi/scsi_scan.c +3 -28

index 054923e..c455a88 100644
@@ -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

drivers/scsi/scsi_sysfs.c +4 -3

index 21930c9..532c062 100644
@@ -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);
+			starget->reaped = true;
 			spin_unlock_irqrestore(shost->host_lock, flags);
 			__scsi_remove_target(starget);
 			scsi_target_reap(starget);

include/scsi/scsi_device.h +2 -7

index fe89d7c..f11c794 100644
@@ -236,12 +236,6 @@ scmd_printk(const char *, const struct scsi_cmnd *, const char *, ...);
 			sdev_dbg((scmd)->device, fmt, ##a);		   \
 	} while (0)
 
-enum scsi_target_state {
-	STARGET_CREATED = 1,
-	STARGET_RUNNING,
-	STARGET_DEL,
-};
-
 /*
  * scsi_target: representation of a scsi target, for now, this is only
  * used for single_lun devices. If no one has active IO to the target,

@@ -267,6 +261,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		is_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 +276,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!!!! */

Signature · 6 lines

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Bart" == Bart Van Assche <[email protected]> writes:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

"Bart" == Bart Van Assche <[email protected]> writes:

Bart> Instead of representing the states "visible in sysfs" and "has Bart> been removed from the target list" by a single state variable, use
Bart> two variables to represent this information.

James: Are you happy with the latest iteration of this? Should I queue
it?

Quoted 11 lines · Martin K. Petersen

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

"Bart" == Bart Van Assche <[email protected]> writes:

Bart> Instead of representing the states "visible in sysfs" and "has Bart> been removed from the target list" by a single state variable, use
Bart> two variables to represent this information.

James: Are you happy with the latest iteration of this? Should I queue
it?

Well, I'm OK with the patch: it's a simple transformation of the enumerated state to a two bit state. What I can't see is how it fixes any soft lockup.

The only change from the current workflow is that the DEL transition (now the reaped flag) is done before the spin lock is dropped which would fix a tiny window for two threads both trying to remove the same target, but there's nothing that could possibly fix an iterative soft lockup caused by restarting the loop, which is what the changelog says.

James

Quoted 8 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Sebastian" == Sebastian Herbszt <[email protected]> writes:

The only change from the current workflow is that the DEL transition (now the reaped flag) is done before the spin lock is dropped which would fix a tiny window for two threads both trying to remove the same target, but there's nothing that could possibly fix an iterative soft lockup caused by restarting the loop, which is what the changelog says.

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Sebastian" == Sebastian Herbszt <[email protected]> writes:

The only change from the current workflow is that the DEL transition (now the reaped flag) is done before the spin lock is dropped which would fix a tiny window for two threads both trying to remove the same target, but there's nothing that could possibly fix an iterative soft lockup caused by restarting the loop, which is what the changelog says.

Sebastian> James, Martin, what's the status of this patch? I still hit Sebastian> the reported soft lockup on 4.5-rc1.

And you have verified that Bart's patch applied on top of 4.5-rc1 still fixes the lockup? (I know you tested a previous version)

I am concerned about queuing something as a stable fix if it is just masking a fundamental underlying problem. James' comment suggests that there is something else going on.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

I am concerned about queuing something as a stable fix if it is just masking a fundamental underlying problem.

Quoted 3 lines · Martin K. Petersen

I am concerned about queuing something as a stable fix if it is just
masking a fundamental underlying problem.

It's not masking a fundamental problem. It fixes the target state so that we can mark a starget as being under deletion before we have to drop the list protecting the target list iteration. Independ of any any other scanning changes it is the right thing to do.

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Sebastian" == Sebastian Herbszt <[email protected]> writes:

The only change from the current workflow is that the DEL transition (now the reaped flag) is done before the spin lock is dropped which would fix a tiny window for two threads both trying to remove the same target, but there's nothing that could possibly fix an iterative soft lockup caused by restarting the loop, which is what the changelog says.

Sebastian> James, Martin, what's the status of this patch? I still hit Sebastian> the reported soft lockup on 4.5-rc1.

And you have verified that Bart's patch applied on top of 4.5-rc1 still fixes the lockup? (I know you tested a previous version)

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Sebastian" == Sebastian Herbszt <[email protected]> writes:

The only change from the current workflow is that the DEL transition (now the reaped flag) is done before the spin lock is dropped which would fix a tiny window for two threads both trying to remove the same target, but there's nothing that could possibly fix an iterative soft lockup caused by restarting the loop, which is what the changelog says.

Sebastian> James, Martin, what's the status of this patch? I still hit Sebastian> the reported soft lockup on 4.5-rc1.

And you have verified that Bart's patch applied on top of 4.5-rc1 still fixes the lockup? (I know you tested a previous version)

Quoted 11 lines · Martin K. Petersen

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

"Bart" == Bart Van Assche <[email protected]> writes:

Bart> Instead of representing the states "visible in sysfs" and "has Bart> been removed from the target list" by a single state variable, use
Bart> two variables to represent this information.

James: Are you happy with the latest iteration of this? Should I queue
it?

Well, I'm OK with the patch: it's a simple transformation of the enumerated state to a two bit state. What I can't see is how it fixes any soft lockup.

The only change from the current workflow is that the DEL transition (now the reaped flag) is done before the spin lock is dropped which would fix a tiny window for two threads both trying to remove the same target, but there's nothing that could possibly fix an iterative soft lockup caused by restarting the loop, which is what the changelog says.

Quoted 20 lines · James Bottomley

Quoted 10 lines · Martin K. Petersen

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

"Bart" == Bart Van Assche <[email protected]> writes:

Bart> Instead of representing the states "visible in sysfs" and "has
Bart> been removed from the target list" by a single state variable,
use
Bart> two variables to represent this information.

Quoted 3 lines

James: Are you happy with the latest iteration of this? Should I queue
it?

Well, I'm OK with the patch: it's a simple transformation of the enumerated state to a two bit state. What I can't see is how it fixes
any soft lockup.

Quoted 9 lines

The only change from the current workflow is that the DEL transition
(now the reaped flag) is done before the spin lock is dropped which would fix a tiny window for two threads both trying to remove the same
target, but there's nothing that could possibly fix an iterative soft
lockup caused by restarting the loop, which is what the changelog says.

Hello James,

scsi_remove_target() doesn't lock the scan_mutex which means that concurrent SCSI scanning activity is not prohibited. Such scanning activity can postpone the transition of the state of a SCSI target into STARGET_DEL. I think if the scheduler decides to run the thread that executes scsi_remove_target() on the same CPU as the scanning code after the scanning code has obtained a reap ref and before the scanning code has released the reap ref again that the soft lockup can be triggered that has been reported by Sebastian Herbszt.

drivers/scsi/scsi_sysfs.c +1 -1

index 4f18a85..00bc721 100644
@@ -1272,16 +1272,18 @@ 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;
+     struct scsi_target *starget, *last_target = NULL;
      unsigned long flags;
 
  restart:
      spin_lock_irqsave(shost->host_lock, flags);
      list_for_each_entry(starget, &shost->__targets, siblings) {

Quoted 25 lines · James Bottomley

drivers/scsi/scsi_sysfs.c +4 -2

index 4f18a85..00bc721 100644
@@ -1272,16 +1272,18 @@ 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;
+     struct scsi_target *starget, *last_target = NULL;
      unsigned long flags;
 
  restart:
      spin_lock_irqsave(shost->host_lock, flags);
      list_for_each_entry(starget, &shost->__targets, siblings) {
-             if (starget->state == STARGET_DEL)
+             if (starget->state == STARGET_DEL ||
+                 starget == last_target)
                      continue;
              if (starget->dev.parent == dev || &starget->dev ==
 dev) {
                      kref_get(&starget->reap_ref);
+                     last_target = starget;
                      spin_unlock_irqrestore(shost->host_lock,

Quoted 3 lines

flags);
                      __scsi_remove_target(starget);
                      scsi_target_reap(starget);

Hello James,

Do you think it is a robust approach to store the pointer to the last removed target in the last_target variable ?

What if e.g. scsi_target_reap() frees the memory the last_target
pointer points at and another thread reallocates a scsi_target data structure ? Can that last data structure have the same address as the contents of the last_target variable ?

Quoted 32 lines · Bart Van Assche

Quoted 25 lines · James Bottomley

drivers/scsi/scsi_sysfs.c +4 -2

index 4f18a85..00bc721 100644
@@ -1272,16 +1272,18 @@ 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;
+     struct scsi_target *starget, *last_target = NULL;
       unsigned long flags;
 
   restart:
       spin_lock_irqsave(shost->host_lock, flags);
       list_for_each_entry(starget, &shost->__targets, siblings) {
-             if (starget->state == STARGET_DEL)
+             if (starget->state == STARGET_DEL ||
+                 starget == last_target)
                       continue;
               if (starget->dev.parent == dev || &starget->dev ==
 dev) {
                       kref_get(&starget->reap_ref);
+                     last_target = starget;
                       spin_unlock_irqrestore(shost->host_lock,

Quoted 3 lines

flags);
                       __scsi_remove_target(starget);
                       scsi_target_reap(starget);

Hello James,

Quoted 2 lines

Do you think it is a robust approach to store the pointer to the last removed target in the last_target variable ?

Well, yes, I think it will work, if that's what you mean.

Quoted 4 lines

What if e.g. scsi_target_reap() frees the memory the last_target
pointer points at and another thread reallocates a scsi_target data structure ? Can that last data structure have the same address as the contents of the last_target variable ?

Yes, but it doesn't matter, does it? Add/Remove has always (and will always) be racy. Under current conditions you can still add to the target after the list_for_each terminates and have scsi_remove_target() return with attached devices. The only way to close the race is basically to forbid scanning as we shut down the host and wait for all in-progress scans before starting the final removals.

Quoted 50 lines · James Bottomley

Quoted 35 lines · Bart Van Assche

Quoted 24 lines · James Bottomley

drivers/scsi/scsi_sysfs.c +3 -2

index 4f18a85..00bc721 100644
@@ -1272,16 +1272,18 @@ 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;
+     struct scsi_target *starget, *last_target = NULL;
       unsigned long flags;
 
   restart:
       spin_lock_irqsave(shost->host_lock, flags);
       list_for_each_entry(starget, &shost->__targets,
 siblings) {
-             if (starget->state == STARGET_DEL)
+             if (starget->state == STARGET_DEL ||
+                 starget == last_target)
                       continue;
               if (starget->dev.parent == dev || &starget->dev

==

Quoted 7 lines

dev) {
                       kref_get(&starget->reap_ref);
+                     last_target = starget;
                       spin_unlock_irqrestore(shost->host_lock,
flags);
                       __scsi_remove_target(starget);
                       scsi_target_reap(starget);

Hello James,

Quoted 3 lines

Do you think it is a robust approach to store the pointer to the last
removed target in the last_target variable ?

Well, yes, I think it will work, if that's what you mean.

Quoted 7 lines

What if e.g. scsi_target_reap() frees the memory the
last_target
pointer points at and another thread reallocates a scsi_target data
structure ? Can that last data structure have the same address as the
contents of the last_target variable ?

Quoted 9 lines

Yes, but it doesn't matter, does it? Add/Remove has always (and will
always) be racy. Under current conditions you can still add to the target after the list_for_each terminates and have scsi_remove_target()
return with attached devices. The only way to close the race is basically to forbid scanning as we shut down the host and wait for all
in-progress scans before starting the final removals.

Hello James,

Although the scenario I described is unlikely if it happens it might be really hard to figure out what went wrong for someone who has not followed this discussion. This makes me wonder whether the above patch is really the best way to fix the reported soft lockup ...

Quoted 33 lines · Bart Van Assche

Quoted 20 lines · James Bottomley

Quoted 10 lines · Martin K. Petersen

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

"Bart" == Bart Van Assche <[email protected]> writes:

Bart> Instead of representing the states "visible in sysfs" and "has
Bart> been removed from the target list" by a single state variable,
use
Bart> two variables to represent this information.

Quoted 3 lines

James: Are you happy with the latest iteration of this? Should I queue
it?

Well, I'm OK with the patch: it's a simple transformation of the enumerated state to a two bit state. What I can't see is how it fixes
any soft lockup.

Quoted 9 lines

The only change from the current workflow is that the DEL transition
(now the reaped flag) is done before the spin lock is dropped which would fix a tiny window for two threads both trying to remove the same
target, but there's nothing that could possibly fix an iterative soft
lockup caused by restarting the loop, which is what the changelog says.

Hello James,

Quoted 8 lines

scsi_remove_target() doesn't lock the scan_mutex which means that concurrent SCSI scanning activity is not prohibited. Such scanning activity can postpone the transition of the state of a SCSI target into STARGET_DEL. I think if the scheduler decides to run the thread that executes scsi_remove_target() on the same CPU as the scanning code after the scanning code has obtained a reap ref and before the scanning code has released the reap ref again that the soft lockup can be triggered that has been reported by Sebastian Herbszt.

OK, I finally understand the scenario; I'm not sure I understand how we're getting concurrent scanning and removal from a simple rmmod ... I take it this is insmod rmmod in a tight loop?

Quoted 58 lines · James Bottomley

Quoted 42 lines · Bart Van Assche

Quoted 25 lines · James Bottomley

Quoted 12 lines · Martin K. Petersen

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

"Bart" == Bart Van Assche <
[email protected]>
writes:

Bart> Instead of representing the states "visible in sysfs" and
"has
Bart> been removed from the target list" by a single state variable,
use
Bart> two variables to represent this information.

Quoted 4 lines

James: Are you happy with the latest iteration of this? Should I
queue
it?

Well, I'm OK with the patch: it's a simple transformation of the
enumerated state to a two bit state. What I can't see is how it
fixes
any soft lockup.

Quoted 13 lines

The only change from the current workflow is that the DEL transition
(now the reaped flag) is done before the spin lock is dropped which
would fix a tiny window for two threads both trying to remove the
same
target, but there's nothing that could possibly fix an iterative
soft
lockup caused by restarting the loop, which is what the changelog
says.

Hello James,

Quoted 14 lines

scsi_remove_target() doesn't lock the scan_mutex which means that concurrent SCSI scanning activity is not prohibited. Such scanning
activity can postpone the transition of the state of a SCSI target
into STARGET_DEL. I think if the scheduler decides to run the thread
that executes scsi_remove_target() on the same CPU as the scanning
code after the scanning code has obtained a reap ref and before the
scanning code has released the reap ref again that the soft lockup
can be triggered that has been reported by Sebastian Herbszt.

Quoted 5 lines

OK, I finally understand the scenario; I'm not sure I understand how
we're getting concurrent scanning and removal from a simple rmmod ... I
take it this is insmod rmmod in a tight loop?

I am able to trigger the soft lockup with this test case run once:

modprobe lpfc
run fio for 10 seconds
rmmod lpfc

My test setup involves running qla2xxx in target mode (SCST) and lpfc as initiator on the same system with one exported volume.

Dick, how did you trigger the lockup?

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 65 lines · Sebastian Herbszt

Quoted 58 lines · James Bottomley

Quoted 42 lines · Bart Van Assche

Quoted 25 lines · James Bottomley

Quoted 12 lines · Martin K. Petersen

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

"Bart" == Bart Van Assche <
[email protected]>
writes:

Bart> Instead of representing the states "visible in sysfs" and
"has
Bart> been removed from the target list" by a single state variable,
use
Bart> two variables to represent this information.

Quoted 4 lines

James: Are you happy with the latest iteration of this? Should I
queue
it?

Well, I'm OK with the patch: it's a simple transformation of the
enumerated state to a two bit state. What I can't see is how it
fixes
any soft lockup.

Quoted 13 lines

The only change from the current workflow is that the DEL transition
(now the reaped flag) is done before the spin lock is dropped which
would fix a tiny window for two threads both trying to remove the
same
target, but there's nothing that could possibly fix an iterative
soft
lockup caused by restarting the loop, which is what the changelog
says.

Hello James,

Quoted 14 lines

scsi_remove_target() doesn't lock the scan_mutex which means that concurrent SCSI scanning activity is not prohibited. Such scanning
activity can postpone the transition of the state of a SCSI target
into STARGET_DEL. I think if the scheduler decides to run the thread
that executes scsi_remove_target() on the same CPU as the scanning
code after the scanning code has obtained a reap ref and before the
scanning code has released the reap ref again that the soft lockup
can be triggered that has been reported by Sebastian Herbszt.

Quoted 5 lines

OK, I finally understand the scenario; I'm not sure I understand how
we're getting concurrent scanning and removal from a simple rmmod ... I
take it this is insmod rmmod in a tight loop?

Quoted 8 lines

I am able to trigger the soft lockup with this test case run once:

modprobe lpfc
run fio for 10 seconds
rmmod lpfc

My test setup involves running qla2xxx in target mode (SCST) and lpfc as initiator on the same system with one exported volume.

Hm, that doesn't seem to involve scanning colliding with removal.
Probably something else is pinning the target for some reason ...
unless there's some instability or constant change on the FC fabric itself? FC is hotplug, so if devices continually appear and disappear, they'd effectively cause a scan of the added device to take place even during the rmmod.

Anyway, does the last_scan patch fix the issue?

James

Quoted 65 lines · Sebastian Herbszt

Quoted 58 lines · James Bottomley

Quoted 42 lines · Bart Van Assche

Quoted 25 lines · James Bottomley

Quoted 12 lines · Martin K. Petersen

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

"Bart" == Bart Van Assche <
[email protected]>
writes:

Bart> Instead of representing the states "visible in sysfs" and
"has
Bart> been removed from the target list" by a single state variable,
use
Bart> two variables to represent this information.

Quoted 4 lines

James: Are you happy with the latest iteration of this? Should I
queue
it?

Well, I'm OK with the patch: it's a simple transformation of the
enumerated state to a two bit state. What I can't see is how it
fixes
any soft lockup.

Quoted 13 lines

The only change from the current workflow is that the DEL transition
(now the reaped flag) is done before the spin lock is dropped which
would fix a tiny window for two threads both trying to remove the
same
target, but there's nothing that could possibly fix an iterative
soft
lockup caused by restarting the loop, which is what the changelog
says.

Hello James,

Quoted 14 lines

scsi_remove_target() doesn't lock the scan_mutex which means that concurrent SCSI scanning activity is not prohibited. Such scanning
activity can postpone the transition of the state of a SCSI target
into STARGET_DEL. I think if the scheduler decides to run the thread
that executes scsi_remove_target() on the same CPU as the scanning
code after the scanning code has obtained a reap ref and before the
scanning code has released the reap ref again that the soft lockup
can be triggered that has been reported by Sebastian Herbszt.

Quoted 5 lines

OK, I finally understand the scenario; I'm not sure I understand how
we're getting concurrent scanning and removal from a simple rmmod ... I
take it this is insmod rmmod in a tight loop?

Quoted 8 lines

I am able to trigger the soft lockup with this test case run once:

modprobe lpfc
run fio for 10 seconds
rmmod lpfc

My test setup involves running qla2xxx in target mode (SCST) and lpfc as initiator on the same system with one exported volume.

Hm, that doesn't seem to involve scanning colliding with removal.
Probably something else is pinning the target for some reason ...
unless there's some instability or constant change on the FC fabric itself? FC is hotplug, so if devices continually appear and disappear, they'd effectively cause a scan of the added device to take place even during the rmmod.

Anyway, does the last_scan patch fix the issue?

James

Quoted 43 lines · James Bottomley

Quoted 33 lines · Bart Van Assche

Quoted 20 lines · James Bottomley

Quoted 10 lines · Martin K. Petersen

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

"Bart" == Bart Van Assche <[email protected]> writes:

Bart> Instead of representing the states "visible in sysfs" and "has
Bart> been removed from the target list" by a single state variable,
use
Bart> two variables to represent this information.

Quoted 3 lines

James: Are you happy with the latest iteration of this? Should I queue
it?

Well, I'm OK with the patch: it's a simple transformation of the enumerated state to a two bit state. What I can't see is how it fixes
any soft lockup.

Quoted 9 lines

The only change from the current workflow is that the DEL transition
(now the reaped flag) is done before the spin lock is dropped which would fix a tiny window for two threads both trying to remove the same
target, but there's nothing that could possibly fix an iterative soft
lockup caused by restarting the loop, which is what the changelog says.

Hello James,

Quoted 8 lines

scsi_remove_target() doesn't lock the scan_mutex which means that concurrent SCSI scanning activity is not prohibited. Such scanning activity can postpone the transition of the state of a SCSI target into STARGET_DEL. I think if the scheduler decides to run the thread that executes scsi_remove_target() on the same CPU as the scanning code after the scanning code has obtained a reap ref and before the scanning code has released the reap ref again that the soft lockup can be triggered that has been reported by Sebastian Herbszt.

Quoted 3 lines

OK, I finally understand the scenario; I'm not sure I understand how we're getting concurrent scanning and removal from a simple rmmod ... I take it this is insmod rmmod in a tight loop?

I am able to trigger the soft lockup with this test case run once:

modprobe lpfc
run fio for 10 seconds
rmmod lpfc

My test setup involves running qla2xxx in target mode (SCST) and lpfc as initiator on the same system with one exported volume.

Dick, how did you trigger the lockup?

Sebastian

Quoted 72 lines · Sebastian Herbszt

Quoted 59 lines · James Bottomley

Quoted 42 lines · Bart Van Assche

Quoted 25 lines · James Bottomley

Quoted 12 lines · Martin K. Petersen

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

"Bart" == Bart Van Assche <
[email protected]>
writes:

Bart> Instead of representing the states "visible in sysfs" and
"has
Bart> been removed from the target list" by a single state variable,
use
Bart> two variables to represent this information.

Quoted 4 lines

James: Are you happy with the latest iteration of this? Should I
queue
it?

Well, I'm OK with the patch: it's a simple transformation of the
enumerated state to a two bit state. What I can't see is how it
fixes
any soft lockup.

Quoted 13 lines

The only change from the current workflow is that the DEL transition
(now the reaped flag) is done before the spin lock is dropped which
would fix a tiny window for two threads both trying to remove the
same
target, but there's nothing that could possibly fix an iterative
soft
lockup caused by restarting the loop, which is what the changelog
says.

Hello James,

Quoted 15 lines

scsi_remove_target() doesn't lock the scan_mutex which means that
concurrent SCSI scanning activity is not prohibited. Such scanning
activity can postpone the transition of the state of a SCSI target
into STARGET_DEL. I think if the scheduler decides to run the thread
that executes scsi_remove_target() on the same CPU as the scanning
code after the scanning code has obtained a reap ref and before the
scanning code has released the reap ref again that the soft lockup
can be triggered that has been reported by Sebastian Herbszt.

Quoted 5 lines

OK, I finally understand the scenario; I'm not sure I understand how
we're getting concurrent scanning and removal from a simple rmmod ... I
take it this is insmod rmmod in a tight loop?

I am able to trigger the soft lockup with this test case run once:

modprobe lpfc
run fio for 10 seconds
rmmod lpfc

Quoted 6 lines

My test setup involves running qla2xxx in target mode (SCST) and lpfc as initiator on the same system with one exported volume.

Dick, how did you trigger the lockup?

Sebastian

Hi James, Bart, Martin

Have you already decided, which of the two patches you favour and when it'llbe included?

I have several customer reports that hit this lockup and I don't want to include one of the patches from the list just to find out the other one's is used in mainline.

Quoted 7 lines · Johannes Thumshirn

Quoted 6 lines · Sebastian Herbszt

Quoted 5 lines · James Bottomley

Quoted 4 lines · Bart Van Assche

Quoted 3 lines · James Bottomley

Quoted 2 lines · Martin K. Petersen

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

"Bart" == Bart Van Assche <
[email protected]>

Quoted 11 lines

Quoted 2 lines

Sebastian

Hi James, Bart, Martin

Have you already decided, which of the two patches you favour and when it'llbe included?

I have several customer reports that hit this lockup and I don't want to include one of the patches from the list just to find out the other one's is used in mainline.

Well, unless the target allocation bug gets fixed in Bart's, it will have to be the last_scan one. It's more a hack than a fix, but I suppose it will do as a bandaid in the meantime.

If you have diagnosed this at customers, I'd still like to know what's holding the devices on removal.

Thanks,

James