[RFC PATCH v2 0/2] dm: dm_blk_ioctl(): implement failover for SG_IO on dm-multipath

[RFC PATCH v2 0/2] dm: dm_blk_ioctl(): implement failover for SG_IO on dm-multipath

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Mike,

here is v2 of my attempt to add retry logic to SG_IO on dm-multipath devices.

Regards
Martin

Changes v1->v2:

 - applied modifications from Mike Snitzer
 - moved SG_IO dependent code to a separate file, no scsi includes in
   dm.c any more
 - made the new code depend on a configuration option 
 - separated out scsi changes, made scsi_result_to_blk_status()
   inline to avoid dependency of dm_mod from scsi_mod (Paolo Bonzini)

Martin Wilck (2):
  scsi: convert scsi_result_to_blk_status() to inline
  dm: add CONFIG_DM_MULTIPATH_SG_IO - failover for SG_IO on dm-multipath

 block/scsi_ioctl.c         |   5 +-
 drivers/md/Kconfig         |  11 ++++
 drivers/md/Makefile        |   4 ++
 drivers/md/dm-core.h       |   5 ++
 drivers/md/dm-rq.h         |  11 ++++
 drivers/md/dm-scsi_ioctl.c | 127 +++++++++++++++++++++++++++++++++++++
 drivers/md/dm.c            |  20 +++++-
 drivers/scsi/scsi_lib.c    |  40 ------------
 include/linux/blkdev.h     |   2 +
 include/scsi/scsi_cmnd.h   |  83 ++++++++++++++++++++++--
 10 files changed, 259 insertions(+), 49 deletions(-)
 create mode 100644 drivers/md/dm-scsi_ioctl.c

-- 
2.31.1

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

[mwilck]

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

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

[RFC PATCH v2 2/2] dm: add CONFIG_DM_MULTIPATH_SG_IO - failover for SG_IO on dm-multipath

[mwilck]

From: Martin Wilck <mwilck@suse.com>

In virtual deployments, SCSI passthrough over dm-multipath devices is a
common setup. The qemu "pr-helper" was specifically invented for it. I
believe that this is the most important real-world scenario for sending
SG_IO ioctls to device-mapper devices.

In this configuration, guests send SCSI IO to the hypervisor in the form of
SG_IO ioctls issued by qemu. But on the device-mapper level, these SCSI
ioctls aren't treated like regular IO. Until commit 2361ae595352 ("dm mpath:
switch paths in dm_blk_ioctl() code path"), no path switching was done at
all. Worse though, if an SG_IO call fails because of a path error,
dm-multipath doesn't retry the IO on a another path; rather, the failure is
passed back to the guest, and paths are not marked as faulty.  This is in
stark contrast with regular block IO of guests on dm-multipath devices, and
certainly comes as a surprise to users who switch to SCSI passthrough in
qemu. In general, users of dm-multipath devices would probably expect failover
to work at least in a basic way.

This patch fixes this by taking a special code path for SG_IO on request-
based device mapper targets if CONFIG_DM_MULTIPATH_SG_IO is set.  Rather then
just choosing a single path, sending the IO to it, and failing to the caller
if the IO on the path failed, it retries the same IO on another path for
certain error codes, using blk_path_error() to determine if a retry would
make sense for the given error code. Moreover, it sends a message to the
multipath target to mark the path as failed.

One problem remains open: if all paths in a multipath map are failed,
normal multipath IO may switch to queueing mode (depending on
configuration). This isn't possible for SG_IO, as SG_IO requests can't
easily be queued like regular block I/O. Thus in the "no path" case, the
guest will still see an error.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 block/scsi_ioctl.c         |   5 +-
 drivers/md/Kconfig         |  11 ++++
 drivers/md/Makefile        |   4 ++
 drivers/md/dm-core.h       |   5 ++
 drivers/md/dm-rq.h         |  11 ++++
 drivers/md/dm-scsi_ioctl.c | 127 +++++++++++++++++++++++++++++++++++++
 drivers/md/dm.c            |  20 +++++-
 include/linux/blkdev.h     |   2 +
 8 files changed, 180 insertions(+), 5 deletions(-)
 create mode 100644 drivers/md/dm-scsi_ioctl.c

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 6599bac0a78c..bcc60552f7b1 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -279,8 +279,8 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
 	return ret;
 }
 
