Discussion:
I/O path cleanup
Christoph Hellwig
2014-09-07 16:31:01 UTC
Permalink
This series cleans up a couple of lose ends I noticed during the scsi-mq
work, but which weren't important enough to address during the last cycle.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-09-07 16:31:02 UTC
Permalink
scsi_reset_provider already manually runs all queues for the given host,
so it doesn't need the scsi_run_queues call from it, and it doesn't need
a reference on the device because it's synchronous.

So let's just call scsi_put_command directly and avoid the device reference
dance to simplify the code.

Signed-off-by: Christoph Hellwig <***@lst.de>
---
drivers/scsi/scsi_error.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 6b20ef3..14cece9 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2317,15 +2317,9 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
if (scsi_autopm_get_host(shost) < 0)
return FAILED;

- if (!get_device(&dev->sdev_gendev)) {
- rtn = FAILED;
- goto out_put_autopm_host;
- }
-
scmd = scsi_get_command(dev, GFP_KERNEL);
if (!scmd) {
rtn = FAILED;
- put_device(&dev->sdev_gendev);
goto out_put_autopm_host;
}

@@ -2381,10 +2375,9 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
"waking up host to restart after TMF\n"));

wake_up(&shost->host_wait);
-
scsi_run_host_queues(shost);

- scsi_next_command(scmd);
+ scsi_put_command(scmd);
out_put_autopm_host:
scsi_autopm_put_host(shost);
return rtn;
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bart Van Assche
2014-10-01 12:03:45 UTC
Permalink
Post by Christoph Hellwig
scsi_reset_provider already manually runs all queues for the given host,
so it doesn't need the scsi_run_queues call from it, and it doesn't need
a reference on the device because it's synchronous.
So let's just call scsi_put_command directly and avoid the device reference
dance to simplify the code.
---
drivers/scsi/scsi_error.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 6b20ef3..14cece9 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2317,15 +2317,9 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
if (scsi_autopm_get_host(shost) < 0)
return FAILED;
- if (!get_device(&dev->sdev_gendev)) {
- rtn = FAILED;
- goto out_put_autopm_host;
- }
-
scmd = scsi_get_command(dev, GFP_KERNEL);
if (!scmd) {
rtn = FAILED;
- put_device(&dev->sdev_gendev);
goto out_put_autopm_host;
}
@@ -2381,10 +2375,9 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
"waking up host to restart after TMF\n"));
wake_up(&shost->host_wait);
-
scsi_run_host_queues(shost);
- scsi_next_command(scmd);
+ scsi_put_command(scmd);
scsi_autopm_put_host(shost);
return rtn;
This patch looks fine to me.

A minor nit: if this patch has to be resent, please insert a blank line
above the out_put_autopm_host label.

Reviewed-by: Bart Van Assche <***@acm.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-09-07 16:31:03 UTC
Permalink
There's only one caller left, so inline it and reduce the blk-mq vs !blk-mq
diff a litte bit.

Signed-off-by: Christoph Hellwig <***@lst.de>
---
drivers/scsi/scsi_lib.c | 18 ++++--------------
drivers/scsi/scsi_priv.h | 1 -
2 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8b76231..f21e661 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -542,17 +542,6 @@ static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
put_device(&sdev->sdev_gendev);
}

-void scsi_next_command(struct scsi_cmnd *cmd)
-{
- struct scsi_device *sdev = cmd->device;
- struct request_queue *q = sdev->request_queue;
-
- scsi_put_command(cmd);
- scsi_run_queue(q);
-
- put_device(&sdev->sdev_gendev);
-}
-
void scsi_run_host_queues(struct Scsi_Host *shost)
{
struct scsi_device *sdev;
@@ -730,8 +719,6 @@ static bool scsi_end_request(struct request *req, int error,
kblockd_schedule_work(&sdev->requeue_work);
else
blk_mq_start_stopped_hw_queues(q, true);
-
- put_device(&sdev->sdev_gendev);
} else {
unsigned long flags;

@@ -742,9 +729,12 @@ static bool scsi_end_request(struct request *req, int error,
if (bidi_bytes)
scsi_release_bidi_buffers(cmd);
scsi_release_buffers(cmd);
- scsi_next_command(cmd);
+
+ scsi_put_command(cmd);
+ scsi_run_queue(q);
}

+ put_device(&sdev->sdev_gendev);
return false;
}

diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 12b8e1b..2a382c1 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -84,7 +84,6 @@ int scsi_noretry_cmd(struct scsi_cmnd *scmd);
extern int scsi_maybe_unblock_host(struct scsi_device *sdev);
extern void scsi_device_unbusy(struct scsi_device *sdev);
extern void scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
-extern void scsi_next_command(struct scsi_cmnd *cmd);
extern void scsi_io_completion(struct scsi_cmnd *, unsigned int);
extern void scsi_run_host_queues(struct Scsi_Host *shost);
extern struct request_queue *scsi_alloc_queue(struct scsi_device *sdev);
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bart Van Assche
2014-10-01 12:06:03 UTC
Permalink
Post by Christoph Hellwig
There's only one caller left, so inline it and reduce the blk-mq vs !blk-mq
diff a litte bit.
---
drivers/scsi/scsi_lib.c | 18 ++++--------------
drivers/scsi/scsi_priv.h | 1 -
2 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8b76231..f21e661 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -542,17 +542,6 @@ static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
put_device(&sdev->sdev_gendev);
}
-void scsi_next_command(struct scsi_cmnd *cmd)
-{
- struct scsi_device *sdev = cmd->device;
- struct request_queue *q = sdev->request_queue;
-
- scsi_put_command(cmd);
- scsi_run_queue(q);
-
- put_device(&sdev->sdev_gendev);
-}
-
void scsi_run_host_queues(struct Scsi_Host *shost)
{
struct scsi_device *sdev;
@@ -730,8 +719,6 @@ static bool scsi_end_request(struct request *req, int error,
kblockd_schedule_work(&sdev->requeue_work);
else
blk_mq_start_stopped_hw_queues(q, true);
-
- put_device(&sdev->sdev_gendev);
} else {
unsigned long flags;
@@ -742,9 +729,12 @@ static bool scsi_end_request(struct request *req, int error,
if (bidi_bytes)
scsi_release_bidi_buffers(cmd);
scsi_release_buffers(cmd);
- scsi_next_command(cmd);
+
+ scsi_put_command(cmd);
+ scsi_run_queue(q);
}
+ put_device(&sdev->sdev_gendev);
return false;
}
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 12b8e1b..2a382c1 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -84,7 +84,6 @@ int scsi_noretry_cmd(struct scsi_cmnd *scmd);
extern int scsi_maybe_unblock_host(struct scsi_device *sdev);
extern void scsi_device_unbusy(struct scsi_device *sdev);
extern void scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
-extern void scsi_next_command(struct scsi_cmnd *cmd);
extern void scsi_io_completion(struct scsi_cmnd *, unsigned int);
extern void scsi_run_host_queues(struct Scsi_Host *shost);
extern struct request_queue *scsi_alloc_queue(struct scsi_device *sdev);
Reviewed-by: Bart Van Assche <***@acm.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-09-07 16:31:04 UTC
Permalink
Now that we are using the split completion model for the legacy request
path as well we can use scsi_mq_free_sgtables unconditionally.

Rename it to scsi_free_sgtables, use it for the legacy path and remove
scsi_release_(bidi_)buffers.

Signed-off-by: Christoph Hellwig <***@lst.de>
---
drivers/scsi/scsi_lib.c | 76 +++++++++++--------------------------------------
1 file changed, 17 insertions(+), 59 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f21e661..0062865 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -621,7 +621,7 @@ static void scsi_uninit_cmd(struct scsi_cmnd *cmd)
}
}

