All of lore.kernel.org
 help / color / mirror / Atom feed
* misc scsi midlayer updates
@ 2014-03-27 16:14 Christoph Hellwig
  2014-03-27 16:14 ` [PATCH 1/4] scsi: explicitly release bidi buffers Christoph Hellwig
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Christoph Hellwig @ 2014-03-27 16:14 UTC (permalink / raw)
  To: James Bottomley; +Cc: linux-scsi

Various patches from the scsi multiqueue development that make sense on their
own.


^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH 1/4] scsi: explicitly release bidi buffers
  2014-03-27 16:14 misc scsi midlayer updates Christoph Hellwig
@ 2014-03-27 16:14 ` Christoph Hellwig
  2014-03-31  5:58   ` Mike Christie
  2014-03-27 16:14 ` [PATCH 2/4] scsi: remove scsi_end_request Christoph Hellwig
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2014-03-27 16:14 UTC (permalink / raw)
  To: James Bottomley; +Cc: linux-scsi

Instead of trying to guess when we have a BIDI buffer in scsi_release_buffers
add a function to explicitly free the BIDI ressoures in the one place that
handles them.  This avoids needing a special __scsi_release_buffers for the
case where we already have freed the request as well.

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5681c05..5454d7d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -511,8 +511,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
 		scsi_run_queue(sdev->request_queue);
 }
 
-static void __scsi_release_buffers(struct scsi_cmnd *, int);
-
 /*
  * Function:    scsi_end_request()
  *
@@ -568,7 +566,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
 	 * This will goose the queue request function at the end, so we don't
 	 * need to worry about launching another command.
 	 */
-	__scsi_release_buffers(cmd, 0);
+	scsi_release_buffers(cmd);
 	scsi_next_command(cmd);
 	return NULL;
 }
@@ -624,30 +622,10 @@ static void scsi_free_sgtable(struct scsi_data_buffer *sdb)
 	__sg_free_table(&sdb->table, SCSI_MAX_SG_SEGMENTS, scsi_sg_free);
 }
 
-static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
-{
-
-	if (cmd->sdb.table.nents)
-		scsi_free_sgtable(&cmd->sdb);
-
-	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
-
-	if (do_bidi_check && scsi_bidi_cmnd(cmd)) {
-		struct scsi_data_buffer *bidi_sdb =
-			cmd->request->next_rq->special;
-		scsi_free_sgtable(bidi_sdb);
-		kmem_cache_free(scsi_sdb_cache, bidi_sdb);
-		cmd->request->next_rq->special = NULL;
-	}
-
-	if (scsi_prot_sg_count(cmd))
-		scsi_free_sgtable(cmd->prot_sdb);
-}
-
 /*
  * Function:    scsi_release_buffers()
  *
- * Purpose:     Completion processing for block device I/O requests.
+ * Purpose:     Free resources allocate for a scsi_command.
  *
  * Arguments:   cmd	- command that we are bailing.
  *
@@ -658,15 +636,29 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
  * 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, and potentially any bounce
- *		buffers.
+ *		the scatter-gather table.
  */
 void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
-	__scsi_release_buffers(cmd, 1);
+	if (cmd->sdb.table.nents)
+		scsi_free_sgtable(&cmd->sdb);
+
+	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
+
+	if (scsi_prot_sg_count(cmd))
+		scsi_free_sgtable(cmd->prot_sdb);
 }
 EXPORT_SYMBOL(scsi_release_buffers);
 
+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);
+	kmem_cache_free(scsi_sdb_cache, bidi_sdb);
+	cmd->request->next_rq->special = NULL;
+}
+
 /**
  * __scsi_error_from_host_byte - translate SCSI error code into errno
  * @cmd:	SCSI command (unused)
@@ -800,6 +792,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			req->next_rq->resid_len = scsi_in(cmd)->resid;
 
 			scsi_release_buffers(cmd);
+			scsi_release_bidi_buffers(cmd);
 			blk_end_request_all(req, 0);
 
 			scsi_next_command(cmd);
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH 2/4] scsi: remove scsi_end_request
  2014-03-27 16:14 misc scsi midlayer updates Christoph Hellwig
  2014-03-27 16:14 ` [PATCH 1/4] scsi: explicitly release bidi buffers Christoph Hellwig
@ 2014-03-27 16:14 ` Christoph Hellwig
  2014-03-31  5:26   ` Nicholas A. Bellinger
  2014-03-27 16:14 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2014-03-27 16:14 UTC (permalink / raw)
  To: James Bottomley; +Cc: linux-scsi

Simply the I/O completion logic by folding scsi_end_request into its only
caller.  There is a single site to either requeue the command or move on
to the next one instead of of the previous convoluted logic.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c |  114 +++++++++++++----------------------------------
 1 file changed, 32 insertions(+), 82 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5454d7d..a73751b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -511,66 +511,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
 		scsi_run_queue(sdev->request_queue);
 }
 
-/*
- * Function:    scsi_end_request()
- *
- * Purpose:     Post-processing of completed commands (usually invoked at end
- *		of upper level post-processing and scsi_io_completion).
- *
- * Arguments:   cmd	 - command that is complete.
- *              error    - 0 if I/O indicates success, < 0 for I/O error.
- *              bytes    - number of bytes of completed I/O
- *		requeue  - indicates whether we should requeue leftovers.
- *
- * Lock status: Assumed that lock is not held upon entry.
- *
- * Returns:     cmd if requeue required, NULL otherwise.
- *
- * Notes:       This is called for block device requests in order to
- *              mark some number of sectors as complete.
- * 
- *		We are guaranteeing that the request queue will be goosed
- *		at some point during this call.
- * Notes:	If cmd was requeued, upon return it will be a stale pointer.
- */
-static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
-					  int bytes, int requeue)
-{
-	struct request_queue *q = cmd->device->request_queue;
-	struct request *req = cmd->request;
-
-	/*
-	 * If there are blocks left over at the end, set up the command
-	 * to queue the remainder of them.
-	 */
-	if (blk_end_request(req, error, bytes)) {
-		/* kill remainder if no retrys */
-		if (error && scsi_noretry_cmd(cmd))
-			blk_end_request_all(req, error);
-		else {
-			if (requeue) {
-				/*
-				 * Bleah.  Leftovers again.  Stick the
-				 * leftovers in the front of the
-				 * queue, and goose the queue again.
-				 */
-				scsi_release_buffers(cmd);
-				scsi_requeue_command(q, cmd);
-				cmd = NULL;
-			}
-			return cmd;
-		}
-	}
-
-	/*
-	 * This will goose the queue request function at the end, so we don't
-	 * need to worry about launching another command.
-	 */
-	scsi_release_buffers(cmd);
-	scsi_next_command(cmd);
-	return NULL;
-}
-
 static inline unsigned int scsi_sgtable_index(unsigned short nents)
 {
 	unsigned int index;
@@ -716,16 +656,9 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
  *
  * Returns:     Nothing
  *
- * Notes:       This function is matched in terms of capabilities to
- *              the function that created the scatter-gather list.
- *              In other words, if there are no bounce buffers
- *              (the normal case for most drivers), we don't need
- *              the logic to deal with cleaning up afterwards.
- *
- *		We must call scsi_end_request().  This 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
+ * 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
@@ -734,7 +667,7 @@ static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
  *		   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
+ *		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.
  *
@@ -793,6 +726,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 
 			scsi_release_buffers(cmd);
 			scsi_release_bidi_buffers(cmd);
+
 			blk_end_request_all(req, 0);
 
 			scsi_next_command(cmd);
@@ -832,12 +766,25 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	}
 
 	/*
-	 * A number of bytes were successfully read.  If there
-	 * are leftovers and there is some kind of error
-	 * (result != 0), retry the rest.
+	 * If we finished all bytes in the request we are done now.
 	 */
-	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
-		return;
+	if (!blk_end_request(req, error, good_bytes))
+		goto next_command;
+
+	/*
+	 * Kill remainder if no retrys.
+	 */
+	if (error && scsi_noretry_cmd(cmd)) {
+		blk_end_request_all(req, error);
+		goto next_command;
+	}
+
+	/*
+	 * If there had been no error, but we have leftover bytes in the
+	 * requeues just queue the command up again.
+	 */
+	if (result == 0)
+		goto requeue;
 
 	error = __scsi_error_from_host_byte(cmd, result);
 
@@ -965,7 +912,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	switch (action) {
 	case ACTION_FAIL:
 		/* Give up and fail the remainder of the request */
-		scsi_release_buffers(cmd);
 		if (!(req->cmd_flags & REQ_QUIET)) {
 			if (description)
 				scmd_printk(KERN_INFO, cmd, "%s\n",
@@ -975,12 +921,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				scsi_print_sense("", cmd);
 			scsi_print_command(cmd);
 		}
-		if (blk_end_request_err(req, error))
-			scsi_requeue_command(q, cmd);
-		else
-			scsi_next_command(cmd);
-		break;
+		if (!blk_end_request_err(req, error))
+			goto next_command;
+		/*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.
 		 */
@@ -996,6 +941,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		__scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, 0);
 		break;
 	}
