misc I/O submission cleanups

misc I/O submission cleanups

[Christoph Hellwig]

Hi Martin,

this series tidies up various loose ends in the SCSI I/O submission path.

[PATCH 01/10] scsi: don't export scsi_device_from_queue

[Christoph Hellwig]

This function is only used by code built into scsi_mod.ko.

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f0ee11dc07e4b0..b95e00ff346b09 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1959,7 +1959,6 @@ struct scsi_device *scsi_device_from_queue(struct request_queue *q)
 
 	return sdev;
 }
-EXPORT_SYMBOL_GPL(scsi_device_from_queue);
 
 /**
  * scsi_block_requests - Utility function used by low-level drivers to prevent
-- 
2.28.0

Re: [PATCH 01/10] scsi: don't export scsi_device_from_queue

[Hannes Reinecke]

On 10/5/20 10:41 AM, Christoph Hellwig wrote:
> This function is only used by code built into scsi_mod.ko.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f0ee11dc07e4b0..b95e00ff346b09 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1959,7 +1959,6 @@ struct scsi_device *scsi_device_from_queue(struct request_queue *q)
>   
>   	return sdev;
>   }
> -EXPORT_SYMBOL_GPL(scsi_device_from_queue);
>   
>   /**
>    * scsi_block_requests - Utility function used by low-level drivers to prevent
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 01/10] scsi: don't export scsi_device_from_queue

[Martin K. Petersen]

On Mon, 5 Oct 2020 10:41:21 +0200, Christoph Hellwig wrote:

> This function is only used by code built into scsi_mod.ko.

Applied to 5.10/scsi-queue, thanks!

[01/10] scsi: core: Don't export scsi_device_from_queue()
        https://git.kernel.org/mkp/scsi/c/2ba87c43872f
[02/10] scsi: core: Remove scsi_init_cmd_errh
        https://git.kernel.org/mkp/scsi/c/3a8dc5bbc8c0
[03/10] scsi: core: Move command size detection out of the fast path
        https://git.kernel.org/mkp/scsi/c/2ceda20f0a99
[05/10] scsi: core: Use rq_dma_dir in scsi_setup_cmnd()
        https://git.kernel.org/mkp/scsi/c/40b93836a136
[06/10] scsi: core: Rename scsi_prep_state_check() to scsi_device_state_check()
        https://git.kernel.org/mkp/scsi/c/822bd2db798b
[07/10] scsi: core: Rename scsi_mq_prep_fn() to scsi_prepare_cmd()
        https://git.kernel.org/mkp/scsi/c/5843cc3d5acd
[08/10] scsi: core: Clean up allocation and freeing of sgtables
        https://git.kernel.org/mkp/scsi/c/7007e9dd5676
[09/10] scsi: core: Remove scsi_setup_cmnd() and scsi_setup_fs_cmnd()
        https://git.kernel.org/mkp/scsi/c/74e5e6c1b18c
[10/10] scsi: core: Only start the request just before dispatching
        https://git.kernel.org/mkp/scsi/c/ed7fb2d018fd

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 02/10] scsi: remove scsi_init_cmd_errh

[Christoph Hellwig]

There is no good reason to keep this functionality as a separate
function, just merge it into the only caller.

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b95e00ff346b09..8a7ae46b5943da 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -293,21 +293,6 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 }
 EXPORT_SYMBOL(__scsi_execute);
 
-/**
- * scsi_init_cmd_errh - Initialize cmd fields related to error handling.
- * @cmd:  command that is ready to be queued.
- *
- * This function has the job of initializing a number of fields related to error
- * handling. Typically this will be called once for each command, as required.
- */
-static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
-{
-	scsi_set_resid(cmd, 0);
-	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
-	if (cmd->cmd_len == 0)
-		cmd->cmd_len = scsi_command_size(cmd->cmnd);
-}
-
 /*
  * Wake up the error handler if necessary. Avoid as follows that the error
  * handler is not woken up if host in-flight requests number ==
@@ -1698,7 +1683,10 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (bd->last)
 		cmd->flags |= SCMD_LAST;
 
-	scsi_init_cmd_errh(cmd);
+	scsi_set_resid(cmd, 0);
+	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
+	if (cmd->cmd_len == 0)
+		cmd->cmd_len = scsi_command_size(cmd->cmnd);
 	cmd->scsi_done = scsi_mq_done;
 
 	reason = scsi_dispatch_cmd(cmd);
-- 
2.28.0

Re: [PATCH 02/10] scsi: remove scsi_init_cmd_errh

[Hannes Reinecke]

On 10/5/20 10:41 AM, Christoph Hellwig wrote:
> There is no good reason to keep this functionality as a separate
> function, just merge it into the only caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c | 20 ++++----------------
>   1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index b95e00ff346b09..8a7ae46b5943da 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -293,21 +293,6 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
>   }
>   EXPORT_SYMBOL(__scsi_execute);
>   
> -/**
> - * scsi_init_cmd_errh - Initialize cmd fields related to error handling.
> - * @cmd:  command that is ready to be queued.
> - *
> - * This function has the job of initializing a number of fields related to error
> - * handling. Typically this will be called once for each command, as required.
> - */
> -static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
> -{
> -	scsi_set_resid(cmd, 0);
> -	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
> -	if (cmd->cmd_len == 0)
> -		cmd->cmd_len = scsi_command_size(cmd->cmnd);
> -}
> -
>   /*
>    * Wake up the error handler if necessary. Avoid as follows that the error
>    * handler is not woken up if host in-flight requests number ==
> @@ -1698,7 +1683,10 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>   	if (bd->last)
>   		cmd->flags |= SCMD_LAST;
>   
> -	scsi_init_cmd_errh(cmd);
> +	scsi_set_resid(cmd, 0);
> +	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
> +	if (cmd->cmd_len == 0)
> +		cmd->cmd_len = scsi_command_size(cmd->cmnd);
>   	cmd->scsi_done = scsi_mq_done;
>   
>   	reason = scsi_dispatch_cmd(cmd);
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH 03/10] scsi: move command size detection out of the fast path

[Christoph Hellwig]

We only need to detect the command size for ioctl request from userspace,
which is limited to the passthrough path.  Move the check there instead
of doing it for all queuecommand invocations.

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8a7ae46b5943da..3c551f06ebe9be 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1166,6 +1166,8 @@ static blk_status_t scsi_setup_scsi_cmnd(struct scsi_device *sdev,
 	}
 
 	cmd->cmd_len = scsi_req(req)->cmd_len;
+	if (cmd->cmd_len == 0)
+		cmd->cmd_len = scsi_command_size(cmd->cmnd);
 	cmd->cmnd = scsi_req(req)->cmd;
 	cmd->transfersize = blk_rq_bytes(req);
 	cmd->allowed = scsi_req(req)->retries;
@@ -1685,8 +1687,6 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 	scsi_set_resid(cmd, 0);
 	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
-	if (cmd->cmd_len == 0)
-		cmd->cmd_len = scsi_command_size(cmd->cmnd);
 	cmd->scsi_done = scsi_mq_done;
 
 	reason = scsi_dispatch_cmd(cmd);
-- 
2.28.0

Re: [PATCH 03/10] scsi: move command size detection out of the fast path

[Hannes Reinecke]

On 10/5/20 10:41 AM, Christoph Hellwig wrote:
> We only need to detect the command size for ioctl request from userspace,
> which is limited to the passthrough path.  Move the check there instead
> of doing it for all queuecommand invocations.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8a7ae46b5943da..3c551f06ebe9be 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1166,6 +1166,8 @@ static blk_status_t scsi_setup_scsi_cmnd(struct scsi_device *sdev,
>   	}
>   
>   	cmd->cmd_len = scsi_req(req)->cmd_len;
> +	if (cmd->cmd_len == 0)
> +		cmd->cmd_len = scsi_command_size(cmd->cmnd);
>   	cmd->cmnd = scsi_req(req)->cmd;
>   	cmd->transfersize = blk_rq_bytes(req);
>   	cmd->allowed = scsi_req(req)->retries;
> @@ -1685,8 +1687,6 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>   
>   	scsi_set_resid(cmd, 0);
>   	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
> -	if (cmd->cmd_len == 0)
> -		cmd->cmd_len = scsi_command_size(cmd->cmnd);
>   	cmd->scsi_done = scsi_mq_done;
>   
>   	reason = scsi_dispatch_cmd(cmd);
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH 04/10] scsi: simplify varlen CDB length checking

[Christoph Hellwig]

Directly access the cdb array like we do everywhere else insted of
overlaying a structure on top of it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_logging.c |  2 +-
 include/scsi/scsi_common.h  | 11 +++--------
 include/scsi/scsi_proto.h   | 10 ----------
 3 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
index 8ea44c6595efa7..b6222df7254a3a 100644
--- a/drivers/scsi/scsi_logging.c
+++ b/drivers/scsi/scsi_logging.c
@@ -111,7 +111,7 @@ static size_t scsi_format_opcode_name(char *buffer, size_t buf_len,
 
 	cdb0 = cdbp[0];
 	if (cdb0 == VARIABLE_LENGTH_CMD) {
-		int len = scsi_varlen_cdb_length(cdbp);
+		int len = cdbp[7] + 8;
 
 		if (len < 10) {
 			off = scnprintf(buffer, buf_len,
diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
index 731ac09ed23135..297fc1881607b6 100644
--- a/include/scsi/scsi_common.h
+++ b/include/scsi/scsi_common.h
@@ -9,20 +9,15 @@
 #include <linux/types.h>
 #include <scsi/scsi_proto.h>
 
-static inline unsigned
-scsi_varlen_cdb_length(const void *hdr)
-{
-	return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
-}
-
 extern const unsigned char scsi_command_size_tbl[8];
 #define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
 
 static inline unsigned
 scsi_command_size(const unsigned char *cmnd)
 {
-	return (cmnd[0] == VARIABLE_LENGTH_CMD) ?
-		scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]);
+	if (cmnd[0] == VARIABLE_LENGTH_CMD)
+		return cmnd[7] + 8;
+	return COMMAND_SIZE(cmnd[0]);
 }
 
 /* Returns a human-readable name for the device */
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index c3686011193224..c57f9cd8185526 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -176,16 +176,6 @@
 
 #define SCSI_MAX_VARLEN_CDB_SIZE 260
 