-static void scsi_mq_free_sgtables(struct scsi_cmnd *cmd)
+static void scsi_free_sgtables(struct scsi_cmnd *cmd)
{
if (cmd->sdb.table.nents)
scsi_free_sgtable(&cmd->sdb, true);
@@ -637,7 +637,6 @@ static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
struct Scsi_Host *shost = sdev->host;
unsigned long flags;

- scsi_mq_free_sgtables(cmd);
scsi_uninit_cmd(cmd);

if (shost->use_cmd_list) {
@@ -648,42 +647,6 @@ static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
}
}

-/*
- * Function: scsi_release_buffers()
- *
- * Purpose: Free resources allocate for a scsi_command.
- *
- * Arguments: cmd - command that we are bailing.
- *
- * Lock status: Assumed that no lock is held upon entry.
- *
- * Returns: Nothing
- *
- * Notes: In the event that an upper level driver rejects a
- * command, we must release resources allocated during
- * the __init_io() function. Primarily this would involve
- * the scatter-gather table.
- */
-static void scsi_release_buffers(struct scsi_cmnd *cmd)
-{
- if (cmd->sdb.table.nents)
- scsi_free_sgtable(&cmd->sdb, false);
-
- memset(&cmd->sdb, 0, sizeof(cmd->sdb));
-
- if (scsi_prot_sg_count(cmd))
- scsi_free_sgtable(cmd->prot_sdb, false);
-}
-
-static void scsi_release_bidi_buffers(struct scsi_cmnd *cmd)
-{
- struct scsi_data_buffer *bidi_sdb = cmd->request->next_rq->special;
-
- scsi_free_sgtable(bidi_sdb, false);
- kmem_cache_free(scsi_sdb_cache, bidi_sdb);
- cmd->request->next_rq->special = NULL;
-}
-
static bool scsi_end_request(struct request *req, int error,
unsigned int bytes, unsigned int bidi_bytes)
{
@@ -702,16 +665,14 @@ static bool scsi_end_request(struct request *req, int error,
if (blk_queue_add_random(q))
add_disk_randomness(req->rq_disk);

+ /*
+ * In the MQ case the command gets freed by __blk_mq_end_io, so we have
+ * to do all cleanup that depends on the command before that.
+ */
+ scsi_free_sgtables(cmd);
+
if (req->mq_ctx) {
- /*
- * In the MQ case the command gets freed by __blk_mq_end_io,
- * so we have to do all cleanup that depends on it earlier.
- *
- * We also can't kick the queues from irq context, so we
- * will have to defer it to a workqueue.
- */
scsi_mq_uninit_cmd(cmd);
-
__blk_mq_end_io(req, error);

if (scsi_target(sdev)->single_lun ||
@@ -726,10 +687,6 @@ static bool scsi_end_request(struct request *req, int error,
blk_finish_request(req, error);
spin_unlock_irqrestore(q->queue_lock, flags);

- if (bidi_bytes)
- scsi_release_bidi_buffers(cmd);
- scsi_release_buffers(cmd);
-
scsi_put_command(cmd);
scsi_run_queue(q);
}
@@ -1041,14 +998,13 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
/* Unprep the request and put it back at the head of the queue.
* A new command will be prepared and issued.
*/
+ scsi_free_sgtables(cmd);
if (q->mq_ops) {
cmd->request->cmd_flags &= ~REQ_DONTPREP;
scsi_mq_uninit_cmd(cmd);
scsi_mq_requeue_cmd(cmd);
- } else {
- scsi_release_buffers(cmd);
+ } else
scsi_requeue_command(q, cmd);
- }
break;
case ACTION_RETRY:
/* Retry the same command immediately */
@@ -1149,10 +1105,8 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)

return BLKPREP_OK;
err_exit:
- if (is_mq) {
- scsi_mq_free_sgtables(cmd);
- } else {
- scsi_release_buffers(cmd);
+ scsi_free_sgtables(cmd);
+ if (!is_mq) {
cmd->request->special = NULL;
scsi_put_command(cmd);
put_device(&sdev->sdev_gendev);
@@ -1322,7 +1276,9 @@ scsi_prep_return(struct request_queue *q, struct request *req, int ret)
/* release the command and kill it */
if (req->special) {
struct scsi_cmnd *cmd = req->special;
- scsi_release_buffers(cmd);
+
+ scsi_free_sgtables(cmd);
+
scsi_put_command(cmd);
put_device(&sdev->sdev_gendev);
req->special = NULL;
@@ -1912,8 +1868,10 @@ out:
* we hit an error, as we will never see this command
* again.
*/
- if (req->cmd_flags & REQ_DONTPREP)
+ if (req->cmd_flags & REQ_DONTPREP) {
+ scsi_free_sgtables(cmd);
scsi_mq_uninit_cmd(cmd);
+ }
break;
default:
break;
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bart Van Assche
2014-10-01 12:22:18 UTC
Permalink
Post by Christoph Hellwig
-static void scsi_release_buffers(struct scsi_cmnd *cmd)
-{
- if (cmd->sdb.table.nents)
- scsi_free_sgtable(&cmd->sdb, false);
-
- memset(&cmd->sdb, 0, sizeof(cmd->sdb));
-
- if (scsi_prot_sg_count(cmd))
- scsi_free_sgtable(cmd->prot_sdb, false);
-}
-
-static void scsi_release_bidi_buffers(struct scsi_cmnd *cmd)
-{
- struct scsi_data_buffer *bidi_sdb = cmd->request->next_rq->special;
-
- scsi_free_sgtable(bidi_sdb, false);
- kmem_cache_free(scsi_sdb_cache, bidi_sdb);
- cmd->request->next_rq->special = NULL;
-}
I can see that the scsi_free_sgtable(&cmd->sdb, false) call, the
scsi_free_sgtable(cmd->prot_sdb, false) call and the
scsi_free_sgtable(bidi_sdb, false) call are now performed by the new
function scsi_free_sgtables(). But what's not clear to me is to which
function the kmem_cache_free(scsi_sdb_cache, bidi_sdb) call has been moved ?
Post by Christoph Hellwig
+ scsi_free_sgtables(cmd);
if (q->mq_ops) {
cmd->request->cmd_flags &= ~REQ_DONTPREP;
scsi_mq_uninit_cmd(cmd);
scsi_mq_requeue_cmd(cmd);
- } else {
- scsi_release_buffers(cmd);
+ } else
scsi_requeue_command(q, cmd);
- }
Apparently braces have been removed from around the else-part. Isn't the
preferred style to use braces around single-line else-clauses if the
if-clause consists of multiple statements ?

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-10-01 21:05:51 UTC
Permalink
Post by Bart Van Assche
I can see that the scsi_free_sgtable(&cmd->sdb, false) call, the
scsi_free_sgtable(cmd->prot_sdb, false) call and the
scsi_free_sgtable(bidi_sdb, false) call are now performed by the new
function scsi_free_sgtables(). But what's not clear to me is to which
function the kmem_cache_free(scsi_sdb_cache, bidi_sdb) call has been moved
Seems like we lost if for the !mq case, oops.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-09-07 16:31:06 UTC
Permalink
scsi_lib.c is where the rest of the I/O submission path lives, so move
scsi_dispatch_cmd there and mark it static.

Signed-off-by: Christoph Hellwig <***@lst.de>
---
drivers/scsi/scsi.c | 81 ------------------------------------------------
drivers/scsi/scsi_lib.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/scsi/scsi_priv.h | 1 -
3 files changed, 81 insertions(+), 82 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 534bf26..a86ccb7 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -626,87 +626,6 @@ void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
EXPORT_SYMBOL(scsi_cmd_get_serial);

/**
- * scsi_dispatch_command - Dispatch a command to the low-level driver.
- * @cmd: command block we are dispatching.
- *
- * Return: nonzero return request was rejected and device's queue needs to be
- * plugged.
- */
-int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
-{
- struct Scsi_Host *host = cmd->device->host;
- int rtn = 0;
-
- atomic_inc(&cmd->device->iorequest_cnt);
-
- /* check if the device is still usable */
- if (unlikely(cmd->device->sdev_state == SDEV_DEL)) {
- /* in SDEV_DEL we error all commands. DID_NO_CONNECT
- * returns an immediate error upwards, and signals
- * that the device is no longer present */
- cmd->result = DID_NO_CONNECT << 16;
- goto done;
- }
-
- /* Check to see if the scsi lld made this device blocked. */
- if (unlikely(scsi_device_blocked(cmd->device))) {
- /*
- * in blocked state, the command is just put back on
- * the device queue. The suspend state has already
- * blocked the queue so future requests should not
- * occur until the device transitions out of the
- * suspend state.
- */
- SCSI_LOG_MLQUEUE(3, scmd_printk(KERN_INFO, cmd,
- "queuecommand : device blocked\n"));
- return SCSI_MLQUEUE_DEVICE_BUSY;
- }
-
- /* Store the LUN value in cmnd, if needed. */
- if (cmd->device->lun_in_cdb)
- cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
- (cmd->device->lun << 5 & 0xe0);
-
- scsi_log_send(cmd);
-
- /*
- * Before we queue this command, check if the command
- * length exceeds what the host adapter can handle.
- */
- if (cmd->cmd_len > cmd->device->host->max_cmd_len) {
- SCSI_LOG_MLQUEUE(3, scmd_printk(KERN_INFO, cmd,
- "queuecommand : command too long. "
- "cdb_size=%d host->max_cmd_len=%d\n",
- cmd->cmd_len, cmd->device->host->max_cmd_len));
- cmd->result = (DID_ABORT << 16);
- goto done;
- }
-
- if (unlikely(host->shost_state == SHOST_DEL)) {
- cmd->result = (DID_NO_CONNECT << 16);
- goto done;
-
- }
-
- trace_scsi_dispatch_cmd_start(cmd);
- rtn = host->hostt->queuecommand(host, cmd);
- if (rtn) {
- trace_scsi_dispatch_cmd_error(cmd, rtn);
- if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&
- rtn != SCSI_MLQUEUE_TARGET_BUSY)
- rtn = SCSI_MLQUEUE_HOST_BUSY;
-
- SCSI_LOG_MLQUEUE(3, scmd_printk(KERN_INFO, cmd,
- "queuecommand : request rejected\n"));
- }
-
- return rtn;
- done:
- cmd->scsi_done(cmd);
- return 0;
-}
-
-/**
* scsi_finish_command - cleanup and pass command back to upper layer
* @cmd: the command
*
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 877ea3d..c398343 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1581,6 +1581,87 @@ static void scsi_softirq_done(struct request *rq)
}

/**
+ * scsi_dispatch_command - Dispatch a command to the low-level driver.
+ * @cmd: command block we are dispatching.
+ *
+ * Return: nonzero return request was rejected and device's queue needs to be
+ * plugged.
+ */
+static int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
+{
+ struct Scsi_Host *host = cmd->device->host;
+ int rtn = 0;
+
+ atomic_inc(&cmd->device->iorequest_cnt);
+
+ /* check if the device is still usable */
+ if (unlikely(cmd->device->sdev_state == SDEV_DEL)) {
+ /* in SDEV_DEL we error all commands. DID_NO_CONNECT
+ * returns an immediate error upwards, and signals
+ * that the device is no longer present */
+ cmd->result = DID_NO_CONNECT << 16;
+ goto done;
+ }
+
+ /* Check to see if the scsi lld made this device blocked. */
+ if (unlikely(scsi_device_blocked(cmd->device))) {
+ /*
+ * in blocked state, the command is just put back on
+ * the device queue. The suspend state has already
+ * blocked the queue so future requests should not
+ * occur until the device transitions out of the
+ * suspend state.
+ */
+ SCSI_LOG_MLQUEUE(3, scmd_printk(KERN_INFO, cmd,
+ "queuecommand : device blocked\n"));
+ return SCSI_MLQUEUE_DEVICE_BUSY;
+ }
+
+ /* Store the LUN value in cmnd, if needed. */
+ if (cmd->device->lun_in_cdb)
+ cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) |
+ (cmd->device->lun << 5 & 0xe0);
+
+ scsi_log_send(cmd);
+
+ /*
+ * Before we queue this command, check if the command
+ * length exceeds what the host adapter can handle.
+ */
+ if (cmd->cmd_len > cmd->device->host->max_cmd_len) {
+ SCSI_LOG_MLQUEUE(3, scmd_printk(KERN_INFO, cmd,
+ "queuecommand : command too long. "
+ "cdb_size=%d host->max_cmd_len=%d\n",
+ cmd->cmd_len, cmd->device->host->max_cmd_len));
+ cmd->result = (DID_ABORT << 16);
+ goto done;
+ }
+
+ if (unlikely(host->shost_state == SHOST_DEL)) {
+ cmd->result = (DID_NO_CONNECT << 16);
+ goto done;
+
+ }
+
+ trace_scsi_dispatch_cmd_start(cmd);
+ rtn = host->hostt->queuecommand(host, cmd);
+ if (rtn) {
+ trace_scsi_dispatch_cmd_error(cmd, rtn);
+ if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&
+ rtn != SCSI_MLQUEUE_TARGET_BUSY)
+ rtn = SCSI_MLQUEUE_HOST_BUSY;
+
+ SCSI_LOG_MLQUEUE(3, scmd_printk(KERN_INFO, cmd,
+ "queuecommand : request rejected\n"));
+ }
+
+ return rtn;
+ done:
+ cmd->scsi_done(cmd);
+ return 0;
+}
+
+/**
* scsi_done - Invoke completion on finished SCSI command.
* @cmd: The SCSI Command for which a low-level device driver (LLDD) gives
* ownership back to SCSI Core -- i.e. the LLDD has finished with it.
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 2a382c1..2dc4a83 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -29,7 +29,6 @@ extern int scsi_init_hosts(void);
extern void scsi_exit_hosts(void);

/* scsi.c */
-extern int scsi_dispatch_cmd(struct scsi_cmnd *cmd);
extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
#ifdef CONFIG_SCSI_LOGGING
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bart Van Assche
2014-10-01 12:30:20 UTC
Permalink
Post by Christoph Hellwig
scsi_lib.c is where the rest of the I/O submission path lives, so move
scsi_dispatch_cmd there and mark it static.
Reviewed-by: Bart Van Assche <***@acm.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-09-07 16:31:05 UTC
Permalink
There is no reason for ULDs to pass in a flag on how to allocate the S/G
lists. While we don't need GFP_ATOMIC for the blk-mq case because we
don't hold locks, that decision can be made way down the chain without
having to pass a pointless gfp_mask argument.

Signed-off-by: Christoph Hellwig <***@lst.de>
---
drivers/scsi/scsi_lib.c | 20 +++++++++-----------
drivers/scsi/sd.c | 6 +++---
drivers/scsi/sr.c | 2 +-
include/scsi/scsi_cmnd.h | 2 +-
4 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0062865..877ea3d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -587,10 +587,10 @@ static void scsi_free_sgtable(struct scsi_data_buffer *sdb, bool mq)
__sg_free_table(&sdb->table, SCSI_MAX_SG_SEGMENTS, mq, scsi_sg_free);
}

