Re: [PATCH 2/2] scsi: scsi_debug: Support hostwide tags

[John Garry <john.garry@huawei.com>]

On 07/07/2020 17:28, Douglas Gilbert wrote:


>> +	if (sdebug_host_max_queue)
>> +		sd_dp->hc_idx = get_tag(cmnd);
>> +
>>    	if (ndelay > 0 && ndelay < INCLUSIVE_TIMING_MAX_NS)
>>    		ns_from_boot = ktime_get_boottime_ns();
>>    
>> @@ -5585,6 +5604,8 @@ module_param_named(lbpws10, sdebug_lbpws10, int, S_IRUGO);
>>    module_param_named(lowest_aligned, sdebug_lowest_aligned, int, S_IRUGO);
>>    module_param_named(max_luns, sdebug_max_luns, int, S_IRUGO | S_IWUSR);
>>    module_param_named(max_queue, sdebug_max_queue, int, S_IRUGO | S_IWUSR);
>> +module_param_named(host_max_queue, sdebug_host_max_queue, int,
>> +		   S_IRUGO | S_IWUSR);
> I can't see a "_store" method to match that S_IWUSR. If you don't want that
> parameter to be run-time changeable, then please drop the S_IWUSR.

Right, my intention was for RO, so I can drop S_IWUSR

> 
>>    module_param_named(medium_error_count, sdebug_medium_error_count, int,
>>    		   S_IRUGO | S_IWUSR);
>>    module_param_named(medium_error_start, sdebug_medium_error_start, int,
>> @@ -5654,6 +5675,7 @@ MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit (de
>>    MODULE_PARM_DESC(lowest_aligned, "lowest aligned lba (def=0)");
>>    MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
>>    MODULE_PARM_DESC(max_queue, "max number of queued commands (1 to max(def))");
>> +MODULE_PARM_DESC(host_max_queue, "max number of queued commands per host (0 to max(def))");
>>    MODULE_PARM_DESC(medium_error_count, "count of sectors to return follow on MEDIUM error");
>>    MODULE_PARM_DESC(medium_error_start, "starting sector number to return MEDIUM error");
>>    MODULE_PARM_DESC(ndelay, "response delay in nanoseconds (def=0 -> ignore)");
>> @@ -6141,7 +6163,8 @@ static ssize_t max_queue_store(struct device_driver *ddp, const char *buf,
>>    	struct sdebug_queue *sqp;
>>    

[...]

>>    
>> @@ -7272,9 +7317,13 @@ static int sdebug_driver_probe(struct device *dev)
>>    			my_name, submit_queues, nr_cpu_ids);
>>    		submit_queues = nr_cpu_ids;
>>    	}
>> -	/* Decide whether to tell scsi subsystem that we want mq */
>> -	/* Following should give the same answer for each host */
>> -	hpnt->nr_hw_queues = submit_queues;
>> +	/*
>> +	 * Decide whether to tell scsi subsystem that we want mq. The
>> +	 * following should give the same answer for each host. If the host
>> +	 * has a limit of hostwide max commands, then do not set.
>> +	 */
>> +	if (!sdebug_host_max_queue)
>> +		hpnt->nr_hw_queues = submit_queues;
>>    
>>    	sdbg_host->shost = hpnt;
>>    	*((struct sdebug_host_info **)hpnt->hostdata) = sdbg_host;
>>
> John,
> Looks good apart from the issue above. Another minor point: in a year's
> time (when this patch isn't (near) top of mind) then the output from
> 'modinfo scsi_debug' can be bewildering: so many parameter options, how
> to find the one(s) I need. That is why I try to put them in alphabetical
> order (namely the module_param_named() and MODULE_PARM_DESC() entries).
> Maybe the "DESC" could be expanded to hint at the relationship with
> max_queue.

ok, I'll fix that up as requested.

Thanks for checking,
John