-/* defined in T10 SCSI Primary Commands-2 (SPC2) */
-struct scsi_varlen_cdb_hdr {
-	__u8 opcode;        /* opcode always == VARIABLE_LENGTH_CMD */
-	__u8 control;
-	__u8 misc[5];
-	__u8 additional_cdb_length;         /* total cdb length - 8 */
-	__be16 service_action;
-	/* service specific data follows */
-};
-
 /*
  *  SCSI Architecture Model (SAM) Status codes. Taken from SAM-3 draft
  *  T10/1561-D Revision 4 Draft dated 7th November 2002.
-- 
2.28.0

Re: [PATCH 04/10] scsi: simplify varlen CDB length checking

[Hannes Reinecke]

On 10/5/20 10:41 AM, Christoph Hellwig wrote:
> Directly access the cdb array like we do everywhere else insted of
> overlaying a structure on top of it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_logging.c |  2 +-
>   include/scsi/scsi_common.h  | 11 +++--------
>   include/scsi/scsi_proto.h   | 10 ----------
>   3 files changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
> index 8ea44c6595efa7..b6222df7254a3a 100644
> --- a/drivers/scsi/scsi_logging.c
> +++ b/drivers/scsi/scsi_logging.c
> @@ -111,7 +111,7 @@ static size_t scsi_format_opcode_name(char *buffer, size_t buf_len,
>   
>   	cdb0 = cdbp[0];
>   	if (cdb0 == VARIABLE_LENGTH_CMD) {
> -		int len = scsi_varlen_cdb_length(cdbp);
> +		int len = cdbp[7] + 8;
>   
>   		if (len < 10) {
>   			off = scnprintf(buffer, buf_len,
> diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
> index 731ac09ed23135..297fc1881607b6 100644
> --- a/include/scsi/scsi_common.h
> +++ b/include/scsi/scsi_common.h
> @@ -9,20 +9,15 @@
>   #include <linux/types.h>
>   #include <scsi/scsi_proto.h>
>   
> -static inline unsigned
> -scsi_varlen_cdb_length(const void *hdr)
> -{
> -	return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
> -}
> -
>   extern const unsigned char scsi_command_size_tbl[8];
>   #define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
>   
>   static inline unsigned
>   scsi_command_size(const unsigned char *cmnd)
>   {
> -	return (cmnd[0] == VARIABLE_LENGTH_CMD) ?
> -		scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]);
> +	if (cmnd[0] == VARIABLE_LENGTH_CMD)
> +		return cmnd[7] + 8;
> +	return COMMAND_SIZE(cmnd[0]);
>   }
>   
>   /* Returns a human-readable name for the device */
> diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
> index c3686011193224..c57f9cd8185526 100644
> --- a/include/scsi/scsi_proto.h
> +++ b/include/scsi/scsi_proto.h
> @@ -176,16 +176,6 @@
>   
>   #define SCSI_MAX_VARLEN_CDB_SIZE 260
>   
> -/* defined in T10 SCSI Primary Commands-2 (SPC2) */
> -struct scsi_varlen_cdb_hdr {
> -	__u8 opcode;        /* opcode always == VARIABLE_LENGTH_CMD */
> -	__u8 control;
> -	__u8 misc[5];
> -	__u8 additional_cdb_length;         /* total cdb length - 8 */
> -	__be16 service_action;
> -	/* service specific data follows */
> -};
> -
>   /*
>    *  SCSI Architecture Model (SAM) Status codes. Taken from SAM-3 draft
>    *  T10/1561-D Revision 4 Draft dated 7th November 2002.
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 04/10] scsi: simplify varlen CDB length checking

[Bart Van Assche]

On 10/5/20 1:41 AM, Christoph Hellwig wrote:
> diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
> index 8ea44c6595efa7..b6222df7254a3a 100644
> --- a/drivers/scsi/scsi_logging.c
> +++ b/drivers/scsi/scsi_logging.c
> @@ -111,7 +111,7 @@ static size_t scsi_format_opcode_name(char *buffer, size_t buf_len,
>   
>   	cdb0 = cdbp[0];
>   	if (cdb0 == VARIABLE_LENGTH_CMD) {
> -		int len = scsi_varlen_cdb_length(cdbp);
> +		int len = cdbp[7] + 8;
>   
>   		if (len < 10) {
>   			off = scnprintf(buffer, buf_len,
> diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
> index 731ac09ed23135..297fc1881607b6 100644
> --- a/include/scsi/scsi_common.h
> +++ b/include/scsi/scsi_common.h
> @@ -9,20 +9,15 @@
>   #include <linux/types.h>
>   #include <scsi/scsi_proto.h>
>   
> -static inline unsigned
> -scsi_varlen_cdb_length(const void *hdr)
> -{
> -	return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
> -}
> -
>   extern const unsigned char scsi_command_size_tbl[8];
>   #define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
>   
>   static inline unsigned
>   scsi_command_size(const unsigned char *cmnd)
>   {
> -	return (cmnd[0] == VARIABLE_LENGTH_CMD) ?
> -		scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]);
> +	if (cmnd[0] == VARIABLE_LENGTH_CMD)
> +		return cmnd[7] + 8;
> +	return COMMAND_SIZE(cmnd[0]);
>   }
>   
>   /* Returns a human-readable name for the device */
> diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
> index c3686011193224..c57f9cd8185526 100644
> --- a/include/scsi/scsi_proto.h
> +++ b/include/scsi/scsi_proto.h
> @@ -176,16 +176,6 @@
>   
>   #define SCSI_MAX_VARLEN_CDB_SIZE 260
>   
> -/* defined in T10 SCSI Primary Commands-2 (SPC2) */
> -struct scsi_varlen_cdb_hdr {
> -	__u8 opcode;        /* opcode always == VARIABLE_LENGTH_CMD */
> -	__u8 control;
> -	__u8 misc[5];
> -	__u8 additional_cdb_length;         /* total cdb length - 8 */
> -	__be16 service_action;
> -	/* service specific data follows */
> -};