+	return;
+
+next_command:
+	scsi_release_buffers(cmd);
+	scsi_next_command(cmd);
 }
 
 static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH 3/4] scsi: reintroduce scsi_driver.init_command
  2014-03-27 16:14 misc scsi midlayer updates Christoph Hellwig
  2014-03-27 16:14 ` [PATCH 1/4] scsi: explicitly release bidi buffers Christoph Hellwig
  2014-03-27 16:14 ` [PATCH 2/4] scsi: remove scsi_end_request Christoph Hellwig
@ 2014-03-27 16:14 ` Christoph Hellwig
  2014-03-31  5:45   ` Nicholas A. Bellinger
                     ` (2 more replies)
  2014-03-27 16:14 ` [PATCH 4/4] scsi: handle command allocation failure in scsi_reset_provider Christoph Hellwig
                   ` (2 subsequent siblings)
  5 siblings, 3 replies; 18+ messages in thread
From: Christoph Hellwig @ 2014-03-27 16:14 UTC (permalink / raw)
  To: James Bottomley; +Cc: linux-scsi

Move control of the prep_fn back from the ULDs into the scsi layer.  Besides
cleaning up the code and removing the only use of the unprep_fn
requeuest_queue method this also prepares for usinng blk-mq, which doesn't
have equivalent functionality to the prep_fn method and requires the driver
to provide just a single I/O submission method.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c    |   66 ++++++++++++++++++++++++++------------------
 drivers/scsi/sd.c          |   46 +++++++++++-------------------
 drivers/scsi/sr.c          |   19 ++++---------
 include/scsi/scsi_driver.h |    8 ++----
 4 files changed, 63 insertions(+), 76 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a73751b..48c5c77 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -456,6 +456,16 @@ void scsi_requeue_run_queue(struct work_struct *work)
 	scsi_run_queue(q);
 }
 
+static void scsi_uninit_command(struct scsi_cmnd *cmd)
+{
+	if (cmd->request->cmd_type == REQ_TYPE_FS) {
+		struct scsi_driver *drv = scsi_cmd_to_driver(cmd);
+
+		if (drv->uninit_command)
+			drv->uninit_command(cmd);
+	}
+}
+
 /*
  * Function:	scsi_requeue_command()
  *
@@ -480,6 +490,8 @@ static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
 	struct request *req = cmd->request;
 	unsigned long flags;
 
+	scsi_uninit_command(cmd);
+
 	spin_lock_irqsave(q->queue_lock, flags);
 	blk_unprep_request(req);
 	req->special = NULL;
@@ -1071,15 +1083,7 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
 
 int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 {
-	struct scsi_cmnd *cmd;
-	int ret = scsi_prep_state_check(sdev, req);
-
-	if (ret != BLKPREP_OK)
-		return ret;
-
-	cmd = scsi_get_cmd_from_req(sdev, req);
-	if (unlikely(!cmd))
-		return BLKPREP_DEFER;
+	struct scsi_cmnd *cmd = req->special;
 
 	/*
 	 * BLOCK_PC requests may transfer data, in which case they must
@@ -1123,15 +1127,11 @@ EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd);
  */
 int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 {
-	struct scsi_cmnd *cmd;
-	int ret = scsi_prep_state_check(sdev, req);
-
-	if (ret != BLKPREP_OK)
-		return ret;
+	struct scsi_cmnd *cmd = req->special;
 
 	if (unlikely(sdev->scsi_dh_data && sdev->scsi_dh_data->scsi_dh
 			 && sdev->scsi_dh_data->scsi_dh->prep_fn)) {
-		ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req);
+		int ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req);
 		if (ret != BLKPREP_OK)
 			return ret;
 	}
@@ -1141,16 +1141,13 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 	 */
 	BUG_ON(!req->nr_phys_segments);
 
-	cmd = scsi_get_cmd_from_req(sdev, req);
-	if (unlikely(!cmd))
-		return BLKPREP_DEFER;
-
 	memset(cmd->cmnd, 0, BLK_MAX_CDB);
 	return scsi_init_io(cmd, GFP_ATOMIC);
 }
 EXPORT_SYMBOL(scsi_setup_fs_cmnd);
 
-int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
+static int
+scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 {
 	int ret = BLKPREP_OK;
 
@@ -1202,9 +1199,9 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 	}
 	return ret;
 }
-EXPORT_SYMBOL(scsi_prep_state_check);
 
-int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
+static int
+scsi_prep_return(struct request_queue *q, struct request *req, int ret)
 {
 	struct scsi_device *sdev = q->queuedata;
 
@@ -1235,18 +1232,33 @@ int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
 
 	return ret;
 }
-EXPORT_SYMBOL(scsi_prep_return);
 
-int scsi_prep_fn(struct request_queue *q, struct request *req)
+static int scsi_prep_fn(struct request_queue *q, struct request *req)
 {
 	struct scsi_device *sdev = q->queuedata;
-	int ret = BLKPREP_KILL;
+	struct scsi_cmnd *cmd;
+	int ret;
 
-	if (req->cmd_type == REQ_TYPE_BLOCK_PC)
+	ret = scsi_prep_state_check(sdev, req);
+	if (ret != BLKPREP_OK)
+		goto out;
+
+	cmd = scsi_get_cmd_from_req(sdev, req);
+	if (unlikely(!cmd)) {
+		ret = BLKPREP_DEFER;
+		goto out;
+	}
+
+	if (req->cmd_type == REQ_TYPE_FS)
+		ret = scsi_cmd_to_driver(cmd)->init_command(cmd);
+	else if (req->cmd_type == REQ_TYPE_BLOCK_PC)
 		ret = scsi_setup_blk_pc_cmnd(sdev, req);
+	else
+		ret = BLKPREP_KILL;
+
+out:
 	return scsi_prep_return(q, req, ret);
 }
-EXPORT_SYMBOL(scsi_prep_fn);
 
 /*
  * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 89e6c04..d95c4fd 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -109,6 +109,8 @@ static int sd_suspend_system(struct device *);
 static int sd_suspend_runtime(struct device *);
 static int sd_resume(struct device *);
 static void sd_rescan(struct device *);
+static int sd_init_command(struct scsi_cmnd *SCpnt);
+static void sd_uninit_command(struct scsi_cmnd *SCpnt);
 static int sd_done(struct scsi_cmnd *);
 static int sd_eh_action(struct scsi_cmnd *, int);
 static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
@@ -503,6 +505,8 @@ static struct scsi_driver sd_template = {
 		.pm		= &sd_pm_ops,
 	},
 	.rescan			= sd_rescan,
+	.init_command		= sd_init_command,
+	.uninit_command		= sd_uninit_command,
 	.done			= sd_done,
 	.eh_action		= sd_eh_action,
 };
@@ -838,9 +842,9 @@ static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
 	return scsi_setup_blk_pc_cmnd(sdp, rq);
 }
 
-static void sd_unprep_fn(struct request_queue *q, struct request *rq)
+static void sd_uninit_command(struct scsi_cmnd *SCpnt)
 {
-	struct scsi_cmnd *SCpnt = rq->special;
+	struct request *rq = SCpnt->request;
 
 	if (rq->cmd_flags & REQ_DISCARD) {
 		free_page((unsigned long)rq->buffer);
@@ -853,18 +857,10 @@ static void sd_unprep_fn(struct request_queue *q, struct request *rq)
 	}
 }
 
-/**
- *	sd_prep_fn - build a scsi (read or write) command from
- *	information in the request structure.
- *	@SCpnt: pointer to mid-level's per scsi command structure that
- *	contains request and into which the scsi command is written
- *
- *	Returns 1 if successful and 0 if error (or cannot be done now).
- **/
-static int sd_prep_fn(struct request_queue *q, struct request *rq)
+static int sd_init_command(struct scsi_cmnd *SCpnt)
 {
-	struct scsi_cmnd *SCpnt;
-	struct scsi_device *sdp = q->queuedata;
+	struct request *rq = SCpnt->request;
+	struct scsi_device *sdp = SCpnt->device;
 	struct gendisk *disk = rq->rq_disk;
 	struct scsi_disk *sdkp;
 	sector_t block = blk_rq_pos(rq);
@@ -886,12 +882,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	} else if (rq->cmd_flags & REQ_FLUSH) {
 		ret = scsi_setup_flush_cmnd(sdp, rq);
 		goto out;
-	} else if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
-		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
-		goto out;
-	} else if (rq->cmd_type != REQ_TYPE_FS) {
-		ret = BLKPREP_KILL;
-		goto out;
 	}
 	ret = scsi_setup_fs_cmnd(sdp, rq);
 	if (ret != BLKPREP_OK)
@@ -903,11 +893,10 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	 * is used for a killable error condition */
 	ret = BLKPREP_KILL;
 
-	SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt,
-					"sd_prep_fn: block=%llu, "
-					"count=%d\n",
-					(unsigned long long)block,
-					this_count));
+	SCSI_LOG_HLQUEUE(1,
+		scmd_printk(KERN_INFO, SCpnt,
+			"%s: block=%llu, count=%d\n",
+			__func__, (unsigned long long)block, this_count));
 
 	if (!sdp || !scsi_device_online(sdp) ||
 	    block + blk_rq_sectors(rq) > get_capacity(disk)) {
@@ -1127,7 +1116,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	 */
 	ret = BLKPREP_OK;
  out:
-	return scsi_prep_return(q, rq, ret);
+	return ret;
 }
 
 /**
@@ -1663,6 +1652,8 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 	unsigned char op = SCpnt->cmnd[0];
 	unsigned char unmap = SCpnt->cmnd[1] & 8;
 
+	sd_uninit_command(SCpnt);
+
 	if (req->cmd_flags & REQ_DISCARD || req->cmd_flags & REQ_WRITE_SAME) {
 		if (!result) {
 			good_bytes = blk_rq_bytes(req);
@@ -2878,9 +2869,6 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 
 	sd_revalidate_disk(gd);
 
-	blk_queue_prep_rq(sdp->request_queue, sd_prep_fn);
-	blk_queue_unprep_rq(sdp->request_queue, sd_unprep_fn);
-
 	gd->driverfs_dev = &sdp->sdev_gendev;
 	gd->flags = GENHD_FL_EXT_DEVT;
 	if (sdp->removable) {
@@ -3027,8 +3015,6 @@ static int sd_remove(struct device *dev)
 	scsi_autopm_get_device(sdkp->device);
 
 	async_synchronize_full_domain(&scsi_sd_probe_domain);
-	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
-	blk_queue_unprep_rq(sdkp->device->request_queue, NULL);
 	device_del(&sdkp->dev);
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 40d8592..93cbd36 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -79,6 +79,7 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_WORM);
 static DEFINE_MUTEX(sr_mutex);
 static int sr_probe(struct device *);
 static int sr_remove(struct device *);
+static int sr_init_command(struct scsi_cmnd *SCpnt);
 static int sr_done(struct scsi_cmnd *);
 static int sr_runtime_suspend(struct device *dev);
 
@@ -94,6 +95,7 @@ static struct scsi_driver sr_template = {
 		.remove		= sr_remove,
 		.pm		= &sr_pm_ops,
 	},
+	.init_command		= sr_init_command,
 	.done			= sr_done,
 };
 
@@ -378,21 +380,14 @@ static int sr_done(struct scsi_cmnd *SCpnt)
 	return good_bytes;
 }
 
-static int sr_prep_fn(struct request_queue *q, struct request *rq)
+static int sr_init_command(struct scsi_cmnd *SCpnt)
 {
 	int block = 0, this_count, s_size;
 	struct scsi_cd *cd;
-	struct scsi_cmnd *SCpnt;
-	struct scsi_device *sdp = q->queuedata;
+	struct request *rq = SCpnt->request;
+	struct scsi_device *sdp = SCpnt->device;
 	int ret;
 
-	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
-		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
-		goto out;
-	} else if (rq->cmd_type != REQ_TYPE_FS) {
-		ret = BLKPREP_KILL;
-		goto out;
-	}
 	ret = scsi_setup_fs_cmnd(sdp, rq);
 	if (ret != BLKPREP_OK)
 		goto out;
@@ -517,7 +512,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	 */
 	ret = BLKPREP_OK;
  out:
-	return scsi_prep_return(q, rq, ret);
+	return ret;
 }
 
 static int sr_block_open(struct block_device *bdev, fmode_t mode)
@@ -718,7 +713,6 @@ static int sr_probe(struct device *dev)
 
 	/* FIXME: need to handle a get_capabilities failure properly ?? */
 	get_capabilities(cd);
-	blk_queue_prep_rq(sdev->request_queue, sr_prep_fn);
 	sr_vendor_init(cd);
 
 	disk->driverfs_dev = &sdev->sdev_gendev;
@@ -993,7 +987,6 @@ static int sr_remove(struct device *dev)
 
 	scsi_autopm_get_device(cd->device);
 
-	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);
 
 	mutex_lock(&sr_ref_mutex);
diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
index 20fdfc2..b507729 100644
--- a/include/scsi/scsi_driver.h
+++ b/include/scsi/scsi_driver.h
@@ -6,15 +6,14 @@
 struct module;
 struct scsi_cmnd;
 struct scsi_device;
-struct request;
-struct request_queue;
-
 
 struct scsi_driver {
 	struct module		*owner;
 	struct device_driver	gendrv;
 
 	void (*rescan)(struct device *);
+	int (*init_command)(struct scsi_cmnd *);
+	void (*uninit_command)(struct scsi_cmnd *);
 	int (*done)(struct scsi_cmnd *);
 	int (*eh_action)(struct scsi_cmnd *, int);
 };
@@ -31,8 +30,5 @@ extern int scsi_register_interface(struct class_interface *);
 
 int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
 int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
-int scsi_prep_state_check(struct scsi_device *sdev, struct request *req);
-int scsi_prep_return(struct request_queue *q, struct request *req, int ret);
-int scsi_prep_fn(struct request_queue *, struct request *);
 
 #endif /* _SCSI_SCSI_DRIVER_H */
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH 4/4] scsi: handle command allocation failure in scsi_reset_provider
  2014-03-27 16:14 misc scsi midlayer updates Christoph Hellwig
                   ` (2 preceding siblings ...)
  2014-03-27 16:14 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig
