Re: [PATCH, resend] scsi: Avoid that .queuecommand() gets called for a quiesced SCSI device

2018-03-14 Thread James Bottomley
On Wed, 2018-03-14 at 15:45 -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.
> 
> Notes:
> - 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 are allocated.
> - Converting the .queuecommand() call in scsi_send_eh_cmnd() into a
>   blk_execute_rq() or blk_mq_requeue_request() call is not an option
> either
>   because that would require to change every individual function in
> the I/O
>   path. Each function in the I/O path would have to be modified such
> that it
>   handles requests received from the block layer core and request
> received
>   from the SCSI EH differently. Since struct scsi_cmnd is not
> initialized by
>   the block layer for filesystem requests, it is not possible to
> determine
>   in scsi_queue_rq() whether or not a request has been submitted by
> the
>   SCSI EH without modifying the block layer.
> 
> Signed-off-by: Bart Van Assche 
> Cc: Hannes Reinecke 
> Cc: Johannes Thumshirn 
> ---
>  drivers/scsi/scsi_error.c  | 13 +
>  drivers/scsi/scsi_lib.c|  2 ++
>  drivers/scsi/scsi_scan.c   |  1 +
>  include/scsi/scsi_device.h |  1 +
>  4 files changed, 17 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 946039117bf4..cfc805851a2a 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1042,6 +1042,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd
> *scmd, unsigned char *cmnd,
>   unsigned long timeleft = timeout;
>   struct scsi_eh_save ses;
>   const unsigned long stall_for = msecs_to_jiffies(100);
> + DEFINE_WAIT(wait);
>   int rtn;
>  
>  retry:
> @@ -1050,7 +1051,19 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd
> *scmd, unsigned char *cmnd,
>  
>   scsi_log_send(scmd);
>   scmd->scsi_done = scsi_eh_done;
> + mutex_lock(&sdev->state_mutex);
> + while (sdev->sdev_state == SDEV_QUIESCE) {
> + prepare_to_wait(&sdev->state_wq, &wait,
> TASK_INTERRUPTIBLE);
> + 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_QUIESCE));
> + schedule();
> + mutex_lock(&sdev->state_mutex);
> + }
> + finish_wait(&sdev->state_wq, &wait);
>   rtn = shost->hostt->queuecommand(shost, scmd);
> + mutex_unlock(&sdev->state_mutex);
>   if (rtn) {
>   if (timeleft > stall_for) {

This has got to be minutely rare: why not just use the existing
stall_for timeout infrastructure instead of adding a waitqueue to every
device?

James



[PATCH, resend] scsi: Avoid that .queuecommand() gets called for a quiesced SCSI device

2018-03-14 Thread Bart Van Assche
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.

Notes:
- 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 are allocated.
- Converting the .queuecommand() call in scsi_send_eh_cmnd() into a
  blk_execute_rq() or blk_mq_requeue_request() call is not an option either
  because that would require to change every individual function in the I/O
  path. Each function in the I/O path would have to be modified such that it
  handles requests received from the block layer core and request received
  from the SCSI EH differently. Since struct scsi_cmnd is not initialized by
  the block layer for filesystem requests, it is not possible to determine
  in scsi_queue_rq() whether or not a request has been submitted by the
  SCSI EH without modifying the block layer.

Signed-off-by: Bart Van Assche 
Cc: Hannes Reinecke 
Cc: Johannes Thumshirn 
---
 drivers/scsi/scsi_error.c  | 13 +
 drivers/scsi/scsi_lib.c|  2 ++
 drivers/scsi/scsi_scan.c   |  1 +
 include/scsi/scsi_device.h |  1 +
 4 files changed, 17 insertions(+)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 946039117bf4..cfc805851a2a 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1042,6 +1042,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, 
unsigned char *cmnd,
unsigned long timeleft = timeout;
struct scsi_eh_save ses;
const unsigned long stall_for = msecs_to_jiffies(100);
+   DEFINE_WAIT(wait);
int rtn;
 
 retry:
@@ -1050,7 +1051,19 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, 
unsigned char *cmnd,
 
scsi_log_send(scmd);
scmd->scsi_done = scsi_eh_done;
+   mutex_lock(&sdev->state_mutex);
+   while (sdev->sdev_state == SDEV_QUIESCE) {
+   prepare_to_wait(&sdev->state_wq, &wait, TASK_INTERRUPTIBLE);
+   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_QUIESCE));
+   schedule();
+   mutex_lock(&sdev->state_mutex);
+   }
+   finish_wait(&sdev->state_wq, &wait);
rtn = shost->hostt->queuecommand(shost, scmd);
+   mutex_unlock(&sdev->state_mutex);
if (rtn) {
if (timeleft > stall_for) {
scsi_eh_restore_cmnd(scmd, &ses);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1d83f29aee74..99741858b6da 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2757,6 +2757,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum 
scsi_device_state state)
 
}
sdev->sdev_state = state;
+   wake_up_all(&sdev->state_wq);
return 0;
 
  illegal:
@@ -3216,6 +3217,7 @@ int scsi_internal_device_unblock_nowait(struct 
scsi_device *sdev,
default:
return -EINVAL;
}
+   wake_up_all(&sdev->state_wq);
scsi_start_queue(sdev);
 
return 0;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 0880d975eed3..32439ca61c18 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -232,6 +232,7 @@ static struct scsi_device *scsi_alloc_sdev(struct 
scsi_target *starget,
sdev->id = starget->id;
sdev->lun = lun;
sdev->channel = starget->channel;
+   init_waitqueue_head(&sdev->state_wq);
mutex_init(&sdev->state_mutex);
sdev->sdev_state = SDEV_CREATED;
INIT_LIST_HEAD(&sdev->siblings);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7ae177c8e399..eb9063202036 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -222,6 +222,7 @@ struct scsi_device {
void*handler_data;
 
unsigned char   access_state;
+   struct wait_queue_head  state_wq;
struct mutexstate_mutex;
enum scsi_device_state sdev_state;
struct task_struct  *quiesced_by;
-- 
2.16.2