diff mbox series

[RFC,v2,1/2] scsi: convert scsi_result_to_blk_status() to inline

Message ID 20210429155024.4947-2-mwilck@suse.com (mailing list archive)
State New, archived
Headers show
Series dm: dm_blk_ioctl(): implement failover for SG_IO on dm-multipath | expand

Commit Message

Martin Wilck April 29, 2021, 3:50 p.m. UTC 
From: Martin Wilck <mwilck@suse.com>

This makes it possible to use scsi_result_to_blk_status() from
code that shouldn't depend on scsi_mod (e.g. device mapper).

Also, create variants of set_host_byte() etc. that don't expect
a struct scsi_cmnd *, but just a pointer to the result to be
modified/fixed.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_lib.c  | 40 -------------------
 include/scsi/scsi_cmnd.h | 83 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 79 insertions(+), 44 deletions(-)

Comments

Bart Van Assche April 29, 2021, 4:20 p.m. UTC | #1 
On 4/29/21 8:50 AM, mwilck@suse.com wrote:
> This makes it possible to use scsi_result_to_blk_status() from
> code that shouldn't depend on scsi_mod (e.g. device mapper).

I think that's the wrong reason to make a function inline. Please
consider moving scsi_result_to_blk_status() into one of the block layer
source code files that already deals with SG I/O, e.g. block/scsi_ioctl.c.

> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 83f7e520be48..ba1e69d3bed9 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -311,24 +311,44 @@ static inline struct scsi_data_buffer *scsi_prot(struct scsi_cmnd *cmd)
>  #define scsi_for_each_prot_sg(cmd, sg, nseg, __i)		\
>  	for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
>  
> +static inline void __set_status_byte(int *result, char status)
> +{
> +	*result = (*result & 0xffffff00) | status;
> +}
> +
>  static inline void set_status_byte(struct scsi_cmnd *cmd, char status)
>  {
> -	cmd->result = (cmd->result & 0xffffff00) | status;
> +	__set_status_byte(&cmd->result, status);
> +}
> +
> +static inline void __set_msg_byte(int *result, char status)
> +{
> +	*result = (*result & 0xffff00ff) | (status << 8);
>  }
>  
>  static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
>  {
> -	cmd->result = (cmd->result & 0xffff00ff) | (status << 8);
> +	__set_msg_byte(&cmd->result, status);
> +}
> +
> +static inline void __set_host_byte(int *result, char status)
> +{
> +	*result = (*result & 0xff00ffff) | (status << 16);
>  }
>  
>  static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
>  {
> -	cmd->result = (cmd->result & 0xff00ffff) | (status << 16);
> +	__set_host_byte(&cmd->result, status);
> +}
> +
> +static inline void __set_driver_byte(int *result, char status)
> +{
> +	*result = (*result & 0x00ffffff) | (status << 24);
>  }
>  
>  static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
>  {
> -	cmd->result = (cmd->result & 0x00ffffff) | (status << 24);
> +	__set_driver_byte(&cmd->result, status);
>  }

The layout of the SCSI result won't change since it is used in multiple
UAPI data structures. I'd prefer to open-code host_byte() in
scsi_result_to_blk_status() instead of making the above changes.

Thanks,

Bart.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Martin Wilck April 29, 2021, 8:33 p.m. UTC | #2 
Hi Bart,

thanks for looking at this.

On Thu, 2021-04-29 at 09:20 -0700, Bart Van Assche wrote:
> On 4/29/21 8:50 AM, mwilck@suse.com wrote:
> > This makes it possible to use scsi_result_to_blk_status() from
> > code that shouldn't depend on scsi_mod (e.g. device mapper).
> 
> I think that's the wrong reason to make a function inline. Please
> consider moving scsi_result_to_blk_status() into one of the block
> layer
> source code files that already deals with SG I/O, e.g.
> block/scsi_ioctl.c.

scsi_ioctl.c, are you certain? scsi_result_to_blk_status() is an
important part of the block/scsi interface... You're right that that
this function is not a prime candidate for inlining, but I'm still
wondering where it belongs if we don't.

Looking forward to see what others think.