-static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb, int nents,
- gfp_t gfp_mask, bool mq)
+static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb, int nents, bool mq)
{
struct scatterlist *first_chunk = NULL;
+ gfp_t gfp_mask = mq ? GFP_NOIO : GFP_ATOMIC;
int ret;

BUG_ON(!nents);
@@ -1017,8 +1017,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
}
}

-static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
- gfp_t gfp_mask)
+static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb)
{
int count;

@@ -1026,7 +1025,7 @@ static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
* If sg table allocation fails, requeue request later.
*/
if (unlikely(scsi_alloc_sgtable(sdb, req->nr_phys_segments,
- gfp_mask, req->mq_ctx != NULL)))
+ req->mq_ctx != NULL)))
return BLKPREP_DEFER;

/*
@@ -1051,7 +1050,7 @@ static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
* BLKPREP_DEFER if the failure is retryable
* BLKPREP_KILL if the failure is fatal
*/
-int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+int scsi_init_io(struct scsi_cmnd *cmd)
{
struct scsi_device *sdev = cmd->device;
struct request *rq = cmd->request;
@@ -1060,7 +1059,7 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)

BUG_ON(!rq->nr_phys_segments);

- error = scsi_init_sgtable(rq, &cmd->sdb, gfp_mask);
+ error = scsi_init_sgtable(rq, &cmd->sdb);
if (error)
goto err_exit;

