Re: [PATCH v5 10/10] scsi: scsi_debug: Add param to control sdev's allow_restart

[Douglas Gilbert]

On 2023-09-22 05:29, Wenchao Hao wrote:

Add new module param "allow_restart" to control if setup
scsi_device's allow_restart flag. This is used to test scsi
command finished with sense_key 0x6, asc 0x4 and ascq 0x2

Signed-off-by: Wenchao Hao 


Hi,
Looked at this and verified that the allow_restart flag of scsi_debug
devices (disks ?) is usually 0 and when the scsi_debug module is
started with allow_restart=1 then the allow_restart flag does indeed
change to 1. For example:
   # cat /sys/class/scsi_disk/1\:0\:0\:0/allow_restart
   1

That ASC/ASCQ code means: "Logical unit not ready, initializing command
required" according to my library. Played around with sg_start but didn't
see any change in how it reacts. According to scsi_device.h that flag's
description is: "issue START_UNIT in error handler" which implies it
changes how the EH handler reacts.

Perhaps the 3 line patch description could say a little more about how
to use this new parameter...

Tested-by: Douglas Gilbert 






--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/d61e88d3-e1b7-44e0-ba9b-f633be0b5b30%40interlog.com.

Re: [PATCH v5 09/10] scsi: scsi_debug: Add debugfs interface to fail target reset

[Douglas Gilbert]

On 2023-09-22 05:29, Wenchao Hao wrote:

The interface is found at
/sys/kernel/debug/scsi_debug/target/fail_reset where 
identifies the target to inject errors on. It's a simple bool type
interface which would make this target's reset fail if set to 'Y'.

Signed-off-by: Wenchao Hao 
Reported-by: kernel test robot 


Tested by setting 'echo 1 > /sys/bus/pseudo/drivers/scsi_debug/opts'
and observing 'tail -f /var/log/syslog'. Looks good including that
fail_reset is readable so its current state can be checked.

Tested-by: Douglas Gilbert 




--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/a517343d-cd37-4faa-8c26-c4e0c121%40interlog.com.

Re: [PATCH v5 08/10] scsi: scsi_debug: Add new error injection reset lun failed

[Douglas Gilbert]

On 2023-09-22 05:29, Wenchao Hao wrote:

Add error injection type 3 to make scsi_debug_device_reset() return FAILED.
Fail abort command foramt:


s/foramt/format/



   ++--+---+
   | Column | Type | Description   |
   ++--+---+
   |   1|  u8  | Error type, fixed to 0x4  |
   ++--+---+
   |   2|  s32 | Error count   |
   ||  |  0: this rule will be ignored |
   ||  |  positive: the rule will always take effect   |
   ||  |  negative: the rule takes effect n times where -n is  |
   ||  |the value given. Ignored after n times |
   ++--+---+
   |   3|  x8  | SCSI command opcode, 0xff for all commands|
   ++--+---+

Examples:
 error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
 echo "0 -10 0x12" > ${error}


These examples are misleading. Same with the one in patch 7/10 . The example
should be showing an invocation that exercises _this_ patch. So the first
byte of the echo should be 4 not the 0 shown above.

Doug Gilbert


will make the device return FAILED when try to reset lun with inquiry
command 10 times.
 error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
 echo "0 -10 0xff" > ${error}
will make the device return FAILED when try to reset lun 10 times.

Usually we do not care about what command it is when trying to perform
reset LUN, so 0xff could be applied.

