On ata passthru commands scsih_qcmd() ends up spinning in
scsi_wait_for_queuecommand() indefinitely. scsih_qcmd() is called from
__blk_run_queue_uncond() which first increments request_fn_active to a
non-zero value. Thus, scsi_wait_for_queuecommand() never completes because
its spinning waiting for request_fn_active to become 0.
Two patches interact here. The first:
commit 18f6084a989b ("scsi: mpt3sas: Fix secure erase premature
termination") calls scsi_internal_device_block() for ata passthru commands.
The second patch:
commit 669f044170d8 ("scsi: srp_transport: Move queuecommand() wait code
to SCSI core") adds a call to scsi_wait_for_queuecommand() from
scsi_internal_device_block().
Add a new parameter to scsi_internal_device_block() to decide whether
or not to invoke scsi_wait_for_queuecommand().
Signed-off-by: Jason Baron <[email protected]>
Cc: Sathya Prakash <[email protected]>
Cc: Chaitra P B <[email protected]>
Cc: Suganath Prabu Subramani <[email protected]>
Cc: Sreekanth Reddy <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Martin K. Petersen <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: James Bottomley <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Doug Ledford <[email protected]>
Cc: David Miller <[email protected]>
---
drivers/scsi/mpt3sas/mpt3sas_base.h | 2 +-
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 6 +++---
drivers/scsi/scsi_lib.c | 11 +++++++----
drivers/scsi/scsi_priv.h | 2 +-
4 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 394fe13..5da3427 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -1431,7 +1431,7 @@ void mpt3sas_transport_update_links(struct MPT3SAS_ADAPTER *ioc,
u64 sas_address, u16 handle, u8 phy_number, u8 link_rate);
extern struct sas_function_template mpt3sas_transport_functions;
extern struct scsi_transport_template *mpt3sas_transport_template;
-extern int scsi_internal_device_block(struct scsi_device *sdev);
+extern int scsi_internal_device_block(struct scsi_device *sdev, bool flush);
extern int scsi_internal_device_unblock(struct scsi_device *sdev,
enum scsi_device_state new_state);
/* trigger data externs */
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index b5c966e..509ef8a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2839,7 +2839,7 @@ _scsih_internal_device_block(struct scsi_device *sdev,
sas_device_priv_data->sas_target->handle);
sas_device_priv_data->block = 1;
- r = scsi_internal_device_block(sdev);
+ r = scsi_internal_device_block(sdev, true);
if (r == -EINVAL)
sdev_printk(KERN_WARNING, sdev,
"device_block failed with return(%d) for handle(0x%04x)\n",
@@ -2875,7 +2875,7 @@ _scsih_internal_device_unblock(struct scsi_device *sdev,
"performing a block followed by an unblock\n",
r, sas_device_priv_data->sas_target->handle);
sas_device_priv_data->block = 1;
- r = scsi_internal_device_block(sdev);
+ r = scsi_internal_device_block(sdev, true);
if (r)
sdev_printk(KERN_WARNING, sdev, "retried device_block "
"failed with return(%d) for handle(0x%04x)\n",
@@ -4068,7 +4068,7 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
* done.
*/
if (ata_12_16_cmd(scmd))
- scsi_internal_device_block(scmd->device);
+ scsi_internal_device_block(scmd->device, false);
sas_device_priv_data = scmd->device->hostdata;
if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c35b6de..2ee2db9 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2856,9 +2856,11 @@ EXPORT_SYMBOL(scsi_target_resume);
/**
* scsi_internal_device_block - internal function to put a device temporarily into the SDEV_BLOCK state
* @sdev: device to block
+ * @flush: wait for oustanding queuecommand calls to finish
*
* Block request made by scsi lld's to temporarily stop all
- * scsi commands on the specified device. May sleep.
+ * scsi commands on the specified device. May sleep if
+ * flush is set
*
* Returns zero if successful or error if not
*
@@ -2873,7 +2875,7 @@ EXPORT_SYMBOL(scsi_target_resume);
* remove the rport mutex lock and unlock calls from srp_queuecommand().
*/
int
-scsi_internal_device_block(struct scsi_device *sdev)
+scsi_internal_device_block(struct scsi_device *sdev, bool flush)
{
struct request_queue *q = sdev->request_queue;
unsigned long flags;
@@ -2898,7 +2900,8 @@ scsi_internal_device_block(struct scsi_device *sdev)
spin_lock_irqsave(q->queue_lock, flags);
blk_stop_queue(q);
spin_unlock_irqrestore(q->queue_lock, flags);
- scsi_wait_for_queuecommand(sdev);
+ if (flush)
+ scsi_wait_for_queuecommand(sdev);
}
return 0;
@@ -2960,7 +2963,7 @@ EXPORT_SYMBOL_GPL(scsi_internal_device_unblock);
static void
device_block(struct scsi_device *sdev, void *data)
{
- scsi_internal_device_block(sdev);
+ scsi_internal_device_block(sdev, true);
}
static int
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 193636a..c0f79b8 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -189,7 +189,7 @@ static inline void scsi_dh_remove_device(struct scsi_device *sdev) { }
*/
#define SCSI_DEVICE_BLOCK_MAX_TIMEOUT 600 /* units in seconds */
-extern int scsi_internal_device_block(struct scsi_device *sdev);
+extern int scsi_internal_device_block(struct scsi_device *sdev, bool flush);
extern int scsi_internal_device_unblock(struct scsi_device *sdev,
enum scsi_device_state new_state);
--
2.6.1
On Wed, Dec 28, 2016 at 11:30:24PM -0500, Jason Baron wrote:
> Add a new parameter to scsi_internal_device_block() to decide whether
> or not to invoke scsi_wait_for_queuecommand().
We'll also need to deal with the blk-mq wait path that Bart has been
working on (I think it's already in the scsi tree, but I'd have to
check).
Also adding a bool flag for the last call in a function is style that's
a little annoying.
I'd prefer to add a scsi_internal_device_block_nowait that contains
all the code except for the waiting, and then make
scsi_internal_device_block_nowait a wrapper around it. Or drop the
annoying internal for both while we're at it :)
On 12/29/2016 03:02 AM, Christoph Hellwig wrote:
> On Wed, Dec 28, 2016 at 11:30:24PM -0500, Jason Baron wrote:
>> Add a new parameter to scsi_internal_device_block() to decide whether
>> or not to invoke scsi_wait_for_queuecommand().
> We'll also need to deal with the blk-mq wait path that Bart has been
> working on (I think it's already in the scsi tree, but I'd have to
> check).
Ok, I'm not sure either.
> Also adding a bool flag for the last call in a function is style that's
> a little annoying.
>
> I'd prefer to add a scsi_internal_device_block_nowait that contains
> all the code except for the waiting, and then make
> scsi_internal_device_block_nowait a wrapper around it. Or drop the
> annoying internal for both while we're at it :)
The proposed patch brings the code in-line with what is in 4.8 stable
where scsi_internal_device_block() does not call
scsi_wait_for_queuecommand(). So I saw it as a minimal fix to make
my system boot again :)
I was wondering if the original fix is racy in that there could be multiple
threads in the queuecommand. Perhaps we should do something like:
if (ata_12_16_cmd(scmd))
{
if (!test_and_set_bit(MPT_DEVICE_EXCLUSIVE,
&sas_device_priv_data->flags))
{
scsi_internal_device_block(scmd->device);
}
else
return
SCSI_MLQUEUE_HOST_BUSY;
}
where scsi_internal_device_block() could be taught to wait for
request_fn_active becoming 1 instead of 0.
Thanks,
-Jason
From: Jason Baron <[email protected]>
Date: Wed, 28 Dec 2016 23:30:24 -0500
> On ata passthru commands scsih_qcmd() ends up spinning in
> scsi_wait_for_queuecommand() indefinitely. scsih_qcmd() is called from
> __blk_run_queue_uncond() which first increments request_fn_active to a
> non-zero value. Thus, scsi_wait_for_queuecommand() never completes because
> its spinning waiting for request_fn_active to become 0.
>
> Two patches interact here. The first:
>
> commit 18f6084a989b ("scsi: mpt3sas: Fix secure erase premature
> termination") calls scsi_internal_device_block() for ata passthru commands.
>
> The second patch:
>
> commit 669f044170d8 ("scsi: srp_transport: Move queuecommand() wait code
> to SCSI core") adds a call to scsi_wait_for_queuecommand() from
> scsi_internal_device_block().
>
> Add a new parameter to scsi_internal_device_block() to decide whether
> or not to invoke scsi_wait_for_queuecommand().
>
> Signed-off-by: Jason Baron <[email protected]>
Tested-by: David S. Miller <[email protected]>
On Thu, 2016-12-29 at 00:02 -0800, Christoph Hellwig wrote:
> On Wed, Dec 28, 2016 at 11:30:24PM -0500, Jason Baron wrote:
> > Add a new parameter to scsi_internal_device_block() to decide
> > whether or not to invoke scsi_wait_for_queuecommand().
>
> We'll also need to deal with the blk-mq wait path that Bart has been
> working on (I think it's already in the scsi tree, but I'd have to
> check).
>
> Also adding a bool flag for the last call in a function is style
> that's a little annoying.
>
> I'd prefer to add a scsi_internal_device_block_nowait that contains
> all the code except for the waiting, and then make
> scsi_internal_device_block_nowait a wrapper around it. Or drop the
> annoying internal for both while we're at it :)
OK, I know it's new year, but this is an unpatched regression in -rc1
that's causing serious issues. I would like this fixed by -rc3 so we
have 3 options
1. revert all the queuecommand wait stuff until it proves it's actually
working without regressions
2. apply this patch and fix the style issues later
3. someone else supplies the correctly styled fix patch
The conservative in me says that 1. is probably the most correct thing
to do because it gives us time to get the queuecommand wait stuff
right; that's what I'll probably do if there's no movement next week.
However, since we're reasonably early in the -rc cycle, so either 2 or
3 are viable provided no further regressions caused by the queuecommand
wait stuff pop up.
James