@@ -1076,8 +1075,7 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
rq->next_rq->special = bidi_sdb;
}

- error = scsi_init_sgtable(rq->next_rq, rq->next_rq->special,
- GFP_ATOMIC);
+ error = scsi_init_sgtable(rq->next_rq, rq->next_rq->special);
if (error)
goto err_exit;
}
@@ -1089,7 +1087,7 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
BUG_ON(prot_sdb == NULL);
ivecs = blk_rq_count_integrity_sg(rq->q, rq->bio);

- if (scsi_alloc_sgtable(prot_sdb, ivecs, gfp_mask, is_mq)) {
+ if (scsi_alloc_sgtable(prot_sdb, ivecs, is_mq)) {
error = BLKPREP_DEFER;
goto err_exit;
}
@@ -1156,7 +1154,7 @@ static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
* submit a request without an attached bio.
*/
if (req->bio) {
- int ret = scsi_init_io(cmd, GFP_ATOMIC);
+ int ret = scsi_init_io(cmd);
if (unlikely(ret))
return ret;
} else {
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index aa43496..6f7588d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -769,7 +769,7 @@ static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
* amount of blocks described by the request.
*/
blk_add_request_payload(rq, page, len);
- ret = scsi_init_io(cmd, GFP_ATOMIC);
+ ret = scsi_init_io(cmd);
rq->__data_len = nr_bytes;

out:
@@ -863,7 +863,7 @@ static int sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
* knows how much to actually write.
*/
rq->__data_len = sdp->sector_size;
- ret = scsi_init_io(cmd, GFP_ATOMIC);
+ ret = scsi_init_io(cmd);
rq->__data_len = nr_bytes;
return ret;
}
@@ -896,7 +896,7 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
int ret, host_dif;
unsigned char protect;

- ret = scsi_init_io(SCpnt, GFP_ATOMIC);
+ ret = scsi_init_io(SCpnt);
if (ret != BLKPREP_OK)
goto out;
SCpnt = rq->special;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7eeb936..5d1e3ac 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -387,7 +387,7 @@ static int sr_init_command(struct scsi_cmnd *SCpnt)
struct request *rq = SCpnt->request;
int ret;

- ret = scsi_init_io(SCpnt, GFP_ATOMIC);
+ ret = scsi_init_io(SCpnt);
if (ret != BLKPREP_OK)
goto out;
SCpnt = rq->special;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 73f3490..fb0a848 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -157,7 +157,7 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
size_t *offset, size_t *len);
extern void scsi_kunmap_atomic_sg(void *virt);

-extern int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask);
+extern int scsi_init_io(struct scsi_cmnd *cmd);