I'm OK with removing struct scsi_varlen_cdb_hdr but not with the removal of the
scsi_varlen_cdb_length() function. I'd like to keep that function because I think
it makes code that handles variable length CDBs easier to read.

Thanks,

Bart.

Re: [PATCH 04/10] scsi: simplify varlen CDB length checking

[Martin K. Petersen]

Bart,

> I'm OK with removing struct scsi_varlen_cdb_hdr but not with the
> removal of the scsi_varlen_cdb_length() function. I'd like to keep
> that function because I think it makes code that handles variable
> length CDBs easier to read.

I agree. Please drop the header and simplify the wrapper function.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 05/10] scsi: use rq_dma_dir in scsi_setup_cmnd

[Christoph Hellwig]

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 3c551f06ebe9be..670ad06812b419 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1200,12 +1200,7 @@ static blk_status_t scsi_setup_cmnd(struct scsi_device *sdev,
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
 	blk_status_t ret;
 
-	if (!blk_rq_bytes(req))
-		cmd->sc_data_direction = DMA_NONE;
-	else if (rq_data_dir(req) == WRITE)
-		cmd->sc_data_direction = DMA_TO_DEVICE;
-	else
-		cmd->sc_data_direction = DMA_FROM_DEVICE;
+	cmd->sc_data_direction = rq_dma_dir(req);
 
 	if (blk_rq_is_scsi(req))
 		ret = scsi_setup_scsi_cmnd(sdev, req);
-- 
2.28.0

Re: [PATCH 05/10] scsi: use rq_dma_dir in scsi_setup_cmnd

[Hannes Reinecke]

On 10/5/20 10:41 AM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 3c551f06ebe9be..670ad06812b419 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1200,12 +1200,7 @@ static blk_status_t scsi_setup_cmnd(struct scsi_device *sdev,
>   	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
>   	blk_status_t ret;
>   
> -	if (!blk_rq_bytes(req))
> -		cmd->sc_data_direction = DMA_NONE;
> -	else if (rq_data_dir(req) == WRITE)
> -		cmd->sc_data_direction = DMA_TO_DEVICE;
> -	else
> -		cmd->sc_data_direction = DMA_FROM_DEVICE;
> +	cmd->sc_data_direction = rq_dma_dir(req);
>   
>   	if (blk_rq_is_scsi(req))
>   		ret = scsi_setup_scsi_cmnd(sdev, req);
> 
Makes me wonder if we can't kill sc_data_direction completely.
But maybe later.

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

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH 06/10] scsi: rename scsi_prep_state_check to scsi_device_state_check

[Christoph Hellwig]

The old name is rather confusing now that the the legacy prep_fn is gone.

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 670ad06812b419..3940641052f90b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1214,7 +1214,7 @@ static blk_status_t scsi_setup_cmnd(struct scsi_device *sdev,
 }
 
 static blk_status_t
-scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
+scsi_device_state_check(struct scsi_device *sdev, struct request *req)
 {
 	switch (sdev->sdev_state) {
 	case SDEV_OFFLINE:
@@ -1653,7 +1653,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 	 * commands.
 	 */
 	if (unlikely(sdev->sdev_state != SDEV_RUNNING)) {
-		ret = scsi_prep_state_check(sdev, req);
+		ret = scsi_device_state_check(sdev, req);
 		if (ret != BLK_STS_OK)
 			goto out_put_budget;
 	}
-- 
2.28.0

Re: [PATCH 06/10] scsi: rename scsi_prep_state_check to scsi_device_state_check

[Hannes Reinecke]

On 10/5/20 10:41 AM, Christoph Hellwig wrote:
> The old name is rather confusing now that the the legacy prep_fn is gone.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 670ad06812b419..3940641052f90b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1214,7 +1214,7 @@ static blk_status_t scsi_setup_cmnd(struct scsi_device *sdev,
>   }
>   
>   static blk_status_t
> -scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
> +scsi_device_state_check(struct scsi_device *sdev, struct request *req)
>   {
>   	switch (sdev->sdev_state) {
>   	case SDEV_OFFLINE:
> @@ -1653,7 +1653,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>   	 * commands.
>   	 */
>   	if (unlikely(sdev->sdev_state != SDEV_RUNNING)) {
> -		ret = scsi_prep_state_check(sdev, req);
> +		ret = scsi_device_state_check(sdev, req);
>   		if (ret != BLK_STS_OK)
>   			goto out_put_budget;
>   	}
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH 07/10] scsi: rename scsi_mq_prep_fn to scsi_prepare_cmd

[Christoph Hellwig]

The old name is rather confusing now that the the legacy prep_fn is gone.

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 3940641052f90b..8420e42d618bb0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1562,7 +1562,7 @@ static unsigned int scsi_mq_inline_sgl_size(struct Scsi_Host *shost)
 		sizeof(struct scatterlist);
 }
 
-static blk_status_t scsi_mq_prep_fn(struct request *req)
+static blk_status_t scsi_prepare_cmd(struct request *req)
 {
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
 	struct scsi_device *sdev = req->q->queuedata;
@@ -1665,7 +1665,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 		goto out_dec_target_busy;
 
 	if (!(req->rq_flags & RQF_DONTPREP)) {
-		ret = scsi_mq_prep_fn(req);
+		ret = scsi_prepare_cmd(req);
 		if (ret != BLK_STS_OK)
 			goto out_dec_host_busy;
 		req->rq_flags |= RQF_DONTPREP;
-- 
2.28.0

Re: [PATCH 07/10] scsi: rename scsi_mq_prep_fn to scsi_prepare_cmd

[Hannes Reinecke]

On 10/5/20 10:41 AM, Christoph Hellwig wrote:
> The old name is rather confusing now that the the legacy prep_fn is gone.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 3940641052f90b..8420e42d618bb0 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1562,7 +1562,7 @@ static unsigned int scsi_mq_inline_sgl_size(struct Scsi_Host *shost)
>   		sizeof(struct scatterlist);
>   }
>   
> -static blk_status_t scsi_mq_prep_fn(struct request *req)
> +static blk_status_t scsi_prepare_cmd(struct request *req)
>   {
>   	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
>   	struct scsi_device *sdev = req->q->queuedata;
> @@ -1665,7 +1665,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>   		goto out_dec_target_busy;
>   
>   	if (!(req->rq_flags & RQF_DONTPREP)) {
> -		ret = scsi_mq_prep_fn(req);
> +		ret = scsi_prepare_cmd(req);
>   		if (ret != BLK_STS_OK)
>   			goto out_dec_host_busy;
>   		req->rq_flags |= RQF_DONTPREP;
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH 08/10] scsi: cleanup allocation and freeing of sgtables

[Christoph Hellwig]

Rename scsi_init_io to scsi_alloc_sgtables, and ensure callers call
scsi_free_sgtables to cleanup failures close to scsi_init_io instead
of leaking it down the generic I/O submission path.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c  | 22 ++++++++--------------
 drivers/scsi/sd.c        | 27 +++++++++++++++------------
 drivers/scsi/sr.c        | 16 ++++++----------
 include/scsi/scsi_cmnd.h |  3 ++-
 4 files changed, 31 insertions(+), 37 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8420e42d618bb0..c65ecb99c6994b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -515,7 +515,7 @@ static void scsi_uninit_cmd(struct scsi_cmnd *cmd)
 	}
 }
 
-static void scsi_free_sgtables(struct scsi_cmnd *cmd)
+void scsi_free_sgtables(struct scsi_cmnd *cmd)
 {
 	if (cmd->sdb.table.nents)
 		sg_free_table_chained(&cmd->sdb.table,
@@ -524,6 +524,7 @@ static void scsi_free_sgtables(struct scsi_cmnd *cmd)
 		sg_free_table_chained(&cmd->prot_sdb->table,
 				SCSI_INLINE_PROT_SG_CNT);
 }
+EXPORT_SYMBOL_GPL(scsi_free_sgtables);
 
 static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
 {
@@ -968,7 +969,7 @@ static inline bool scsi_cmd_needs_dma_drain(struct scsi_device *sdev,
 }
 
 /**
- * scsi_init_io - SCSI I/O initialization function.
+ * scsi_alloc_sgtables - allocate S/G tables for a command
  * @cmd:  command descriptor we wish to initialize
  *
  * Returns:
@@ -976,7 +977,7 @@ static inline bool scsi_cmd_needs_dma_drain(struct scsi_device *sdev,
  * * BLK_STS_RESOURCE - if the failure is retryable
  * * BLK_STS_IOERR    - if the failure is fatal
  */
-blk_status_t scsi_init_io(struct scsi_cmnd *cmd)
+blk_status_t scsi_alloc_sgtables(struct scsi_cmnd *cmd)
 {
 	struct scsi_device *sdev = cmd->device;
 	struct request *rq = cmd->request;
@@ -1068,7 +1069,7 @@ blk_status_t scsi_init_io(struct scsi_cmnd *cmd)
 	scsi_free_sgtables(cmd);
 	return ret;
 }
-EXPORT_SYMBOL(scsi_init_io);
+EXPORT_SYMBOL(scsi_alloc_sgtables);
 
 /**
  * scsi_initialize_rq - initialize struct scsi_cmnd partially
@@ -1156,7 +1157,7 @@ static blk_status_t scsi_setup_scsi_cmnd(struct scsi_device *sdev,
 	 * submit a request without an attached bio.
 	 */
 	if (req->bio) {
-		blk_status_t ret = scsi_init_io(cmd);
+		blk_status_t ret = scsi_alloc_sgtables(cmd);
 		if (unlikely(ret != BLK_STS_OK))
 			return ret;
 	} else {
@@ -1198,19 +1199,12 @@ static blk_status_t scsi_setup_cmnd(struct scsi_device *sdev,
 		struct request *req)
 {
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
-	blk_status_t ret;
 
 	cmd->sc_data_direction = rq_dma_dir(req);
 
 	if (blk_rq_is_scsi(req))
-		ret = scsi_setup_scsi_cmnd(sdev, req);
-	else
-		ret = scsi_setup_fs_cmnd(sdev, req);
-
-	if (ret != BLK_STS_OK)
-		scsi_free_sgtables(cmd);
-
-	return ret;
+		return scsi_setup_scsi_cmnd(sdev, req);
+	return scsi_setup_fs_cmnd(sdev, req);
 }
 
 static blk_status_t
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 95018e650f2d0c..b29f4cbb4ee076 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -866,7 +866,7 @@ static blk_status_t sd_setup_unmap_cmnd(struct scsi_cmnd *cmd)
 	cmd->transfersize = data_len;
 	rq->timeout = SD_TIMEOUT;
 
-	return scsi_init_io(cmd);
+	return scsi_alloc_sgtables(cmd);
 }
 
 static blk_status_t sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd,
@@ -897,7 +897,7 @@ static blk_status_t sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd,
 	cmd->transfersize = data_len;
 	rq->timeout = unmap ? SD_TIMEOUT : SD_WRITE_SAME_TIMEOUT;
 
-	return scsi_init_io(cmd);
+	return scsi_alloc_sgtables(cmd);
 }
 
 static blk_status_t sd_setup_write_same10_cmnd(struct scsi_cmnd *cmd,
@@ -928,7 +928,7 @@ static blk_status_t sd_setup_write_same10_cmnd(struct scsi_cmnd *cmd,
 	cmd->transfersize = data_len;
 	rq->timeout = unmap ? SD_TIMEOUT : SD_WRITE_SAME_TIMEOUT;
 
-	return scsi_init_io(cmd);
+	return scsi_alloc_sgtables(cmd);
 }
 
 static blk_status_t sd_setup_write_zeroes_cmnd(struct scsi_cmnd *cmd)
@@ -1069,7 +1069,7 @@ static blk_status_t sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
 	 * knows how much to actually write.
 	 */
 	rq->__data_len = sdp->sector_size;
-	ret = scsi_init_io(cmd);
+	ret = scsi_alloc_sgtables(cmd);
 	rq->__data_len = blk_rq_bytes(rq);
 
 	return ret;
@@ -1187,23 +1187,24 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
 	unsigned int dif;
 	bool dix;
 
-	ret = scsi_init_io(cmd);
+	ret = scsi_alloc_sgtables(cmd);
 	if (ret != BLK_STS_OK)
 		return ret;
 
+	ret = BLK_STS_IOERR;
 	if (!scsi_device_online(sdp) || sdp->changed) {
 		scmd_printk(KERN_ERR, cmd, "device offline or changed\n");
-		return BLK_STS_IOERR;
+		goto fail;
 	}
 
 	if (blk_rq_pos(rq) + blk_rq_sectors(rq) > get_capacity(rq->rq_disk)) {
 		scmd_printk(KERN_ERR, cmd, "access beyond end of device\n");
-		return BLK_STS_IOERR;
+		goto fail;
 	}
 
 	if ((blk_rq_pos(rq) & mask) || (blk_rq_sectors(rq) & mask)) {
 		scmd_printk(KERN_ERR, cmd, "request not aligned to the logical block size\n");
-		return BLK_STS_IOERR;
+		goto fail;
 	}
 
 	/*
@@ -1225,7 +1226,7 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
 	if (req_op(rq) == REQ_OP_ZONE_APPEND) {
 		ret = sd_zbc_prepare_zone_append(cmd, &lba, nr_blocks);
 		if (ret)
-			return ret;
+			goto fail;
 	}
 
 	fua = rq->cmd_flags & REQ_FUA ? 0x8 : 0;
@@ -1253,7 +1254,7 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
 	}
 
 	if (unlikely(ret != BLK_STS_OK))
-		return ret;
+		goto fail;
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb
@@ -1277,10 +1278,12 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
 				     blk_rq_sectors(rq)));
 
 	/*
-	 * This indicates that the command is ready from our end to be
-	 * queued.
+	 * This indicates that the command is ready from our end to be queued.
 	 */
 	return BLK_STS_OK;
+fail:
+	scsi_free_sgtables(cmd);
+	return ret;
 }
 
 static blk_status_t sd_init_command(struct scsi_cmnd *cmd)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 0c4aa4665a2f9d..b74dfd8dc1165e 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -392,15 +392,11 @@ static blk_status_t sr_init_command(struct scsi_cmnd *SCpnt)
 	struct request *rq = SCpnt->request;
 	blk_status_t ret;
 
-	ret = scsi_init_io(SCpnt);
+	ret = scsi_alloc_sgtables(SCpnt);
 	if (ret != BLK_STS_OK)
-		goto out;
+		return ret;
 	cd = scsi_cd(rq->rq_disk);
 
-	/* from here on until we're complete, any goto out
-	 * is used for a killable error condition */
-	ret = BLK_STS_IOERR;
-
 	SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt,
 		"Doing sr request, block = %d\n", block));
 
@@ -509,12 +505,12 @@ static blk_status_t sr_init_command(struct scsi_cmnd *SCpnt)
 	SCpnt->allowed = MAX_RETRIES;
 
 	/*
-	 * This indicates that the command is ready from our end to be
-	 * queued.
+	 * This indicates that the command is ready from our end to be queued.
 	 */
-	ret = BLK_STS_OK;
+	return BLK_STS_OK;
  out:
-	return ret;
+	scsi_free_sgtables(SCpnt);
+	return BLK_STS_IOERR;
 }
 
 static int sr_block_open(struct block_device *bdev, fmode_t mode)
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index e76bac4d14c51b..69ade4fb71aabf 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -165,7 +165,8 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
 extern void scsi_kunmap_atomic_sg(void *virt);
 
-extern blk_status_t scsi_init_io(struct scsi_cmnd *cmd);
+blk_status_t scsi_alloc_sgtables(struct scsi_cmnd *cmd);
+void scsi_free_sgtables(struct scsi_cmnd *cmd);
 
 #ifdef CONFIG_SCSI_DMA
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
-- 
2.28.0

Re: [PATCH 08/10] scsi: cleanup allocation and freeing of sgtables

[Hannes Reinecke]

On 10/5/20 10:41 AM, Christoph Hellwig wrote:
> Rename scsi_init_io to scsi_alloc_sgtables, and ensure callers call
> scsi_free_sgtables to cleanup failures close to scsi_init_io instead
> of leaking it down the generic I/O submission path.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c  | 22 ++++++++--------------
>   drivers/scsi/sd.c        | 27 +++++++++++++++------------
>   drivers/scsi/sr.c        | 16 ++++++----------
>   include/scsi/scsi_cmnd.h |  3 ++-
>   4 files changed, 31 insertions(+), 37 deletions(-)
> Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH 09/10] scsi: remove scsi_setup_cmnd and scsi_setup_fs_cmnd