> 
> > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> > index 83f7e520be48..ba1e69d3bed9 100644
> > --- a/include/scsi/scsi_cmnd.h
> > +++ b/include/scsi/scsi_cmnd.h
> > @@ -311,24 +311,44 @@ static inline struct scsi_data_buffer
> > *scsi_prot(struct scsi_cmnd *cmd)
> >  #define scsi_for_each_prot_sg(cmd, sg, nseg, __i)              \
> >         for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
> >  
> > +static inline void __set_status_byte(int *result, char status)
> > +{
> > +       *result = (*result & 0xffffff00) | status;
> > +}
> > +
> >  static inline void set_status_byte(struct scsi_cmnd *cmd, char
> > status)
> >  {
> > -       cmd->result = (cmd->result & 0xffffff00) | status;
> > +       __set_status_byte(&cmd->result, status);
> > +}
> > +
> > +static inline void __set_msg_byte(int *result, char status)
> > +{
> > +       *result = (*result & 0xffff00ff) | (status << 8);
> >  }
> >  
> >  static inline void set_msg_byte(struct scsi_cmnd *cmd, char
> > status)
> >  {
> > -       cmd->result = (cmd->result & 0xffff00ff) | (status << 8);
> > +       __set_msg_byte(&cmd->result, status);
> > +}
> > +
> > +static inline void __set_host_byte(int *result, char status)
> > +{
> > +       *result = (*result & 0xff00ffff) | (status << 16);
> >  }
> >  
> >  static inline void set_host_byte(struct scsi_cmnd *cmd, char
> > status)
> >  {
> > -       cmd->result = (cmd->result & 0xff00ffff) | (status << 16);
> > +       __set_host_byte(&cmd->result, status);
> > +}
> > +
> > +static inline void __set_driver_byte(int *result, char status)
> > +{
> > +       *result = (*result & 0x00ffffff) | (status << 24);
> >  }
> >  
> >  static inline void set_driver_byte(struct scsi_cmnd *cmd, char
> > status)
> >  {
> > -       cmd->result = (cmd->result & 0x00ffffff) | (status << 24);
> > +       __set_driver_byte(&cmd->result, status);
> >  }
> 
> The layout of the SCSI result won't change since it is used in
> multiple
> UAPI data structures. I'd prefer to open-code host_byte() in
> scsi_result_to_blk_status() instead of making the above changes.

OK. I'm generally no fan of hard-coding but I guess you're right
in this case.

Thanks,
Martin





--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Bart Van Assche April 30, 2021, 9:12 p.m. UTC | #3 
On 4/29/21 1:33 PM, Martin Wilck wrote:
> On Thu, 2021-04-29 at 09:20 -0700, Bart Van Assche wrote:
>> On 4/29/21 8:50 AM, mwilck@suse.com wrote:
>>> This makes it possible to use scsi_result_to_blk_status() from
>>> code that shouldn't depend on scsi_mod (e.g. device mapper).
>>
>> I think that's the wrong reason to make a function inline. Please
>> consider moving scsi_result_to_blk_status() into one of the block
>> layer
>> source code files that already deals with SG I/O, e.g.
>> block/scsi_ioctl.c.
> 
> scsi_ioctl.c, are you certain? scsi_result_to_blk_status() is an
> important part of the block/scsi interface... You're right that that
> this function is not a prime candidate for inlining, but I'm still
> wondering where it belongs if we don't.

The block/scsi_ioctl.c file is included in the kernel if and only if
BLK_SCSI_REQUEST is enabled. And that Kconfig symbol only selects the
block/scsi_ioctl.c file. Additionally, the following occurs in the SCSI
Kconfig file:

config SCSI
	tristate "SCSI device support"
	[ ... ]
	select BLK_SCSI_REQUEST

In other words, block/scsi_ioctl.c is built unconditionally if SCSI is
enabled. Adding the scsi_result_to_blk_status() function to the
block/scsi_ioctl.c file would increase the size of kernels that enable
bsg, ide, the SCSI target code or nfsd but not the SCSI initiator code.
If the latter is a concern, how about moving scsi_result_to_blk_status()
into a new file in the block directory?

Thanks,

Bart.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d7c0d5a5f263..e423184f2bba 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -610,46 +610,6 @@  static bool scsi_end_request(struct request *req, blk_status_t error,
 	return false;
 }
 
