I/O path cleanup V2

I/O path cleanup V2

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

These are also available at:

   git://git.infradead.org/users/hch/scsi.git scsi-io-path-cleanups

Changes since V1:
 - rebased to core-for-3.19
 - addressed review comments from Bart Van Assche <bvanassche@acm.org>
 - dropped the last two patches for now.  There is too much churn in
   this area at the moment.

[PATCH 1/6] 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>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_error.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ba19687..a1ffdb0 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2335,14 +2335,9 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
 		return -EIO;
 
 	error = -EIO;
-	if (!get_device(&dev->sdev_gendev))
-		goto out_put_autopm_host;
-
 	scmd = scsi_get_command(dev, GFP_KERNEL);
-	if (!scmd) {
-		put_device(&dev->sdev_gendev);
+	if (!scmd)
 		goto out_put_autopm_host;
-	}
 
 	blk_rq_init(NULL, &req);
 	scmd->request = &req;
@@ -2404,10 +2399,10 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
 			     "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 error;
-- 
1.9.1

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

[Hannes Reinecke]

On 11/06/2014 08:40 AM, 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>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/scsi_error.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index ba19687..a1ffdb0 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -2335,14 +2335,9 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
>  		return -EIO;
>  
>  	error = -EIO;
> -	if (!get_device(&dev->sdev_gendev))
> -		goto out_put_autopm_host;
> -
>  	scmd = scsi_get_command(dev, GFP_KERNEL);
> -	if (!scmd) {
> -		put_device(&dev->sdev_gendev);
> +	if (!scmd)
>  		goto out_put_autopm_host;
> -	}
>  
>  	blk_rq_init(NULL, &req);
>  	scmd->request = &req;
> @@ -2404,10 +2399,10 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
>  			     "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 error;
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] 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>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 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 38f8c85..1a3546e8 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -543,17 +543,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;
@@ -731,8 +720,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;
 
@@ -744,9 +731,12 @@ static bool scsi_end_request(struct request *req, int error,
 		spin_unlock_irqrestore(q->queue_lock, flags);
 
 		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/6] scsi: remove scsi_next_command

[Hannes Reinecke]

On 11/06/2014 08:40 AM, 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>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  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 38f8c85..1a3546e8 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -543,17 +543,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;
> @@ -731,8 +720,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;
>  
> @@ -744,9 +731,12 @@ static bool scsi_end_request(struct request *req, int error,
>  		spin_unlock_irqrestore(q->queue_lock, flags);
>  
>  		scsi_release_buffers(cmd);
> -		scsi_next_command(cmd);
> +
> +		scsi_put_command(cmd);
> +		scsi_run_queue(q);
>  	}
>  
> +	put_device(&sdev->sdev_gendev);
>  	return false;
>  }
>  
Hmm? Isn't there a scsi_put_comand() too many?
You dropped it from the 'if' branch, moved it out of
the condition, but kept in in the 'else' branch ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Christoph Hellwig]

On Thu, Nov 06, 2014 at 09:21:05AM +0100, Hannes Reinecke wrote:
> Hmm? Isn't there a scsi_put_comand() too many?
> You dropped it from the 'if' branch, moved it out of
> the condition, but kept in in the 'else' branch ...

The put_device for the 'else' branch was hidden inside scsi_next_command.

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

[Christoph Hellwig]

On Thu, Nov 06, 2014 at 04:34:13PM +0100, Christoph Hellwig wrote:
> On Thu, Nov 06, 2014 at 09:21:05AM +0100, Hannes Reinecke wrote:
> > Hmm? Isn't there a scsi_put_comand() too many?
> > You dropped it from the 'if' branch, moved it out of
> > the condition, but kept in in the 'else' branch ...
> 
> The put_device for the 'else' branch was hidden inside scsi_next_command.

Is this a good enough explanation to get a Reviewed-by?

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

[Hannes Reinecke]

On 11/11/2014 05:37 PM, Christoph Hellwig wrote:
> On Thu, Nov 06, 2014 at 04:34:13PM +0100, Christoph Hellwig wrote:
>> On Thu, Nov 06, 2014 at 09:21:05AM +0100, Hannes Reinecke wrote:
>>> Hmm? Isn't there a scsi_put_comand() too many?
>>> You dropped it from the 'if' branch, moved it out of
>>> the condition, but kept in in the 'else' branch ...
>>
>> The put_device for the 'else' branch was hidden inside scsi_next_command.
> 
> Is this a good enough explanation to get a Reviewed-by?
> 
Yep.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] 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 | 77 +++++++++++++------------------------------------
 1 file changed, 20 insertions(+), 57 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1a3546e8..a737032 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -622,7 +622,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);
