misc scsi midlayer updates

PATCH In review Christoph Hellwig 5 participants 18 messages Mar 27, 2014 – Mar 31, 2014 SCSI

lore

Share & tools

lore

Christoph Hellwig

author

2014-03-27 16:30 UTC #0 lore raw

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

Christoph Hellwig

author

2014-03-27 16:30 UTC #1 lore raw

PATCH 1/4 scsi: explicitly release bidi buffers

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 <[email protected]>

---
 drivers/scsi/scsi_lib.c |   47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

drivers/scsi/scsi_lib.c +20 ~~-27~~

index 5681c05..5454d7d 100644
@@ -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);

Signature · 1 line

1.7.10.4

Mike Christie

2014-03-31 05:58 UTC #2 lore raw

Re: [PATCH 1/4] scsi: explicitly release bidi buffers

Quoted 14 lines · Christoph Hellwig

drivers/scsi/scsi_lib.c +9 -0

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

drivers/scsi/scsi_lib.c void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes) · line 798

Quoted 7 lines

drivers/scsi/scsi_lib.c +1 -0

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

Christoph Hellwig

author

2014-03-31 06:10 UTC #3 lore raw

Re: [PATCH 1/4] scsi: explicitly release bidi buffers

Quoted 9 lines · Mike Christie

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));

Christoph Hellwig

author

2014-03-27 16:30 UTC #4 lore raw

PATCH 2/4 scsi: remove scsi_end_request

In reply to Christoph Hellwig

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 <[email protected]>

---
 drivers/scsi/scsi_lib.c |  114 +++++++++++++----------------------------------
 1 file changed, 32 insertions(+), 82 deletions(-)

drivers/scsi/scsi_lib.c +32 ~~-82~~

index 5454d7d..a73751b 100644
@@ -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,

Signature · 1 line

1.7.10.4

Nicholas A. Bellinger

Reviewed-by on v1 2/4

2014-03-31 05:26 UTC #5 lore raw

Re: [PATCH 2/4] scsi: remove scsi_end_request

Hi hch & jejb,

Quoted 9 lines · Christoph Hellwig

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 <[email protected]>
---
 drivers/scsi/scsi_lib.c |  114 +++++++++++++----------------------------------
 1 file changed, 32 insertions(+), 82 deletions(-)

Looks like a reasonable simplification.

Reviewed-by Nicholas Bellinger <[email protected]>

Christoph Hellwig

author

2014-03-27 16:30 UTC #6 lore raw

PATCH 3/4 scsi: reintroduce scsi_driver.init_command

In reply to Christoph Hellwig

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 <[email protected]>

---
 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(-)

drivers/scsi/scsi_lib.c +39 ~~-27~~

index a73751b..48c5c77 100644
@@ -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

drivers/scsi/sd.c +16 ~~-30~~

108 lines not on this page · raw message

drivers/scsi/sr.c +6 ~~-13~~

index 40d8592..93cbd36 100644
@@ -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);

include/scsi/scsi_driver.h +2 -6

index 20fdfc2..b507729 100644
@@ -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 */

Signature · 1 line

1.7.10.4

Nicholas A. Bellinger

2014-03-31 05:45 UTC #7 lore raw

Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command

Quoted 18 lines · Christoph Hellwig

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 <[email protected]>
---
 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(-)
drivers/scsi/scsi_lib.c +0 -0
index a73751b..48c5c77 100644

<SNIP>

Quoted 5 lines

drivers/scsi/scsi_lib.c +0 -1

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

drivers/scsi/scsi_lib.c static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev, · line 1087

Quoted 10 lines

drivers/scsi/scsi_lib.c +1 -8

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

drivers/scsi/scsi_lib.c EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd); · line 1131

Quoted 13 lines

