| Message ID | 20190402161624.148359-2-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Avoid that .queuecommand() gets called for a quiesced SCSI device | expand |
On Tue, Apr 02, 2019 at 09:16:23AM -0700, Bart Van Assche wrote: > Several SCSI transport and LLD drivers surround code that does not > tolerate concurrent calls of .queuecommand() with scsi_target_block() / > scsi_target_unblock(). These last two functions use > blk_mq_quiesce_queue() / blk_mq_unquiesce_queue() for scsi-mq request > queues to prevent concurrent .queuecommand() calls. However, that is > not sufficient to prevent .queuecommand() calls from scsi_send_eh_cmnd(). > Hence surround the .queuecommand() call from the SCSI error handler with > code that avoids that .queuecommand() gets called in the quiesced state. > > Note: converting the .queuecommand() call in scsi_send_eh_cmnd() into > code that calls blk_get_request() + blk_execute_rq() is not an option > since scsi_send_eh_cmnd() must be able to make forward progress even > if all requests have been allocated. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: Hannes Reinecke <hare@suse.de> > Cc: Johannes Thumshirn <jthumshirn@suse.de> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/scsi_error.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index 8e9680572b9f..d516dd1b824d 100644 > --- a/drivers/scsi/scsi_error.c > +++ b/drivers/scsi/scsi_error.c > @@ -1054,7 +1054,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, > struct scsi_device *sdev = scmd->device; > struct Scsi_Host *shost = sdev->host; > DECLARE_COMPLETION_ONSTACK(done); > - unsigned long timeleft = timeout; > + unsigned long timeleft = timeout, delay; > struct scsi_eh_save ses; > const unsigned long stall_for = msecs_to_jiffies(100); > int rtn; > @@ -1065,7 +1065,29 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, > > scsi_log_send(scmd); > scmd->scsi_done = scsi_eh_done; > - rtn = shost->hostt->queuecommand(shost, scmd); > + > + /* > + * Lock sdev->state_mutex to avoid that scsi_device_quiesce() can > + * change the SCSI device state after we have examined it and before > + * .queuecommand() is called. > + */ > + mutex_lock(&sdev->state_mutex); > + while (sdev->sdev_state == SDEV_BLOCK && timeleft > 0) { > + mutex_unlock(&sdev->state_mutex); > + SCSI_LOG_ERROR_RECOVERY(5, sdev_printk(KERN_DEBUG, sdev, > + "%s: state %d <> %d\n", __func__, sdev->sdev_state, > + SDEV_BLOCK)); > + delay = min(timeleft, stall_for); > + timeleft -= delay; > + msleep(jiffies_to_msecs(delay)); > + mutex_lock(&sdev->state_mutex); > + } > + if (sdev->sdev_state != SDEV_BLOCK) > + rtn = shost->hostt->queuecommand(shost, scmd); > + else > + rtn = SCSI_MLQUEUE_DEVICE_BUSY; > + mutex_unlock(&sdev->state_mutex); > + > if (rtn) { > if (timeleft > stall_for) { > scsi_eh_restore_cmnd(scmd, &ses); > -- > 2.21.0.196.g041f5ea1cf98 > Still not sure if it is safe to do that in case of SDEV_BLOCK. SDEV_BLOCK can be set via sysfs interface, then what if there are in-flight IOs which need EH to retry just after someone sets 'blocked'? People may complain this patch causes data loss on above test case. Thanks, Ming
On Wed, 2019-04-03 at 12:15 +0800, Ming Lei wrote: > Still not sure if it is safe to do that in case of SDEV_BLOCK. > > SDEV_BLOCK can be set via sysfs interface, then what if there are > in-flight IOs which need EH to retry just after someone sets 'blocked'? > > People may complain this patch causes data loss on above test case. Hi Ming, scsi_send_eh_cmnd() is only used to request sense data, to submit a TUR or to submit a START UNIT. None of these commands modify the data stored on the SCSI device so there is no risk of data loss. The ability to modify the SCSI device state was introduced by commit 638127e579a4 ("[PATCH] Fix error handler offline behaviour"; v2.6.12). That same commit introduced the following device states: { SDEV_CREATED, "created" }, { SDEV_RUNNING, "running" }, { SDEV_CANCEL, "cancel" }, { SDEV_DEL, "deleted" }, { SDEV_QUIESCE, "quiesce" }, { SDEV_OFFLINE, "offline" }, The SDEV_BLOCK state was introduced later to avoid that an FC cable pull would immediately result in an I/O error (commit 1094e682310e; "[PATCH] suspending I/Os to a device"; v2.6.12). I'm not sure whether the ability to set the SDEV_BLOCK state from user space was introduced on purpose or accidentally. I think there are three alternatives: (1) Accept that some error handling steps are skipped if a user sets the device state to "blocked". (2) Prevent users to change the device state to "blocked". (3) Split SDEV_BLOCK into SDEV_BLOCKED_BY_USER and SDEV_BLOCKED_BY_TRANSPORT and only skip sending EH commands to the device in state SDEV_BLOCKED_BY_TRANSPORT. Thanks, Bart.
On Wed, 2019-04-03 at 09:06 -0700, Bart Van Assche wrote: > scsi_send_eh_cmnd() is only used to request sense data, to submit a TUR or > to submit a START UNIT. None of these commands modify the data stored on > the SCSI device so there is no risk of data loss. > > The ability to modify the SCSI device state was introduced by commit > 638127e579a4 ("[PATCH] Fix error handler offline behaviour"; v2.6.12). That > same commit introduced the following device states: > > { SDEV_CREATED, "created" }, > { SDEV_RUNNING, "running" }, > { SDEV_CANCEL, "cancel" }, > { SDEV_DEL, "deleted" }, > { SDEV_QUIESCE, "quiesce" }, > { SDEV_OFFLINE, "offline" }, > > The SDEV_BLOCK state was introduced later to avoid that an FC cable pull > would immediately result in an I/O error (commit 1094e682310e; "[PATCH] > suspending I/Os to a device"; v2.6.12). I'm not sure whether the ability to > set the SDEV_BLOCK state from user space was introduced on purpose or > accidentally. > > I think there are three alternatives: > (1) Accept that some error handling steps are skipped if a user sets the > device state to "blocked". > (2) Prevent users to change the device state to "blocked". > (3) Split SDEV_BLOCK into SDEV_BLOCKED_BY_USER and SDEV_BLOCKED_BY_TRANSPORT > and only skip sending EH commands to the device in state > SDEV_BLOCKED_BY_TRANSPORT. (repyling to my own e-mail) Does anyone want to share an opinion about the above? Thanks, Bart.
Bart, >> (1) Accept that some error handling steps are skipped if a user sets the >> device state to "blocked". >> (2) Prevent users to change the device state to "blocked". We can't really remove an exposed interface. So at the very least the string "blocked" would have to trigger something semantically close. Quiesce, maybe? >> (3) Split SDEV_BLOCK into SDEV_BLOCKED_BY_USER and >> SDEV_BLOCKED_BY_TRANSPORT and only skip sending EH commands to the >> device in state SDEV_BLOCKED_BY_TRANSPORT.
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 8e9680572b9f..d516dd1b824d 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1054,7 +1054,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, struct scsi_device *sdev = scmd->device; struct Scsi_Host *shost = sdev->host; DECLARE_COMPLETION_ONSTACK(done); - unsigned long timeleft = timeout; + unsigned long timeleft = timeout, delay; struct scsi_eh_save ses; const unsigned long stall_for = msecs_to_jiffies(100); int rtn; @@ -1065,7 +1065,29 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, scsi_log_send(scmd); scmd->scsi_done = scsi_eh_done; - rtn = shost->hostt->queuecommand(shost, scmd); + + /* + * Lock sdev->state_mutex to avoid that scsi_device_quiesce() can + * change the SCSI device state after we have examined it and before + * .queuecommand() is called. + */ + mutex_lock(&sdev->state_mutex); + while (sdev->sdev_state == SDEV_BLOCK && timeleft > 0) { + mutex_unlock(&sdev->state_mutex); + SCSI_LOG_ERROR_RECOVERY(5, sdev_printk(KERN_DEBUG, sdev, + "%s: state %d <> %d\n", __func__, sdev->sdev_state, + SDEV_BLOCK)); + delay = min(timeleft, stall_for); + timeleft -= delay; + msleep(jiffies_to_msecs(delay)); + mutex_lock(&sdev->state_mutex); + } + if (sdev->sdev_state != SDEV_BLOCK) + rtn = shost->hostt->queuecommand(shost, scmd); + else + rtn = SCSI_MLQUEUE_DEVICE_BUSY; + mutex_unlock(&sdev->state_mutex); + if (rtn) { if (timeleft > stall_for) { scsi_eh_restore_cmnd(scmd, &ses);
Several SCSI transport and LLD drivers surround code that does not tolerate concurrent calls of .queuecommand() with scsi_target_block() / scsi_target_unblock(). These last two functions use blk_mq_quiesce_queue() / blk_mq_unquiesce_queue() for scsi-mq request queues to prevent concurrent .queuecommand() calls. However, that is not sufficient to prevent .queuecommand() calls from scsi_send_eh_cmnd(). Hence surround the .queuecommand() call from the SCSI error handler with code that avoids that .queuecommand() gets called in the quiesced state. Note: converting the .queuecommand() call in scsi_send_eh_cmnd() into code that calls blk_get_request() + blk_execute_rq() is not an option since scsi_send_eh_cmnd() must be able to make forward progress even if all requests have been allocated. Cc: Christoph Hellwig <hch@lst.de> Cc: Ming Lei <ming.lei@redhat.com> Cc: Hannes Reinecke <hare@suse.de> Cc: Johannes Thumshirn <jthumshirn@suse.de> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/scsi_error.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)