@ 2014-03-27 16:14 ` Christoph Hellwig
  2014-03-31  5:45   ` Nicholas A. Bellinger
  2014-03-30 15:33 ` misc scsi midlayer updates Boaz Harrosh
  2014-03-31 21:55 ` Mike Christie
  5 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2014-03-27 16:14 UTC (permalink / raw)
  To: James Bottomley; +Cc: linux-scsi

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_error.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 771c16b..b989d31 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2294,6 +2294,12 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	}
 
 	scmd = scsi_get_command(dev, GFP_KERNEL);
+	if (!scmd) {
+		rtn = FAILED;
+		put_device(&dev->sdev_gendev);
+		goto out_put_autopm_host;
+	}
+
 	blk_rq_init(NULL, &req);
 	scmd->request = &req;
 
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: misc scsi midlayer updates
  2014-03-27 16:14 misc scsi midlayer updates Christoph Hellwig
                   ` (3 preceding siblings ...)
  2014-03-27 16:14 ` [PATCH 4/4] scsi: handle command allocation failure in scsi_reset_provider Christoph Hellwig
@ 2014-03-30 15:33 ` Boaz Harrosh
  2014-03-31  6:20   ` Christoph Hellwig
  2014-03-31 21:55 ` Mike Christie
  5 siblings, 1 reply; 18+ messages in thread
From: Boaz Harrosh @ 2014-03-30 15:33 UTC (permalink / raw)
  To: Christoph Hellwig, James Bottomley; +Cc: linux-scsi

[-- Attachment #1: Type: text/plain, Size: 1420 bytes --]

On 03/27/2014 06:14 PM, Christoph Hellwig wrote:
> Various patches from the scsi multiqueue development that make sense on their
> own.
> 
> --
> 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
> 

Hi Christoph

Many years ago I have sent these exact patches but no-one cared Including
me I guess.

(one instance is here, I sent it several times)
	http://comments.gmane.org/gmane.linux.scsi/63347

Please note the 4th patch which is a real BUG, titled:
	scsi_lib: Can't RETRY scsi_cmnd if some bytes were completed

I think my:
	scsi_lib: Remove that __scsi_release_buffers contraption
Is more elegant, is layered better and is smaller code. (please
consider my version)

Also the first patch is some more cleanup you'd like.

The main patch of collapsing  scsi_end_request is basically the same.

Funny that I've just rebased an exactly 2 years ago version and it
rebased cleanly. So here they are for your reference.

[PATCH 1/4] scsi_lib: request_queue is only needed inside
[PATCH 2/4] scsi_lib: Remove that __scsi_release_buffers contraption
[PATCH 3/4] scsi_lib: Collapse scsi_end_request into only user
[PATCH 4/4] scsi_lib: BUG: Can't RETRY scsi_cmnd if some bytes were

[Your patches have been tested within my MQ testing right?]

Thanks for pushing on this
Boaz


[-- Attachment #2: 0001-scsi_lib-request_queue-is-only-needed-inside-scsi_re.patch --]
[-- Type: text/x-patch, Size: 2571 bytes --]

>From 9809b82bbb9813370738ac00400c01d61b0c51b4 Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Mon, 4 Jan 2010 10:45:56 +0200
Subject: [PATCH 1/4] scsi_lib: request_queue is only needed inside
 scsi_requeue_command

This is a pure cleanup. Just starting the engines for things
to come.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_lib.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 62ec84b..dd834ca 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -491,10 +491,11 @@ void scsi_requeue_run_queue(struct work_struct *work)
  *		sector.
  * Notes:	Upon return, cmd is a stale pointer.
  */
-static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
+static void scsi_requeue_command(struct scsi_cmnd *cmd)
 {
 	struct scsi_device *sdev = cmd->device;
 	struct request *req = cmd->request;
+	struct request_queue *q = req->q;
 	unsigned long flags;
 
 	/*
@@ -565,7 +566,6 @@ static void __scsi_release_buffers(struct scsi_cmnd *, int);
 static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
 					  int bytes, int requeue)
 {
-	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 
 	/*
@@ -584,7 +584,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
 				 * queue, and goose the queue again.
 				 */
 				scsi_release_buffers(cmd);
-				scsi_requeue_command(q, cmd);
+				scsi_requeue_command(cmd);
 				cmd = NULL;
 			}
 			return cmd;
@@ -779,7 +779,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;
@@ -1003,7 +1002,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			scsi_print_command(cmd);
 		}
 		if (blk_end_request_err(req, error))
-			scsi_requeue_command(q, cmd);
+			scsi_requeue_command(cmd);
 		else
 			scsi_next_command(cmd);
 		break;
@@ -1012,7 +1011,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		 * A new command will be prepared and issued.
 		 */
 		scsi_release_buffers(cmd);
-		scsi_requeue_command(q, cmd);
+		scsi_requeue_command(cmd);
 		break;
 	case ACTION_RETRY:
 		/* Retry the same command immediately */
-- 
1.8.5.3


[-- Attachment #3: 0002-scsi_lib-Remove-that-__scsi_release_buffers-contrapt.patch --]
[-- Type: text/x-patch, Size: 2660 bytes --]

>From aac579b02c7d3665665a1f1f7c064a82b70b873a Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Mon, 4 Jan 2010 12:46:06 +0200
Subject: [PATCH 2/4] scsi_lib: Remove that __scsi_release_buffers contraption

Remove "do_bidi_check" by checking and nullifying cmd->request
when blk_end_* indicates the request is no longer valid.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_lib.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index dd834ca..c4cdc6a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -539,8 +539,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
 		scsi_run_queue(sdev->request_queue);
 }
 
-static void __scsi_release_buffers(struct scsi_cmnd *, int);
-
 /*
  * Function:    scsi_end_request()
  *
@@ -591,11 +589,12 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
 		}
 	}
 
+	cmd->request = NULL;
 	/*
 	 * This will goose the queue request function at the end, so we don't
 	 * need to worry about launching another command.
 	 */
-	__scsi_release_buffers(cmd, 0);
+	scsi_release_buffers(cmd);
 	scsi_next_command(cmd);
 	return NULL;
 }
@@ -651,26 +650,6 @@ static void scsi_free_sgtable(struct scsi_data_buffer *sdb)
 	__sg_free_table(&sdb->table, SCSI_MAX_SG_SEGMENTS, scsi_sg_free);
 }
 
-static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
-{
-
-	if (cmd->sdb.table.nents)
-		scsi_free_sgtable(&cmd->sdb);
-
-	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
-
-	if (do_bidi_check && scsi_bidi_cmnd(cmd)) {
-		struct scsi_data_buffer *bidi_sdb =
-			cmd->request->next_rq->special;
-		scsi_free_sgtable(bidi_sdb);
-		kmem_cache_free(scsi_sdb_cache, bidi_sdb);
-		cmd->request->next_rq->special = NULL;
-	}
-
-	if (scsi_prot_sg_count(cmd))
-		scsi_free_sgtable(cmd->prot_sdb);
-}
-
 /*
  * Function:    scsi_release_buffers()
  *
@@ -690,7 +669,21 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
  */
 void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
-	__scsi_release_buffers(cmd, 1);
+	if (cmd->sdb.table.nents)
+		scsi_free_sgtable(&cmd->sdb);
+
+	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
+
+	if (cmd->request && scsi_bidi_cmnd(cmd)) {
+		struct scsi_data_buffer *bidi_sdb =
+			cmd->request->next_rq->special;
+		scsi_free_sgtable(bidi_sdb);
+		kmem_cache_free(scsi_sdb_cache, bidi_sdb);
+		cmd->request->next_rq->special = NULL;
+	}
+
+	if (scsi_prot_sg_count(cmd))
+		scsi_free_sgtable(cmd->prot_sdb);
 }
 EXPORT_SYMBOL(scsi_release_buffers);
 
-- 
1.8.5.3


[-- Attachment #4: 0003-scsi_lib-Collapse-scsi_end_request-into-only-user.patch --]
[-- Type: text/x-patch, Size: 5102 bytes --]

>From a93c49e5cbc1b3bd0bcda49318499b557a25e622 Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <Boaz Harrosh bharrosh@panasas.com>
Date: Tue, 5 Apr 2011 18:40:10 -0700
Subject: [PATCH 3/4] scsi_lib: Collapse scsi_end_request into only user

Embedding scsi_end_request() into scsi_io_completion actually simplifies
the code and makes it clearer what's going on.

There is absolutely no functional and/or side effects changes after this
patch.

Patch was inspired by Alan Stern.

CC: Hannes Reinecke <hare@suse.de>
CC: Alan Stern <stern@rowland.harvard.edu>
CC: Matthew Wilcox <matthew@wil.cx>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_lib.c | 94 +++++++++++--------------------------------------
 1 file changed, 21 insertions(+), 73 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c4cdc6a..fa8612d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -539,66 +539,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
 		scsi_run_queue(sdev->request_queue);
 }
 
-/*
- * Function:    scsi_end_request()
- *
- * Purpose:     Post-processing of completed commands (usually invoked at end
- *		of upper level post-processing and scsi_io_completion).
- *
- * Arguments:   cmd	 - command that is complete.
- *              error    - 0 if I/O indicates success, < 0 for I/O error.
- *              bytes    - number of bytes of completed I/O
- *		requeue  - indicates whether we should requeue leftovers.
- *
- * Lock status: Assumed that lock is not held upon entry.
- *
- * Returns:     cmd if requeue required, NULL otherwise.
- *
- * Notes:       This is called for block device requests in order to
- *              mark some number of sectors as complete.
- * 
- *		We are guaranteeing that the request queue will be goosed
- *		at some point during this call.
- * Notes:	If cmd was requeued, upon return it will be a stale pointer.
- */
-static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
-					  int bytes, int requeue)
-{
-	struct request *req = cmd->request;
-
-	/*
-	 * If there are blocks left over at the end, set up the command
-	 * to queue the remainder of them.
-	 */
-	if (blk_end_request(req, error, bytes)) {
-		/* kill remainder if no retrys */
-		if (error && scsi_noretry_cmd(cmd))
-			blk_end_request_all(req, error);
-		else {
-			if (requeue) {
-				/*
-				 * Bleah.  Leftovers again.  Stick the
-				 * leftovers in the front of the
-				 * queue, and goose the queue again.
-				 */
-				scsi_release_buffers(cmd);
-				scsi_requeue_command(cmd);
-				cmd = NULL;
-			}
-			return cmd;
-		}
-	}
-
-	cmd->request = NULL;
-	/*
-	 * This will goose the queue request function at the end, so we don't
-	 * need to worry about launching another command.
-	 */
-	scsi_release_buffers(cmd);
-	scsi_next_command(cmd);
-	return NULL;
-}
-
 static inline unsigned int scsi_sgtable_index(unsigned short nents)
 {
 	unsigned int index;
@@ -777,7 +717,7 @@ 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,
+	enum {ACTION_NEXT_CMND, ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
 	      ACTION_DELAYED_RETRY} action;
 	char *description = NULL;
 
@@ -856,17 +796,19 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		error = 0;
 	}
 
-	/*
-	 * A number of bytes were successfully read.  If there
-	 * are leftovers and there is some kind of error
-	 * (result != 0), retry the rest.
-	 */
-	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
-		return;
-
-	error = __scsi_error_from_host_byte(cmd, result);
-
-	if (host_byte(result) == DID_RESET) {
+	if (likely(0 == blk_end_request(req, error, good_bytes))) {
+		/* All is done and good move to next command */
+		cmd->request = NULL;
+		action = ACTION_NEXT_CMND;
+	} else if (result == 0) {
+		/* Wrote some bytes but request was split */
+		action = ACTION_REPREP;
+	} else if (error && scsi_noretry_cmd(cmd)) {
+		/* kill remainder if no retries */
+		blk_end_request_all(req, error);
+		cmd->request = NULL;
+		action = ACTION_NEXT_CMND;
+	} else if (host_byte(result) == DID_RESET) {
 		/* Third party bus reset or reset for error recovery
 		 * reasons.  Just retry the command and see what
 		 * happens.
@@ -981,7 +923,13 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		action = ACTION_FAIL;
 	}
 
+	error = __scsi_error_from_host_byte(cmd, result);
+
 	switch (action) {
+	case ACTION_NEXT_CMND :
+		scsi_release_buffers(cmd);
+		scsi_next_command(cmd);
+		break;
 	case ACTION_FAIL:
 		/* Give up and fail the remainder of the request */
 		scsi_release_buffers(cmd);
@@ -994,7 +942,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				scsi_print_sense("", cmd);
 			scsi_print_command(cmd);
 		}
-		if (blk_end_request_err(req, error))
+		if (blk_end_request_err(req, error ? error : -EIO))
 			scsi_requeue_command(cmd);
 		else
 			scsi_next_command(cmd);
-- 
1.8.5.3


[-- Attachment #5: 0004-scsi_lib-BUG-Can-t-RETRY-scsi_cmnd-if-some-bytes-wer.patch --]
[-- Type: text/x-patch, Size: 1810 bytes --]

>From 3879be0877262643aa0d1884a13e6f7296cb3738 Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <Boaz Harrosh bharrosh@panasas.com>
Date: Tue, 5 Apr 2011 18:54:44 -0700
Subject: [PATCH 4/4] scsi_lib: BUG: Can't RETRY scsi_cmnd if some bytes were
 completed

In scsi_io_completion() there are many cases where action is
set to ACTION_RETRY or ACTION_DELAYED_RETRY. But we are not
allowed to just RETRY a command if some bytes where already
completed by blk_end_request(). This is a bad memory overrun
of DMA writing/reading random memory. We must re-prepare the
command in this case.

It is possible that all the cases that set ACTION_RETRY* have
actually come with good_bytes=0 (.i.e resid = everything) But
both the error and resid value come from LLDs and/or targets
and should not be trusted with such a BAD crash. Better safe
than sorry.

It is possible that this fix is actually not good enough and
in the case of some of these RETRYs we need to not call
blk_end_request() in the first place. But this calls for
a structural reorganisation of scsi_io_completion(). James
this is your turf please have a look.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_lib.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fa8612d..50a0bc6 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -925,6 +925,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 
 	error = __scsi_error_from_host_byte(cmd, result);
 
+	if (action >= ACTION_RETRY && good_bytes)
+		/* We cannot just retry if above blk_end_request advanced on
+		 * some good_bytes, so ACTION_REPREP
+		 */
+		action = ACTION_REPREP;
+
 	switch (action) {
 	case ACTION_NEXT_CMND :
 		scsi_release_buffers(cmd);
-- 
1.8.5.3


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] scsi: remove scsi_end_request
  2014-03-27 16:14 ` [PATCH 2/4] scsi: remove scsi_end_request Christoph Hellwig