-static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
-		struct sg_io_hdr *hdr, fmode_t mode)
+int sg_io(struct request_queue *q, struct gendisk *bd_disk,
+	  struct sg_io_hdr *hdr, fmode_t mode)
 {
 	unsigned long start_time;
 	ssize_t ret = 0;
@@ -369,6 +369,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
 	blk_put_request(rq);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(sg_io);
 
 /**
  * sg_scsi_ioctl  --  handle deprecated SCSI_IOCTL_SEND_COMMAND ioctl
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index f2014385d48b..f28f29e3bd11 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -473,6 +473,17 @@ config DM_MULTIPATH_IOA
 
 	  If unsure, say N.
 
+config DM_MULTIPATH_SG_IO
+       bool "Retry SCSI generic I/O on multipath devices"
+       depends on DM_MULTIPATH && BLK_SCSI_REQUEST
+       help
+	 With this option, SCSI generic (SG) requests issued on multipath
+	 devices will behave similar to regular block I/O: upon failure,
+	 they are repeated on a different path, and the erroring device
+	 is marked as failed.
+
+	 If unsure, say N.
+
 config DM_DELAY
 	tristate "I/O delaying target"
 	depends on BLK_DEV_DM
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index ef7ddc27685c..187ea469f64a 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -88,6 +88,10 @@ ifeq ($(CONFIG_DM_INIT),y)
 dm-mod-objs			+= dm-init.o
 endif
 
+ifeq ($(CONFIG_DM_MULTIPATH_SG_IO),y)
+dm-mod-objs			+= dm-scsi_ioctl.o
+endif
+
 ifeq ($(CONFIG_DM_UEVENT),y)
 dm-mod-objs			+= dm-uevent.o
 endif
diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index 5953ff2bd260..8bd8a8e3916e 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -189,4 +189,9 @@ extern atomic_t dm_global_event_nr;
 extern wait_queue_head_t dm_global_eventq;
 void dm_issue_global_event(void);
 
+int __dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
+		       struct block_device **bdev,
+		       struct dm_target **target);
+void dm_unprepare_ioctl(struct mapped_device *md, int srcu_idx);
+
 #endif
diff --git a/drivers/md/dm-rq.h b/drivers/md/dm-rq.h
index 1eea0da641db..c6d2853e4d1d 100644
--- a/drivers/md/dm-rq.h
+++ b/drivers/md/dm-rq.h
@@ -44,4 +44,15 @@ ssize_t dm_attr_rq_based_seq_io_merge_deadline_show(struct mapped_device *md, ch
 ssize_t dm_attr_rq_based_seq_io_merge_deadline_store(struct mapped_device *md,
 						     const char *buf, size_t count);
 
+#ifdef CONFIG_DM_MULTIPATH_SG_IO
+int dm_sg_io_ioctl(struct block_device *bdev, fmode_t mode,
+		   unsigned int cmd, unsigned long uarg);
+#else
+static inline int dm_sg_io_ioctl(struct block_device *bdev, fmode_t mode,
+				 unsigned int cmd, unsigned long uarg)
+{
+	return -ENOTTY;
+}
+#endif
+
 #endif
diff --git a/drivers/md/dm-scsi_ioctl.c b/drivers/md/dm-scsi_ioctl.c
new file mode 100644
index 000000000000..70c5eb763101
--- /dev/null
+++ b/drivers/md/dm-scsi_ioctl.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Martin Wilck, SUSE LLC
+ */
+
+#include "dm-core.h"
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/uaccess.h>
+#include <linux/blk_types.h>
+#include <linux/blkdev.h>
+#include <linux/device-mapper.h>
+#include <scsi/sg.h>
+#include <scsi/scsi_cmnd.h>
+
+#define DM_MSG_PREFIX "sg_io"
+
+int dm_sg_io_ioctl(struct block_device *bdev, fmode_t mode,
+		   unsigned int cmd, unsigned long uarg)
+{
+	struct mapped_device *md = bdev->bd_disk->private_data;
+	struct sg_io_hdr hdr;
+	void __user *arg = (void __user *)uarg;
+	int rc, srcu_idx;
+	char path_name[BDEVNAME_SIZE];
+
+	if (cmd != SG_IO)
+		return -ENOTTY;
+
+	if (copy_from_user(&hdr, arg, sizeof(hdr)))
+		return -EFAULT;
+
+	if (hdr.interface_id != 'S')
+		return -EINVAL;
+
+	if (hdr.dxfer_len > (queue_max_hw_sectors(bdev->bd_disk->queue) << 9))
+		return -EIO;
+
+	for (;;) {
+		struct dm_target *tgt;
+		struct sg_io_hdr rhdr;
+
+		rc = __dm_prepare_ioctl(md, &srcu_idx, &bdev, &tgt);
+		if (rc < 0) {
+			DMERR("%s: failed to get path: %d",
+			      __func__, rc);
+			goto out;
+		}
+
+		rhdr = hdr;
+
+		rc = sg_io(bdev->bd_disk->queue, bdev->bd_disk, &rhdr, mode);
+
+		DMDEBUG("SG_IO via %s: rc = %d D%02xH%02xM%02xS%02x",
+			bdevname(bdev, path_name), rc,
+			rhdr.driver_status, rhdr.host_status,
+			rhdr.msg_status, rhdr.status);
+
+		/*
+		 * Errors resulting from invalid parameters shouldn't be retried
+		 * on another path.
+		 */
+		switch (rc) {
+		case -ENOIOCTLCMD:
+		case -EFAULT:
+		case -EINVAL:
+		case -EPERM:
+			goto out;
+		default:
+			break;
+		}
+
+		if (rhdr.info & SG_INFO_CHECK) {
+			int result;
+			blk_status_t sts;
+
+			__set_status_byte(&result, rhdr.status);
+			__set_msg_byte(&result, rhdr.msg_status);
+			__set_host_byte(&result, rhdr.host_status);
+			__set_driver_byte(&result, rhdr.driver_status);
+
+			sts = __scsi_result_to_blk_status(&result, result);
+			rhdr.host_status = host_byte(result);
+
+			/* See if this is a target or path error. */
+			if (sts == BLK_STS_OK)
+				rc = 0;
+			else if (blk_path_error(sts))
+				rc = -EIO;
+			else {
+				rc = blk_status_to_errno(sts);
+				goto out;
+			}
+		}
+
+		if (rc == 0) {
+			/* success */
+			if (copy_to_user(arg, &rhdr, sizeof(rhdr)))
+				rc = -EFAULT;
+			goto out;
+		}
+
+		/* Failure - fail path by sending a message to the target */
+		if (!tgt->type->message) {
+			DMWARN("invalid target!");
+			rc = -EIO;
+			goto out;
+		} else {
+			char bdbuf[BDEVT_SIZE];
+			char *argv[2] = { "fail_path", bdbuf };
+
+			scnprintf(bdbuf, sizeof(bdbuf), "%u:%u",
+				  MAJOR(bdev->bd_dev), MINOR(bdev->bd_dev));
+
+			DMDEBUG("sending \"%s %s\" to target", argv[0], argv[1]);
+			rc = tgt->type->message(tgt, 2, argv, NULL, 0);
+			if (rc < 0)
+				goto out;
+		}
+
+		dm_unprepare_ioctl(md, srcu_idx);
+	}
+out:
+	dm_unprepare_ioctl(md, srcu_idx);
+	return rc;
+}
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 50b693d776d6..5c2c205bf20e 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -522,8 +522,9 @@ static int dm_blk_report_zones(struct gendisk *disk, sector_t sector,
 #define dm_blk_report_zones		NULL
 #endif /* CONFIG_BLK_DEV_ZONED */
 
-static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
-			    struct block_device **bdev)
+int __dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
+		       struct block_device **bdev,
+		       struct dm_target **target)
 {
 	struct dm_target *tgt;
 	struct dm_table *map;
@@ -553,10 +554,19 @@ static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
 		goto retry;
 	}
 
