Separate target visibility from reaped state information
PATCH v2 1/1 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).
--- 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(-)
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.
This patch has been tested on top of kernel v4.4-rc1 with the SRP initiator driver.
drivers/scsi/scsi_scan.c +3 -28
@@ -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
@@ -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
@@ -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 3char 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 · 1 line
2.1.4
James Bottomley
maintainerQuoted 4 lines · 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.
Quoted 8 lines
This patch avoids that SCSI device removal can trigger a soft lockup.
See also:
* "scsi: restart list search after unlock inscsi_remove_target" (commit40998193560d).
* "scsi_remove_target: fix softlockup regression on hot remove" (commitbc3f02a795d3).
OK, could you justify this, please ... like with traces and things.
The theory on which
commit 40998193560dab6c3ce8d25f4fa58a23e252ef38
Author: Christoph Hellwig <[email protected]> Date: Mon Oct 19 16:35:46 2015 +0200
scsi: restart list search after unlock in scsi_remove_target
Was based is that the race you're claiming to be fixing no longer exists because it was fixed by
commit f2495e228fce9f9cec84367547813cbb0d6db15a
Author: James Bottomley <[email protected]> Date: Tue Jan 21 07:01:41 2014 -0800
[SCSI] dual scan thread bug fix
If that isn't the case, we can fix it, but I'd like to see the evidence.
Thanks,
James
Quoted 3 lines · James Bottomley
OK, could you justify this, please ... like with traces and things. [ ... ]
If that isn't the case, we can fix it, but I'd like to see the evidence.
Hello James,
To my own surprise so far I have not yet been able to trigger a lockup without this patch. After I had inserted an msleep() call at the start of scsi_target_reap_ref_release() to delay the starget->state = STARGET_DEL assignment I rebuilt and rebooted the kernel, started multipathd and held a SCSI device open by creating a filesystem on it and by mounting it. Next I removed all SRP targets (for p in /sys/class/srp_remote_ports/*; do echo 1 >$p/delete & done). During this test no lockup was reported.
Bart.