I/O path cleanup

I/O path cleanup

[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.

[PATCH 1/8] scsi: don't use scsi_next_command in scsi_reset_provider

[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.

Signed-off-by: Christoph Hellwig <hch@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

Re: [PATCH 1/8] scsi: don't use scsi_next_command in scsi_reset_provider

[Bart Van Assche]

On 09/07/14 18:31, Christoph Hellwig wrote:
> 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 <hch@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;

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 <bvanassche@acm.org>

[PATCH 2/8] scsi: remove scsi_next_command

[Christoph Hellwig]

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 <hch@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

Re: [PATCH 2/8] scsi: remove scsi_next_command

[Bart Van Assche]

On 09/07/14 18:31, Christoph Hellwig wrote:
> 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 <hch@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);
>


Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 3/8] scsi: clean up S/G table freeing

[Christoph Hellwig]

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 <hch@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

Re: [PATCH 3/8] scsi: clean up S/G table freeing

[Bart Van Assche]

On 09/07/14 18:31, Christoph Hellwig wrote:
> -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 ?

> +		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.

Re: [PATCH 3/8] scsi: clean up S/G table freeing

[Christoph Hellwig]

On Wed, Oct 01, 2014 at 02:22:18PM +0200, Bart Van Assche wrote:
> 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.

[PATCH 4/8] scsi: stop passing a gfp_mask argument down the command setup path

[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.

Signed-off-by: Christoph Hellwig <hch@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

Re: [PATCH 4/8] scsi: stop passing a gfp_mask argument down the command setup path

[Bart Van Assche]

On 09/07/14 18:31, Christoph Hellwig wrote:
> 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 <bvanassche@acm.org>

[PATCH 5/8] scsi: move scsi_dispatch_cmd to scsi_lib.c

[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.

Signed-off-by: Christoph Hellwig <hch@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

Re: [PATCH 5/8] scsi: move scsi_dispatch_cmd to scsi_lib.c

[Bart Van Assche]

On 09/07/14 18:31, Christoph Hellwig wrote:
> 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 <bvanassche@acm.org>

[PATCH 6/8] scsi: move more requeue handling into scsi_requeue_command

[Christoph Hellwig]

Move a bit code out of scsi_io_completion and into scsi_requeue_command
in preparation for further refactoring.

Signed-off-by: Christoph Hellwig <hch@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

Re: [PATCH 6/8] scsi: move more requeue handling into scsi_requeue_command

[Bart Van Assche]

On 09/07/14 18:31, Christoph Hellwig wrote:
> Move a bit code out of scsi_io_completion and into scsi_requeue_command
> in preparation for further refactoring.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 7/8] scsi: split error handling slow path out of scsi_io_completion

[Christoph Hellwig]

Move the error handling path out of scsi_io_completion and into an
out of line helper.

Signed-off-by: Christoph Hellwig <hch@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

Re: [PATCH 7/8] scsi: split error handling slow path out of scsi_io_completion

[Bart Van Assche]

On 09/07/14 18:31, Christoph Hellwig wrote:
> Move the error handling path out of scsi_io_completion and into an
> out of line helper.
>
> Signed-off-by: Christoph Hellwig <hch@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)

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.

[PATCH 8/8] scsi: merge scsi_finish_command and scsi_io_completion

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@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

Re: [PATCH 8/8] scsi: merge scsi_finish_command and scsi_io_completion

[Bart Van Assche]

On 09/07/14 18:31, Christoph Hellwig wrote:
>  [ ... ]

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.

Re: I/O path cleanup

[Bart Van Assche]

On 09/07/14 18:31, Christoph Hellwig wrote:
> 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 <hch@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 
<hch@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 
<hch@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 <hch@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 <hch@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.

Re: I/O path cleanup

[Christoph Hellwig]

On Mon, Sep 08, 2014 at 09:22:40AM +0200, Bart Van Assche wrote:
> On 09/07/14 18:31, Christoph Hellwig wrote:
>> 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:

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.

Re: I/O path cleanup

[Bart Van Assche]

On 09/08/14 17:13, Christoph Hellwig wrote:
> On Mon, Sep 08, 2014 at 09:22:40AM +0200, Bart Van Assche wrote:
>> On 09/07/14 18:31, Christoph Hellwig wrote:
>>> 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
>> without success:
>
> 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.

Re: I/O path cleanup

[Christoph Hellwig]

On Tue, Sep 30, 2014 at 03:31:25PM +0200, Bart Van Assche wrote:
> 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?