[Christoph Hellwig]

Move this trivial functionality into scsi_prepare_cmd instead of
splitting it over multiple small functions, and update the comments
to better document passthrough commands as the special case.

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c65ecb99c6994b..f7b88d8cf975d5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1175,38 +1175,6 @@ static blk_status_t scsi_setup_scsi_cmnd(struct scsi_device *sdev,
 	return BLK_STS_OK;
 }
 
-/*
- * Setup a normal block command.  These are simple request from filesystems
- * that still need to be translated to SCSI CDBs from the ULD.
- */
-static blk_status_t scsi_setup_fs_cmnd(struct scsi_device *sdev,
-		struct request *req)
-{
-	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
-
-	if (unlikely(sdev->handler && sdev->handler->prep_fn)) {
-		blk_status_t ret = sdev->handler->prep_fn(sdev, req);
-		if (ret != BLK_STS_OK)
-			return ret;
-	}
-
-	cmd->cmnd = scsi_req(req)->cmd = scsi_req(req)->__cmd;
-	memset(cmd->cmnd, 0, BLK_MAX_CDB);
-	return scsi_cmd_to_driver(cmd)->init_command(cmd);
-}
-
-static blk_status_t scsi_setup_cmnd(struct scsi_device *sdev,
-		struct request *req)
-{
-	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
-
-	cmd->sc_data_direction = rq_dma_dir(req);
-
-	if (blk_rq_is_scsi(req))
-		return scsi_setup_scsi_cmnd(sdev, req);
-	return scsi_setup_fs_cmnd(sdev, req);
-}
-
 static blk_status_t
 scsi_device_state_check(struct scsi_device *sdev, struct request *req)
 {
@@ -1568,6 +1536,7 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
 	cmd->request = req;
 	cmd->tag = req->tag;
 	cmd->prot_op = SCSI_PROT_NORMAL;
+	cmd->sc_data_direction = rq_dma_dir(req);
 
 	sg = (void *)cmd + sizeof(struct scsi_cmnd) + shost->hostt->cmd_size;
 	cmd->sdb.table.sgl = sg;
@@ -1581,7 +1550,22 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
 
 	blk_mq_start_request(req);
 
-	return scsi_setup_cmnd(sdev, req);
+	/*
+	 * Special handling for passthrough commands, which don't go to the ULP
+	 * at all:
+	 */
+	if (blk_rq_is_scsi(req))
+		return scsi_setup_scsi_cmnd(sdev, req);
+
+	if (sdev->handler && sdev->handler->prep_fn) {
+		blk_status_t ret = sdev->handler->prep_fn(sdev, req);
+		if (ret != BLK_STS_OK)
+			return ret;
+	}
+
+	cmd->cmnd = scsi_req(req)->cmd = scsi_req(req)->__cmd;
+	memset(cmd->cmnd, 0, BLK_MAX_CDB);
+	return scsi_cmd_to_driver(cmd)->init_command(cmd);
 }
 
 static void scsi_mq_done(struct scsi_cmnd *cmd)
