scsi_debug: randomize command duration option + %p

Add an option to use the given command delay (in nanoseconds) as the upper limit for command durations. A pseudo random number generator chooses each duration from the range:

       [0..delay_in_ns)

Main benefit: allows testing with out-of-order responses.

Quoted 6 lines · Douglas Gilbert

Add an option to use the given command delay (in nanoseconds) as the upper limit for command durations. A pseudo random number generator chooses each duration from the range:
       [0..delay_in_ns)

Main benefit: allows testing with out-of-order responses.

Please clarify which code you want to test. I think out-of-order response handling in the SCSI core and block layer core is already being triggered by many storage workloads.

drivers/scsi/scsi_debug.c static int schedule_resp(struct scsi_cmnd *cmnd, struct · line 4360

Quoted 9 lines

drivers/scsi/scsi_debug.c +1 -3

@@ -4354,9 +4357,21 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct 
 sdebug_dev_info *devip,
           ktime_t kt;
           if (delta_jiff > 0) {
-            kt = ns_to_ktime((u64)delta_jiff * (NSEC_PER_SEC / HZ));
-        } else
-            kt = ndelay;
+            u64 ns = (u64)delta_jiff * (NSEC_PER_SEC / HZ);

Has it been considered to use jiffies_to_nsecs() instead of open-coding that function?

drivers/scsi/scsi_debug.c static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip, · line 4370

Quoted 9 lines

drivers/scsi/scsi_debug.c +9 -0

+            if (sdebug_random && ns < U32_MAX) {
+                ns = prandom_u32_max((u32)ns);
+            } else if (sdebug_random) {
+                ns >>= 10;    /* divide by 1024 */
+                if (ns < U32_MAX)  /* an hour and a bit */
+                    ns = prandom_u32_max((u32)ns);
+                ns <<= 10;
+            }
+            kt = ns_to_ktime(ns);

Is it really necessary to use nanosecond resolution? Can the above code be simplified by using microseconds as time unit instead of nanoseconds?

Review static int schedule_resp(struct scsi_cmnd *cmnd, struct · line 4371

Quoted 2 lines

(quoted diff) +1 -0

+MODULE_PARM_DESC(random, "1-> command duration chosen from [0..delay_in_ns) 
 (def=0)");

Would this description become more clear if it would be changed into something like the following: "If set, uniformly randomize command duration between 0 and delay_in_ns" ?

drivers/scsi/scsi_debug.c static ssize_t map_show(struct device_driver *ddp, char *buf) · line 5124

Quoted 4 lines

drivers/scsi/scsi_debug.c +4 -0

+static ssize_t random_show(struct device_driver *ddp, char *buf)
+{
+    return scnprintf(buf, PAGE_SIZE, "%d\n", sdebug_random ? 1 : 0);
+}

Since sdebug_random is either 0 or 1, is the "? 1 : 0" part necessary?

Quoted 5 lines

drivers/scsi/scsi_debug.c +5 -0

+static ssize_t random_store(struct device_driver *ddp, const char *buf,
+                size_t count)
+{
+    int n;
+

Quoted 6 lines

+    if (count > 0 && 1 == sscanf(buf, "%d", &n) && n >= 0) { +        sdebug_random = (n > 0);
+        return count;
+    }
+    return -EINVAL;
+}

Has this patch been verified with checkpatch? I'm asking since checkpatch should complain about "1 == sscanf(...)". See also commit c5595fa2f1ce ("checkpatch: add constant comparison on left side test").

drivers/scsi/scsi_debug.c static int __init scsi_debug_init(void) · line 5377

Quoted 7 lines

drivers/scsi/scsi_debug.c +1 -1

@@ -5338,7 +5373,7 @@ static int __init scsi_debug_init(void)
           dif_size = sdebug_store_sectors * sizeof(struct t10_pi_tuple);
           dif_storep = vmalloc(dif_size);
-        pr_err("dif_storep %u bytes @ %p\n", dif_size, dif_storep);
+        pr_err("dif_storep %u bytes @ %pK\n", dif_size, dif_storep);
           if (dif_storep == NULL) {
               pr_err("out of mem. (DIX)\n");

Is it useful to print the kernel pointer 'dif_storep'?

Quoted 1 line

Since sdebug_random is either 0 or 1, is the "? 1 : 0" part necessary?

No.
Why didn't you complain when MKP wrote that? That is where I cut and pasted that code from (sdebug_removable).

Again, copy and paste from MKP's code (which no doubt was a copy + paste from my earlier code).

Quoted 9 lines

drivers/scsi/scsi_debug.c static int __init scsi_debug_init(void) · line 5377

Quoted 7 lines

drivers/scsi/scsi_debug.c +1 -1

@@ -5338,7 +5373,7 @@ static int __init scsi_debug_init(void)
           dif_size = sdebug_store_sectors * sizeof(struct t10_pi_tuple);
           dif_storep = vmalloc(dif_size);
-        pr_err("dif_storep %u bytes @ %p\n", dif_size, dif_storep);
+        pr_err("dif_storep %u bytes @ %pK\n", dif_size, dif_storep);
           if (dif_storep == NULL) {
               pr_err("out of mem. (DIX)\n");

Is it useful to print the kernel pointer 'dif_storep'?

Ask MKP, it's his code. All I do know is that doing a printk("%p" ...) is useless in lk 5.3 (and probably lk 5.2). Taking the above snippet, if vmalloc() returned NULL then the existing pr_err() would print out a random number rather than <null>. Sick ...