+	if (r >= 0 && target)
+		*target = tgt;
+
 	return r;
 }
 
-static void dm_unprepare_ioctl(struct mapped_device *md, int srcu_idx)
+static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
+			    struct block_device **bdev)
+{
+	return __dm_prepare_ioctl(md, srcu_idx, bdev, NULL);
+}
+
+void dm_unprepare_ioctl(struct mapped_device *md, int srcu_idx)
 {
 	dm_put_live_table(md, srcu_idx);
 }
@@ -567,6 +577,10 @@ static int dm_blk_ioctl(struct block_device *bdev, fmode_t mode,
 	struct mapped_device *md = bdev->bd_disk->private_data;
 	int r, srcu_idx;
 
+	if ((dm_get_md_type(md) == DM_TYPE_REQUEST_BASED) &&
+	    ((r = dm_sg_io_ioctl(bdev, mode, cmd, arg)) != -ENOTTY))
+		return r;
+
 	r = dm_prepare_ioctl(md, &srcu_idx, &bdev);
 	if (r < 0)
 		goto out;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c032cfe133c7..bded0e6546da 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -934,6 +934,8 @@ extern int scsi_cmd_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 			  unsigned int, void __user *);
 extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 			 struct scsi_ioctl_command __user *);
+extern int sg_io(struct request_queue *, struct gendisk *,
+		 struct sg_io_hdr *, fmode_t);
 extern int get_sg_io_hdr(struct sg_io_hdr *hdr, const void __user *argp);
 extern int put_sg_io_hdr(const struct sg_io_hdr *hdr, void __user *argp);
 