drivers/scsi/scsi_lib.c +1 -5

 	/*
 	 * 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

Quoted 64 lines

drivers/scsi/scsi_lib.c +18 ~~-12~~

 	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;
+	}
+

Quoted 1 line

From here the req->special pointer may or may not be set depending on mq

operation, no..?

Quoted 18 lines

drivers/scsi/scsi_lib.c +7 -1

+	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

drivers/scsi/sd.c +0 -0

index 89e6c04..d95c4fd 100644

<SNIP>

drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 887

Quoted 13 lines

drivers/scsi/sd.c +0 -6

@@ -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()

Quoted 57 lines

drivers/scsi/sd.c +7 ~~-11~~

@@ -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);

drivers/scsi/sr.c +0 -0

index 40d8592..93cbd36 100644

<SNIP>

Quoted 15 lines

drivers/scsi/sr.c +3 -3

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

drivers/scsi/sr.c static int sr_done(struct scsi_cmnd *SCpnt) · line 393

Quoted 10 lines

drivers/scsi/sr.c +0 -7

-	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

Christoph Hellwig

author

2014-03-31 06:08 UTC #8 lore raw

Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command

Quoted 8 lines · Nicholas A. Bellinger

Quoted 8 lines
int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) {
drivers/scsi/scsi_lib.c +0 -6
-	struct scsi_cmnd *cmd;
-	int ret = scsi_prep_state_check(sdev, req);
-
-	if (ret != BLKPREP_OK)
-		return ret;
-

Quoted 8 lines

drivers/scsi/scsi_lib.c static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev, · line 1086
Quoted 5 lines
drivers/scsi/scsi_lib.c +1 -3
-	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.

Quoted 5 lines

drivers/scsi/scsi_lib.c +5 -0

+	ret = scsi_prep_state_check(sdev, req);
+	if (ret != BLKPREP_OK)
+		goto out;
+
+	cmd = scsi_get_cmd_from_req(sdev, req);

Quoted 8 lines

drivers/scsi/scsi_lib.c int scsi_prep_return(struct request_queue *q, struct request *req, int ret) · line 1251
Quoted 5 lines
drivers/scsi/scsi_lib.c +5 -0
+	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;
}

Quoted 6 lines

drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 887
Quoted 3 lines
drivers/scsi/sd.c +0 -0
 	}
 	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.

Nicholas A. Bellinger

Reviewed-by

2014-03-31 06:20 UTC #9 lore raw

Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command

Quoted 18 lines · Christoph Hellwig

Quoted 8 lines · Nicholas A. Bellinger
Quoted 8 lines
int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) {
drivers/scsi/scsi_lib.c +0 -6
-	struct scsi_cmnd *cmd;
-	int ret = scsi_prep_state_check(sdev, req);
-
-	if (ret != BLKPREP_OK)
-		return ret;
-
Quoted 8 lines
drivers/scsi/scsi_lib.c static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev, · line 1086
Quoted 5 lines
drivers/scsi/scsi_lib.c +1 -3
-	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..?

Quoted 3 lines

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 <[email protected]>

Mike Christie

2014-03-31 06:56 UTC #10 lore raw

Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command

In reply to Christoph Hellwig

drivers/scsi/sd.c static int sd_done(struct scsi_cmnd *SCpnt) · line 1656

Quoted 6 lines · Christoph Hellwig

drivers/scsi/sd.c +2 -0

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

Christoph Hellwig

2014-03-31 08:09 UTC #11 lore raw

Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command

Quoted 8 lines · Mike Christie

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.

Christoph Hellwig

2014-03-31 19:00 UTC #12 lore raw

PATCH v2 3/4 scsi: reintroduce scsi_driver.init_command

In reply to Christoph Hellwig

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 <[email protected]>

---
 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(-)

drivers/scsi/scsi_lib.c +39 ~~-27~~

index a73751b..ed81fc3 100644
@@ -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);

drivers/scsi/sd.c +14 ~~-30~~

100 lines not on this page · raw message

drivers/scsi/sr.c +6 ~~-13~~

index 40d8592..93cbd36 100644
@@ -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);

include/scsi/scsi_driver.h +2 -6

index 20fdfc2..b507729 100644
@@ -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 */

Signature · 1 line

1.7.10.4

Christoph Hellwig

author

2014-03-27 16:30 UTC #13 lore raw

PATCH 4/4 scsi: handle command allocation failure in scsi_reset_provider

In reply to Christoph Hellwig

Signed-off-by Christoph Hellwig <[email protected]>

---
 drivers/scsi/scsi_error.c |    6 ++++++
 1 file changed, 6 insertions(+)

drivers/scsi/scsi_error.c +6 -0

index 771c16b..b989d31 100644
@@ -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;

Signature · 1 line

1.7.10.4

Nicholas A. Bellinger

Reviewed-by on v1 4/4

2014-03-31 05:45 UTC #14 lore raw

Re: [PATCH 4/4] scsi: handle command allocation failure in scsi_reset_provider

Quoted 5 lines · Christoph Hellwig

Signed-off-by Christoph Hellwig <[email protected]>
---
 drivers/scsi/scsi_error.c |    6 ++++++
 1 file changed, 6 insertions(+)

Reviewed-by Nicholas Bellinger <[email protected]>

Boaz Harrosh

2014-03-30 15:33 UTC #15 lore raw

In reply to Christoph Hellwig

Quoted 8 lines · Christoph Hellwig

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 [email protected]
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

Quoted 1 line

From 9809b82bbb9813370738ac00400c01d61b0c51b4 Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <[email protected]>
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 <[email protected]>

---
 drivers/scsi/scsi_lib.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/scsi/scsi_lib.c +5 -6

index 62ec84b..dd834ca 100644
@@ -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 */

Signature · 1 line

1.8.5.3

Quoted 1 line

From aac579b02c7d3665665a1f1f7c064a82b70b873a Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <[email protected]>
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 <[email protected]>

---
 drivers/scsi/scsi_lib.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

drivers/scsi/scsi_lib.c +17 ~~-24~~

69 lines not on this page · raw message

Signature · 1 line

1.8.5.3

Quoted 1 line

From a93c49e5cbc1b3bd0bcda49318499b557a25e622 Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <Boaz Harrosh [email protected]>
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 <[email protected]>

CC Alan Stern <[email protected]>

CC Matthew Wilcox <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/scsi_lib.c | 94 +++++++++++--------------------------------------
 1 file changed, 21 insertions(+), 73 deletions(-)

drivers/scsi/scsi_lib.c +21 ~~-73~~

125 lines not on this page · raw message

Signature · 1 line

1.8.5.3

Quoted 1 line

From 3879be0877262643aa0d1884a13e6f7296cb3738 Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <Boaz Harrosh [email protected]>
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 <[email protected]>

---
 drivers/scsi/scsi_lib.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/scsi/scsi_lib.c +6 -0

index fa8612d..50a0bc6 100644
@@ -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);

Signature · 1 line

1.8.5.3

Christoph Hellwig

author

2014-03-31 06:20 UTC #16 lore raw

Hi Boaz,

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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

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

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 1 line

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

Yes.

Mike Christie

Reviewed-by

2014-03-31 21:56 UTC #17 lore raw

In reply to Christoph Hellwig

Quoted 2 lines · Christoph Hellwig

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 <[email protected]>