@ 2014-03-31  5:26   ` Nicholas A. Bellinger
  0 siblings, 0 replies; 18+ messages in thread
From: Nicholas A. Bellinger @ 2014-03-31  5:26 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi

Hi hch & jejb,

On Thu, 2014-03-27 at 17:14 +0100, Christoph Hellwig wrote:
> Simply the I/O completion logic by folding scsi_end_request into its only
> caller.  There is a single site to either requeue the command or move on
> to the next one instead of of the previous convoluted logic.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi_lib.c |  114 +++++++++++++----------------------------------
>  1 file changed, 32 insertions(+), 82 deletions(-)
> 

Looks like a reasonable simplification.

Reviewed-by: Nicholas Bellinger <nab@linux-iscsi.org>



^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command
  2014-03-27 16:14 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig
@ 2014-03-31  5:45   ` Nicholas A. Bellinger
  2014-03-31  6:08     ` Christoph Hellwig
  2014-03-31  6:56   ` Mike Christie
  2014-03-31 19:00   ` [PATCH 3/4 v2] " Christoph Hellwig
  2 siblings, 1 reply; 18+ messages in thread
From: Nicholas A. Bellinger @ 2014-03-31  5:45 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi

On Thu, 2014-03-27 at 17:14 +0100, Christoph Hellwig wrote:
> Move control of the prep_fn back from the ULDs into the scsi layer.  Besides
> cleaning up the code and removing the only use of the unprep_fn
> requeuest_queue method this also prepares for usinng blk-mq, which doesn't
> have equivalent functionality to the prep_fn method and requires the driver
> to provide just a single I/O submission method.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi_lib.c    |   66 ++++++++++++++++++++++++++------------------
>  drivers/scsi/sd.c          |   46 +++++++++++-------------------
>  drivers/scsi/sr.c          |   19 ++++---------
>  include/scsi/scsi_driver.h |    8 ++----
>  4 files changed, 63 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a73751b..48c5c77 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c

<SNIP>

> @@ -1071,15 +1083,7 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
>  
>  int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>  {
> -	struct scsi_cmnd *cmd;
> -	int ret = scsi_prep_state_check(sdev, req);
> -
> -	if (ret != BLKPREP_OK)
> -		return ret;
> -
> -	cmd = scsi_get_cmd_from_req(sdev, req);
> -	if (unlikely(!cmd))
> -		return BLKPREP_DEFER;
> +	struct scsi_cmnd *cmd = req->special;
>  

Mmm, I thought that req->special was only holding a pointer to a
pre-allocated scsi_cmnd during mq operation, no..?

>  	/*
>  	 * BLOCK_PC requests may transfer data, in which case they must
> @@ -1123,15 +1127,11 @@ EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd);
>   */
>  int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
>  {
> -	struct scsi_cmnd *cmd;
> -	int ret = scsi_prep_state_check(sdev, req);
> -
> -	if (ret != BLKPREP_OK)
> -		return ret;
> +	struct scsi_cmnd *cmd = req->special;
>  

Ditto here for REQ_TYPE_FS_CMND

>  	if (unlikely(sdev->scsi_dh_data && sdev->scsi_dh_data->scsi_dh
>  			 && sdev->scsi_dh_data->scsi_dh->prep_fn)) {
> -		ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req);
> +		int ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req);
>  		if (ret != BLKPREP_OK)
>  			return ret;
>  	}
> @@ -1141,16 +1141,13 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
>  	 */
>  	BUG_ON(!req->nr_phys_segments);
>  
> -	cmd = scsi_get_cmd_from_req(sdev, req);
> -	if (unlikely(!cmd))
> -		return BLKPREP_DEFER;
> -
>  	memset(cmd->cmnd, 0, BLK_MAX_CDB);
>  	return scsi_init_io(cmd, GFP_ATOMIC);
>  }
>  EXPORT_SYMBOL(scsi_setup_fs_cmnd);
>  
> -int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
> +static int
> +scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
>  {
>  	int ret = BLKPREP_OK;
>  
> @@ -1202,9 +1199,9 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
>  	}
>  	return ret;
>  }
> -EXPORT_SYMBOL(scsi_prep_state_check);
>  
> -int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
> +static int
> +scsi_prep_return(struct request_queue *q, struct request *req, int ret)
>  {
>  	struct scsi_device *sdev = q->queuedata;
>  
> @@ -1235,18 +1232,33 @@ int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL(scsi_prep_return);
>  
> -int scsi_prep_fn(struct request_queue *q, struct request *req)
> +static int scsi_prep_fn(struct request_queue *q, struct request *req)
>  {
>  	struct scsi_device *sdev = q->queuedata;
> -	int ret = BLKPREP_KILL;
> +	struct scsi_cmnd *cmd;
> +	int ret;
>  
> -	if (req->cmd_type == REQ_TYPE_BLOCK_PC)
> +	ret = scsi_prep_state_check(sdev, req);
> +	if (ret != BLKPREP_OK)
> +		goto out;
> +
> +	cmd = scsi_get_cmd_from_req(sdev, req);
> +	if (unlikely(!cmd)) {
> +		ret = BLKPREP_DEFER;
> +		goto out;
> +	}
> +

>From here the req->special pointer may or may not be set depending on mq
operation, no..?

> +	if (req->cmd_type == REQ_TYPE_FS)
> +		ret = scsi_cmd_to_driver(cmd)->init_command(cmd);
> +	else if (req->cmd_type == REQ_TYPE_BLOCK_PC)
>  		ret = scsi_setup_blk_pc_cmnd(sdev, req);
> +	else
> +		ret = BLKPREP_KILL;
> +
> +out:
>  	return scsi_prep_return(q, req, ret);
>  }
> -EXPORT_SYMBOL(scsi_prep_fn);
>  
>  /*
>   * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 89e6c04..d95c4fd 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c

<SNIP>

> @@ -886,12 +882,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  	} else if (rq->cmd_flags & REQ_FLUSH) {
>  		ret = scsi_setup_flush_cmnd(sdp, rq);
>  		goto out;
> -	} else if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
> -		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
> -		goto out;
> -	} else if (rq->cmd_type != REQ_TYPE_FS) {
> -		ret = BLKPREP_KILL;
> -		goto out;
>  	}
>  	ret = scsi_setup_fs_cmnd(sdp, rq);
>  	if (ret != BLKPREP_OK)

And this currently assumes req->special is always set when calling
scsi_setup_fs_cmnd()

> @@ -903,11 +893,10 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  	 * is used for a killable error condition */
>  	ret = BLKPREP_KILL;
>  
> -	SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt,
> -					"sd_prep_fn: block=%llu, "
> -					"count=%d\n",
> -					(unsigned long long)block,
> -					this_count));
> +	SCSI_LOG_HLQUEUE(1,
> +		scmd_printk(KERN_INFO, SCpnt,
> +			"%s: block=%llu, count=%d\n",
> +			__func__, (unsigned long long)block, this_count));
>  
>  	if (!sdp || !scsi_device_online(sdp) ||
>  	    block + blk_rq_sectors(rq) > get_capacity(disk)) {
> @@ -1127,7 +1116,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  	 */
>  	ret = BLKPREP_OK;
>   out:
> -	return scsi_prep_return(q, rq, ret);
> +	return ret;
>  }
>  
>  /**
> @@ -1663,6 +1652,8 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  	unsigned char op = SCpnt->cmnd[0];
>  	unsigned char unmap = SCpnt->cmnd[1] & 8;
>  
> +	sd_uninit_command(SCpnt);
> +
>  	if (req->cmd_flags & REQ_DISCARD || req->cmd_flags & REQ_WRITE_SAME) {
>  		if (!result) {
>  			good_bytes = blk_rq_bytes(req);
> @@ -2878,9 +2869,6 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>  
>  	sd_revalidate_disk(gd);
>  
> -	blk_queue_prep_rq(sdp->request_queue, sd_prep_fn);
> -	blk_queue_unprep_rq(sdp->request_queue, sd_unprep_fn);
> -
>  	gd->driverfs_dev = &sdp->sdev_gendev;
>  	gd->flags = GENHD_FL_EXT_DEVT;
>  	if (sdp->removable) {
> @@ -3027,8 +3015,6 @@ static int sd_remove(struct device *dev)
>  	scsi_autopm_get_device(sdkp->device);
>  
>  	async_synchronize_full_domain(&scsi_sd_probe_domain);
> -	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
> -	blk_queue_unprep_rq(sdkp->device->request_queue, NULL);
>  	device_del(&sdkp->dev);
>  	del_gendisk(sdkp->disk);
>  	sd_shutdown(dev);
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 40d8592..93cbd36 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c

<SNIP>

> @@ -378,21 +380,14 @@ static int sr_done(struct scsi_cmnd *SCpnt)
>  	return good_bytes;
>  }
>  
> -static int sr_prep_fn(struct request_queue *q, struct request *rq)
> +static int sr_init_command(struct scsi_cmnd *SCpnt)
>  {
>  	int block = 0, this_count, s_size;
>  	struct scsi_cd *cd;
> -	struct scsi_cmnd *SCpnt;
> -	struct scsi_device *sdp = q->queuedata;
> +	struct request *rq = SCpnt->request;
> +	struct scsi_device *sdp = SCpnt->device;
>  	int ret;
>  
> -	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
> -		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
> -		goto out;
> -	} else if (rq->cmd_type != REQ_TYPE_FS) {
> -		ret = BLKPREP_KILL;
> -		goto out;
> -	}
>  	ret = scsi_setup_fs_cmnd(sdp, rq);
>  	if (ret != BLKPREP_OK)
>  		goto out;

Ditto to the scsi_setup_fs_cmnd() call here as well.

--nab


^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] scsi: handle command allocation failure in scsi_reset_provider
  2014-03-27 16:14 ` [PATCH 4/4] scsi: handle command allocation failure in scsi_reset_provider Christoph Hellwig
