Re: [bug report] shared tags causes IO hang and performance drop

[Douglas Gilbert <dgilbert@interlog.com>]

On 2021-04-14 2:19 p.m., John Garry wrote:
> On 14/04/2021 18:03, Douglas Gilbert wrote:
>> On 2021-04-14 9:59 a.m., Kashyap Desai wrote:
>>>>> I tried both - 5.12.0-rc1 and 5.11.0-rc2+ and there is a same
>>> behavior.
>>>>> Let me also check  megaraid_sas and see if anything generic or this is
>>>>> a special case of scsi_debug.
>>>>
>>>> As I mentioned, it could be one generic issue wrt. SCHED_RESTART.
>>>> shared tags might have to restart all hctx since all share same tags.
>>>
>>> Ming - I tried many combination on MR shared host tag driver but there is
>>> no single instance of IO hang.
>>> I will keep trying, but when I look at scsi_debug driver code I found
>>> below odd settings in scsi_debug driver.
>>> can_queue of adapter is set to 128 but queue_depth of sdev is set to 255.
>>>
>>> If I apply below patch, scsi_debug driver's hang is also resolved. Ideally
>>> sdev->queue depth cannot exceed shost->can_queue.
>>> Not sure why cmd_per_lun is 255 in scsi_debug driver which can easily
>>> exceed can_queue.  I will simulate something similar in MR driver and see
>>> how it behaves w.r.t IO hang issue.
>>>
>>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>>> index 70165be10f00..dded762540ee 100644
>>> --- a/drivers/scsi/scsi_debug.c
>>> +++ b/drivers/scsi/scsi_debug.c
>>> @@ -218,7 +218,7 @@ static const char *sdebug_version_date = "20200710";
>>>    */
>>>   #define SDEBUG_CANQUEUE_WORDS  3       /* a WORD is bits in a long */
>>>   #define SDEBUG_CANQUEUE  (SDEBUG_CANQUEUE_WORDS * BITS_PER_LONG)
>>
>> So SDEBUG_CANQUEUE is 3*64 = 192 and is a hard limit (it is used to
>> dimension an array). Should it be upped to 4, say? [That will slow things
>> down a bit if that is an issue.]
> 
> sdev_store_queue_depth() enforces that the sdev queue depth cannot exceed 
> can_queue.

That is only invoked by the user doing:
     echo [qdepth] > /sys/class/scsi_device/[hctl]/device/queue_depth

For scsi_debug that sysfs store leads to sdebug_change_qdepth() being
invoked. That function currently contains this hack:

         /* allow to exceed max host qc_arr elements for testing */
         if (qdepth > SDEBUG_CANQUEUE + 10)
                 qdepth = SDEBUG_CANQUEUE + 10;
         scsi_change_queue_depth(sdev, qdepth);

Maybe that hack should be replaced with:
	if (qdepth <= SDEBUG_CANQUEUE)
		scsi_change_queue_depth(sdev, qdepth);
	else
		<print warning>
	return sdev->queue_depth;

That hack has been useful in my testing (e.g. currently I'm tracking down
an oops caused by it in my sg driver rewrite) but it may confuse others.
There are more clear cut ways to make the scsi_debug driver inject errors.

Doug Gilbert

> I don't know why this is not also enforced in scsi_alloc_sdev(), or even when 
> registering the shost (for cmd_per_lun)
> 
>>
>>> -#define DEF_CMD_PER_LUN  255
>>> +#define DEF_CMD_PER_LUN  SDEBUG_CANQUEUE
>>>
>>>   /* UA - Unit Attention; SA - Service Action; SSU - Start Stop Unit */
>>>   #define F_D_IN                 1       /* Data-in command (e.g. READ) */
>>> @@ -7558,6 +7558,7 @@ static int sdebug_driver_probe(struct device *dev)
>>>          sdbg_host = to_sdebug_host(dev);
>>>
>>>          sdebug_driver_template.can_queue = sdebug_max_queue;
>>> +       sdebug_driver_template.cmd_per_lun = sdebug_max_queue;
>>
>> I'll  push out a patch shortly.
>>
>> Doug Gilbert
>>
>>
>>>          if (!sdebug_clustering)
>>>                  sdebug_driver_template.dma_boundary = PAGE_SIZE - 1;
>>>>
>>>>
>>>> Thanks,
>>>> Ming
>>
>> .
>