[PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[Wenchao Hao]

The original error injection mechanism was based on scsi_host which
could not inject fault for a single SCSI device.

This patchset provides the ability to inject errors for a single
SCSI device. Now we supports inject timeout errors, queuecommand
errors, and hostbyte, driverbyte, statusbyte, and sense data for
specific SCSI Command.

The first patch add an sysfs interface to add and inquiry single
device's error injection info; the second patch defined how to remove
an injection which has been added. The following 3 patches use the
injection info and generate the related error type.

Wenchao Hao (5):
  scsi:scsi_debug: Add sysfs interface to manage scsi devices' error
    injection
  scsi:scsi_debug: Define grammar to remove added error injection
  scsi:scsi_debug: timeout command if the error is injected
  scsi:scsi_debug: Return failed value if the error is injected
  scsi:scsi_debug: set command's result and sense data if the error is
    injected

 drivers/scsi/scsi_debug.c | 296 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 296 insertions(+)

-- 
2.35.3

[PATCH 1/5] scsi:scsi_debug: Add sysfs interface to manage scsi devices' error injection

[Wenchao Hao]

Add interface "/sys/class/scsi_device/<hctl>/device/error_inject/error" to
manage scsi_device's error injection rules. Write it to add/remove an error
injection rule, read it to get error injection rules.

The interface format is line-by-line, each line is an error injection rule.
Each rule contains integers separated by spaces, first three columns are
fixed to "Error code", "Error count" and "SCSI command", other columns
depend on error code.

  +--------+------+-------------------------------------------------------+
  | Column | Type | Description                                           |
  +--------+------+-------------------------------------------------------+
  |   1    |  u8  | Error code                                            |
  |        |      |  0: timeout SCSI command                              |
  |        |      |  1: fail queuecommand, make queuecommand return       |
  |        |      |     specific value                                    |
  |        |      |  2: fail command, finish command with specific status |
  |        |      |     and sense date                                    |
  +--------+------+-------------------------------------------------------+
  |   2    |  s32 | Error Count                                           |
  |        |      |  0: the rule would not take effect                    |
  |        |      |  positive: the rule would always take effect          |
  |        |      |  negative: the rule take effect for the absolute of   |
  |        |      |            this value                                 |
  +--------+------+-------------------------------------------------------+
  |   3    |  x8  | SCSI command this rule is for, 0xff for all command   |
  +--------+------+-------------------------------------------------------+
  |  ...   |  xxx | Error type specific fields                            |
  +--------+------+-------------------------------------------------------+

- When multiple error injects are added to one scsi command, the one
  with smaller error code would take effect.
- If an same error is added to one device, old one would be overwritten.
- Currently, basic types are (u8/u16/u32/u64/s8/s16/s32/s64), hexadecimal
  types (x8/x16/x32/x64)

Timeout SCSI command:

  +--------+------+-------------------------------------------------------+
  | Column | Type | Description                                           |
  +--------+------+-------------------------------------------------------+
  |   1    |  u8  | Error type, fixed to 0x0                              |
  +--------+------+-------------------------------------------------------+
  |   2    |  s32 | Error Count                                           |
  |        |      |  0: the rule would not take effect                    |
  |        |      |  positive: the rule would always take effect          |
  |        |      |  negative: the rule take effect for the absolute of   |
  |        |      |            this value                                 |
  +--------+------+-------------------------------------------------------+
  |   3    |  x8  | SCSI command this rule is for, 0xff for all command   |
  +--------+------+-------------------------------------------------------+

  Examples:
    error=/sys/class/scsi_device/0:0:0:1/device/error_inject/error
    echo "0 -10 0x12" > $error
  would make the device's inquiry command timeout for 10 time.
    error=/sys/class/scsi_device/0:0:0:1/device/error_inject/error
    echo "0 1 0x12" > $error
  would make the device's inquiry timeout forever.

Make queuecommand return specific value:

  +--------+------+-------------------------------------------------------+
  | Column | Type | Description                                           |
  +--------+------+-------------------------------------------------------+
  |   1    |  u8  | Error type, fixed to 0x1                              |
  +--------+------+-------------------------------------------------------+
  |   2    |  s32 | Error Count                                           |
  |        |      |  0: the rule would not take effect                    |
  |        |      |  positive: the rule would always take effect          |
  |        |      |  negative: the rule take effect for the absolute of   |
  |        |      |            this value                                 |
  +--------+------+-------------------------------------------------------+
  |   3    |  x8  | SCSI command this rule is for, 0xff for all command   |
  +--------+------+-------------------------------------------------------+
  |   4    |  x32 | The return value of queuecommand we desired           |
  +--------+------+-------------------------------------------------------+

  Examples:
    error=/sys/class/scsi_device/0:0:0:1/device/error_inject/error
    echo "1 1 0x12 0x1055" > $error
  would make device's inquiry command return 0x1055(SCSI_MLQUEUE_HOST_BUSY)
  forever.

Set scsi_cmd's status and sense data to specific value:

  +--------+------+-------------------------------------------------------+
  | Column | Type | Description                                           |
  +--------+------+-------------------------------------------------------+
  |   1    |  u8  | Error type, fixed to 0x2                              |
  +--------+------+-------------------------------------------------------+
  |   2    |  s32 | Error Count                                           |
  |        |      |  0: the rule would not take effect                    |
  |        |      |  positive: the rule would always take effect          |
  |        |      |  negative: the rule take effect for the absolute of   |
  |        |      |            this value                                 |
  +--------+------+-------------------------------------------------------+
  |   3    |  x8  | SCSI command this rule is for, 0xff for all command   |
  +--------+------+-------------------------------------------------------+
  |   4    |  x8  | Host byte in scsi_cmd's status                        |
  +--------+------+-------------------------------------------------------+
  |   5    |  x8  | Driver byte in scsi_cmd's status                      |
  +--------+------+-------------------------------------------------------+
  |   6    |  x8  | Status byte in scsi_cmd's status                      |
  +--------+------+-------------------------------------------------------+
  |   7    |  x8  | Sense Key of scsi_cmnd                                |
  +--------+------+-------------------------------------------------------+
  |   8    |  x8  | ASC of scsi_cmnd                                      |
  +--------+------+-------------------------------------------------------+
  |   9    |  x8  | ASQ of scsi_cmnd                                      |
  +--------+------+-------------------------------------------------------+

  Examples:
    error=/sys/class/scsi_device/0:0:0:1/device/error_inject/error
    echo "2 -10 0x88 0 0 0x2 0x3 0x11 0x0" >$error
  would make device's read command return with media error UNC:

Signed-off-by: Wenchao Hao <haowenchao2@huawei.com>
---
 drivers/scsi/scsi_debug.c | 161 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 161 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 7ed117e78bd4..8cfa19f8b034 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -280,6 +280,41 @@ struct sdeb_zone_state {	/* ZBC: per zone state */
 	sector_t z_wp;
 };
 
+enum sdebug_err_type {
+	ERR_TMOUT_CMD		= 0,	/* make specific scsi command timeout */
+	ERR_FAIL_QUEUE_CMD	= 1,	/* make specific scsi command's */
+					/* queuecmd return failed */
+	ERR_FAIL_CMD		= 2,	/* make specific scsi command's */
+					/* queuecmd return succeed but */
+					/* with errors set in scsi_cmnd */
+};
+
+struct sdebug_err_inject {
+	int type;
+	struct list_head list;
+	int cnt;
+	unsigned char cmd;
+
+	union {
+		/*
+		 * For ERR_FAIL_QUEUE_CMD
+		 */
+		int queuecmd_ret;
+
+		/*
+		 * For ERR_FAIL_CMD
+		 */
+		struct {
+			unsigned char host_byte;
+			unsigned char driver_byte;
+			unsigned char status_byte;
+			unsigned char sense_key;
+			unsigned char asc;
+			unsigned char asq;
+		};
+	};
+};
+
 struct sdebug_dev_info {
 	struct list_head dev_list;
 	unsigned int channel;
@@ -305,6 +340,8 @@ struct sdebug_dev_info {
 	unsigned int max_open;
 	ktime_t create_ts;	/* time since bootup that this device was created */
 	struct sdeb_zone_state *zstate;
+
+	struct list_head inject_err_list;
 };
 
 struct sdebug_host_info {
@@ -5152,6 +5189,7 @@ static struct sdebug_dev_info *sdebug_device_create(
 		}
 		devip->create_ts = ktime_get_boottime();
 		atomic_set(&devip->stopped, (sdeb_tur_ms_to_ready > 0 ? 2 : 0));
+		INIT_LIST_HEAD(&devip->inject_err_list);
 		list_add_tail(&devip->dev_list, &sdbg_host->dev_info_list);
 	}
 	return devip;
@@ -7553,6 +7591,128 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
 	return num_entries;
 }
 
+static void sdebug_err_add(struct device *dev, struct sdebug_err_inject *new)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdev->hostdata;
+	struct sdebug_err_inject *tmp, *err;
+
+	list_for_each_entry_safe(err, tmp, &devip->inject_err_list, list) {
+		if (err->type == new->type && err->cmd == new->cmd) {
+			sdev_printk(KERN_INFO, sdev, "Substituted %d 0x%x\n",
+				err->type, err->cmd);
+			list_del(&err->list);
+			kfree(err);
+		}
+	}
+
+	list_add_tail(&new->list, &devip->inject_err_list);
+}
+
+static ssize_t error_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	int tmp, ret = 0;
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdev->hostdata;
+	struct sdebug_err_inject *err;
+
+	tmp = sysfs_emit_at(buf, ret, "Type\tCount\tCommand\n");
+	ret += tmp;
+
+	list_for_each_entry(err, &devip->inject_err_list, list) {
+		switch (err->type) {
+		case ERR_TMOUT_CMD:
+			tmp = sysfs_emit_at(buf, ret, "%d\t%d\t0x%x\n",
+				err->type, err->cnt, err->cmd);
+			ret += tmp;
+		break;
+
+		case ERR_FAIL_QUEUE_CMD:
+			tmp = sysfs_emit_at(buf, ret, "%d\t%d\t0x%x\t0x%x\n",
+				err->type, err->cnt, err->cmd, err->queuecmd_ret);
+			ret += tmp;
+		break;
+
+		case ERR_FAIL_CMD:
+			tmp = sysfs_emit_at(buf, ret,
+				"%d\t%d\t0x%x\t0x%x 0x%x 0x%x 0x%x 0x%x 0x%x\n",
+				err->type, err->cnt, err->cmd,
+				err->host_byte, err->driver_byte,
+				err->status_byte, err->sense_key,
+				err->asc, err->asq);
+			ret += tmp;
+		break;
+		}
+	}
+
+	return ret;
+}
+static ssize_t error_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	unsigned int inject_type;
+	struct sdebug_err_inject *inject;
+
+	if (sscanf(buf, "%d", &inject_type) != 1)
+		return -EINVAL;
+
+	inject = kzalloc(sizeof(struct sdebug_err_inject), GFP_KERNEL);
+	if (!inject)
+		return -ENOMEM;
+
+	switch (inject_type) {
+	case ERR_TMOUT_CMD:
+		if (sscanf(buf, "%d %d %hhx", &inject->type, &inject->cnt,
+			   &inject->cmd) != 3)
+			goto out_error;
+	break;
+
+	case ERR_FAIL_QUEUE_CMD:
+		if (sscanf(buf, "%d %d %hhx %x", &inject->type, &inject->cnt,
+			   &inject->cmd, &inject->queuecmd_ret) != 4)
+			goto out_error;
+	break;
+
+	case ERR_FAIL_CMD:
+		if (sscanf(buf, "%d %d %hhx %hhx %hhx %hhx %hhx %hhx %hhx",
+			   &inject->type, &inject->cnt, &inject->cmd,
+			   &inject->host_byte, &inject->driver_byte,
+			   &inject->status_byte, &inject->sense_key,
+			   &inject->asc, &inject->asq) != 9)
+			goto out_error;
+	break;
+
+	default:
+		goto out_error;
+	break;
+	}
+
+	sdebug_err_add(dev, inject);
+
+	return count;
+
+out_error:
+	kfree(inject);
+	return -EINVAL;
+}
+static DEVICE_ATTR_RW(error);
+
+static struct attribute *sdebug_sdev_attrs[] = {
+	&dev_attr_error.attr,
+	NULL
+};
+
+static const struct attribute_group sdebug_sdev_attr_group = {
+	.name = "error_inject",
+	.attrs = sdebug_sdev_attrs,
+};
+
+const struct attribute_group *sdebug_sdev_groups[] = {
+	&sdebug_sdev_attr_group,
+	NULL
+};
+
 static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 				   struct scsi_cmnd *scp)
 {
@@ -7732,6 +7892,7 @@ static struct scsi_host_template sdebug_driver_template = {
 	.ioctl =		scsi_debug_ioctl,
 	.queuecommand =		scsi_debug_queuecommand,
 	.change_queue_depth =	sdebug_change_qdepth,
+	.sdev_groups =		sdebug_sdev_groups,
 	.map_queues =		sdebug_map_queues,
 	.mq_poll =		sdebug_blk_mq_poll,
 	.eh_abort_handler =	scsi_debug_abort,
-- 
2.35.3

[PATCH 2/5] scsi:scsi_debug: Define grammar to remove added error injection

[Wenchao Hao]

The grammar to remove error injection is a line with fixed 3 columns
separated by spaces.

First column is fixed to "-". It tells this is a removal operation.
Second column is the error code to match.
Third column is the scsi command to match.

For example the following command would remove timeout injection of
inquiry command.

  echo "- 0 0x12" > /sys/class/scsi_device/0:0:0:1/device/error_inect/error

Signed-off-by: Wenchao Hao <haowenchao2@huawei.com>
---
 drivers/scsi/scsi_debug.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 8cfa19f8b034..e5ee75ce6d27 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -7609,6 +7609,30 @@ static void sdebug_err_add(struct device *dev, struct sdebug_err_inject *new)
 	list_add_tail(&new->list, &devip->inject_err_list);
 }
 
+static int sdebug_err_remove(struct device *dev, const char *buf, size_t count)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdev->hostdata;
+	struct sdebug_err_inject *tmp, *err;
+	int type;
+	unsigned char cmd;
+
+	if (sscanf(buf, "- %d %hhx", &type, &cmd) != 2)
+		return -EINVAL;
+
+	list_for_each_entry_safe(err, tmp, &devip->inject_err_list, list) {
+		if (err->type == type && err->cmd == cmd) {
+			sdev_printk(KERN_INFO, sdev, "Remove %d 0x%x\n",
+				err->type, err->cmd);
+			list_del(&err->list);
+			kfree(err);
+			return count;
+		}
+	}
+
+	return -EINVAL;
+}
+
 static ssize_t error_show(struct device *dev, struct device_attribute *attr,
 		char *buf)
 {
@@ -7654,6 +7678,9 @@ static ssize_t error_store(struct device *dev, struct device_attribute *attr,
 	unsigned int inject_type;
 	struct sdebug_err_inject *inject;
 
+	if (buf[0] == '-')
+		return sdebug_err_remove(dev, buf, count);
+
 	if (sscanf(buf, "%d", &inject_type) != 1)
 		return -EINVAL;
 
-- 
2.35.3

[PATCH 3/5] scsi:scsi_debug: timeout command if the error is injected

[Wenchao Hao]

If a timeout error is injected, return 0 from scsi_debug_queuecommand to
make the command timeout.

For example, the following command would inject timeout error for
inquiry(0x12) command for 10 times:
  error=/sys/class/scsi_device/0:0:0:1/device/error_inject/error
  echo "0 -10 0x12" > $error

Signed-off-by: Wenchao Hao <haowenchao2@huawei.com>
---
 drivers/scsi/scsi_debug.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index e5ee75ce6d27..06a830eb2ca1 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -7740,6 +7740,30 @@ const struct attribute_group *sdebug_sdev_groups[] = {
 	NULL
 };
 
+static int sdebug_timeout_cmd(struct scsi_cmnd *cmnd)
+{
+	struct scsi_device *sdp = cmnd->device;
+	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+	struct sdebug_err_inject *err;
+	unsigned char *cmd = cmnd->cmnd;
+	int ret = 0;
+
+	if (devip == NULL)
+		return 0;
+
+	list_for_each_entry(err, &devip->inject_err_list, list) {
+		if (err->type == ERR_TMOUT_CMD &&
+		    (err->cmd == cmd[0] || err->cmd == 0xff)) {
+			ret = !!err->cnt;
+			if (err->cnt < 0)
+				err->cnt++;
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 				   struct scsi_cmnd *scp)
 {
@@ -7798,6 +7822,12 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 		if (NULL == devip)
 			goto err_out;
 	}
+
+	if (sdebug_timeout_cmd(scp)) {
+		scmd_printk(KERN_INFO, scp, "timeout command 0x%x\n", opcode);
+		return 0;
+	}
+
 	if (unlikely(inject_now && !atomic_read(&sdeb_inject_pending)))
 		atomic_set(&sdeb_inject_pending, 1);
 
-- 
2.35.3

[PATCH 4/5] scsi:scsi_debug: Return failed value if the error is injected

[Wenchao Hao]

If a fail queuecommand error is injected, return the failed value defined
in the rule from queuecommand.

We can make any scsi command's queuecommand return the value we desired,
for example make it return SCSI_MLQUEUE_HOST_BUSY.

  error=/sys/class/scsi_device/0:0:0:1/device/error_inject/error
  echo "1 1 0x12 0x1055" > $error

would make all inquiry(0x12) command's queuecommand return 0x1055:

Signed-off-by: Wenchao Hao <haowenchao2@huawei.com>
---
 drivers/scsi/scsi_debug.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 06a830eb2ca1..1deccf8dfc90 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -7764,6 +7764,30 @@ static int sdebug_timeout_cmd(struct scsi_cmnd *cmnd)
 	return 0;
 }
 
+static int sdebug_fail_queue_cmd(struct scsi_cmnd *cmnd)
+{
+	struct scsi_device *sdp = cmnd->device;
+	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+	struct sdebug_err_inject *err;
+	unsigned char *cmd = cmnd->cmnd;
+	int ret = 0;
+
+	if (devip == NULL)
+		return 0;
+
+	list_for_each_entry(err, &devip->inject_err_list, list) {
+		if (err->type == ERR_FAIL_QUEUE_CMD &&
+		    (err->cmd == cmd[0] || err->cmd == 0xff)) {
+			ret = err->cnt ? err->queuecmd_ret : 0;
+			if (err->cnt < 0)
+				err->cnt++;
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 				   struct scsi_cmnd *scp)
 {
@@ -7783,6 +7807,7 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 	u8 opcode = cmd[0];
 	bool has_wlun_rl;
 	bool inject_now;
+	int ret = 0;
 
 	scsi_set_resid(scp, 0);
 	if (sdebug_statistics) {
@@ -7828,6 +7853,13 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 		return 0;
 	}
 
+	ret = sdebug_fail_queue_cmd(scp);
+	if (ret) {
+		scmd_printk(KERN_INFO, scp, "fail queue command 0x%x with 0x%x\n",
+				opcode, ret);
+		return ret;
+	}
+
 	if (unlikely(inject_now && !atomic_read(&sdeb_inject_pending)))
 		atomic_set(&sdeb_inject_pending, 1);
 
-- 
2.35.3

[PATCH 5/5] scsi:scsi_debug: set command's result and sense data if the error is injected

[Wenchao Hao]

If a fail commnd error is injected, set the command's status and sense
data then finish this scsi command.

For example, the following command would make read(0x88) command finished
with UNC for 8 times:
  error=/sys/class/scsi_device/0:0:0:1/device/error_inject/error
  echo "2 -8 0x88 0 0 0x2 0x3 0x11 0x0" >$error

Signed-off-by: Wenchao Hao <haowenchao2@huawei.com>
---
 drivers/scsi/scsi_debug.c | 46 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 1deccf8dfc90..863e1a23c055 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -7788,6 +7788,41 @@ static int sdebug_fail_queue_cmd(struct scsi_cmnd *cmnd)
 	return 0;
 }
 
+static int sdebug_fail_cmd(struct scsi_cmnd *cmnd, int *retval,
+			   struct sdebug_err_inject *info)
+{
+	struct scsi_device *sdp = cmnd->device;
+	struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+	struct sdebug_err_inject *err;
+	unsigned char *cmd = cmnd->cmnd;
+	int ret = 0;
+	int result;
+
+	if (devip == NULL)
+		return 0;
+
+	list_for_each_entry(err, &devip->inject_err_list, list) {
+		if (err->type == ERR_FAIL_CMD &&
+		    (err->cmd == cmd[0] || err->cmd == 0xff)) {
+			if (!err->cnt)
+				return 0;
+			ret = !!err->cnt;
+			goto out_handle;
+		}
+	}
+	return 0;
+
+out_handle:
+	if (err->cnt < 0)
+		err->cnt++;
+	mk_sense_buffer(cmnd, err->sense_key, err->asc, err->asq);
+	result = err->status_byte | err->host_byte << 16 | err->driver_byte << 24;
+	*info = *err;
+	*retval = schedule_resp(cmnd, devip, result, NULL, 0, 0);
+
+	return ret;
+}
+
 static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 				   struct scsi_cmnd *scp)
 {
@@ -7808,6 +7843,7 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 	bool has_wlun_rl;
 	bool inject_now;
 	int ret = 0;
+	struct sdebug_err_inject err;
 
 	scsi_set_resid(scp, 0);
 	if (sdebug_statistics) {
@@ -7860,6 +7896,16 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 		return ret;
 	}
 
+	if (sdebug_fail_cmd(scp, &ret, &err)) {
+		scmd_printk(KERN_INFO, scp,
+			"fail command 0x%x with hostbyte=0x%x, "
+			"driverbyte=0x%x, statusbyte=0x%x, "
+			"sense_key=0x%x, asc=0x%x, asq=0x%x\n",
+			opcode, err.host_byte, err.driver_byte,
+			err.status_byte, err.sense_key, err.asc, err.asq);
+		return ret;
+	}
+
 	if (unlikely(inject_now && !atomic_read(&sdeb_inject_pending)))
 		atomic_set(&sdeb_inject_pending, 1);
 
-- 
2.35.3

Re: [PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[John Garry]

On 23/03/2023 11:55, Wenchao Hao wrote:
> The original error injection mechanism was based on scsi_host which
> could not inject fault for a single SCSI device.
> 
> This patchset provides the ability to inject errors for a single
> SCSI device. Now we supports inject timeout errors, queuecommand
> errors, and hostbyte, driverbyte, statusbyte, and sense data for
> specific SCSI Command.

There is already a basic mechanism to generate errors - like timeouts - 
on "nth" command. Can you say why you want this new interface? What 
special scenarios are you trying to test/validate (which could not be 
achieved based on the current mechanism)?

With this series we would have 2x methods to inject errors, which is 
less than ideal, and they seem to possibly conflict as well, e.g. I set 
timeout for nth command via current interface and then use the new 
interface to set timeout for some other cadence. What behavior to expect 
...?

I'm not saying that I am a huge fan of the current inject mechanism, but 
at the very least you need to provide more justification for this series.

> 
> The first patch add an sysfs interface to add and inquiry single
> device's error injection info; the second patch defined how to remove
> an injection which has been added. The following 3 patches use the
> injection info and generate the related error type.
> 
> Wenchao Hao (5):
>    scsi:scsi_debug: Add sysfs interface to manage scsi devices' error
>      injection
>    scsi:scsi_debug: Define grammar to remove added error injection
>    scsi:scsi_debug: timeout command if the error is injected
>    scsi:scsi_debug: Return failed value if the error is injected
>    scsi:scsi_debug: set command's result and sense data if the error is
>      injected
> 
>   drivers/scsi/scsi_debug.c | 296 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 296 insertions(+)
> 

Re: [PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[haowenchao (C)]

On 2023/3/23 20:40, John Garry wrote:
> On 23/03/2023 11:55, Wenchao Hao wrote:
>> The original error injection mechanism was based on scsi_host which
>> could not inject fault for a single SCSI device.
>>
>> This patchset provides the ability to inject errors for a single
>> SCSI device. Now we supports inject timeout errors, queuecommand
>> errors, and hostbyte, driverbyte, statusbyte, and sense data for
>> specific SCSI Command.
> 
> There is already a basic mechanism to generate errors - like timeouts - on "nth" command. Can you say why you want this new interface? What special scenarios are you trying to test/validate (which could not be achieved based on the current mechanism)?
>

I am testing a new error handle policy which is based on single scsi_device
without set host to RECOVERY. So I need a method to generate errors for
single SCSI devices.

While we can not generate errors for single device with current mechanism
because it is designed for host-wide error generation.
  
> With this series we would have 2x methods to inject errors, which is less than ideal, and they seem to possibly conflict as well, e.g. I set timeout for nth command via current interface and then use the new interface to set timeout for some other cadence. What behavior to expect ...?

I did not take this issue in consideration. I now assume the users would
not use these 2 methods at same time.

What's more, I don not know where to write the usage of this newly added
interface, maybe we can explain these in doc?

> 
> I'm not saying that I am a huge fan of the current inject mechanism, but at the very least you need to provide more justification for this series.
>>>
>> The first patch add an sysfs interface to add and inquiry single
>> device's error injection info; the second patch defined how to remove
>> an injection which has been added. The following 3 patches use the
>> injection info and generate the related error type.
>>
>> Wenchao Hao (5):
>>    scsi:scsi_debug: Add sysfs interface to manage scsi devices' error
>>      injection
>>    scsi:scsi_debug: Define grammar to remove added error injection
>>    scsi:scsi_debug: timeout command if the error is injected
>>    scsi:scsi_debug: Return failed value if the error is injected
>>    scsi:scsi_debug: set command's result and sense data if the error is
>>      injected
>>
>>   drivers/scsi/scsi_debug.c | 296 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 296 insertions(+)
>>
> 
> 

Re: [PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[John Garry]

On 23/03/2023 13:13, haowenchao (C) wrote:
> On 2023/3/23 20:40, John Garry wrote:
>> On 23/03/2023 11:55, Wenchao Hao wrote:
>>> The original error injection mechanism was based on scsi_host which
>>> could not inject fault for a single SCSI device.
>>>
>>> This patchset provides the ability to inject errors for a single
>>> SCSI device. Now we supports inject timeout errors, queuecommand
>>> errors, and hostbyte, driverbyte, statusbyte, and sense data for
>>> specific SCSI Command.
>>
>> There is already a basic mechanism to generate errors - like timeouts 
>> - on "nth" command. Can you say why you want this new interface? What 
>> special scenarios are you trying to test/validate (which could not be 
>> achieved based on the current mechanism)?
>>
> 
> I am testing a new error handle policy which is based on single scsi_device
> without set host to RECOVERY. So I need a method to generate errors for
> single SCSI devices.
> 
> While we can not generate errors for single device with current mechanism
> because it is designed for host-wide error generation.
> 
>> With this series we would have 2x methods to inject errors, which is 
>> less than ideal, and they seem to possibly conflict as well, e.g. I 
>> set timeout for nth command via current interface and then use the new 
>> interface to set timeout for some other cadence. What behavior to 
>> expect ...?
> 
> I did not take this issue in consideration. I now assume the users would
> not use these 2 methods at same time.
> 
> What's more, I don not know where to write the usage of this newly added
> interface, maybe we can explain these in doc?

sysfs entries are described in Documentation/ABI, but please don't add 
elaborate programming interfaces in sysfs files (like in these patches) 
- a sysfs file should be just for reading or writing a single value

> 
>>
>> I'm not saying that I am a huge fan of the current inject mechanism, 
>> but at the very least you need to provide more justification for this 
>> series.
>>>>
>>> The first patch add an sysfs interface to add and inquiry single
>>> device's error injection info; the second patch defined how to remove
>>> an injection which has been added. The following 3 patches use the
>>> injection info and generate the related error type.
>>>
>>> Wenchao Hao (5):
>>>    scsi:scsi_debug: Add sysfs interface to manage scsi devices' error
>>>      injection
>>>    scsi:scsi_debug: Define grammar to remove added error injection
>>>    scsi:scsi_debug: timeout command if the error is injected
>>>    scsi:scsi_debug: Return failed value if the error is injected
>>>    scsi:scsi_debug: set command's result and sense data if the error is
>>>      injected
>>>
>>>   drivers/scsi/scsi_debug.c | 296 ++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 296 insertions(+)
>>>
>>
>>
> 

Re: [PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[Douglas Gilbert]

On 2023-03-23 12:25, John Garry wrote:
> On 23/03/2023 13:13, haowenchao (C) wrote:
>> On 2023/3/23 20:40, John Garry wrote:
>>> On 23/03/2023 11:55, Wenchao Hao wrote:
>>>> The original error injection mechanism was based on scsi_host which
>>>> could not inject fault for a single SCSI device.
>>>>
>>>> This patchset provides the ability to inject errors for a single
>>>> SCSI device. Now we supports inject timeout errors, queuecommand
>>>> errors, and hostbyte, driverbyte, statusbyte, and sense data for
>>>> specific SCSI Command.
>>>
>>> There is already a basic mechanism to generate errors - like timeouts - on 
>>> "nth" command. Can you say why you want this new interface? What special 
>>> scenarios are you trying to test/validate (which could not be achieved based 
>>> on the current mechanism)?
>>>
>>
>> I am testing a new error handle policy which is based on single scsi_device
>> without set host to RECOVERY. So I need a method to generate errors for
>> single SCSI devices.
>>
>> While we can not generate errors for single device with current mechanism
>> because it is designed for host-wide error generation.
>>
>>> With this series we would have 2x methods to inject errors, which is less 
>>> than ideal, and they seem to possibly conflict as well, e.g. I set timeout 
>>> for nth command via current interface and then use the new interface to set 
>>> timeout for some other cadence. What behavior to expect ...?
>>
>> I did not take this issue in consideration. I now assume the users would
>> not use these 2 methods at same time.
>>
>> What's more, I don not know where to write the usage of this newly added
>> interface, maybe we can explain these in doc?
> 
> sysfs entries are described in Documentation/ABI, but please don't add elaborate 
> programming interfaces in sysfs files (like in these patches) - a sysfs file 
> should be just for reading or writing a single value

Hi,
Maybe this link might help for scsi_debug documentation:
     https://doug-gilbert.github.io/scsi_debug.html

And rather than sysfs for complicated, per (pseudo_ device
settings, perhaps we could think about a SCSI mechanism like
the "Unit Attention" mode page [0x0] which is vendor specific
and used by Seagate and WDC for this sort of thing.
A framework is already in the scsi_debug driver to change
some mode page settings:

# sdparm /dev/sg0
     /dev/sg0: Linux     scsi_debug        0191
Read write error recovery mode page:
   AWRE          1  [cha: n, def:  1]
   ARRE          1  [cha: n, def:  1]
   PER           0  [cha: n, def:  0]
Caching (SBC) mode page:
   WCE           1  [cha: y, def:  1]
   RCD           0  [cha: n, def:  0]
Control mode page:
   SWP           0  [cha: n, def:  0]
Informational exceptions control mode page:
   EWASC         0  [cha: n, def:  0]
   DEXCPT        1  [cha: n, def:  1]
   MRIE          0  [cha: y, def:  0]

As can be seen WCE and MRIE are changeable, so

# sdparm --clear=WCE /dev/sg0
# sdparm --get=WCE /dev/sg0
     /dev/sg0: Linux     scsi_debug        0191
WCE           0  [cha: y, def:  1]


Doug Gilbert


>>> I'm not saying that I am a huge fan of the current inject mechanism, but at 
>>> the very least you need to provide more justification for this series.
>>>>>
>>>> The first patch add an sysfs interface to add and inquiry single
>>>> device's error injection info; the second patch defined how to remove
>>>> an injection which has been added. The following 3 patches use the
>>>> injection info and generate the related error type.
>>>>
>>>> Wenchao Hao (5):
>>>>    scsi:scsi_debug: Add sysfs interface to manage scsi devices' error
>>>>      injection
>>>>    scsi:scsi_debug: Define grammar to remove added error injection
>>>>    scsi:scsi_debug: timeout command if the error is injected
>>>>    scsi:scsi_debug: Return failed value if the error is injected
>>>>    scsi:scsi_debug: set command's result and sense data if the error is
>>>>      injected
>>>>
>>>>   drivers/scsi/scsi_debug.c | 296 ++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 296 insertions(+)
>>>>
>>>
>>>
>>
> 

Re: [PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[haowenchao (C)]

On 2023/3/24 0:25, John Garry wrote:
> On 23/03/2023 13:13, haowenchao (C) wrote:
>> On 2023/3/23 20:40, John Garry wrote:
>>> On 23/03/2023 11:55, Wenchao Hao wrote:
>>>> The original error injection mechanism was based on scsi_host which
>>>> could not inject fault for a single SCSI device.
>>>>
>>>> This patchset provides the ability to inject errors for a single
>>>> SCSI device. Now we supports inject timeout errors, queuecommand
>>>> errors, and hostbyte, driverbyte, statusbyte, and sense data for
>>>> specific SCSI Command.
>>>
>>> There is already a basic mechanism to generate errors - like timeouts - on "nth" command. Can you say why you want this new interface? What special scenarios are you trying to test/validate (which could not be achieved based on the current mechanism)?
>>>
>>
>> I am testing a new error handle policy which is based on single scsi_device
>> without set host to RECOVERY. So I need a method to generate errors for
>> single SCSI devices.
>>
>> While we can not generate errors for single device with current mechanism
>> because it is designed for host-wide error generation.
>>
>>> With this series we would have 2x methods to inject errors, which is less than ideal, and they seem to possibly conflict as well, e.g. I set timeout for nth command via current interface and then use the new interface to set timeout for some other cadence. What behavior to expect ...?
>>
>> I did not take this issue in consideration. I now assume the users would
>> not use these 2 methods at same time.
>>
>> What's more, I don not know where to write the usage of this newly added
>> interface, maybe we can explain these in doc?
> 
> sysfs entries are described in Documentation/ABI, but please don't add elaborate programming interfaces in sysfs files (like in these patches) - a sysfs file should be just for reading or writing a single value
> 

If sysfs is not recommended, what about proc?

>>
>>>
>>> I'm not saying that I am a huge fan of the current inject mechanism, but at the very least you need to provide more justification for this series.
>>>>>
>>>> The first patch add an sysfs interface to add and inquiry single
>>>> device's error injection info; the second patch defined how to remove
>>>> an injection which has been added. The following 3 patches use the
>>>> injection info and generate the related error type.
>>>>
>>>> Wenchao Hao (5):
>>>>    scsi:scsi_debug: Add sysfs interface to manage scsi devices' error
>>>>      injection
>>>>    scsi:scsi_debug: Define grammar to remove added error injection
>>>>    scsi:scsi_debug: timeout command if the error is injected
>>>>    scsi:scsi_debug: Return failed value if the error is injected
>>>>    scsi:scsi_debug: set command's result and sense data if the error is
>>>>      injected
>>>>
>>>>   drivers/scsi/scsi_debug.c | 296 ++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 296 insertions(+)
>>>>
>>>
>>>
>>
> 

Re: [PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[haowenchao (C)]

On 2023/3/24 1:24, Douglas Gilbert wrote:
> On 2023-03-23 12:25, John Garry wrote:
>> On 23/03/2023 13:13, haowenchao (C) wrote:
>>> On 2023/3/23 20:40, John Garry wrote:
>>>> On 23/03/2023 11:55, Wenchao Hao wrote:
>>>>> The original error injection mechanism was based on scsi_host which
>>>>> could not inject fault for a single SCSI device.
>>>>>
>>>>> This patchset provides the ability to inject errors for a single
>>>>> SCSI device. Now we supports inject timeout errors, queuecommand
>>>>> errors, and hostbyte, driverbyte, statusbyte, and sense data for
>>>>> specific SCSI Command.
>>>>
>>>> There is already a basic mechanism to generate errors - like timeouts - on "nth" command. Can you say why you want this new interface? What special scenarios are you trying to test/validate (which could not be achieved based on the current mechanism)?
>>>>
>>>
>>> I am testing a new error handle policy which is based on single scsi_device
>>> without set host to RECOVERY. So I need a method to generate errors for
>>> single SCSI devices.
>>>
>>> While we can not generate errors for single device with current mechanism
>>> because it is designed for host-wide error generation.
>>>
>>>> With this series we would have 2x methods to inject errors, which is less than ideal, and they seem to possibly conflict as well, e.g. I set timeout for nth command via current interface and then use the new interface to set timeout for some other cadence. What behavior to expect ...?
>>>
>>> I did not take this issue in consideration. I now assume the users would
>>> not use these 2 methods at same time.
>>>
>>> What's more, I don not know where to write the usage of this newly added
>>> interface, maybe we can explain these in doc?
>>
>> sysfs entries are described in Documentation/ABI, but please don't add elaborate programming interfaces in sysfs files (like in these patches) - a sysfs file should be just for reading or writing a single value
> 
> Hi,
> Maybe this link might help for scsi_debug documentation:
>      https://doug-gilbert.github.io/scsi_debug.html
> 
> And rather than sysfs for complicated, per (pseudo_ device
> settings, perhaps we could think about a SCSI mechanism like
> the "Unit Attention" mode page [0x0] which is vendor specific
> and used by Seagate and WDC for this sort of thing.
> A framework is already in the scsi_debug driver to change
> some mode page settings:
> 
> # sdparm /dev/sg0
>      /dev/sg0: Linux     scsi_debug        0191
> Read write error recovery mode page:
>    AWRE          1  [cha: n, def:  1]
>    ARRE          1  [cha: n, def:  1]
>    PER           0  [cha: n, def:  0]
> Caching (SBC) mode page:
>    WCE           1  [cha: y, def:  1]
>    RCD           0  [cha: n, def:  0]
> Control mode page:
>    SWP           0  [cha: n, def:  0]
> Informational exceptions control mode page:
>    EWASC         0  [cha: n, def:  0]
>    DEXCPT        1  [cha: n, def:  1]
>    MRIE          0  [cha: y, def:  0]
> 
> As can be seen WCE and MRIE are changeable, so
> 
> # sdparm --clear=WCE /dev/sg0
> # sdparm --get=WCE /dev/sg0
>      /dev/sg0: Linux     scsi_debug        0191
> WCE           0  [cha: y, def:  1]
> 
> 
> Doug Gilbert
> 
> 

Do you mean define scsi_debug's own format of mode page0(Vendor specific)
which contains these error injection info, and set/get these parameters
via sdparm?
If so, do we need to modify the sdparm code for these changes?

I want to add more injections in scsi_debug to test the SCSI middle layer,
for example, control return SUCCESS in scsi_debug_abort() or
scsi_debug_device_reset().

These injections are more oriented to developers to trigger and observe
the error handler of SCSI middle layer.

We can extend other error injections conveniently via my interface,
for example, add a new error code to add a new injection to control the
return value of scsi_debug_abort().

If it's not recommended to add this interface in sysfs, what about proc? Like
/proc/scsi/scsi, we can write "scsi remove-single-device h:c:t:l" to manage
device.

>>>> I'm not saying that I am a huge fan of the current inject mechanism, but at the very least you need to provide more justification for this series.
>>>>>>
>>>>> The first patch add an sysfs interface to add and inquiry single
>>>>> device's error injection info; the second patch defined how to remove
>>>>> an injection which has been added. The following 3 patches use the
>>>>> injection info and generate the related error type.
>>>>>
>>>>> Wenchao Hao (5):
>>>>>    scsi:scsi_debug: Add sysfs interface to manage scsi devices' error
>>>>>      injection
>>>>>    scsi:scsi_debug: Define grammar to remove added error injection
>>>>>    scsi:scsi_debug: timeout command if the error is injected
>>>>>    scsi:scsi_debug: Return failed value if the error is injected
>>>>>    scsi:scsi_debug: set command's result and sense data if the error is
>>>>>      injected
>>>>>
>>>>>   drivers/scsi/scsi_debug.c | 296 ++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 296 insertions(+)
>>>>>
>>>>
>>>>
>>>
>>
> 
> 

Re: [PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[Bart Van Assche]

On 3/23/23 20:42, haowenchao (C) wrote:
> If sysfs is not recommended, what about proc?

procfs is for process information and should not be used for any other 
purpose.

Thanks,

Bart.

Re: [PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[Douglas Gilbert]

On 2023-03-23 23:42, haowenchao (C) wrote:
> On 2023/3/24 1:24, Douglas Gilbert wrote:
>> On 2023-03-23 12:25, John Garry wrote:
>>> On 23/03/2023 13:13, haowenchao (C) wrote:
>>>> On 2023/3/23 20:40, John Garry wrote:
>>>>> On 23/03/2023 11:55, Wenchao Hao wrote:
>>>>>> The original error injection mechanism was based on scsi_host which
>>>>>> could not inject fault for a single SCSI device.
>>>>>>
>>>>>> This patchset provides the ability to inject errors for a single
>>>>>> SCSI device. Now we supports inject timeout errors, queuecommand
>>>>>> errors, and hostbyte, driverbyte, statusbyte, and sense data for
>>>>>> specific SCSI Command.
>>>>>
>>>>> There is already a basic mechanism to generate errors - like timeouts - on 
>>>>> "nth" command. Can you say why you want this new interface? What special 
>>>>> scenarios are you trying to test/validate (which could not be achieved 
>>>>> based on the current mechanism)?
>>>>>
>>>>
>>>> I am testing a new error handle policy which is based on single scsi_device
>>>> without set host to RECOVERY. So I need a method to generate errors for
>>>> single SCSI devices.
>>>>
>>>> While we can not generate errors for single device with current mechanism
>>>> because it is designed for host-wide error generation.
>>>>
>>>>> With this series we would have 2x methods to inject errors, which is less 
>>>>> than ideal, and they seem to possibly conflict as well, e.g. I set timeout 
>>>>> for nth command via current interface and then use the new interface to set 
>>>>> timeout for some other cadence. What behavior to expect ...?
>>>>
>>>> I did not take this issue in consideration. I now assume the users would
>>>> not use these 2 methods at same time.
>>>>
>>>> What's more, I don not know where to write the usage of this newly added
>>>> interface, maybe we can explain these in doc?
>>>
>>> sysfs entries are described in Documentation/ABI, but please don't add 
>>> elaborate programming interfaces in sysfs files (like in these patches) - a 
>>> sysfs file should be just for reading or writing a single value
>>
>> Hi,
>> Maybe this link might help for scsi_debug documentation:
>>      https://doug-gilbert.github.io/scsi_debug.html
>>
>> And rather than sysfs for complicated, per (pseudo_ device
>> settings, perhaps we could think about a SCSI mechanism like
>> the "Unit Attention" mode page [0x0] which is vendor specific
>> and used by Seagate and WDC for this sort of thing.
>> A framework is already in the scsi_debug driver to change
>> some mode page settings:
>>
>> # sdparm /dev/sg0
>>      /dev/sg0: Linux     scsi_debug        0191
>> Read write error recovery mode page:
>>    AWRE          1  [cha: n, def:  1]
>>    ARRE          1  [cha: n, def:  1]
>>    PER           0  [cha: n, def:  0]
>> Caching (SBC) mode page:
>>    WCE           1  [cha: y, def:  1]
>>    RCD           0  [cha: n, def:  0]
>> Control mode page:
>>    SWP           0  [cha: n, def:  0]
>> Informational exceptions control mode page:
>>    EWASC         0  [cha: n, def:  0]
>>    DEXCPT        1  [cha: n, def:  1]
>>    MRIE          0  [cha: y, def:  0]
>>
>> As can be seen WCE and MRIE are changeable, so
>>
>> # sdparm --clear=WCE /dev/sg0
>> # sdparm --get=WCE /dev/sg0
>>      /dev/sg0: Linux     scsi_debug        0191
>> WCE           0  [cha: y, def:  1]
>>
>>
>> Doug Gilbert
>>
>>
> 
> Do you mean define scsi_debug's own format of mode page0(Vendor specific)
> which contains these error injection info, and set/get these parameters
> via sdparm?
> If so, do we need to modify the sdparm code for these changes?

Not a problem.

> I want to add more injections in scsi_debug to test the SCSI middle layer,
> for example, control return SUCCESS in scsi_debug_abort() or
> scsi_debug_device_reset().
> 
> These injections are more oriented to developers to trigger and observe
> the error handler of SCSI middle layer.
> 
> We can extend other error injections conveniently via my interface,
> for example, add a new error code to add a new injection to control the
> return value of scsi_debug_abort().
> 
> If it's not recommended to add this interface in sysfs, what about proc? Like
> /proc/scsi/scsi, we can write "scsi remove-single-device h:c:t:l" to manage
> device.

In the past, procfs has been used for this sort of thing
but the powers that be want to phase that usage out.

debugfs, even though it is usually mounted under sysfs at
/sys/kernel/debug , does not seem to have the "one value
per variable" restriction. So debugfs or configfs
( /sys/kernel/config ) might be better candidates.
See 'df -ahT' .

>>>>> I'm not saying that I am a huge fan of the current inject mechanism, but at 
>>>>> the very least you need to provide more justification for this series.
>>>>>>>
>>>>>> The first patch add an sysfs interface to add and inquiry single
>>>>>> device's error injection info; the second patch defined how to remove
>>>>>> an injection which has been added. The following 3 patches use the
>>>>>> injection info and generate the related error type.
>>>>>>
>>>>>> Wenchao Hao (5):
>>>>>>    scsi:scsi_debug: Add sysfs interface to manage scsi devices' error
>>>>>>      injection
>>>>>>    scsi:scsi_debug: Define grammar to remove added error injection
>>>>>>    scsi:scsi_debug: timeout command if the error is injected
>>>>>>    scsi:scsi_debug: Return failed value if the error is injected
>>>>>>    scsi:scsi_debug: set command's result and sense data if the error is
>>>>>>      injected
>>>>>>
>>>>>>   drivers/scsi/scsi_debug.c | 296 ++++++++++++++++++++++++++++++++++++++
>>>>>>   1 file changed, 296 insertions(+)
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
> 

Re: [PATCH 0/5]scsi:scsi_debug: Add error injection for single device

[haowenchao (C)]

On 2023/3/25 1:31, Douglas Gilbert wrote:
> On 2023-03-23 23:42, haowenchao (C) wrote:
>> On 2023/3/24 1:24, Douglas Gilbert wrote:
>>> On 2023-03-23 12:25, John Garry wrote:
>>>> On 23/03/2023 13:13, haowenchao (C) wrote:
>>>>> On 2023/3/23 20:40, John Garry wrote:
>>>>>> On 23/03/2023 11:55, Wenchao Hao wrote:
>>>>>>> The original error injection mechanism was based on scsi_host which
>>>>>>> could not inject fault for a single SCSI device.
>>>>>>>
>>>>>>> This patchset provides the ability to inject errors for a single
>>>>>>> SCSI device. Now we supports inject timeout errors, queuecommand
>>>>>>> errors, and hostbyte, driverbyte, statusbyte, and sense data for
>>>>>>> specific SCSI Command.
>>>>>>
>>>>>> There is already a basic mechanism to generate errors - like timeouts - on "nth" command. Can you say why you want this new interface? What special scenarios are you trying to test/validate (which could not be achieved based on the current mechanism)?
>>>>>>
>>>>>
>>>>> I am testing a new error handle policy which is based on single scsi_device
>>>>> without set host to RECOVERY. So I need a method to generate errors for
>>>>> single SCSI devices.
>>>>>
>>>>> While we can not generate errors for single device with current mechanism
>>>>> because it is designed for host-wide error generation.
>>>>>
>>>>>> With this series we would have 2x methods to inject errors, which is less than ideal, and they seem to possibly conflict as well, e.g. I set timeout for nth command via current interface and then use the new interface to set timeout for some other cadence. What behavior to expect ...?
>>>>>
>>>>> I did not take this issue in consideration. I now assume the users would
>>>>> not use these 2 methods at same time.
>>>>>
>>>>> What's more, I don not know where to write the usage of this newly added
>>>>> interface, maybe we can explain these in doc?
>>>>
>>>> sysfs entries are described in Documentation/ABI, but please don't add elaborate programming interfaces in sysfs files (like in these patches) - a sysfs file should be just for reading or writing a single value
>>>
>>> Hi,
>>> Maybe this link might help for scsi_debug documentation:
>>>      https://doug-gilbert.github.io/scsi_debug.html
>>>
>>> And rather than sysfs for complicated, per (pseudo_ device
>>> settings, perhaps we could think about a SCSI mechanism like
>>> the "Unit Attention" mode page [0x0] which is vendor specific
>>> and used by Seagate and WDC for this sort of thing.
>>> A framework is already in the scsi_debug driver to change
>>> some mode page settings:
>>>
>>> # sdparm /dev/sg0
>>>      /dev/sg0: Linux     scsi_debug        0191
>>> Read write error recovery mode page:
>>>    AWRE          1  [cha: n, def:  1]
>>>    ARRE          1  [cha: n, def:  1]
>>>    PER           0  [cha: n, def:  0]
>>> Caching (SBC) mode page:
>>>    WCE           1  [cha: y, def:  1]
>>>    RCD           0  [cha: n, def:  0]
>>> Control mode page:
>>>    SWP           0  [cha: n, def:  0]
>>> Informational exceptions control mode page:
>>>    EWASC         0  [cha: n, def:  0]
>>>    DEXCPT        1  [cha: n, def:  1]
>>>    MRIE          0  [cha: y, def:  0]
>>>
>>> As can be seen WCE and MRIE are changeable, so
>>>
>>> # sdparm --clear=WCE /dev/sg0
>>> # sdparm --get=WCE /dev/sg0
>>>      /dev/sg0: Linux     scsi_debug        0191
>>> WCE           0  [cha: y, def:  1]
>>>
>>>
>>> Doug Gilbert
>>>
>>>
>>
>> Do you mean define scsi_debug's own format of mode page0(Vendor specific)
>> which contains these error injection info, and set/get these parameters
>> via sdparm?
>> If so, do we need to modify the sdparm code for these changes?
> 
> Not a problem.
> 
>> I want to add more injections in scsi_debug to test the SCSI middle layer,
>> for example, control return SUCCESS in scsi_debug_abort() or
>> scsi_debug_device_reset().
>>
>> These injections are more oriented to developers to trigger and observe
>> the error handler of SCSI middle layer.
>>
>> We can extend other error injections conveniently via my interface,
>> for example, add a new error code to add a new injection to control the
>> return value of scsi_debug_abort().
>>
>> If it's not recommended to add this interface in sysfs, what about proc? Like
>> /proc/scsi/scsi, we can write "scsi remove-single-device h:c:t:l" to manage
>> device.
> 
> In the past, procfs has been used for this sort of thing
> but the powers that be want to phase that usage out.
> 
> debugfs, even though it is usually mounted under sysfs at
> /sys/kernel/debug , does not seem to have the "one value
> per variable" restriction. So debugfs or configfs
> ( /sys/kernel/config ) might be better candidates.
> See 'df -ahT' .
> 

Define scsi_debug's own format of mode page0 seems too complex and we have to
change the sdparm code. In order to make a better scalability, I prefer to add
these interface via debugfs. Actually, I design the format of the "error interface"
described in patch1 by referring to interfaces of ftrace.

The new interfaces based on debugfs would look like following:
/sys/kernel/debug/scsi_debug
	|
	+-- 0:0:0:1
	|	|
	|	\-- error
	|
	\-- 0:0:0:2
		|
		\-- error

>>>>>> I'm not saying that I am a huge fan of the current inject mechanism, but at the very least you need to provide more justification for this series.
>>>>>>>>
>>>>>>> The first patch add an sysfs interface to add and inquiry single
>>>>>>> device's error injection info; the second patch defined how to remove
>>>>>>> an injection which has been added. The following 3 patches use the
>>>>>>> injection info and generate the related error type.
>>>>>>>
>>>>>>> Wenchao Hao (5):
>>>>>>>    scsi:scsi_debug: Add sysfs interface to manage scsi devices' error
>>>>>>>      injection
>>>>>>>    scsi:scsi_debug: Define grammar to remove added error injection
>>>>>>>    scsi:scsi_debug: timeout command if the error is injected
>>>>>>>    scsi:scsi_debug: Return failed value if the error is injected
>>>>>>>    scsi:scsi_debug: set command's result and sense data if the error is
>>>>>>>      injected
>>>>>>>
>>>>>>>   drivers/scsi/scsi_debug.c | 296 ++++++++++++++++++++++++++++++++++++++
>>>>>>>   1 file changed, 296 insertions(+)
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
> 
>