@ 2014-03-31  5:45   ` Nicholas A. Bellinger
  0 siblings, 0 replies; 18+ messages in thread
From: Nicholas A. Bellinger @ 2014-03-31  5:45 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi

On Thu, 2014-03-27 at 17:14 +0100, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi_error.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Nicholas Bellinger <nab@linux-iscsi.org>


^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] scsi: explicitly release bidi buffers
  2014-03-27 16:14 ` [PATCH 1/4] scsi: explicitly release bidi buffers Christoph Hellwig
@ 2014-03-31  5:58   ` Mike Christie
  2014-03-31  6:10     ` Christoph Hellwig
  0 siblings, 1 reply; 18+ messages in thread
From: Mike Christie @ 2014-03-31  5:58 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi

On 03/27/2014 11:14 AM, Christoph Hellwig wrote:
> +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);
> +	kmem_cache_free(scsi_sdb_cache, bidi_sdb);
> +	cmd->request->next_rq->special = NULL;
> +}
> +
>  /**
>   * __scsi_error_from_host_byte - translate SCSI error code into errno
>   * @cmd:	SCSI command (unused)
> @@ -800,6 +792,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  			req->next_rq->resid_len = scsi_in(cmd)->resid;
>  
>  			scsi_release_buffers(cmd);
> +			scsi_release_bidi_buffers(cmd);
>  			blk_end_request_all(req, 0);
>  
>  			scsi_next_command(cmd);
> 

Hey,

I just wanted to double check that the only time we do bidi requests is
when they come through as REQ_TYPE_BLOCK_PC requests. I saw some of the
code comments on the init side that indicated that, but there was that
scsi_end_request() -> scsi_release_buffers() -> __scsi_release_buffers()
 call which then did not make sense because we were passing in 1 for the
bidi check argument.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command
  2014-03-31  5:45   ` Nicholas A. Bellinger
@ 2014-03-31  6:08     ` Christoph Hellwig
  2014-03-31  6:20       ` Nicholas A. Bellinger
  0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2014-03-31  6:08 UTC (permalink / raw)
  To: Nicholas A. Bellinger; +Cc: Christoph Hellwig, James Bottomley, linux-scsi

On Sun, Mar 30, 2014 at 10:45:04PM -0700, Nicholas A. Bellinger wrote:
> >  int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
> >  {
> > -	struct scsi_cmnd *cmd;
> > -	int ret = scsi_prep_state_check(sdev, req);
> > -
> > -	if (ret != BLKPREP_OK)
> > -		return ret;
> > -
> > -	cmd = scsi_get_cmd_from_req(sdev, req);
> > -	if (unlikely(!cmd))
> > -		return BLKPREP_DEFER;
> > +	struct scsi_cmnd *cmd = req->special;
> >  
> 
> Mmm, I thought that req->special was only holding a pointer to a
> pre-allocated scsi_cmnd during mq operation, no..?

For the mq case the block layer sets up req->special.  For the old case
scsi_get_cmd_from_req sets up req->special the first it is called, and
leaves it in there until the command is done.

> > +	ret = scsi_prep_state_check(sdev, req);
> > +	if (ret != BLKPREP_OK)
> > +		goto out;
> > +
> > +	cmd = scsi_get_cmd_from_req(sdev, req);
> > +	if (unlikely(!cmd)) {
> > +		ret = BLKPREP_DEFER;
> > +		goto out;
> > +	}
> > +


>From here the req->special pointer may or may not be set depending on mq
operation, no..?

a) there is no blk-mq support yet in James tree (+ these patches)
b) even in my scsi-mq tree this code path won't be called for the mq case
c) after scsi_get_cmd_from_req returns req->special will always be set up
   to point to the scsi_cmnd:

static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
		struct request *req)
{
	struct scsi_cmnd *cmd;

	if (!req->special) {
		...

		req->special = cmd;
	} else {
		cmd = req->special;
	}

        /* pull a tag out of the request if we have one */
	cmd->tag = req->tag;
	cmd->request = req;

	cmd->cmnd = req->cmd;   
	cmd->prot_op = SCSI_PROT_NORMAL;

	return cmd;
}

