SCSI device removal fixes
v14 cover SCSI device removal fixes
Fix a few issues related to SCSI device removal:
- Fix a race between starved list processing and device removal that can trigger a kernel oops.
- Avoid that __scsi_remove_device() is called twice for the same SCSI device, which also can cause a kernel oops.
- Restrict the SCSI device state changes allowed via sysfs.
- Avoid re-enabling I/O after the transport layer became offline.
Changes compared to v13:
- Reworked the patch for avoiding to re-enable I/O while removing an offline device by introducing a new SCSI device state (SDEV_CANCEL_OFFLINE).
Changes compared to v12:
- Clarified the description of the patch for handling a transport layer failure during LUN scanning: mentioned that this patch was developed after analyzing the cause of a kernel oops triggered by asynchronous LUN scanning.
- Restored the previous version of the patch for restricting sysfs SCSI device state changes, namely the version that only disallows changing the device state into "cancel" or "deleted".
- Added a comment in patch 4/4.
- Left out a patch that was the result of source reading and also an intermediate patch that is no longer needed in this series.
Changes compared to v11:
- Left out a patch that was not a device removal bug fix.
- Left out the patches about which there is not yet an agreement.
Changes compared to v10:
- Rebased and retested on top of Linux kernel v3.10-rc5.
Changes compared to v9:
- Changed one WARN_ON() statement into a WARN() statement.
Changes compared to v8:
- Addressed the feedback from Joe Lawrence - dropped the patch that makes scsi_remove_host() wait until the last sdev user is gone.
- Eliminated Scsi_Host.tmf_in_progress since it duplicates state information available in Scsi_Host.eh_active.
- Added a patch to avoid reenabling I/O after the transport layer became offline.
Changes compared to v7:
- Addressed the review comments posted by Hannes Reinecke and Rolf Eike Beer.
- Modified patch "Make scsi_remove_host() wait until error handling finished" such that it is also safe for SCSI timeout values below the maximum LLD response time by modifying scsi_send_eh_cmnd() such that it does not invoke any LLD code after scsi_remove_host() started.
- Added a patch to save and restore the host_scribble field.
- Refined / clarified several patch descriptions.
- Rebased and retested on top of kernel v3.8-rc6.
Changes compared to v6:
- Dropped the first six patches since Jens queued these for 3.8.
- Added patch to avoid that __scsi_remove_device() is invoked twice.
- Restore error recovery in the SHOST_CANCEL state.
Changes compared to v5:
- Avoid that block layer work can be scheduled on a dead queue.
- Do not invoke any SCSI LLD callback after scsi_remove_host() finished.
- Stop error handling as soon as scsi_remove_host() started.
- Remove the unused function bsg_goose_queue().
- Avoid that scsi_device_set_state() triggers a race condition.
Changes compared to v4:
- Moved queue_flag_set(QUEUE_FLAG_DEAD, q) from blk_drain_queue() into blk_cleanup_queue().
- Declared the new __blk_run_queue_uncond() function inline. Checked in the generated assembler code that this function is really inlined in __blk_run_queue().
- Elaborated several patch descriptions.
- Added sparse annotations to scsi_request_fn().
- Split several patches.
Changes compared to v3:
- Fixed a race condition by setting QUEUE_FLAG_DEAD earlier.
- Added a patch for fixing a race between starved list processing and device removal to this series.
Changes compared to v2:
- Split second patch into two patches.
- Refined patch descriptions.
Changes compared to v1:
- Included a patch to rename QUEUE_FLAG_DEAD.
- Refined the descriptions of the __blk_run_queue_uncond() and blk_cleanup_queue() functions.
PATCH v14 1/6 Fix race between starved list and device removal
scsi_run_queue() examines all SCSI devices that are present on the starved list. Since scsi_run_queue() unlocks the SCSI host lock a SCSI device can get removed after it has been removed from the starved list and before its queue is run. Protect against that race condition by holding a reference on the queue while running it.
Reference: http://lkml.org/lkml/2012/8/2/96
Cc: Mike Christie <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: <[email protected]>
--- drivers/scsi/scsi_lib.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)
drivers/scsi/scsi_lib.c +21 -5
@@ -434,6 +434,8 @@ static void scsi_run_queue(struct request_queue *q)list_splice_init (&shost->starved_list, &starved_list);while (!list_empty (&starved_list)) { +struct request_queue *slq; +/* * As long as shost is accepting commands and we have * starved queues, call blk_run_queue. scsi_request_fn
@@ -456,11 +458,25 @@ static void scsi_run_queue(struct request_queue *q)continue ; } -spin_unlock (shost->host_lock); -spin_lock (sdev->request_queue->queue_lock); -__blk_run_queue (sdev->request_queue); -spin_unlock (sdev->request_queue->queue_lock); -spin_lock (shost->host_lock); +/* +* Once we drop the host lock, a racing scsi_remove_device() +* call may remove the sdev from the starved list and destroy +* it and the queue. Mitigate by taking a reference to the +* queue and never touching the sdev again after we drop the +* host lock. Note: if __scsi_remove_device() invokes +* blk_cleanup_queue() before the queue is run from this +* function then blk_run_queue() will return immediately since +* blk_cleanup_queue() marks the queue with QUEUE_FLAG_DYING. +*/ + slq = sdev->request_queue; +if (!blk_get_queue (slq)) +continue ; +spin_unlock_irqrestore (shost->host_lock, flags); + +blk_run_queue (slq); +blk_put_queue (slq); + +spin_lock_irqsave (shost->host_lock, flags); }/* put any unprocessed entries back */ list_splice (&starved_list, &shost->starved_list);
Signature · 1 line
1.7.10.4
PATCH v14 2/6 Avoid calling __scsi_remove_device() twice
In reply to Bart Van AsscheIf something goes wrong during LUN scanning, e.g. a transport layer failure occurs, then __scsi_remove_device() can get invoked by the LUN scanning code for a SCSI device in state SDEV_CREATED_BLOCK and before the SCSI device has been added to sysfs (is_visible == 0). Make sure that even in this case the transition into state SDEV_DEL occurs. This avoids that __scsi_remove_device() can get invoked a second time by scsi_forget_host() if this last function is invoked from another thread than the thread that performs LUN scanning.
--- drivers/scsi/scsi_lib.c | 1 + 1 file changed, 1 insertion(+)
drivers/scsi/scsi_lib.c +1 -0
@@ -2193,6 +2193,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)case SDEV_OFFLINE :case SDEV_TRANSPORT_OFFLINE :case SDEV_CANCEL : +case SDEV_CREATED_BLOCK :break ;default : goto illegal;
Signature · 1 line
1.7.10.4
PATCH v14 3/6 Introduce scsi_device_being_removed()
In reply to Bart Van AsscheIntroduce the helper function scsi_device_being_removed(). This patch does not change any functionality.
--- drivers/scsi/device_handler/scsi_dh.c | 7 ++----- include/scsi/scsi_device.h | 5 +++++ 2 files changed, 7 insertions(+), 5 deletions(-)
drivers/scsi/device_handler/scsi_dh.c +2 -5
@@ -156,8 +156,7 @@ store_dh_state(struct device *dev, struct device_attribute *attr,struct scsi_device_handler *scsi_dh;int err = -EINVAL ; -if (sdev->sdev_state ==SDEV_CANCEL || - sdev->sdev_state ==SDEV_DEL ) +if (scsi_device_being_removed (sdev))return -ENODEV ;if (!sdev->scsi_dh_data) {
@@ -400,9 +399,7 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)if (sdev->scsi_dh_data) scsi_dh = sdev->scsi_dh_data->scsi_dh; dev =get_device (&sdev->sdev_gendev); -if (!scsi_dh || !dev || - sdev->sdev_state ==SDEV_CANCEL || - sdev->sdev_state ==SDEV_DEL ) +if (!scsi_dh || !dev ||scsi_device_being_removed (sdev)) err =SCSI_DH_NOSYS ;if (sdev->sdev_state ==SDEV_OFFLINE ) err =SCSI_DH_DEV_OFFLINED ;
include/scsi/scsi_device.h +5 -0
@@ -454,6 +454,11 @@ static inline int scsi_device_created(struct scsi_device *sdev)return sdev->sdev_state ==SDEV_CREATED || sdev->sdev_state ==SDEV_CREATED_BLOCK ; } +static inline int scsi_device_being_removed (struct scsi_device *sdev) +{ +return sdev->sdev_state ==SDEV_CANCEL || + sdev->sdev_state ==SDEV_DEL ; +}/* accessor functions for the SCSI parameters */ static inline int scsi_device_sync (struct scsi_device *sdev)
Signature · 1 line
1.7.10.4
PATCH v14 4/6 Rework scsi_internal_device_unblock()
In reply to Bart Van AsscheModify scsi_internal_device_unblock() such that it uses scsi_device_set_state() to change the device state. This is only possible by changing scsi_device_set_state() such that it allows the transition from SDEV_CREATED_BLOCK to the SDEV_OFFLINE and SDEV_TRANSPORT_OFFLINE states. Notes:
- All callers of scsi_internal_device_unblock() ignore the return value of this function.
- Since the SDEV_CREATED_BLOCK to SDEV_{TRANSPORT_,}OFFLINE transition is now allowed, direct scsi_device_set_state() calls that change the device state from SDEV_CREATED_BLOCK into SDEV_*OFFLINE will now proceed instead of being rejected.
--- drivers/scsi/scsi_lib.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)
drivers/scsi/scsi_lib.c +11 -18
@@ -2147,6 +2147,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)case SDEV_RUNNING :case SDEV_QUIESCE :case SDEV_BLOCK : +case SDEV_CREATED_BLOCK :break ;default : goto illegal;
@@ -2501,29 +2502,21 @@ scsi_internal_device_unblock(struct scsi_device *sdev, {struct request_queue *q = sdev->request_queue;unsigned long flags; +int res;/* * Try to transition the scsi device to SDEV_RUNNING or one of the * offlined states and goose the device queue if successful. */ -if ((sdev->sdev_state ==SDEV_BLOCK ) || - (sdev->sdev_state ==SDEV_TRANSPORT_OFFLINE )) - sdev->sdev_state = new_state; -else if (sdev->sdev_state ==SDEV_CREATED_BLOCK ) { -if (new_state ==SDEV_TRANSPORT_OFFLINE || - new_state ==SDEV_OFFLINE ) - sdev->sdev_state = new_state; -else - sdev->sdev_state =SDEV_CREATED ; - }else if (sdev->sdev_state !=SDEV_CANCEL && - sdev->sdev_state !=SDEV_OFFLINE ) -return -EINVAL ; - -spin_lock_irqsave (q->queue_lock, flags); -blk_start_queue (q); -spin_unlock_irqrestore (q->queue_lock, flags); - -return 0 ; +if (sdev->sdev_state ==SDEV_CREATED_BLOCK && new_state ==SDEV_RUNNING ) + new_state =SDEV_CREATED ; + res =scsi_device_set_state (sdev, new_state); +if (!scsi_device_blocked (sdev)) { +spin_lock_irqsave (q->queue_lock, flags); +blk_start_queue (q); +spin_unlock_irqrestore (q->queue_lock, flags); + } +return res; }EXPORT_SYMBOL_GPL (scsi_internal_device_unblock);
Signature · 1 line
1.7.10.4
PATCH v14 5/6 Avoid re-enabling I/O after the transport became offline
In reply to Bart Van AsscheDisallow the SDEV_TRANSPORT_OFFLINE to SDEV_CANCEL transition such that no I/O is sent to devices for which the transport is offline. Notes:
- Functions like sd_shutdown() use scsi_execute_req() and hence set the REQ_PREEMPT flag. Such requests are passed to the LLD queuecommand callback in the SDEV_CANCEL state.
- This patch does not affect Fibre Channel LLD drivers since these drivers invoke fc_remote_port_chkready() before submitting a SCSI request to the HBA. That prevents a timeout to occur in state SDEV_CANCEL if the transport is offline.
--- drivers/scsi/scsi_lib.c | 12 +++++++++++- drivers/scsi/scsi_sysfs.c | 4 +++- include/scsi/scsi_device.h | 4 ++++ 3 files changed, 18 insertions(+), 2 deletions(-)
drivers/scsi/scsi_lib.c +11 -1
@@ -1232,6 +1232,7 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)switch (sdev->sdev_state) {case SDEV_OFFLINE :case SDEV_TRANSPORT_OFFLINE : +case SDEV_CANCEL_OFFLINE :/* * If the device is offline we refuse to process any * commands. The device must be brought online
@@ -2178,9 +2179,17 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)case SDEV_CREATED :case SDEV_RUNNING :case SDEV_QUIESCE : +case SDEV_BLOCK : +break ; +default : + goto illegal; + } +break ; + +case SDEV_CANCEL_OFFLINE : +switch (oldstate) {case SDEV_OFFLINE :case SDEV_TRANSPORT_OFFLINE : -case SDEV_BLOCK :break ;default : goto illegal;
@@ -2194,6 +2203,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)case SDEV_OFFLINE :case SDEV_TRANSPORT_OFFLINE :case SDEV_CANCEL : +case SDEV_CANCEL_OFFLINE :case SDEV_CREATED_BLOCK :break ;default :
drivers/scsi/scsi_sysfs.c +3 -1
@@ -32,6 +32,7 @@ static const struct { { SDEV_CREATED, "created" }, { SDEV_RUNNING, "running" }, { SDEV_CANCEL, "cancel" }, + { SDEV_CANCEL_OFFLINE, "cancel-offline" }, { SDEV_DEL, "deleted" }, { SDEV_QUIESCE, "quiesce" }, { SDEV_OFFLINE, "offline" },
@@ -955,7 +956,8 @@ void __scsi_remove_device(struct scsi_device *sdev)struct device *dev = &sdev->sdev_gendev;if (sdev->is_visible) { -if (scsi_device_set_state (sdev,SDEV_CANCEL ) !=0 ) +if (scsi_device_set_state (sdev,SDEV_CANCEL ) !=0 && +scsi_device_set_state (sdev,SDEV_CANCEL_OFFLINE ) !=0 )return ;bsg_unregister_queue (sdev->request_queue);
include/scsi/scsi_device.h +4 -0
@@ -35,6 +35,8 @@ enum scsi_device_state {* All commands allowed */ SDEV_CANCEL ,/* beginning to delete device * Only error handler commands allowed */ +SDEV_CANCEL_OFFLINE ,/* beginning to delete offline device +* No commands allowed */ SDEV_DEL ,/* device deleted * no commands allowed */ SDEV_QUIESCE, /* Device quiescent. No block commands
@@ -442,6 +444,7 @@ static inline int scsi_device_online(struct scsi_device *sdev) {return (sdev->sdev_state !=SDEV_OFFLINE && sdev->sdev_state !=SDEV_TRANSPORT_OFFLINE && + sdev->sdev_state !=SDEV_CANCEL_OFFLINE && sdev->sdev_state !=SDEV_DEL ); }static inline int scsi_device_blocked (struct scsi_device *sdev)
@@ -457,6 +460,7 @@ static inline int scsi_device_created(struct scsi_device *sdev)static inline int scsi_device_being_removed (struct scsi_device *sdev) {return sdev->sdev_state ==SDEV_CANCEL || + sdev->sdev_state ==SDEV_CANCEL_OFFLINE || sdev->sdev_state ==SDEV_DEL ; }
Signature · 1 line
1.7.10.4
PATCH v14 6/6 Disallow changing the device state via sysfs into "deleted"
In reply to Bart Van AsscheChanging the state of a SCSI device via sysfs into "cancel", "cancel-offline" or "deleted" prevents removal of these devices by scsi_remove_host(). Hence do not allow this.
--- drivers/scsi/scsi_sysfs.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/scsi/scsi_sysfs.c +3 -4
@@ -606,10 +606,9 @@ store_state_field(struct device *dev, struct device_attribute *attr,break ; } } -if (!state) -return -EINVAL ; - -if (scsi_device_set_state (sdev, state)) +if (state ==0 || state ==SDEV_CANCEL || + state ==SDEV_CANCEL_OFFLINE || state ==SDEV_DEL || +scsi_device_set_state (sdev, state) !=0 )return -EINVAL ;return count; }
Signature · 1 line
1.7.10.4