Signed-off-by: Wenchao Hao 
---
  drivers/scsi/scsi_debug.c | 39 +++
  1 file changed, 39 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 8a16cb9642a6..db8ab6cad078 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -295,6 +295,8 @@ enum sdebug_err_type {
/* with errors set in scsi_cmnd */
ERR_ABORT_CMD_FAILED= 3,/* control return FAILED from */
/* scsi_debug_abort() */
+   ERR_LUN_RESET_FAILED= 4,/* control return FAILED from */
+   /* 
scsi_debug_device_reseLUN_RESET_FAILEDt() */
  };
  
  struct sdebug_err_inject {

@@ -973,6 +975,7 @@ static int sdebug_error_show(struct seq_file *m, void *p)
switch (err->type) {
case ERR_TMOUT_CMD:
case ERR_ABORT_CMD_FAILED:
+   case ERR_LUN_RESET_FAILED:
seq_printf(m, "%d\t%d\t0x%x\n", err->type, err->cnt,
err->cmd);
break;
@@ -1035,6 +1038,7 @@ static ssize_t sdebug_error_write(struct file *file, 
const char __user *ubuf,
switch (inject_type) {
case ERR_TMOUT_CMD:
case ERR_ABORT_CMD_FAILED:
+   case ERR_LUN_RESET_FAILED:
if (sscanf(buf, "%d %d %hhx", >type, >cnt,
   >cmd) != 3)
goto out_error;
@@ -5578,10 +5582,40 @@ static void scsi_debug_stop_all_queued(struct 
scsi_device *sdp)
scsi_debug_stop_all_queued_iter, sdp);
  }
  
+static int sdebug_fail_lun_reset(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;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(err, >inject_err_list, list) {
+   if (err->type == ERR_LUN_RESET_FAILED &&
+   (err->cmd == cmd[0] || err->cmd == 0xff)) {
+   ret = !!err->cnt;
+   if (err->cnt < 0)
+   err->cnt++;
+
+   rcu_read_unlock();
+   return ret;
+   }
+   }
+   rcu_read_unlock();
+
+   return 0;
+}
+
  static int scsi_debug_device_reset(struct scsi_cmnd *SCpnt)
  {
struct scsi_device *sdp = SCpnt->device;
struct sdebug_dev_info *devip = sdp->hostdata;
+   u8 *cmd = SCpnt->cmnd;
+   u8 opcode = cmd[0];
  
  	++num_dev_resets;
  
@@ -5592,6 +5626,11 @@ static int scsi_debug_device_reset(struct scsi_cmnd *SCpnt)

if (devip)
set_bit(SDEBUG_UA_POR, devip->uas_bm);
  
+	if (sdebug_fail_lun_reset(SCpnt)) {

+   scmd_printk(KERN_INFO, SCpnt, "fail lun reset 0x%x\n", opcode);
+   return FAILED;
+   }
+
return SUCCESS;
  }
  


--
You received 

Re: [PATCH v5 07/10] scsi: scsi_debug: Add new error injection abort failed

[Douglas Gilbert]

On 2023-09-22 05:29, Wenchao Hao wrote:

Add error injection type 3 to make scsi_debug_abort() return FAILED.
Fail abort command foramt:


s/foramt/format/



   ++--+---+
   | Column | Type | Description   |
   ++--+---+
   |   1|  u8  | Error type, fixed to 0x3  |
   ++--+---+
   |   2|  s32 | Error count   |
   ||  |  0: this rule will be ignored |
   ||  |  positive: the rule will always take effect   |
   ||  |  negative: the rule takes effect n times where -n is  |
   ||  |the value given. Ignored after n times |
   ++--+---+
   |   3|  x8  | SCSI command opcode, 0xff for all commands|
   ++--+---+

Examples:
 error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
 echo "0 -10 0x12" > ${error}
will make the device return FAILED when abort inquiry command 10 times.


Tested with:
   # sg_raw -t 10 -r 1k /dev/sg1 12 00 00 00 60 00

After 10 seconds (the timeout specified to sg_raw) I saw this:
   >>> transport error: Host_status=0x03 [DID_TIME_OUT]

And the
# cat /sys/kernel/debug/scsi_debug/1\:0\:0\:0/error

Count value changed from -10 up to 0 for each invocation of the INQUIRY
command. Thereafter the INQUIRY command worked.

Looks good.

  Tested-by: Douglas Gilbert 



Signed-off-by: Wenchao Hao 
---
  drivers/scsi/scsi_debug.c | 40 +++
  1 file changed, 40 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index fe1f7421f617..8a16cb9642a6 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -293,6 +293,8 @@ enum sdebug_err_type {
ERR_FAIL_CMD= 2,/* make specific scsi command's */
/* queuecmd return succeed but */
/* with errors set in scsi_cmnd */
+   ERR_ABORT_CMD_FAILED= 3,/* control return FAILED from */
+   /* scsi_debug_abort() */
  };
  
  struct sdebug_err_inject {

@@ -970,6 +972,7 @@ static int sdebug_error_show(struct seq_file *m, void *p)
list_for_each_entry_rcu(err, >inject_err_list, list) {
switch (err->type) {
case ERR_TMOUT_CMD:
+   case ERR_ABORT_CMD_FAILED:
seq_printf(m, "%d\t%d\t0x%x\n", err->type, err->cnt,
err->cmd);
break;
@@ -1031,6 +1034,7 @@ static ssize_t sdebug_error_write(struct file *file, 
const char __user *ubuf,
  
  	switch (inject_type) {

case ERR_TMOUT_CMD:
+   case ERR_ABORT_CMD_FAILED:
if (sscanf(buf, "%d %d %hhx", >type, >cnt,
   >cmd) != 3)
goto out_error;
@@ -5504,9 +5508,39 @@ static void stop_all_queued(void)
mutex_unlock(_host_list_mutex);
  }
  
+static int sdebug_fail_abort(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;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(err, >inject_err_list, list) {
+   if (err->type == ERR_ABORT_CMD_FAILED &&
+   (err->cmd == cmd[0] || err->cmd == 0xff)) {
+   ret = !!err->cnt;
+   if (err->cnt < 0)
+   err->cnt++;
+
+   rcu_read_unlock();
+   return ret;
+   }
+   }
+   rcu_read_unlock();
+
+   return 0;
+}
+
  static int scsi_debug_abort(struct scsi_cmnd *SCpnt)
  {
bool ok = scsi_debug_abort_cmnd(SCpnt);
+   u8 *cmd = SCpnt->cmnd;
+   u8 opcode = cmd[0];
  
  	++num_aborts;
  
@@ -5515,6 +5549,12 @@ static int scsi_debug_abort(struct scsi_cmnd *SCpnt)

"%s: command%s found\n", __func__,
ok ? "" : " not");
  
+	if (sdebug_fail_abort(SCpnt)) {

+   scmd_printk(KERN_INFO, SCpnt, "fail abort command 0x%x\n",
+   opcode);
+   return FAILED;
+   }
+
return SUCCESS;
  }
  


--
You received this message because you are subscr

Re: [PATCH v5 01/10] scsi: scsi_debug: create scsi_debug directory in the debugfs filesystem

[Douglas Gilbert]

On 2023-09-22 05:28, Wenchao Hao wrote:

Create directory scsi_debug in the root of the debugfs filesystem.
Prepare to add interface for manage error injection.

Acked-by: Douglas Gilbert 
Signed-off-by: Wenchao Hao 
---
  drivers/scsi/scsi_debug.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 9c0af50501f9..35c336271b13 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -41,6 +41,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include 
  
@@ -862,6 +863,8 @@ static const int device_qfull_result =
  
  static const int condition_met_result = SAM_STAT_CONDITION_MET;
  
+static struct dentry *sdebug_debugfs_root;

+
  
  /* Only do the extra work involved in logical block provisioning if one or

   * more of the lbpu, lbpws or lbpws10 parameters are given and we are doing
@@ -7011,6 +7014,8 @@ static int __init scsi_debug_init(void)
goto driver_unreg;
}
  
+	sdebug_debugfs_root = debugfs_create_dir("scsi_debug", NULL);


debugfs_create_dir() can fail and return NULL. Looking at other drivers, most
seem to assume it will work. Since the scsi_debug driver is often used to test
abnormal situations, perhaps adding something like:
if (!sdebug_debugfs_root)
pr_info("%s: failed to create initial debugfs directory\n", __func__);

might save someone a bit of time if a NULL dereference on sdebug_debugfs_root
follows later. That is what the mpt3sas driver does.

Doug Gilbert


+
for (k = 0; k < hosts_to_add; k++) {
if (want_store && k == 0) {
ret = sdebug_add_host_helper(idx);
@@ -7057,6 +7062,7 @@ static void __exit scsi_debug_exit(void)
  
  	sdebug_erase_all_stores(false);

xa_destroy(per_store_ap);
+   debugfs_remove(sdebug_debugfs_root);
  }
  
  device_initcall(scsi_debug_init);


--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/8c7cfe09-d145-4387-91cf-da9d4e2398e1%40interlog.com.

Re: [PATCH 21/24] sg: switch to SPDX tags

[Douglas Gilbert]

On 2019-05-01 6:14 p.m., Christoph Hellwig wrote:

Use the the GPLv2+ SPDX tag instead of verbose boilerplate text.

Signed-off-by: Christoph Hellwig 


This scripts/checkpatch.pl noise seems to be related to the patch below:

$ scripts/checkpatch.pl /tmp/t.patch
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in 
from ply import lex, yacc
ImportError: No module named ply
total: 0 errors, 0 warnings, 98 lines checked

That happens both in the mkp/scsi/5.2/scsi-queue and the latest linux-stable
trees.


BTW Are C++ comments (as used in this patch) now permitted in lk code?

Doug Gilbert


---
  drivers/scsi/sg.c | 7 +--
  1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d3f15319b9b3..bcdc28e5ede7 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0+
  /*
   *  History:
   *  Started: Aug 9 by Lawrence Foard (entr...@world.std.com),
@@ -8,12 +9,6 @@
   *Copyright (C) 1992 Lawrence Foard
   * Version 2 and 3 extensions to driver:
   *Copyright (C) 1998 - 2014 Douglas Gilbert
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2, or (at your option)
- * any later version.
- *
   */
  
  static int sg_version_num = 30536;	/* 2 digits for each component */




--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 21/24] sg: switch to SPDX tags

[Douglas Gilbert]

On 2019-05-01 6:14 p.m., Christoph Hellwig wrote:

Use the the GPLv2+ SPDX tag instead of verbose boilerplate text.


IOWs replace 3.5 lines with 1.



Signed-off-by: Christoph Hellwig 


Acked-by: Douglas Gilbert 


---
  drivers/scsi/sg.c | 7 +--
  1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d3f15319b9b3..bcdc28e5ede7 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0+
  /*
   *  History:
   *  Started: Aug 9 by Lawrence Foard (entr...@world.std.com),
@@ -8,12 +9,6 @@
   *Copyright (C) 1992 Lawrence Foard
   * Version 2 and 3 extensions to driver:
   *Copyright (C) 1998 - 2014 Douglas Gilbert
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2, or (at your option)
- * any later version.
- *
   */
  
  static int sg_version_num = 30536;	/* 2 digits for each component */




--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [bug report][scsi blk_mq] data corruption due to a bug in SCSI LLD error handling

[Douglas Gilbert]

On 2019-04-03 4:18 p.m., Jaesoo Lee wrote:

Hello All,

I encountered this bug while trying to enable dm_blk_mq for our
iSCSI/FCP targets.

The bug is that the sg_io issued to scsi_blk_mq would succeed even if
LLD wants to error out those requests.

Let me explain the scenario in more details.

Setup:
0. Host kernel configuration
- 4.19.9, 4.20.16
- boot parameter: dm_mod.use_blk_mq=Y scsi_mod.use_blk_mq=Y
scsi_transport_iscsi.debug_session=1 scsi_transport_iscsi.debug_conn=1

Scenario:
1. Connect the host to iSCSI target via four paths
: A dm device is created for those target devices
2. Start an application in the host which generates sg_io ioctl for
XCOPY and WSAME to the dm device with the ratio of around 50%
(pread/pwrite for the rest).
3. Perform system crash (sysrq-trigger) in the iSCSI target

Expected result:
- Any outstanding IOs should get failed with errors

Actual results:
- Normal read/write IOs get failed as expected
- SG_IO ioctls SUCCEEDED!!


Not all ioctl(SG_IO)s are created equal!

If you are using the sg v3 interface (i.e. struct sg_io_hdr) then I would
expect DRIVER_TIMEOUT in sg_io_obj.driver_status or DID_TIME_OUT in
sg_io_obj.host_status to be set on completion. [BTW You will _not_ see
a ETIMEDOUT errno; only errors prior to submission yield errno style
errors.]

If you don't see that with ioctl(SG_IO) on a block device then try again on
a sg device. If neither report that then the mid-level error processing
is broken.

Doug Gilbert



- log message:
[Tue Apr  2 11:26:34 2019]  session3: session recovery timed out after 11 secs
[Tue Apr  2 11:26:34 2019]  session3: session_recovery_timedout:
Unblocking SCSI target
..
[Tue Apr  2 11:26:34 2019] sd 8:0:0:8: scsi_prep_state_check:
rejecting I/O to offline device
[Tue Apr  2 11:26:34 2019] sd 8:0:0:8: scsi_prep_state_check:
rejecting I/O to offline device
[Tue Apr  2 11:26:34 2019] sd 8:0:0:8: scsi_prep_state_check:
rejecting I/O to offline device
[Tue Apr  2 11:26:34 2019] print_req_error: I/O error, dev sdi, sector 30677580
[Tue Apr  2 11:26:34 2019] device-mapper: multipath: Failing path 8:128.
[Tue Apr  2 11:26:34 2019] SG_IO disk=sdi, result=0x0

- This causes the DATA corruption for the application

Relavant call stacks: (SG_IO issue path)
[Tue Apr  2 11:26:33 2019] sd 8:0:0:8: [sdi] sd_ioctl: disk=sdi, cmd=0x2285
[Tue Apr  2 11:26:33 2019] SG_IO disk=sdi, retried 1 cmd 93
[Tue Apr  2 11:26:33 2019] CPU: 30 PID: 16080 Comm: iostress Not
tainted 4.19.9-purekernel_dbg.x86_64+ #30
[Tue Apr  2 11:26:33 2019] Hardware name:  /0JP31P, BIOS 2.0.19 08/29/2013
[Tue Apr  2 11:26:33 2019] Call Trace:
[Tue Apr  2 11:26:33 2019]  dump_stack+0x63/0x85
[Tue Apr  2 11:26:33 2019]  sg_io+0x41e/0x480
[Tue Apr  2 11:26:33 2019]  scsi_cmd_ioctl+0x297/0x420
[Tue Apr  2 11:26:33 2019]  ? sdev_prefix_printk+0xe9/0x120
[Tue Apr  2 11:26:33 2019]  ? _cond_resched+0x1a/0x50
[Tue Apr  2 11:26:33 2019]  scsi_cmd_blk_ioctl+0x51/0x70
[Tue Apr  2 11:26:33 2019]  sd_ioctl+0x95/0x1a0 [sd_mod]
[Tue Apr  2 11:26:33 2019]  __blkdev_driver_ioctl+0x25/0x30
[Tue Apr  2 11:26:33 2019]  dm_blk_ioctl+0x79/0x100 [dm_mod]
[Tue Apr  2 11:26:33 2019]  blkdev_ioctl+0x89a/0x940
[Tue Apr  2 11:26:33 2019]  ? do_nanosleep+0xae/0x180
[Tue Apr  2 11:26:33 2019]  block_ioctl+0x3d/0x50
[Tue Apr  2 11:26:33 2019]  do_vfs_ioctl+0xa6/0x600
[Tue Apr  2 11:26:33 2019]  ? __audit_syscall_entry+0xe9/0x130
[Tue Apr  2 11:26:33 2019]  ksys_ioctl+0x6d/0x80
).[Tue Apr  2 11:26:33 2019]  __x64_sys_ioctl+0x1a/0x20
[Tue Apr  2 11:26:33 2019]  do_syscall_64+0x60/0x180
[Tue Apr  2 11:26:33 2019]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

According to the analysis, it seems that there is a bug in propagating
errors for iSCSI session timeout (i.e., session_recovery_timedout).
Compared to this, legacy SCSI queue handles the errors in
scsi_request_fn by killing the request as can be seen below.

1872 static void scsi_request_fn(struct request_queue *q)
1873 __releases(q->queue_lock)
1874 __acquires(q->queue_lock)
1875 {
..
1886 for (;;) {
..
1893 req = blk_peek_request(q);
1894 if (!req)
1895 break;
1896
1897 if (unlikely(!scsi_device_online(sdev))) {
1898 sdev_printk(KERN_ERR, sdev,
1899 "scsi_request_fn: rejecting
I/O to offline device\n");
1900 scsi_kill_request(req, q);
1901 continue;
1902 }
1903

I am not sure in which layer we have to fix this, LLD or scsi_queue_rq().

Could someone please take a look? Or, could someone guide me on how to
fix this bug?

Thanks,

Jaesoo Lee.



--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group 

Re: [PATCH] iscsi: Capture iscsi debug messages using tracepoints

[Douglas Gilbert]

On 2018-12-20 4:45 p.m., Fred Herard wrote:

ping(Doug);

Thanks,
Fred

On 12/9/2018 5:01 PM, Fred Herard wrote:

Gentle reminder...

Reviewed-by: Rajan Shanmugavelu 
Reviewed-by: Lee Duncan 

  Reviewed-by: Douglas Gilbert 

Thanks.

Doug Gilbert


On 12/2/2018 1:47 AM, Fred Herard wrote:

Hi Doug,

Here's an updated patch with removal of the misplaced semicolons:

 From da1f3c2ff78881c439d53820f3b82d4c54ac5cf9 Mon Sep 17 00:00:00 2001
From: Fred Herard 
Date: Fri, 12 Oct 2018 15:45:47 -0700
Subject: [PATCH] iscsi: Capture iscsi debug messages using tracepoints

This commit enhances iscsi initiator modules to capture iscsi debug messages
using linux kernel tracepoint facility:

https://www.kernel.org/doc/Documentation/trace/tracepoints.txt

The following tracepoint events have been created under the iscsi tracepoint
event group:

iscsi_dbg_conn - to capture connection debug messages (libiscsi module)
iscsi_dbg_session - to capture session debug messages (libiscsi module)
iscsi_dbg_eh - to capture error handling debug messages (libiscsi module)
iscsi_dbg_tcp - to capture iscsi tcp debug messages (libiscsi_tcp module)
iscsi_dbg_sw_tcp - to capture iscsi sw tcp debug messages (iscsi_tcp module)
iscsi_dbg_trans_session - to cpature iscsi trasnsport sess debug messages
(scsi_transport_iscsi module)
iscsi_dbg_trans_conn - to capture iscsi tansport conn debug messages
(scsi_transport_iscsi module)

Signed-off-by: Fred Herard 
Reviewed-by: Rajan Shanmugavelu 
---
  drivers/scsi/iscsi_tcp.c|   6 +-
  drivers/scsi/libiscsi.c |  16 +-
  drivers/scsi/libiscsi_tcp.c |   6 +-
  drivers/scsi/scsi_transport_iscsi.c |  38 -
  include/trace/events/iscsi.h| 107 
  5 files changed, 165 insertions(+), 8 deletions(-)
  create mode 100644 include/trace/events/iscsi.h

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index e11eff6..33bfb0a 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -44,6 +44,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "iscsi_tcp.h"
  
@@ -72,7 +73,10 @@

iscsi_conn_printk(KERN_INFO, _conn, \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
-   } while (0);
+   iscsi_dbg_trace(trace_iscsi_dbg_sw_tcp, \
+   &(_conn)->cls_conn->dev,  \
+   "%s " dbg_fmt, __func__, ##arg);\
+   } while (0)
  
  
  /**

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index cf8a15e..3a7e3e7 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -40,6 +40,7 @@
  #include 
  #include 
  #include 
+#include 
  
  static int iscsi_dbg_lib_conn;

  module_param_named(debug_libiscsi_conn, iscsi_dbg_lib_conn, int,
@@ -68,7 +69,10 @@
iscsi_conn_printk(KERN_INFO, _conn, \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
-   } while (0);
+   iscsi_dbg_trace(trace_iscsi_dbg_conn,   \
+   &(_conn)->cls_conn->dev,  \
+   "%s " dbg_fmt, __func__, ##arg);\
+   } while (0)
  
  #define ISCSI_DBG_SESSION(_session, dbg_fmt, arg...)			\

do {\
@@ -76,7 +80,10 @@
iscsi_session_printk(KERN_INFO, _session,   \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
-   } while (0);
+   iscsi_dbg_trace(trace_iscsi_dbg_session,\
+   &(_session)->cls_session->dev,\
+   "%s " dbg_fmt, __func__, ##arg);  \
+   } while (0)
  
  #define ISCSI_DBG_EH(_session, dbg_fmt, arg...)\

do {\
@@ -84,7 +91,10 @@
iscsi_session_printk(KERN_INFO, _session,   \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
-   } while (0);
+   iscsi_dbg_trace(trace_iscsi_dbg_eh, \
+   &(_session)->cls_session->dev,\
+   "%s " dbg_fmt, __func__, ##arg);  \
+   } while (0)
  
  inline void iscsi_conn_queue_work(struct iscsi_conn *conn)

  {
diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index 63a1d69..75426b1 100644
--- a/drivers/scsi/libiscsi_t

Re: [PATCH] iscsi: Capture iscsi debug messages using tracepoints

[Douglas Gilbert]

Hi,
Misplaced semi-colons below.

On 2018-11-26 8:01 p.m., Lee Duncan wrote:

On 11/21/18 9:04 AM, fred.her...@oracle.com wrote:

From: Fred Herard 

This commit enhances iscsi initiator modules to capture iscsi debug messages
using linux kernel tracepoint facility:

https://www.kernel.org/doc/Documentation/trace/tracepoints.txt

The following tracepoint events have been created under the iscsi tracepoint
event group:

iscsi_dbg_conn - to capture connection debug messages (libiscsi module)
iscsi_dbg_session - to capture session debug messages (libiscsi module)
iscsi_dbg_eh - to capture error handling debug messages (libiscsi module)
iscsi_dbg_tcp - to capture iscsi tcp debug messages (libiscsi_tcp module)
iscsi_dbg_sw_tcp - to capture iscsi sw tcp debug messages (iscsi_tcp module)
iscsi_dbg_trans_session - to cpature iscsi trasnsport sess debug messages
(scsi_transport_iscsi module)
iscsi_dbg_trans_conn - to capture iscsi tansport conn debug messages
(scsi_transport_iscsi module)

Signed-off-by: Fred Herard 
Reviewed-by: Rajan Shanmugavelu 
---
  drivers/scsi/iscsi_tcp.c|   4 ++
  drivers/scsi/libiscsi.c |  10 +++
  drivers/scsi/libiscsi_tcp.c |   4 ++
  drivers/scsi/scsi_transport_iscsi.c |  34 -
  include/trace/events/iscsi.h| 107 
  5 files changed, 158 insertions(+), 1 deletion(-)
  create mode 100644 include/trace/events/iscsi.h

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index e11eff6b0e97..cf6475af72b1 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -44,6 +44,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "iscsi_tcp.h"
  
@@ -72,6 +73,9 @@ MODULE_PARM_DESC(debug_iscsi_tcp, "Turn on debugging for iscsi_tcp module "

iscsi_conn_printk(KERN_INFO, _conn, \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
+   iscsi_dbg_trace(trace_iscsi_dbg_sw_tcp, \
+   &(_conn)->cls_conn->dev,  \
+   "%s " dbg_fmt, __func__, ##arg);\
} while (0);


I was just looking through the code (to possibly copy it) and noticed
that semi-colon after "while (0)". I'm pretty sure that it shouldn't
be there.

include/linux$ grep "while (0)" *.h | wc
3912361   18267
include/linux$ grep "while (0);" *.h | wc
  6  29 161

Dito every other macro definition.

Doug Gilbert


diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index cf8a15e54d83..088e90308c1f 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -40,6 +40,7 @@
  #include 
  #include 
  #include 
+#include 
  
  static int iscsi_dbg_lib_conn;

  module_param_named(debug_libiscsi_conn, iscsi_dbg_lib_conn, int,
@@ -68,6 +69,9 @@ MODULE_PARM_DESC(debug_libiscsi_eh,
iscsi_conn_printk(KERN_INFO, _conn, \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
+   iscsi_dbg_trace(trace_iscsi_dbg_conn,   \
+   &(_conn)->cls_conn->dev,  \
+   "%s " dbg_fmt, __func__, ##arg);\
} while (0);
  
  #define ISCSI_DBG_SESSION(_session, dbg_fmt, arg...)			\

@@ -76,6 +80,9 @@ MODULE_PARM_DESC(debug_libiscsi_eh,
iscsi_session_printk(KERN_INFO, _session,   \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
+   iscsi_dbg_trace(trace_iscsi_dbg_session,\
+   &(_session)->cls_session->dev,\
+   "%s " dbg_fmt, __func__, ##arg);  \
} while (0);
  
  #define ISCSI_DBG_EH(_session, dbg_fmt, arg...)\

@@ -84,6 +91,9 @@ MODULE_PARM_DESC(debug_libiscsi_eh,
iscsi_session_printk(KERN_INFO, _session,   \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
+   iscsi_dbg_trace(trace_iscsi_dbg_eh, \
+   &(_session)->cls_session->dev,\
+   "%s " dbg_fmt, __func__, ##arg);  \
} while (0);
  
  inline void iscsi_conn_queue_work(struct iscsi_conn *conn)

diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index 63a1d69ff515..d641a72ae033 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -43,6 +43,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "iscsi_tcp.h"
  
@@ -65,6 +66,9 @@ MODULE_PARM_DESC(debug_libiscsi_tcp, "Turn on debugging for libiscsi_tcp "

Re: [PATCH] iscsi: Capture iscsi debug messages using tracepoints

[Douglas Gilbert]

Lee,
See about 3/4 of the way down your patch.

Summary: how about generalizing the message logging to tracefs so the
whole SCSI subsystem can use it?

Doug Gilbert


On 2018-11-26 8:01 p.m., Lee Duncan wrote:

On 11/21/18 9:04 AM, fred.her...@oracle.com wrote:

From: Fred Herard 

This commit enhances iscsi initiator modules to capture iscsi debug messages
using linux kernel tracepoint facility:

https://www.kernel.org/doc/Documentation/trace/tracepoints.txt

The following tracepoint events have been created under the iscsi tracepoint
event group:

iscsi_dbg_conn - to capture connection debug messages (libiscsi module)
iscsi_dbg_session - to capture session debug messages (libiscsi module)
iscsi_dbg_eh - to capture error handling debug messages (libiscsi module)
iscsi_dbg_tcp - to capture iscsi tcp debug messages (libiscsi_tcp module)
iscsi_dbg_sw_tcp - to capture iscsi sw tcp debug messages (iscsi_tcp module)
iscsi_dbg_trans_session - to cpature iscsi trasnsport sess debug messages
(scsi_transport_iscsi module)
iscsi_dbg_trans_conn - to capture iscsi tansport conn debug messages
(scsi_transport_iscsi module)

Signed-off-by: Fred Herard 
Reviewed-by: Rajan Shanmugavelu 
---
  drivers/scsi/iscsi_tcp.c|   4 ++
  drivers/scsi/libiscsi.c |  10 +++
  drivers/scsi/libiscsi_tcp.c |   4 ++
  drivers/scsi/scsi_transport_iscsi.c |  34 -
  include/trace/events/iscsi.h| 107 
  5 files changed, 158 insertions(+), 1 deletion(-)
  create mode 100644 include/trace/events/iscsi.h

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index e11eff6b0e97..cf6475af72b1 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -44,6 +44,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "iscsi_tcp.h"
  
@@ -72,6 +73,9 @@ MODULE_PARM_DESC(debug_iscsi_tcp, "Turn on debugging for iscsi_tcp module "

iscsi_conn_printk(KERN_INFO, _conn, \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
+   iscsi_dbg_trace(trace_iscsi_dbg_sw_tcp, \
+   &(_conn)->cls_conn->dev,  \
+   "%s " dbg_fmt, __func__, ##arg);\
} while (0);
  
  
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c

index cf8a15e54d83..088e90308c1f 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -40,6 +40,7 @@
  #include 
  #include 
  #include 
+#include 
  
  static int iscsi_dbg_lib_conn;

  module_param_named(debug_libiscsi_conn, iscsi_dbg_lib_conn, int,
@@ -68,6 +69,9 @@ MODULE_PARM_DESC(debug_libiscsi_eh,
iscsi_conn_printk(KERN_INFO, _conn, \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
+   iscsi_dbg_trace(trace_iscsi_dbg_conn,   \
+   &(_conn)->cls_conn->dev,  \
+   "%s " dbg_fmt, __func__, ##arg);\
} while (0);
  
  #define ISCSI_DBG_SESSION(_session, dbg_fmt, arg...)			\

@@ -76,6 +80,9 @@ MODULE_PARM_DESC(debug_libiscsi_eh,
iscsi_session_printk(KERN_INFO, _session,   \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
+   iscsi_dbg_trace(trace_iscsi_dbg_session,\
+   &(_session)->cls_session->dev,\
+   "%s " dbg_fmt, __func__, ##arg);  \
} while (0);
  
  #define ISCSI_DBG_EH(_session, dbg_fmt, arg...)\

@@ -84,6 +91,9 @@ MODULE_PARM_DESC(debug_libiscsi_eh,
iscsi_session_printk(KERN_INFO, _session,   \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
+   iscsi_dbg_trace(trace_iscsi_dbg_eh, \
+   &(_session)->cls_session->dev,\
+   "%s " dbg_fmt, __func__, ##arg);  \
} while (0);
  
  inline void iscsi_conn_queue_work(struct iscsi_conn *conn)

diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index 63a1d69ff515..d641a72ae033 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -43,6 +43,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include "iscsi_tcp.h"
  
@@ -65,6 +66,9 @@ MODULE_PARM_DESC(debug_libiscsi_tcp, "Turn on debugging for libiscsi_tcp "

iscsi_conn_printk(KERN_INFO, _conn, \
 "%s " dbg_fmt,   \
 __func__, ##arg);  \
+   

Re: [PATCH] SCSI: LIBSCSI: Fixed codeing style errors

[Douglas Gilbert]

On 16-04-20 03:51 AM, Bob Stlt wrote:

Fixed codeing style formatting errors.

Signed-off-by: Bob Stlt 
---
  drivers/scsi/libiscsi.c | 90 -
  1 file changed, 45 insertions(+), 45 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 6bffd91..41be9d3 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -197,7 +197,7 @@ static int iscsi_prep_ecdb_ahs(struct iscsi_task *task)
pad_len = iscsi_padding(rlen);

rc = iscsi_add_hdr(task, sizeof(ecdb_ahdr->ahslength) +
-  sizeof(ecdb_ahdr->ahstype) + ahslength + pad_len);
+   sizeof(ecdb_ahdr->ahstype) + ahslength + pad_len);


Seriously? checkpatch.pl in the current linux-stable.git is
allowing me to add a minimum number of spaces immediately
before a name so it is aligned to beginning of the first
argument of a function. IMO that is more readable that what is
proposed above.


if (rc)
return rc;

@@ -210,10 +210,10 @@ static int iscsi_prep_ecdb_ahs(struct iscsi_task *task)
memcpy(ecdb_ahdr->ecdb, cmd->cmnd + ISCSI_CDB_SIZE, rlen);

ISCSI_DBG_SESSION(task->conn->session,
- "iscsi_prep_ecdb_ahs: varlen_cdb_len %d "
- "rlen %d pad_len %d ahs_length %d iscsi_headers_size "
- "%u\n", cmd->cmd_len, rlen, pad_len, ahslength,
- task->hdr_len);
+   "iscsi_prep_ecdb_ahs: varlen_cdb_len %d "
+   "rlen %d pad_len %d ahs_length %d iscsi_headers_size "
+   "%u\n", cmd->cmd_len, rlen, pad_len, ahslength,
+   task->hdr_len);
return 0;


When I try something like the above, checkpatch.pl complains
about string concatenation across lines. When I put the whole
string on one line, it complains that the line is longer than 80
characters. That leads me to some string obfuscation which is
much less readable but gets the okay from checkpatch.pl :-)

Doug Gilbert


  }

@@ -236,10 +236,10 @@ static int iscsi_prep_bidi_ahs(struct iscsi_task *task)
rlen_ahdr->read_length = cpu_to_be32(scsi_in(sc)->length);

ISCSI_DBG_SESSION(task->conn->session,
- "bidi-in rlen_ahdr->read_length(%d) "
- "rlen_ahdr->ahslength(%d)\n",
- be32_to_cpu(rlen_ahdr->read_length),
- be16_to_cpu(rlen_ahdr->ahslength));
+   "bidi-in rlen_ahdr->read_length(%d) "
+   "rlen_ahdr->ahslength(%d)\n",
+   be32_to_cpu(rlen_ahdr->read_length),
+   be16_to_cpu(rlen_ahdr->ahslength));
return 0;
  }

@@ -500,7 +500,7 @@ static void iscsi_free_task(struct iscsi_task *task)
if (conn->login_task == task)
return;

-   kfifo_in(>cmdpool.queue, (void*), sizeof(void*));
+   kfifo_in(>cmdpool.queue, (void *), sizeof(void *));

if (sc) {
/* SCSI eh reuses commands to verify us */
@@ -736,7 +736,7 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct 
iscsi_hdr *hdr,
BUG_ON(conn->c_stage == ISCSI_CONN_STOPPED);

if (!kfifo_out(>cmdpool.queue,
-(void*), sizeof(void*)))
+(void *), sizeof(void *)))
return NULL;
}
/*
@@ -831,7 +831,7 @@ static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, 
struct iscsi_hdr *hdr,
struct iscsi_session *session = conn->session;
struct scsi_cmnd *sc = task->sc;

-   iscsi_update_cmdsn(session, (struct iscsi_nopin*)rhdr);
+   iscsi_update_cmdsn(session, (struct iscsi_nopin *)rhdr);
conn->exp_statsn = be32_to_cpu(rhdr->statsn) + 1;

sc->result = (DID_OK << 16) | rhdr->cmd_status;
@@ -901,12 +901,12 @@ invalid_datalen:
}

if (rhdr->flags & (ISCSI_FLAG_CMD_UNDERFLOW |
-  ISCSI_FLAG_CMD_OVERFLOW)) {
+   ISCSI_FLAG_CMD_OVERFLOW)) {
int res_count = be32_to_cpu(rhdr->residual_count);

if (res_count > 0 &&
(rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW ||
-res_count <= scsi_bufflen(sc)))
+   res_count <= scsi_bufflen(sc)))
/* write side for bidi or uni-io set_resid */
scsi_set_resid(sc, res_count);
else
@@ -939,7 +939,7 @@ iscsi_data_in_rsp(struct iscsi_conn *conn, struct iscsi_hdr 
*hdr,
sc->result = (DID_OK << 16) | rhdr->cmd_status;
conn->exp_statsn = be32_to_cpu(rhdr->statsn) + 1;
if (rhdr->flags & (ISCSI_FLAG_DATA_UNDERFLOW |
-  ISCSI_FLAG_DATA_OVERFLOW)) {
+   ISCSI_FLAG_DATA_OVERFLOW)) {
int res_count =