> >  	}
> >  	ret = scsi_setup_fs_cmnd(sdp, rq);
> >  	if (ret != BLKPREP_OK)
> 
> And this currently assumes req->special is always set when calling
> scsi_setup_fs_cmnd()

That is the case both before and after this patch.


^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] scsi: explicitly release bidi buffers
  2014-03-31  5:58   ` Mike Christie
@ 2014-03-31  6:10     ` Christoph Hellwig
  0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2014-03-31  6:10 UTC (permalink / raw)
  To: Mike Christie; +Cc: James Bottomley, linux-scsi

On Mon, Mar 31, 2014 at 12:58:01AM -0500, Mike Christie wrote:
> 
> Hey,
> 
> I just wanted to double check that the only time we do bidi requests is
> when they come through as REQ_TYPE_BLOCK_PC requests. I saw some of the
> code comments on the init side that indicated that, but there was that
> scsi_end_request() -> scsi_release_buffers() -> __scsi_release_buffers()
>  call which then did not make sense because we were passing in 1 for the
> bidi check argument.

We only support BIDI for REQ_TYPE_BLOCK_PC requests, and scsi_io_completion
enforces this:

	if (req->cmd_type == REQ_TYPE_BLOCK_PC) { /* SG_IO ioctl from block level */
		...

		if (scsi_bidi_cmnd(cmd)) {
			...
			< end_io and return >
		}
	}

	/* no bidi support for !REQ_TYPE_BLOCK_PC yet */
	BUG_ON(blk_bidi_rq(req));


^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: misc scsi midlayer updates
  2014-03-30 15:33 ` misc scsi midlayer updates Boaz Harrosh
@ 2014-03-31  6:20   ` Christoph Hellwig
  0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2014-03-31  6:20 UTC (permalink / raw)
  To: Boaz Harrosh; +Cc: James Bottomley, linux-scsi

Hi Boaz,

> Hi Christoph
> 
> Many years ago I have sent these exact patches but no-one cared Including
> me I guess.

I didn't remember your older patches, sorry.

> I think my:
> 	scsi_lib: Remove that __scsi_release_buffers contraption
> Is more elegant, is layered better and is smaller code. (please
> consider my version)

I very much disagree - the bidi code uses a separate request for it's payload,
uses separate functions to set it up at the low-level so mirroring it with
a separate teardown makes sense.  This also avoids having to do any bidi
check at all in the fast path.

> Also the first patch is some more cleanup you'd like.

Doesn't look bad, but not that importan either.

> The main patch of collapsing  scsi_end_request is basically the same.

I like the goto version better beause it avoids additional duplication from
inside the switch and the bidi path, but it should be functionally equivalent.


> Please note the 4th patch which is a real BUG, titled:
> 	scsi_lib: Can't RETRY scsi_cmnd if some bytes were completed

That fix seems very hard to read due to the arithmetic comparism on the enum
value.  The way I try to understand it is that you never want to retry
if ((an error happened) && (bytes were completed)) but the explanation
should be expanded.

> [Your patches have been tested within my MQ testing right?]