@@ -638,7 +638,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) {
@@ -649,42 +648,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)
 {
@@ -703,16 +666,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_request,
-		 * 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_request(req, error);
 
 		if (scsi_target(sdev)->single_lun ||
@@ -723,15 +684,15 @@ static bool scsi_end_request(struct request *req, int error,
 	} else {
 		unsigned long flags;
 
-		if (bidi_bytes)
-			scsi_release_bidi_buffers(cmd);
+		if (bidi_bytes) {
+			kmem_cache_free(scsi_sdb_cache, req->next_rq->special);
+			req->next_rq->special = NULL;
+		}
 
 		spin_lock_irqsave(q->queue_lock, flags);
 		blk_finish_request(req, error);
 		spin_unlock_irqrestore(q->queue_lock, flags);
 
-		scsi_release_buffers(cmd);
-
 		scsi_put_command(cmd);
 		scsi_run_queue(q);
 	}
@@ -1057,12 +1018,12 @@ 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);
 			scsi_requeue_command(q, cmd);
 		}
 		break;
@@ -1165,10 +1126,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);
@@ -1338,7 +1297,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;
@@ -1938,8 +1899,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/6] scsi: clean up S/G table freeing

[Hannes Reinecke]

On 11/06/2014 08:40 AM, Christoph Hellwig wrote:
> 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>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bart Van Assche]

Christoph Hellwig <hch <at> lst.de> writes:
> -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);
> -}

Hello Christoph,

Since I'm traveling I'm using a laptop with a small screen so I might be
missing some context information. What is not clear to me about this patch
is whether it changes a scsi_free_sgtable(cmd->prot_sdb, false) call into a
scsi_free_sgtable(cmd->prot_sdb, true) call for the non-mq path ?

Thanks,

Bart.

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

[Christoph Hellwig]

On Tue, Nov 18, 2014 at 06:14:30PM +0000, Bart Van Assche wrote:
> > -
> > -	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
> > -
> > -	if (scsi_prot_sg_count(cmd))
> > -		scsi_free_sgtable(cmd->prot_sdb, false);
> > -}
> 
> Hello Christoph,
> 
> Since I'm traveling I'm using a laptop with a small screen so I might be
> missing some context information. What is not clear to me about this patch
> is whether it changes a scsi_free_sgtable(cmd->prot_sdb, false) call into a
> scsi_free_sgtable(cmd->prot_sdb, true) call for the non-mq path ?

It does, and that is a bug.  Thanks for catching it!

[PATCH 4/6] 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>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 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 a737032..b2de77b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -588,10 +588,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);
@@ -1038,8 +1038,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;
 
@@ -1047,7 +1046,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;
 
 	/* 
@@ -1072,7 +1071,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;
@@ -1081,7 +1080,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;
 
@@ -1097,8 +1096,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;
 	}
@@ -1110,7 +1108,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;
 		}
@@ -1177,7 +1175,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 b041eca..eca990c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -784,7 +784,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:
@@ -878,7 +878,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;
 }
@@ -912,7 +912,7 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 	int ret;
 	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 3d5399e..5ebadc2 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 522a5f27..767967e 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -159,7 +159,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/6] scsi: stop passing a gfp_mask argument down the command setup path

[Hannes Reinecke]

On 11/06/2014 08:40 AM, 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.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] 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>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 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 bc52bbd..5fcfa02 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -603,87 +603,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 b2de77b..6400c6b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1602,6 +1602,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/6] scsi: move scsi_dispatch_cmd to scsi_lib.c

[Hannes Reinecke]

On 11/06/2014 08:40 AM, 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.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>

A round of applause for that.
One of the most annoying things when debugging.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] 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>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_lib.c | 86 ++++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 48 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6400c6b..e8457fc 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -507,42 +507,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;
@@ -648,6 +612,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)
 {
@@ -779,7 +780,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;
@@ -1015,17 +1015,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/6] scsi: move more requeue handling into scsi_requeue_command

[Hannes Reinecke]

On 11/06/2014 08:40 AM, Christoph Hellwig wrote:
> 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>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html