extern int scsi_dma_map(struct scsi_cmnd *cmd);
extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bart Van Assche
2014-10-01 12:28:58 UTC
Permalink
Post by Christoph Hellwig
There is no reason for ULDs to pass in a flag on how to allocate the S/G
lists. While we don't need GFP_ATOMIC for the blk-mq case because we
don't hold locks, that decision can be made way down the chain without
having to pass a pointless gfp_mask argument.
Reviewed-by: Bart Van Assche <***@acm.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-09-07 16:31:07 UTC
Permalink
Move a bit code out of scsi_io_completion and into scsi_requeue_command
in preparation for further refactoring.

Signed-off-by: Christoph Hellwig <***@lst.de>
---
drivers/scsi/scsi_lib.c | 85 ++++++++++++++++++++++---------------------------
1 file changed, 38 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c398343..2221bf1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -506,42 +506,6 @@ void scsi_requeue_run_queue(struct work_struct *work)
scsi_run_queue(q);
}

-/*
- * Function: scsi_requeue_command()
- *
- * Purpose: Handle post-processing of completed commands.
- *
- * Arguments: q - queue to operate on
- * cmd - command that may need to be requeued.
- *
- * Returns: Nothing
- *
- * Notes: After command completion, there may be blocks left
- * over which weren't finished by the previous command
- * this can be for a number of reasons - the main one is
- * I/O errors in the middle of the request, in which case
- * we need to request the blocks that come after the bad
- * sector.
- * Notes: Upon return, cmd is a stale pointer.
- */
-static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
-{
- struct scsi_device *sdev = cmd->device;
- struct request *req = cmd->request;
- unsigned long flags;
-
- spin_lock_irqsave(q->queue_lock, flags);
- blk_unprep_request(req);
- req->special = NULL;
- scsi_put_command(cmd);
- blk_requeue_request(q, req);
- spin_unlock_irqrestore(q->queue_lock, flags);
-
- scsi_run_queue(q);
-
- put_device(&sdev->sdev_gendev);
-}
-
void scsi_run_host_queues(struct Scsi_Host *shost)
{
struct scsi_device *sdev;
@@ -647,6 +611,43 @@ static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
}
}

+/**
+ * scsi_requeue_command - requeue a command from the I/O completion path.
+ * @cmd : command that needs to be requeued
+ *
+ * After command completion, there may be blocks left over which weren't
+ * finished by the previous command this can be for a number of reasons -
+ * the main one is I/O errors in the middle of the request, in which case
+ * we need to request the blocks that come after the bad sector.
+ */
+static void scsi_requeue_command(struct scsi_cmnd *cmd)
+{
+ struct request *req = cmd->request;
+ struct request_queue *q = req->q;
+
+ scsi_free_sgtables(cmd);
+
+ if (q->mq_ops) {
+ cmd->request->cmd_flags &= ~REQ_DONTPREP;
+ scsi_mq_uninit_cmd(cmd);
+ scsi_mq_requeue_cmd(cmd);
+ } else {
+ struct scsi_device *sdev = cmd->device;
+ unsigned long flags;
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ blk_unprep_request(req);
+ req->special = NULL;
+ scsi_put_command(cmd);
+ blk_requeue_request(q, req);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ scsi_run_queue(q);
+
+ put_device(&sdev->sdev_gendev);
+ }
+}
+
static bool scsi_end_request(struct request *req, int error,
unsigned int bytes, unsigned int bidi_bytes)
{
@@ -773,7 +774,6 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
{
int result = cmd->result;
- struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
int error = 0;
struct scsi_sense_hdr sshdr;
@@ -995,16 +995,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
/*FALLTHRU*/
case ACTION_REPREP:
requeue:
- /* Unprep the request and put it back at the head of the queue.
- * A new command will be prepared and issued.
- */
- scsi_free_sgtables(cmd);
- if (q->mq_ops) {
- cmd->request->cmd_flags &= ~REQ_DONTPREP;
- scsi_mq_uninit_cmd(cmd);
- scsi_mq_requeue_cmd(cmd);
- } else
- scsi_requeue_command(q, cmd);
+ scsi_requeue_command(cmd);
break;
case ACTION_RETRY:
/* Retry the same command immediately */
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bart Van Assche
2014-10-01 12:33:48 UTC
Permalink
Post by Christoph Hellwig
Move a bit code out of scsi_io_completion and into scsi_requeue_command
in preparation for further refactoring.
Reviewed-by: Bart Van Assche <***@acm.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-09-07 16:31:08 UTC
Permalink
Move the error handling path out of scsi_io_completion and into an
out of line helper.

Signed-off-by: Christoph Hellwig <***@lst.de>
---
drivers/scsi/scsi_lib.c | 263 +++++++++++++++++++++++++-----------------------
1 file changed, 136 insertions(+), 127 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 2221bf1..cc5d404 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -742,6 +742,132 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
return error;
}

+static noinline bool
+scsi_handle_ioerror(struct scsi_cmnd *cmd, int result,
+ struct scsi_sense_hdr *sshdr)
+{
+ struct request *req = cmd->request;
+ unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
+ int error = 0;
+ enum {
+ ACTION_FAIL,
+ ACTION_REPREP,
+ ACTION_RETRY,
+ ACTION_DELAYED_RETRY,
+ } action = ACTION_FAIL;
+
+ error = __scsi_error_from_host_byte(cmd, result);
+
+ if (host_byte(result) == DID_RESET) {
+ /* Third party bus reset or reset for error recovery
+ * reasons. Just retry the command and see what
+ * happens.
+ */
+ action = ACTION_RETRY;
+ } else if (sshdr) {
+ switch (sshdr->sense_key) {
+ case UNIT_ATTENTION:
+ if (cmd->device->removable) {
+ /* Detected disc change. Set a bit
+ * and quietly refuse further access.
+ */
+ cmd->device->changed = 1;
+ } else {
+ /* Must have been a power glitch, or a
+ * bus reset. Could not have been a
+ * media change, so we just retry the
+ * command and see what happens.
+ */
+ action = ACTION_RETRY;
+ }
+ break;
+ case ILLEGAL_REQUEST:
+ /* If we had an ILLEGAL REQUEST returned, then
+ * we may have performed an unsupported
+ * command. The only thing this should be
+ * would be a ten byte read where only a six
+ * byte read was supported. Also, on a system
+ * where READ CAPACITY failed, we may have
+ * read past the end of the disk.
+ */
+ if ((cmd->device->use_10_for_rw &&
+ sshdr->asc == 0x20 && sshdr->ascq == 0x00) &&
+ (cmd->cmnd[0] == READ_10 ||
+ cmd->cmnd[0] == WRITE_10)) {
+ /* This will issue a new 6-byte command. */
+ cmd->device->use_10_for_rw = 0;
+ action = ACTION_REPREP;
+ } else if (sshdr->asc == 0x10) /* DIX */ {
+ error = -EILSEQ;
+ /* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
+ } else if (sshdr->asc == 0x20 || sshdr->asc == 0x24) {
+ error = -EREMOTEIO;
+ }
+ break;
+ case ABORTED_COMMAND:
+ if (sshdr->asc == 0x10) /* DIF */
+ error = -EILSEQ;
+ break;
+ case NOT_READY:
+ /* If the device is in the process of becoming
+ * ready, or has a temporary blockage, retry.
+ */
+ if (sshdr->asc == 0x04) {
+ switch (sshdr->ascq) {
+ case 0x01: /* becoming ready */
+ case 0x04: /* format in progress */
+ case 0x05: /* rebuild in progress */
+ case 0x06: /* recalculation in progress */
+ case 0x07: /* operation in progress */
+ case 0x08: /* Long write in progress */
+ case 0x09: /* self test in progress */
+ case 0x14: /* space allocation in progress */
+ action = ACTION_DELAYED_RETRY;
+ break;
+ default:
+ break;
+ }
+ }
+ break;
+ case VOLUME_OVERFLOW:
+ /* See SSC3rXX or current. */
+ break;
+ default:
+ break;
+ }
+ }
+
+ if (action != ACTION_FAIL &&
+ time_before(cmd->jiffies_at_alloc + wait_for, jiffies))
+ action = ACTION_FAIL;
+
+ switch (action) {
+ case ACTION_FAIL:
+ /* Give up and fail the remainder of the request */
+ if (!(req->cmd_flags & REQ_QUIET)) {
+ scsi_print_result(cmd);
+ if (driver_byte(result) & DRIVER_SENSE)
+ scsi_print_sense("", cmd);
+ scsi_print_command(cmd);
+ }
+ if (!scsi_end_request(req, error, blk_rq_err_bytes(req), 0))
+ break;
+ /*FALLTHRU*/
+ case ACTION_REPREP:
+ return false;
+ case ACTION_RETRY:
+ /* Retry the same command immediately */
+ __scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY, 0);
+ break;
+ case ACTION_DELAYED_RETRY:
+ /* Retry the same command after a delay */
+ __scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, 0);
+ break;
+ }
+
+ return true;
+}
+
/*
* Function: scsi_io_completion()
*
@@ -779,9 +905,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
struct scsi_sense_hdr sshdr;
int sense_valid = 0;
int sense_deferred = 0;
- enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
- ACTION_DELAYED_RETRY} action;
- unsigned long wait_for = (cmd->allowed + 1) * req->timeout;

if (result) {
sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
@@ -867,7 +990,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
/*
* If we finished all bytes in the request we are done now.
*/
- if (!scsi_end_request(req, error, good_bytes, 0))
+ if (likely(!scsi_end_request(req, error, good_bytes, 0)))
return;

