Re: [PATCH v1 3/3] scsi_debug: iouring iopoll support

[Douglas Gilbert <dgilbert@interlog.com>]

On 2020-11-30 4:06 a.m., Kashyap Desai wrote:
>>
>> On 2020-10-15 9:37 a.m., Kashyap Desai wrote:
>>> Add support of iouring iopoll interface in scsi_debug.
>>> This feature requires shared hosttag support in kernel and driver.
>>
>> I am continuing to test this patch. There is one fix shown inline below
>> plus a
>> question near the end.
> 
> Hi Doug,  I have created add-on patch which includes all your comment. I am
> also able to see the issue you reported and below patch fix it.
> I will hold V2 revision of the series and I will wait for your Review-by and
> Tested-by Tag.
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 4d9cc6af588c..fb328253086d 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -5675,6 +5675,7 @@ MODULE_PARM_DESC(opt_xferlen_exp, "optimal transfer
> length granularity exponent
>   MODULE_PARM_DESC(opts, "1->noise, 2->medium_err, 4->timeout,
> 8->recovered_err... (def=0)");
>   MODULE_PARM_DESC(per_host_store, "If set, next positive add_host will get
> new store (def=0)");
>   MODULE_PARM_DESC(physblk_exp, "physical block exponent (def=0)");
> +MODULE_PARM_DESC(poll_queues, "support for iouring iopoll queues (1 to
> max(submit_queues - 1)");
>   MODULE_PARM_DESC(ptype, "SCSI peripheral type(def=0[disk])");
>   MODULE_PARM_DESC(random, "If set, uniformly randomize command duration
> between 0 and delay_in_ns");
>   MODULE_PARM_DESC(removable, "claim to have removable media (def=0)");
> @@ -5683,7 +5684,6 @@ MODULE_PARM_DESC(sector_size, "logical block size in
> bytes (def=512)");
>   MODULE_PARM_DESC(statistics, "collect statistics on commands, queues
> (def=0)");
>   MODULE_PARM_DESC(strict, "stricter checks: reserved field in cdb (def=0)");
>   MODULE_PARM_DESC(submit_queues, "support for block multi-queue (def=1)");
> -MODULE_PARM_DESC(poll_queues, "support for iouring iopoll queues");
>   MODULE_PARM_DESC(tur_ms_to_ready, "TEST UNIT READY millisecs before initial
> good status (def=0)");
>   MODULE_PARM_DESC(unmap_alignment, "lowest aligned thin provisioning lba
> (def=0)");
>   MODULE_PARM_DESC(unmap_granularity, "thin provisioning granularity in
> blocks (def=1)");
> @@ -7199,7 +7199,7 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost,
> unsigned int queue_num)
>          do {
>                  spin_lock_irqsave(&sqp->qc_lock, iflags);
>                  qc_idx = find_first_bit(sqp->in_use_bm, sdebug_max_queue);
> -               if (unlikely((qc_idx < 0) || (qc_idx >= SDEBUG_CANQUEUE)))
> +               if (unlikely((qc_idx < 0) || (qc_idx >= sdebug_max_queue)))
>                          goto out;
> 
>                  sqcp = &sqp->qc_arr[qc_idx];
> @@ -7477,10 +7477,17 @@ static int sdebug_driver_probe(struct device *dev)
>                  hpnt->host_tagset = 1;
> 
>          /* poll queues are possible for nr_hw_queues > 1 */
> -       if (hpnt->nr_hw_queues == 1)
> +       if (hpnt->nr_hw_queues == 1 || (poll_queues < 1)) {
> +               pr_warn("%s: trim poll_queues to 0. poll_q/nr_hw = (%d/%d)
> \n",
> +                        my_name, poll_queues, hpnt->nr_hw_queues);
>                  poll_queues = 0;
> +       }
> 
> -       /* poll queues  */
> +       /*
> +        * Poll queues don't need interrupts, but we need at least one I/O
> queue
> +        * left over for non-polled I/O.
> +        * If condition not met, trim poll_queues to 1 (just for
> simplicity).
> +        */
>          if (poll_queues >= submit_queues) {
>                  pr_warn("%s: trim poll_queues to 1\n", my_name);
>                  poll_queues = 1;
> 
>

Kashyap,
I struggled with this patch, first the line wrap, then the last two
patch segments not applying. Could you send me the scsi_debug.c file
attached to an email?

>>> +	do {
>>> +		spin_lock_irqsave(&sqp->qc_lock, iflags);
>>> +		qc_idx = find_first_bit(sqp->in_use_bm, sdebug_max_queue);
>>> +		if (unlikely((qc_idx < 0) || (qc_idx >= SDEBUG_CANQUEUE)))
>>
>> The above line IMO needs to be:
>> 		if (unlikely((qc_idx < 0) || (qc_idx >= sdebug_max_queue)))
>>
>> If not, when sdebug_max_queue < SDEBUG_CANQUEUE and there is no
>> request waiting then "scp is NULL, ..." is reported suggesting there is an
>> error.
> 
> BTW -  Is below piece of code at sdebug_q_cmd_complete() requires similar
> change ?
> Use sdebug_max_queue instead of SDEBUG_CANQUEUE
>          if (unlikely((qc_idx < 0) || (qc_idx >= SDEBUG_CANQUEUE))) {
>                  pr_err("wild qc_idx=%d\n", qc_idx);
>                  return;
>          }

Yes, I need to look at this. sdebug_max_queue is initialized to
SDEBUG_CANQUEUE but then can be overridden by the invocation line parameters.
Several arrays in structures are sized by SDEBUG_CANQUEUE which will
remain. But most SDEBUG_CANQUEUE uses inside driver functions can probably
be replaced by sdebug_max_queue when I confirm that it is safe. Since
sdebug_max_queue <= SDEBUG_CANQUEUE and the fields in between should
always be zero, the current situation just leads to wasted cycles.

Doug Gilbert