-- 
2.31.1

Re: [dm-devel] [RFC PATCH v2 1/2] scsi: convert scsi_result_to_blk_status() to inline

[Bart Van Assche]

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.

Re: [dm-devel] [RFC PATCH v2 2/2] dm: add CONFIG_DM_MULTIPATH_SG_IO - failover for SG_IO on dm-multipath

[Bart Van Assche]

On 4/29/21 8:50 AM, mwilck@suse.com wrote:
> +	if (hdr.dxfer_len > (queue_max_hw_sectors(bdev->bd_disk->queue) << 9))
> +		return -EIO;

How about using SECTOR_SHIFT instead of the number 9?

> +		/*
> +		 * Errors resulting from invalid parameters shouldn't be retried
> +		 * on another path.
> +		 */
> +		switch (rc) {
> +		case -ENOIOCTLCMD:
> +		case -EFAULT:
> +		case -EINVAL:
> +		case -EPERM:
> +			goto out;
> +		default:
> +			break;
> +		}

Will -ENOMEM result in an immediate retry? Is that what's desired?

> +		if (rhdr.info & SG_INFO_CHECK) {
> +			int result;
> +			blk_status_t sts;
> +
> +			__set_status_byte(&result, rhdr.status);
> +			__set_msg_byte(&result, rhdr.msg_status);
> +			__set_host_byte(&result, rhdr.host_status);
> +			__set_driver_byte(&result, rhdr.driver_status);
> +
> +			sts = __scsi_result_to_blk_status(&result, result);
> +			rhdr.host_status = host_byte(result);
> +
> +			/* See if this is a target or path error. */
> +			if (sts == BLK_STS_OK)
> +				rc = 0;
> +			else if (blk_path_error(sts))
> +				rc = -EIO;
> +			else {
> +				rc = blk_status_to_errno(sts);
> +				goto out;
> +			}
> +		}