/*
@@ -880,132 +1003,18 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
}

/*
- * If there had been no error, but we have leftover bytes in the
- * requeues just queue the command up again.
+ * Try to handle errors if we got a non-zero result.
*/
- if (result == 0)
- goto requeue;
-
- error = __scsi_error_from_host_byte(cmd, result);
-
- if (host_byte(result) == DID_RESET) {
- /* Third party bus reset or reset for error recovery
- * reasons. Just retry the command and see what
- * happens.
- */
- action = ACTION_RETRY;
- } else if (sense_valid && !sense_deferred) {
- switch (sshdr.sense_key) {
- case UNIT_ATTENTION:
- if (cmd->device->removable) {
- /* Detected disc change. Set a bit
- * and quietly refuse further access.
- */
- cmd->device->changed = 1;
- action = ACTION_FAIL;
- } else {
- /* Must have been a power glitch, or a
- * bus reset. Could not have been a
- * media change, so we just retry the
- * command and see what happens.
- */
- action = ACTION_RETRY;
- }
- break;
- case ILLEGAL_REQUEST:
- /* If we had an ILLEGAL REQUEST returned, then
- * we may have performed an unsupported
- * command. The only thing this should be
- * would be a ten byte read where only a six
- * byte read was supported. Also, on a system
- * where READ CAPACITY failed, we may have
- * read past the end of the disk.
- */
- if ((cmd->device->use_10_for_rw &&
- sshdr.asc == 0x20 && sshdr.ascq == 0x00) &&
- (cmd->cmnd[0] == READ_10 ||
- cmd->cmnd[0] == WRITE_10)) {
- /* This will issue a new 6-byte command. */
- cmd->device->use_10_for_rw = 0;
- action = ACTION_REPREP;
- } else if (sshdr.asc == 0x10) /* DIX */ {
- action = ACTION_FAIL;
- error = -EILSEQ;
- /* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
- } else if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
- action = ACTION_FAIL;
- error = -EREMOTEIO;
- } else
- action = ACTION_FAIL;
- break;
- case ABORTED_COMMAND:
- action = ACTION_FAIL;
- if (sshdr.asc == 0x10) /* DIF */
- error = -EILSEQ;
- break;
- case NOT_READY:
- /* If the device is in the process of becoming
- * ready, or has a temporary blockage, retry.
- */
- if (sshdr.asc == 0x04) {
- switch (sshdr.ascq) {
- case 0x01: /* becoming ready */
- case 0x04: /* format in progress */
- case 0x05: /* rebuild in progress */
- case 0x06: /* recalculation in progress */
- case 0x07: /* operation in progress */
- case 0x08: /* Long write in progress */
- case 0x09: /* self test in progress */
- case 0x14: /* space allocation in progress */
- action = ACTION_DELAYED_RETRY;
- break;
- default:
- action = ACTION_FAIL;
- break;
- }
- } else
- action = ACTION_FAIL;
- break;
- case VOLUME_OVERFLOW:
- /* See SSC3rXX or current. */
- action = ACTION_FAIL;
- break;
- default:
- action = ACTION_FAIL;
- break;
- }
- } else
- action = ACTION_FAIL;
-
- if (action != ACTION_FAIL &&
- time_before(cmd->jiffies_at_alloc + wait_for, jiffies))
- action = ACTION_FAIL;
-
- switch (action) {
- case ACTION_FAIL:
- /* Give up and fail the remainder of the request */
- if (!(req->cmd_flags & REQ_QUIET)) {
- scsi_print_result(cmd);
- if (driver_byte(result) & DRIVER_SENSE)
- scsi_print_sense("", cmd);
- scsi_print_command(cmd);
- }
- if (!scsi_end_request(req, error, blk_rq_err_bytes(req), 0))
+ if (result != 0) {
+ if (scsi_handle_ioerror(cmd, result,
+ sense_valid && !sense_deferred ? &sshdr : NULL))
return;
- /*FALLTHRU*/
- case ACTION_REPREP:
- requeue:
- scsi_requeue_command(cmd);
- break;
- case ACTION_RETRY:
- /* Retry the same command immediately */
- __scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY, 0);
- break;
- case ACTION_DELAYED_RETRY:
- /* Retry the same command after a delay */
- __scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, 0);
- break;
}
+
+ /*
+ * Queue up the leftovers again.
+ */
+ scsi_requeue_command(cmd);
}