-- 
2.28.0

[PATCH 10/10] scsi: only start the request just before dispatching

[Christoph Hellwig]

This has no change in behavior, but improves the accounting a bit.

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

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f7b88d8cf975d5..f0254f913b3e3f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1548,8 +1548,6 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
 			(struct scatterlist *)(cmd->prot_sdb + 1);
 	}
 
-	blk_mq_start_request(req);
-
 	/*
 	 * Special handling for passthrough commands, which don't go to the ULP
 	 * at all:
@@ -1649,7 +1647,6 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 		req->rq_flags |= RQF_DONTPREP;
 	} else {
 		clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
-		blk_mq_start_request(req);
 	}
 
 	cmd->flags &= SCMD_PRESERVED_FLAGS;
@@ -1662,6 +1659,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
 	cmd->scsi_done = scsi_mq_done;
 
+	blk_mq_start_request(req);
 	reason = scsi_dispatch_cmd(cmd);
 	if (reason) {
 		scsi_set_blocked(cmd, reason);
-- 
2.28.0

Re: [PATCH 10/10] scsi: only start the request just before dispatching

[Bart Van Assche]

On 10/5/20 1:41 AM, Christoph Hellwig wrote:
> This has no change in behavior, but improves the accounting a bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/scsi_lib.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f7b88d8cf975d5..f0254f913b3e3f 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1548,8 +1548,6 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
>   			(struct scatterlist *)(cmd->prot_sdb + 1);
>   	}
>   
> -	blk_mq_start_request(req);
> -
>   	/*
>   	 * Special handling for passthrough commands, which don't go to the ULP
>   	 * at all:
> @@ -1649,7 +1647,6 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>   		req->rq_flags |= RQF_DONTPREP;
>   	} else {
>   		clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
> -		blk_mq_start_request(req);
>   	}
>   
>   	cmd->flags &= SCMD_PRESERVED_FLAGS;
> @@ -1662,6 +1659,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>   	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
>   	cmd->scsi_done = scsi_mq_done;
>   
> +	blk_mq_start_request(req);
>   	reason = scsi_dispatch_cmd(cmd);
>   	if (reason) {
>   		scsi_set_blocked(cmd, reason);

That's a nice cleanup!

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

Re: misc I/O submission cleanups

[Martin K. Petersen]

Christoph,

> this series tidies up various loose ends in the SCSI I/O submission
> path.

Looks good! Applied to 5.10/scsi-staging except for the varlen patch.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: misc I/O submission cleanups

[Qian Cai]

On Mon, 2020-10-05 at 10:41 +0200, Christoph Hellwig wrote:
> Hi Martin,
> 
> this series tidies up various loose ends in the SCSI I/O submission path.

Reverting this patchset on the top of today's linux-next fixed the boot failures
below with libata, i.e.,

git revert --no-edit 653eb7c99d84..ed7fb2d018fd

== Easy to reproduce using qemu-kvm "-hda" CONFIG_ATA_PIIX=y. ==
.config: https://gitlab.com/cailca/linux-mm/-/blob/master/x86.config

[   46.047499][  T757] ata2.00: WARNING: zero len r/w req
[   46.049734][  T644] ata2.00: WARNING: zero len r/w req
[   46.051962][  T757] ata2.00: WARNING: zero len r/w req
[   46.054182][  T644] ata2.00: WARNING: zero len r/w req
[   46.058018][  T757] ata2.00: WARNING: zero len r/w req
[   46.060514][  T644] ata2.00: WARNING: zero len r/w req
[   46.065764][  T757] ata2.00: WARNING: zero len r/w req
[   46.068005][  T644] ata2.00: WARNING: zero len r/w req
[   46.070192][  T644] ata2.00: WARNING: zero len r/w req
[   46.072379][  T644] ata2.00: WARNING: zero len r/w req
[   46.074629][  T644] ata2.00: WARNING: zero len r/w req
[   46.077255][  T644] ata2.00: WARNING: zero len r/w req
[   46.081553][   C36] sr 1:0:0:0: [sr0] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x07 driverbyte=0x00 cmd_age=0s
[   46.086336][   C36] sr 1:0:0:0: [sr0] tag#0 CDB: opcode=0x28 
[   46.089171][   C36] blk_update_request: I/O error, dev sr0, sector 2097136 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
[   46.094979][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.097526][  T757] blk_update_request: I/O error, dev sr0, sector 2097136 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
[   46.102364][  T757] Buffer I/O error on dev sr0, logical block 2097136, async page read
[   46.106080][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.108590][  T757] blk_update_request: I/O error, dev sr0, sector 2097137 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
[   46.113234][  T757] Buffer I/O error on dev sr0, logical block 2097137, async page read
[   46.117053][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.119581][  T757] blk_update_request: I/O error, dev sr0, sector 2097138 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
[   46.124382][  T757] Buffer I/O error on dev sr0, logical block 2097138, async page read
[   46.128545][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.131038][  T757] blk_update_request: I/O error, dev sr0, sector 2097139 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
[   46.135905][  T757] Buffer I/O error on dev sr0, logical block 2097139, async page read
[   46.139835][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.142422][  T757] blk_update_request: I/O error, dev sr0, sector 2097140 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
[   46.147240][  T757] Buffer I/O error on dev sr0, logical block 2097140, async page read
[   46.150764][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.153248][  T757] blk_update_request: I/O error, dev sr0, sector 2097141 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
[   46.158439][  T757] Buffer I/O error on dev sr0, logical block 2097141, async page read
[   46.162383][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.165062][  T757] blk_update_request: I/O error, dev sr0, sector 2097142 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
[   46.169785][  T757] Buffer I/O error on dev sr0, logical block 2097142, async page read
[   46.173252][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.175968][  T757] blk_update_request: I/O error, dev sr0, sector 2097143 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
[   46.181049][  T757] Buffer I/O error on dev sr0, logical block 2097143, async page read
[   46.184779][   C36] sr 1:0:0:0: [sr0] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x07 driverbyte=0x00 cmd_age=0s
[   46.188966][   C36] sr 1:0:0:0: [sr0] tag#0 CDB: opcode=0x28 
[   46.191731][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.194223][  T757] Buffer I/O error on dev sr0, logical block 0, async page read
[   46.197976][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.200781][  T757] Buffer I/O error on dev sr0, logical block 1, async page read
[   46.204221][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.207133][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.209790][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer
[   46.212464][  T757] sr 1:0:0:0: [sr0] tag#0 unaligned transfer

== baremetal with ahci ==
[   14.560235][  T515] ata1.00: WARNING: zero len r/w req
[   14.560397][  T515] ata1.00: WARNING: zero len r/w req
[   14.560450][  T515] ata1.00: WARNING: zero len r/w req
[   14.560502][  T515] ata1.00: WARNING: zero len r/w req
[   14.560594][  T515] ata1.00: WARNING: zero len r/w req
[   14.560644][  T515] ata1.00: WARNING: zero len r/w req
[   14.560709][  C100] sd 0:0:0:0: [sdb] tag#7 UNKNOWN(0x2003) Result: hostbyte=0x07 driverbyte=0x00 cmd_age=0s
[   14.560790][  C100] sd 0:0:0:0: [sdb] tag#7 CDB: opcode=0x35 35 00 00 00 00 00 00 00 00 00
[   14.560869][  C100] blk_update_request: I/O error, dev sdb, sector 0 op 0x1:(WRITE) flags 0x800 phys_seg 0 prio class 0
[   14.562124][ T1274] ata1.00: WARNING: zero len r/w req
[   14.562172][ T1274] ata1.00: WARNING: zero len r/w req
[   14.562217][ T1274] ata1.00: WARNING: zero len r/w req
[   14.562262][ T1274] ata1.00: WARNING: zero len r/w req
[   14.562317][ T1274] ata1.00: WARNING: zero len r/w req
[   14.562361][ T1274] ata1.00: WARNING: zero len r/w req
[   14.562412][  C100] sd 0:0:0:0: [sdb] tag#8 UNKNOWN(0x2003) Result: hostbyte=0x07 driverbyte=0x00 cmd_age=0s
[   14.562465][  C100] sd 0:0:0:0: [sdb] tag#8 CDB: opcode=0x35 35 00 00 00 00 00 00 00 00 00
[   14.562517][  C100] blk_update_request: I/O error, dev sdb, sector 1880188816 op 0x1:(WRITE) flags 0x9800 phys_seg 1 prio class 0