Yes.


^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command
  2014-03-31  6:08     ` Christoph Hellwig
@ 2014-03-31  6:20       ` Nicholas A. Bellinger
  0 siblings, 0 replies; 18+ messages in thread
From: Nicholas A. Bellinger @ 2014-03-31  6:20 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi

On Mon, 2014-03-31 at 08:08 +0200, Christoph Hellwig wrote:
> On Sun, Mar 30, 2014 at 10:45:04PM -0700, Nicholas A. Bellinger wrote:
> > >  int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
> > >  {
> > > -	struct scsi_cmnd *cmd;
> > > -	int ret = scsi_prep_state_check(sdev, req);
> > > -
> > > -	if (ret != BLKPREP_OK)
> > > -		return ret;
> > > -
> > > -	cmd = scsi_get_cmd_from_req(sdev, req);
> > > -	if (unlikely(!cmd))
> > > -		return BLKPREP_DEFER;
> > > +	struct scsi_cmnd *cmd = req->special;
> > >  
> > 
> > Mmm, I thought that req->special was only holding a pointer to a
> > pre-allocated scsi_cmnd during mq operation, no..?
> 
> For the mq case the block layer sets up req->special.  For the old case
> scsi_get_cmd_from_req sets up req->special the first it is called, and
> leaves it in there until the command is done.

Er, yes of course.

Reviewed-by: Nicholas Bellinger <nab@linux-iscsi.org>


^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command
  2014-03-27 16:14 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig
  2014-03-31  5:45   ` Nicholas A. Bellinger
@ 2014-03-31  6:56   ` Mike Christie
  2014-03-31  8:09     ` Christoph Hellwig
  2014-03-31 19:00   ` [PATCH 3/4 v2] " Christoph Hellwig
  2 siblings, 1 reply; 18+ messages in thread
From: Mike Christie @ 2014-03-31  6:56 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi

On 03/27/2014 11:14 AM, Christoph Hellwig wrote:
> @@ -1663,6 +1652,8 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  	unsigned char op = SCpnt->cmnd[0];
>  	unsigned char unmap = SCpnt->cmnd[1] & 8;
>  
> +	sd_uninit_command(SCpnt);
> +

The above call would free the cmnd->cmnd and set it to null. If then
scsi_io_completion was going to do some error processing it looks like
it could try to access the scsi_cmnd->cmnd field.

With the current code that would not be a problem because the blk unprep
callback is not called until the block layer does its request cleanup in
blk_finish_request which as you know is after
scsi_io_completion/scsi_end_request is done with the cmnd.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command
  2014-03-31  6:56   ` Mike Christie
@ 2014-03-31  8:09     ` Christoph Hellwig
  0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2014-03-31  8:09 UTC (permalink / raw)
  To: Mike Christie; +Cc: Christoph Hellwig, James Bottomley, linux-scsi

On Mon, Mar 31, 2014 at 01:56:13AM -0500, Mike Christie wrote:
> The above call would free the cmnd->cmnd and set it to null. If then
> scsi_io_completion was going to do some error processing it looks like
> it could try to access the scsi_cmnd->cmnd field.
> 
> With the current code that would not be a problem because the blk unprep
> callback is not called until the block layer does its request cleanup in
> blk_finish_request which as you know is after
> scsi_io_completion/scsi_end_request is done with the cmnd.

Right, we should probabl call ->uninit_command at that place as well
instead of calling it internall in ->done.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH 3/4 v2] scsi: reintroduce scsi_driver.init_command
  2014-03-27 16:14 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig
  2014-03-31  5:45   ` Nicholas A. Bellinger
  2014-03-31  6:56   ` Mike Christie