-/**
- * scsi_result_to_blk_status - translate a SCSI result code into blk_status_t
- * @cmd:	SCSI command
- * @result:	scsi error code
- *
- * Translate a SCSI result code into a blk_status_t value. May reset the host
- * byte of @cmd->result.
- */
-static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
-{
-	switch (host_byte(result)) {
-	case DID_OK:
-		/*
-		 * Also check the other bytes than the status byte in result
-		 * to handle the case when a SCSI LLD sets result to
-		 * DRIVER_SENSE << 24 without setting SAM_STAT_CHECK_CONDITION.
-		 */
-		if (scsi_status_is_good(result) && (result & ~0xff) == 0)
-			return BLK_STS_OK;
-		return BLK_STS_IOERR;
-	case DID_TRANSPORT_FAILFAST:
-	case DID_TRANSPORT_MARGINAL:
-		return BLK_STS_TRANSPORT;
-	case DID_TARGET_FAILURE:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_TARGET;
-	case DID_NEXUS_FAILURE:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_NEXUS;
-	case DID_ALLOC_FAILURE:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_NOSPC;
-	case DID_MEDIUM_ERROR:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_MEDIUM;
-	default:
-		return BLK_STS_IOERR;
-	}
-}
-
 /* Helper for scsi_io_completion() when "reprep" action required. */
 static void scsi_io_completion_reprep(struct scsi_cmnd *cmd,
 				      struct request_queue *q)
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 83f7e520be48..ba1e69d3bed9 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -311,24 +311,44 @@  static inline struct scsi_data_buffer *scsi_prot(struct scsi_cmnd *cmd)
 #define scsi_for_each_prot_sg(cmd, sg, nseg, __i)		\
 	for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
 
+static inline void __set_status_byte(int *result, char status)
+{
+	*result = (*result & 0xffffff00) | status;
+}
+
 static inline void set_status_byte(struct scsi_cmnd *cmd, char status)
 {
-	cmd->result = (cmd->result & 0xffffff00) | status;
+	__set_status_byte(&cmd->result, status);
+}
+
+static inline void __set_msg_byte(int *result, char status)
+{
+	*result = (*result & 0xffff00ff) | (status << 8);
 }
 
 static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
 {
-	cmd->result = (cmd->result & 0xffff00ff) | (status << 8);
+	__set_msg_byte(&cmd->result, status);
+}
+
+static inline void __set_host_byte(int *result, char status)
+{
+	*result = (*result & 0xff00ffff) | (status << 16);
 }
 
 static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
 {
-	cmd->result = (cmd->result & 0xff00ffff) | (status << 16);
+	__set_host_byte(&cmd->result, status);
+}
+
+static inline void __set_driver_byte(int *result, char status)
+{
+	*result = (*result & 0x00ffffff) | (status << 24);
 }
 
 static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
 {
-	cmd->result = (cmd->result & 0x00ffffff) | (status << 24);
+	__set_driver_byte(&cmd->result, status);
 }
 
 static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
@@ -342,4 +362,59 @@  static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
 	return xfer_len;
 }
 
+/**
+ * scsi_result_to_blk_status - translate a SCSI result code into blk_status_t
+ * @result:	scsi error code
+ * @cmd_result: pointer to scsi cmnd result code to be possibly changed
+ *
+ * Translate a SCSI result code into a blk_status_t value. May reset the host
+ * byte of @cmd_result.
+ */
+static inline blk_status_t __scsi_result_to_blk_status(int *cmd_result, int result)
+{
+	switch (host_byte(result)) {
+	case DID_OK:
+		/*
+		 * Also check the other bytes than the status byte in result
+		 * to handle the case when a SCSI LLD sets result to
+		 * DRIVER_SENSE << 24 without setting SAM_STAT_CHECK_CONDITION.
+		 */
+		if (scsi_status_is_good(result) && (result & ~0xff) == 0)
+			return BLK_STS_OK;
+		return BLK_STS_IOERR;
+	case DID_TRANSPORT_FAILFAST:
+	case DID_TRANSPORT_MARGINAL:
+		return BLK_STS_TRANSPORT;
+	case DID_TARGET_FAILURE:
+		__set_host_byte(cmd_result, DID_OK);
+		return BLK_STS_TARGET;
+	case DID_NEXUS_FAILURE:
+		__set_host_byte(cmd_result, DID_OK);
+		return BLK_STS_NEXUS;
+	case DID_ALLOC_FAILURE:
+		__set_host_byte(cmd_result, DID_OK);
+		return BLK_STS_NOSPC;
+	case DID_MEDIUM_ERROR:
+		__set_host_byte(cmd_result, DID_OK);
+		return BLK_STS_MEDIUM;
+	default:
+		return BLK_STS_IOERR;
+	}
+}
+
+/**
+ * scsi_result_to_blk_status - translate a SCSI result code into blk_status_t
+ * @cmd:	SCSI command
+ * @result:	scsi error code
+ *
+ * Translate a SCSI result code into a blk_status_t value. May reset the host
+ * byte of @cmd->result.
+ */
+static inline blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd,
+						     int result)
+{
+	return __scsi_result_to_blk_status(&cmd->result, result);
+}
+
+
 #endif /* _SCSI_SCSI_CMND_H */