static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb)
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bart Van Assche
2014-10-01 12:46:35 UTC
Permalink
Post by Christoph Hellwig
Move the error handling path out of scsi_io_completion and into an
out of line helper.
---
drivers/scsi/scsi_lib.c | 263 +++++++++++++++++++++++++-----------------------
1 file changed, 136 insertions(+), 127 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 2221bf1..cc5d404 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -742,6 +742,132 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
return error;
}
+static noinline bool
+scsi_handle_ioerror(struct scsi_cmnd *cmd, int result,
+ struct scsi_sense_hdr *sshdr)
Please document the meaning of the return value of this function and
also why it has been marked noinline. Otherwise I'm fine with this patch.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-09-07 16:31:09 UTC
Permalink
Signed-off-by: Christoph Hellwig <***@lst.de>
---
Documentation/scsi/scsi_eh.txt | 12 +++---
drivers/scsi/scsi.c | 58 -----------------------------
drivers/scsi/scsi_lib.c | 84 +++++++++++++++++++++++++++---------------
drivers/scsi/scsi_priv.h | 1 -
4 files changed, 59 insertions(+), 96 deletions(-)

diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index a0c8511..8a7ac48 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -57,13 +57,11 @@ looks at the scmd->result value and sense data to determine what to do
with the command.

- SUCCESS
- scsi_finish_command() is invoked for the command. The
- function does some maintenance chores and then calls
- scsi_io_completion() to finish the I/O.
- scsi_io_completion() then notifies the block layer on
- the completed request by calling blk_end_request and
- friends or figures out what to do with the remainder
- of the data in case of an error.
+ scsi_finish_command() is invoked for the command. The function
+ does some maintenance chores and then notifies the block layer on
+ the completed request by calling blk_end_request / __blk_mq_end_io
+ or figures out what to do with the remainder of the data in case
+ of an error.

- NEEDS_RETRY
- ADD_TO_MLQUEUE
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index a86ccb7..cdc0686 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -626,64 +626,6 @@ void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
EXPORT_SYMBOL(scsi_cmd_get_serial);

/**
- * scsi_finish_command - cleanup and pass command back to upper layer
- * @cmd: the command
- *
- * Description: Pass command off to upper layer for finishing of I/O
- * request, waking processes that are waiting on results,
- * etc.
- */
-void scsi_finish_command(struct scsi_cmnd *cmd)
-{
- struct scsi_device *sdev = cmd->device;
- struct scsi_target *starget = scsi_target(sdev);
- struct Scsi_Host *shost = sdev->host;
- struct scsi_driver *drv;
- unsigned int good_bytes;
-
- scsi_device_unbusy(sdev);
-
- /*
- * Clear the flags that say that the device/target/host is no longer
- * capable of accepting new commands.
- */
- if (atomic_read(&shost->host_blocked))
- atomic_set(&shost->host_blocked, 0);
- if (atomic_read(&starget->target_blocked))
- atomic_set(&starget->target_blocked, 0);
- if (atomic_read(&sdev->device_blocked))
- atomic_set(&sdev->device_blocked, 0);
-
- /*
- * If we have valid sense information, then some kind of recovery
- * must have taken place. Make a note of this.
- */
- if (SCSI_SENSE_VALID(cmd))
- cmd->result |= (DRIVER_SENSE << 24);
-
- SCSI_LOG_MLCOMPLETE(4, sdev_printk(KERN_INFO, sdev,
- "Notifying upper driver of completion "
- "(result %x)\n", cmd->result));
-
- good_bytes = scsi_bufflen(cmd);
- if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
- int old_good_bytes = good_bytes;
- drv = scsi_cmd_to_driver(cmd);
- if (drv->done)
- good_bytes = drv->done(cmd);
- /*
- * USB may not give sense identifying bad sector and
- * simply return a residue instead, so subtract off the
- * residue if drv->done() error processing indicates no
- * change to the completion length.
- */
- if (good_bytes == old_good_bytes)
- good_bytes -= scsi_get_resid(cmd);
- }
- scsi_io_completion(cmd, good_bytes);
-}
-
-/**
* scsi_adjust_queue_depth - Let low level drivers change a device's queue depth
* @sdev: SCSI Device in question
* @tagged: Do we use tagged queueing (non-0) or do we treat
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index cc5d404..e0eb809 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -868,44 +868,68 @@ scsi_handle_ioerror(struct scsi_cmnd *cmd, int result,
return true;
}

-/*
- * Function: scsi_io_completion()
- *
- * Purpose: Completion processing for block device I/O requests.
- *
- * Arguments: cmd - command that is finished.
- *
- * Lock status: Assumed that no lock is held upon entry.
- *
- * Returns: Nothing
- *
- * Notes: We will finish off the specified number of sectors. If we
- * are done, the command block will be released and the queue
- * function will be goosed. If we are not done then we have to
- * figure out what to do next:
- *
- * a) We can call scsi_requeue_command(). The request
- * will be unprepared and put back on the queue. Then
- * a new command will be created for it. This should
- * be used if we made forward progress, or if we want
- * to switch from READ(10) to READ(6) for example.
- *
- * b) We can call __scsi_queue_insert(). The request will
- * be put back on the queue and retried using the same
- * command as before, possibly after a delay.
+/**
+ * scsi_finish_command - handle I/O completion on a command
+ * @cmd: command to complete
*
- * c) We can call scsi_end_request() with -EIO to fail
- * the remainder of the request.
+ * Finish off the number of bytes returned by the driver. If the whole command
+ * has been completed return it to the block layer. If it hasn't figure out
+ * what to do based on the result from the driver and the sense code.
*/
-void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
+void scsi_finish_command(struct scsi_cmnd *cmd)
{
- int result = cmd->result;
struct request *req = cmd->request;
- int error = 0;
+ struct scsi_device *sdev = cmd->device;
+ struct scsi_target *starget = scsi_target(sdev);
+ struct Scsi_Host *shost = sdev->host;
struct scsi_sense_hdr sshdr;
+ struct scsi_driver *drv;
+ unsigned int good_bytes;
int sense_valid = 0;
int sense_deferred = 0;
+ int error = 0, result;
+
+ scsi_device_unbusy(sdev);
+
+ /*
+ * Clear the flags that say that the device/target/host is no longer
+ * capable of accepting new commands.
+ */
+ if (atomic_read(&shost->host_blocked))
+ atomic_set(&shost->host_blocked, 0);
+ if (atomic_read(&starget->target_blocked))
+ atomic_set(&starget->target_blocked, 0);
+ if (atomic_read(&sdev->device_blocked))
+ atomic_set(&sdev->device_blocked, 0);
+
+ /*
+ * If we have valid sense information, then some kind of recovery
+ * must have taken place. Make a note of this.
+ */
+ if (SCSI_SENSE_VALID(cmd))
+ cmd->result |= (DRIVER_SENSE << 24);
+
+ SCSI_LOG_MLCOMPLETE(4, sdev_printk(KERN_INFO, sdev,
+ "Notifying upper driver of completion "
+ "(result %x)\n", cmd->result));
+
+ good_bytes = scsi_bufflen(cmd);
+ if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+ int old_good_bytes = good_bytes;
+ drv = scsi_cmd_to_driver(cmd);
+ if (drv->done)
+ good_bytes = drv->done(cmd);
+ /*
+ * USB may not give sense identifying bad sector and
+ * simply return a residue instead, so subtract off the
+ * residue if drv->done() error processing indicates no
+ * change to the completion length.
+ */
+ if (good_bytes == old_good_bytes)
+ good_bytes -= scsi_get_resid(cmd);
+ }