@ 2014-03-31 19:00   ` Christoph Hellwig
  2 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2014-03-31 19:00 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi

Move control of the prep_fn back from the ULDs into the scsi layer.  Besides
cleaning up the code this also prepares for usinng blk-mq, which doesn't
have equivalent functionality to the prep_fn method and requires the driver
to provide just a single I/O submission method.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c    |   66 ++++++++++++++++++++++++++------------------
 drivers/scsi/sd.c          |   44 ++++++++++-------------------
 drivers/scsi/sr.c          |   19 ++++---------
 include/scsi/scsi_driver.h |    8 ++----
 4 files changed, 61 insertions(+), 76 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a73751b..ed81fc3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1071,15 +1071,7 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
 
 int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 {
-	struct scsi_cmnd *cmd;
-	int ret = scsi_prep_state_check(sdev, req);
-
-	if (ret != BLKPREP_OK)
-		return ret;
-
-	cmd = scsi_get_cmd_from_req(sdev, req);
-	if (unlikely(!cmd))
-		return BLKPREP_DEFER;
+	struct scsi_cmnd *cmd = req->special;
 
 	/*
 	 * BLOCK_PC requests may transfer data, in which case they must
@@ -1123,15 +1115,11 @@ EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd);
  */
 int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 {
-	struct scsi_cmnd *cmd;
-	int ret = scsi_prep_state_check(sdev, req);
-
-	if (ret != BLKPREP_OK)
-		return ret;
+	struct scsi_cmnd *cmd = req->special;
 
 	if (unlikely(sdev->scsi_dh_data && sdev->scsi_dh_data->scsi_dh
 			 && sdev->scsi_dh_data->scsi_dh->prep_fn)) {
-		ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req);
+		int ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req);
 		if (ret != BLKPREP_OK)
 			return ret;
 	}
@@ -1141,16 +1129,13 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 	 */
 	BUG_ON(!req->nr_phys_segments);
 
-	cmd = scsi_get_cmd_from_req(sdev, req);
-	if (unlikely(!cmd))
-		return BLKPREP_DEFER;
-
 	memset(cmd->cmnd, 0, BLK_MAX_CDB);
 	return scsi_init_io(cmd, GFP_ATOMIC);
 }
 EXPORT_SYMBOL(scsi_setup_fs_cmnd);
 
-int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
+static int
+scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 {
 	int ret = BLKPREP_OK;
 
@@ -1202,9 +1187,9 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 	}
 	return ret;
 }
-EXPORT_SYMBOL(scsi_prep_state_check);
 
-int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
+static int
+scsi_prep_return(struct request_queue *q, struct request *req, int ret)
 {
 	struct scsi_device *sdev = q->queuedata;
 
@@ -1235,18 +1220,44 @@ int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
 
 	return ret;
 }
-EXPORT_SYMBOL(scsi_prep_return);
 
-int scsi_prep_fn(struct request_queue *q, struct request *req)
+static int scsi_prep_fn(struct request_queue *q, struct request *req)
 {
 	struct scsi_device *sdev = q->queuedata;
-	int ret = BLKPREP_KILL;
+	struct scsi_cmnd *cmd;
+	int ret;
 
-	if (req->cmd_type == REQ_TYPE_BLOCK_PC)
+	ret = scsi_prep_state_check(sdev, req);
+	if (ret != BLKPREP_OK)
+		goto out;
+
+	cmd = scsi_get_cmd_from_req(sdev, req);
+	if (unlikely(!cmd)) {
+		ret = BLKPREP_DEFER;
+		goto out;
+	}
+
+	if (req->cmd_type == REQ_TYPE_FS)
+		ret = scsi_cmd_to_driver(cmd)->init_command(cmd);
+	else if (req->cmd_type == REQ_TYPE_BLOCK_PC)
 		ret = scsi_setup_blk_pc_cmnd(sdev, req);
+	else
+		ret = BLKPREP_KILL;
+
+out:
 	return scsi_prep_return(q, req, ret);
 }
-EXPORT_SYMBOL(scsi_prep_fn);
+
+static void scsi_unprep_fn(struct request_queue *q, struct request *req)
+{
+	if (req->cmd_type == REQ_TYPE_FS) {
+		struct scsi_cmnd *cmd = req->special;
+		struct scsi_driver *drv = scsi_cmd_to_driver(cmd);
+
+		if (drv->uninit_command)
+			drv->uninit_command(cmd);
+	}
+}
 
 /*
  * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else
@@ -1667,6 +1678,7 @@ struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
 		return NULL;
 
 	blk_queue_prep_rq(q, scsi_prep_fn);
+	blk_queue_unprep_rq(q, scsi_unprep_fn);
 	blk_queue_softirq_done(q, scsi_softirq_done);
 	blk_queue_rq_timed_out(q, scsi_times_out);
 	blk_queue_lld_busy(q, scsi_lld_busy);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 89e6c04..d99cb3f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -109,6 +109,8 @@ static int sd_suspend_system(struct device *);
 static int sd_suspend_runtime(struct device *);
 static int sd_resume(struct device *);
 static void sd_rescan(struct device *);
+static int sd_init_command(struct scsi_cmnd *SCpnt);
+static void sd_uninit_command(struct scsi_cmnd *SCpnt);
 static int sd_done(struct scsi_cmnd *);
 static int sd_eh_action(struct scsi_cmnd *, int);
 static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
@@ -503,6 +505,8 @@ static struct scsi_driver sd_template = {
 		.pm		= &sd_pm_ops,
 	},
 	.rescan			= sd_rescan,
+	.init_command		= sd_init_command,
+	.uninit_command		= sd_uninit_command,
 	.done			= sd_done,
 	.eh_action		= sd_eh_action,
 };
@@ -838,9 +842,9 @@ static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
 	return scsi_setup_blk_pc_cmnd(sdp, rq);
 }
 
-static void sd_unprep_fn(struct request_queue *q, struct request *rq)
+static void sd_uninit_command(struct scsi_cmnd *SCpnt)
 {
-	struct scsi_cmnd *SCpnt = rq->special;
+	struct request *rq = SCpnt->request;
 
 	if (rq->cmd_flags & REQ_DISCARD) {
 		free_page((unsigned long)rq->buffer);
@@ -853,18 +857,10 @@ static void sd_unprep_fn(struct request_queue *q, struct request *rq)
 	}
 }
 
-/**
- *	sd_prep_fn - build a scsi (read or write) command from
- *	information in the request structure.
- *	@SCpnt: pointer to mid-level's per scsi command structure that
- *	contains request and into which the scsi command is written
- *
- *	Returns 1 if successful and 0 if error (or cannot be done now).
- **/
-static int sd_prep_fn(struct request_queue *q, struct request *rq)
+static int sd_init_command(struct scsi_cmnd *SCpnt)
 {
-	struct scsi_cmnd *SCpnt;
-	struct scsi_device *sdp = q->queuedata;
+	struct request *rq = SCpnt->request;
+	struct scsi_device *sdp = SCpnt->device;
 	struct gendisk *disk = rq->rq_disk;
 	struct scsi_disk *sdkp;
 	sector_t block = blk_rq_pos(rq);
@@ -886,12 +882,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	} else if (rq->cmd_flags & REQ_FLUSH) {
 		ret = scsi_setup_flush_cmnd(sdp, rq);
 		goto out;
-	} else if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
-		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
-		goto out;
-	} else if (rq->cmd_type != REQ_TYPE_FS) {
-		ret = BLKPREP_KILL;
-		goto out;
 	}
 	ret = scsi_setup_fs_cmnd(sdp, rq);
 	if (ret != BLKPREP_OK)
@@ -903,11 +893,10 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	 * is used for a killable error condition */
 	ret = BLKPREP_KILL;
 
-	SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt,
-					"sd_prep_fn: block=%llu, "
-					"count=%d\n",
-					(unsigned long long)block,
-					this_count));
+	SCSI_LOG_HLQUEUE(1,
+		scmd_printk(KERN_INFO, SCpnt,
+			"%s: block=%llu, count=%d\n",
+			__func__, (unsigned long long)block, this_count));
 
 	if (!sdp || !scsi_device_online(sdp) ||
 	    block + blk_rq_sectors(rq) > get_capacity(disk)) {
@@ -1127,7 +1116,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	 */
 	ret = BLKPREP_OK;
  out:
-	return scsi_prep_return(q, rq, ret);
+	return ret;
 }
 
 /**
@@ -2878,9 +2867,6 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 
 	sd_revalidate_disk(gd);
 
-	blk_queue_prep_rq(sdp->request_queue, sd_prep_fn);
-	blk_queue_unprep_rq(sdp->request_queue, sd_unprep_fn);
-
 	gd->driverfs_dev = &sdp->sdev_gendev;
 	gd->flags = GENHD_FL_EXT_DEVT;
 	if (sdp->removable) {
@@ -3027,8 +3013,6 @@ static int sd_remove(struct device *dev)
 	scsi_autopm_get_device(sdkp->device);
 
 	async_synchronize_full_domain(&scsi_sd_probe_domain);
-	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
-	blk_queue_unprep_rq(sdkp->device->request_queue, NULL);
 	device_del(&sdkp->dev);
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 40d8592..93cbd36 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -79,6 +79,7 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_WORM);
 static DEFINE_MUTEX(sr_mutex);
 static int sr_probe(struct device *);
 static int sr_remove(struct device *);
+static int sr_init_command(struct scsi_cmnd *SCpnt);
 static int sr_done(struct scsi_cmnd *);
 static int sr_runtime_suspend(struct device *dev);
 
@@ -94,6 +95,7 @@ static struct scsi_driver sr_template = {
 		.remove		= sr_remove,
 		.pm		= &sr_pm_ops,
 	},
+	.init_command		= sr_init_command,
 	.done			= sr_done,
 };
 
@@ -378,21 +380,14 @@ static int sr_done(struct scsi_cmnd *SCpnt)
 	return good_bytes;
 }
 
-static int sr_prep_fn(struct request_queue *q, struct request *rq)
+static int sr_init_command(struct scsi_cmnd *SCpnt)
 {
 	int block = 0, this_count, s_size;
 	struct scsi_cd *cd;
-	struct scsi_cmnd *SCpnt;
-	struct scsi_device *sdp = q->queuedata;
+	struct request *rq = SCpnt->request;
+	struct scsi_device *sdp = SCpnt->device;
 	int ret;
 
-	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
-		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
-		goto out;
-	} else if (rq->cmd_type != REQ_TYPE_FS) {
-		ret = BLKPREP_KILL;
-		goto out;
-	}
 	ret = scsi_setup_fs_cmnd(sdp, rq);
 	if (ret != BLKPREP_OK)
 		goto out;
@@ -517,7 +512,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	 */
 	ret = BLKPREP_OK;
  out:
-	return scsi_prep_return(q, rq, ret);
+	return ret;
 }
 
 static int sr_block_open(struct block_device *bdev, fmode_t mode)
@@ -718,7 +713,6 @@ static int sr_probe(struct device *dev)
 
 	/* FIXME: need to handle a get_capabilities failure properly ?? */
 	get_capabilities(cd);
-	blk_queue_prep_rq(sdev->request_queue, sr_prep_fn);
 	sr_vendor_init(cd);
 
 	disk->driverfs_dev = &sdev->sdev_gendev;
@@ -993,7 +987,6 @@ static int sr_remove(struct device *dev)
 
 	scsi_autopm_get_device(cd->device);
 
-	blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn);
 	del_gendisk(cd->disk);
 
 	mutex_lock(&sr_ref_mutex);
diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
index 20fdfc2..b507729 100644
--- a/include/scsi/scsi_driver.h
+++ b/include/scsi/scsi_driver.h
@@ -6,15 +6,14 @@
 struct module;
 struct scsi_cmnd;
 struct scsi_device;
-struct request;
-struct request_queue;
-
 
 struct scsi_driver {
 	struct module		*owner;
 	struct device_driver	gendrv;
 
 	void (*rescan)(struct device *);
+	int (*init_command)(struct scsi_cmnd *);
+	void (*uninit_command)(struct scsi_cmnd *);
 	int (*done)(struct scsi_cmnd *);
 	int (*eh_action)(struct scsi_cmnd *, int);
 };
@@ -31,8 +30,5 @@ extern int scsi_register_interface(struct class_interface *);
 
 int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req);
 int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req);
-int scsi_prep_state_check(struct scsi_device *sdev, struct request *req);
-int scsi_prep_return(struct request_queue *q, struct request *req, int ret);
-int scsi_prep_fn(struct request_queue *, struct request *);
 
 #endif /* _SCSI_SCSI_DRIVER_H */
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: misc scsi midlayer updates
  2014-03-27 16:14 misc scsi midlayer updates Christoph Hellwig
                   ` (4 preceding siblings ...)
  2014-03-30 15:33 ` misc scsi midlayer updates Boaz Harrosh
@ 2014-03-31 21:55 ` Mike Christie
  5 siblings, 0 replies; 18+ messages in thread
From: Mike Christie @ 2014-03-31 21:55 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi

On 03/27/2014 11:14 AM, Christoph Hellwig wrote:
> Various patches from the scsi multiqueue development that make sense on their
> own.


With the updated patch "[PATCH 3/4 v2] scsi: reintroduce
scsi_driver.init_command", they look ok to me.

Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>

^ permalink raw reply	[flat|nested] 18+ messages in thread
end of thread, other threads:[~2014-03-31 21:56 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-27 16:14 misc scsi midlayer updates Christoph Hellwig
2014-03-27 16:14 ` [PATCH 1/4] scsi: explicitly release bidi buffers Christoph Hellwig
2014-03-31  5:58   ` Mike Christie
2014-03-31  6:10     ` Christoph Hellwig
2014-03-27 16:14 ` [PATCH 2/4] scsi: remove scsi_end_request Christoph Hellwig
2014-03-31  5:26   ` Nicholas A. Bellinger
2014-03-27 16:14 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig
2014-03-31  5:45   ` Nicholas A. Bellinger
2014-03-31  6:08     ` Christoph Hellwig
2014-03-31  6:20       ` Nicholas A. Bellinger
2014-03-31  6:56   ` Mike Christie
2014-03-31  8:09     ` Christoph Hellwig
2014-03-31 19:00   ` [PATCH 3/4 v2] " Christoph Hellwig
2014-03-27 16:14 ` [PATCH 4/4] scsi: handle command allocation failure in scsi_reset_provider Christoph Hellwig
2014-03-31  5:45   ` Nicholas A. Bellinger
2014-03-30 15:33 ` misc scsi midlayer updates Boaz Harrosh
2014-03-31  6:20   ` Christoph Hellwig
2014-03-31 21:55 ` Mike Christie

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.