Re: [PATCH v2] scsi_debug: randomize command duration option + %p

[Bart Van Assche <bvanassche@acm.org>]

On 9/27/19 7:04 AM, Douglas Gilbert wrote:
> 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.

> @@ -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?

> +			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?

> +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" ?

> +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?

> +static ssize_t random_store(struct device_driver *ddp, const char *buf,
> +			    size_t count)
> +{
> +	int n;
> +
> +	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").

> @@ -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'?

Thanks,

Bart.