Will SAM_STAT_CHECK_CONDITION be treated as an I/O error? Is that what's
desired? If not, does that mean that scsi_result_to_blk_status()
shouldn't be used but instead that a custom SCSI result conversion is
needed?

If __scsi_result_to_blk_status() is the right function to call, how
about making that function accept the driver status, host status, msg
and SAM status as four separate arguments such that the __set_*_byte()
calls can be left out?

> +			char *argv[2] = { "fail_path", bdbuf };

Can the above array be declared static?

Thanks,

Bart.

Re: [dm-devel] [RFC PATCH v2 1/2] scsi: convert scsi_result_to_blk_status() to inline

[Martin Wilck]

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

Re: [dm-devel] [RFC PATCH v2 2/2] dm: add CONFIG_DM_MULTIPATH_SG_IO - failover for SG_IO on dm-multipath

[Martin Wilck]

On Thu, 2021-04-29 at 09:32 -0700, Bart Van Assche wrote:
> On 4/29/21 8:50 AM, mwilck@suse.com wrote:
> > +       if (hdr.dxfer_len > (queue_max_hw_sectors(bdev->bd_disk-
> > >queue) << 9))
> > +               return -EIO;
> 
> How about using SECTOR_SHIFT instead of the number 9?

no problem. That line was just copied from the scsi_ioctl code.

> 
> > +               /*
> > +                * Errors resulting from invalid parameters
> > shouldn't be retried
> > +                * on another path.
> > +                */
> > +               switch (rc) {
> > +               case -ENOIOCTLCMD:
> > +               case -EFAULT:
> > +               case -EINVAL:
> > +               case -EPERM:
> > +                       goto out;
> > +               default:
> > +                       break;
> > +               }
> 
> Will -ENOMEM result in an immediate retry? Is that what's desired?

No, I overlooked that case. Thanks for pointing this out. 

> 
> > +               if (rhdr.info & SG_INFO_CHECK) {
> > +                       int result;
> > +                       blk_status_t sts;
> > +
> > +                       __set_status_byte(&result, rhdr.status);
> > +                       __set_msg_byte(&result, rhdr.msg_status);
> > +                       __set_host_byte(&result, rhdr.host_status);
> > +                       __set_driver_byte(&result,
> > rhdr.driver_status);
> > +
> > +                       sts = __scsi_result_to_blk_status(&result,
> > result);
> > +                       rhdr.host_status = host_byte(result);
> > +
> > +                       /* See if this is a target or path error.
> > */
> > +                       if (sts == BLK_STS_OK)
> > +                               rc = 0;
> > +                       else if (blk_path_error(sts))
> > +                               rc = -EIO;
> > +                       else {
> > +                               rc = blk_status_to_errno(sts);
> > +                               goto out;
> > +                       }
> > +               }
> 
> Will SAM_STAT_CHECK_CONDITION be treated as an I/O error? Is that
> what's
> desired? If not, does that mean that scsi_result_to_blk_status()
> shouldn't be used but instead that a custom SCSI result conversion is
> needed?

This mimics the logic for regular SCSI block I/O. By default, CHECK
CONDITION indeed maps to a BLK_STS_IO_ERR, and will be treated as a
path error. As you probably know, there are some exceptions that are
handled in the SCSI mid-layer beforehand. For example, check_sense()
sets DID_TARGET_FAILURE or DID_MEDIUM_ERROR for certain sense codes,
which map to target errors. So yes, I think this is correct.

> If __scsi_result_to_blk_status() is the right function to call, how
> about making that function accept the driver status, host status, msg
> and SAM status as four separate arguments such that the
> __set_*_byte()
> calls can be left out?
> 
> > +                       char *argv[2] = { "fail_path", bdbuf };
> 
> Can the above array be declared static?

How would that work? The function needs to be reentrant, and bdbuf is
on the stack. I don't see an issue here, it's really just two pointers
on the stack.

Regards,
Martin

Re: [dm-devel] [RFC PATCH v2 1/2] scsi: convert scsi_result_to_blk_status() to inline

[Bart Van Assche]

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.