+ result = cmd->result;
if (result) {
sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
if (sense_valid)
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 2dc4a83..87c6c5a 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -83,7 +83,6 @@ int scsi_noretry_cmd(struct scsi_cmnd *scmd);
extern int scsi_maybe_unblock_host(struct scsi_device *sdev);
extern void scsi_device_unbusy(struct scsi_device *sdev);
extern void scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
-extern void scsi_io_completion(struct scsi_cmnd *, unsigned int);
extern void scsi_run_host_queues(struct Scsi_Host *shost);
extern struct request_queue *scsi_alloc_queue(struct scsi_device *sdev);
extern struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev);
--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bart Van Assche
2014-10-01 12:55:33 UTC
Permalink
[ ... ]
Instead of having two functions that each fit on a regular monitor we
have now one large function that does not fit on most monitors. Is it
really necessary to merge these two functions ? Has it been considered
instead of merging these two functions to manually move both functions
to the same source file such that the compiler can do the inlining ?

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bart Van Assche
2014-09-08 07:22:40 UTC
Permalink
Post by Christoph Hellwig
This series cleans up a couple of lose ends I noticed during the scsi-mq
work, but which weren't important enough to address during the last cycle.
Hello Christoph,

Is there perhaps a public git tree available with this patch series ? I
have tried to apply these patches on top of v3.17-rc4 but unfortunately
without success:
$ git checkout v3.17-rc4 -b scsi-core-cleanup
$ for p in \[PATCH\ *; do echo ==== $p && git am -C1 "$p" || break; done
==== [PATCH 1_8] scsi: don't use scsi_next_command in
scsi_reset_provider - Christoph Hellwig <***@lst.de> - 2014-09-07 1831.eml
Applying: scsi: don't use scsi_next_command in scsi_reset_provider
==== [PATCH 2_8] scsi: remove scsi_next_command - Christoph Hellwig
<***@lst.de> - 2014-09-07 1831.eml
Applying: scsi: remove scsi_next_command
==== [PATCH 3_8] scsi: clean up S_G table freeing - Christoph Hellwig
<***@lst.de> - 2014-09-07 1831.eml
Applying: scsi: clean up S/G table freeing
/home/bart/software/linux-kernel/.git/rebase-apply/patch:138: trailing
whitespace.

Context reduced to (1/1) to apply fragment at 640
Context reduced to (2/2) to apply fragment at 646
warning: 1 line adds whitespace errors.
==== [PATCH 4_8] scsi: stop passing a gfp_mask argument down the command
setup path - Christoph Hellwig <***@lst.de> - 2014-09-07 1831.eml
Applying: scsi: stop passing a gfp_mask argument down the command setup path
==== [PATCH 5_8] scsi: move scsi_dispatch_cmd to scsi_lib.c - Christoph
Hellwig <***@lst.de> - 2014-09-07 1831.eml
Applying: scsi: move scsi_dispatch_cmd to scsi_lib.c
error: patch failed: drivers/scsi/scsi.c:626
error: drivers/scsi/scsi.c: patch does not apply
Patch failed at 0001 scsi: move scsi_dispatch_cmd to scsi_lib.c
The copy of the patch that failed is found in:
/home/bart/software/linux-kernel/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-09-08 15:13:01 UTC
Permalink
Post by Bart Van Assche
Post by Christoph Hellwig
This series cleans up a couple of lose ends I noticed during the scsi-mq
work, but which weren't important enough to address during the last cycle.
Hello Christoph,
Is there perhaps a public git tree available with this patch series ? I
have tried to apply these patches on top of v3.17-rc4 but unfortunately
I've pushed a scsi-io-path-cleanups tree to my scsi-queue.git tree.

I think you were missing the patches from mthe core-for-3.18 branch which
overlap a bit.

I will also rebase the core-for-3.18 and drivers-for-18 branches soon
as -rc1 which they are currently based on has the percpu issues the
scsi + blk-mq can trigger easily.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Bart Van Assche
2014-09-30 13:31:25 UTC
Permalink
Post by Christoph Hellwig
Post by Bart Van Assche
Post by Christoph Hellwig
This series cleans up a couple of lose ends I noticed during the scsi-mq
work, but which weren't important enough to address during the last cycle.
Is there perhaps a public git tree available with this patch series ? I
have tried to apply these patches on top of v3.17-rc4 but unfortunately
I've pushed a scsi-io-path-cleanups tree to my scsi-queue.git tree.
I think you were missing the patches from the core-for-3.18 branch which
overlap a bit.
I will also rebase the core-for-3.18 and drivers-for-18 branches soon
as -rc1 which they are currently based on has the percpu issues the
scsi + blk-mq can trigger easily.
(replying to an e-mail of three weeks ago)

Hello Christoph,

At least in the tests I ran myself these patches are working fine in
combination with scsi-mq and the SRP initiator. The tree I have been
testing with has commit ID ec8af1eb08f2 - this is the tree on top of
v3.17-rc4.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig
2014-09-30 13:43:53 UTC
Permalink
Post by Bart Van Assche
At least in the tests I ran myself these patches are working fine in
combination with scsi-mq and the SRP initiator. The tree I have been testing
with has commit ID ec8af1eb08f2 - this is the tree on top of v3.17-rc4.
Thanks. Can you give me formal Reviewed-by: and Tested-by: tags for the
patches?

Also any chance someone in the Cc list can give me a second review so
that I can merge it into the core-for-3.18 tree?
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Continue reading on narkive:
Loading...