Re: [PATCH v3 0/4] io_uring iopoll in scsi layer

[Douglas Gilbert <dgilbert@interlog.com>]

On 2021-02-02 5:32 a.m., Kashyap Desai wrote:
>> Hi,
>> I don't understand how this patchset works. My testing shows scsi_debug is
>> broken and I will be sending a correcting patch shortly (similar to the
>> one I
>> sent you on 20210108).
> 
> Hi Doug -
> 
> scsi_debug patch from this series works on my setup. I was under impression
> that you want this patch to be available in tree and on top of current
> patchset, you want to have further incremental update.
> What do you suggest ? Do you want me to wait for your updated patch OR We
> can ask Martin to pick all the patches except scsi_debug ? You can post
> scsi_debug changes as another series or separate patch.

Kashyap,
I did post a fixing patch titled: "[PATCH] scsi_debug: add new defer_type
for mq_poll" and cc-ed it to you.

At first I also thought your scsi_debug patch worked but on closer
examination it was falling through to the existing
delay_expired-event_generated model. It was only when I checked for
requests with REQ_HIPRI set and in those cases turned off the event
generation, that I saw it was not working.

Another way to tell if iopoll/blk_poll is really working is to turn off
the code that drives blk_poll(). What should happen in that case (due to
the mid layer) is that the command timeout (after 60 seconds?) should
cancel the command. If the command still manages to succeed normally
then iopoll is not really working.

> I have few more megaraid_sas patches in pipeline, so I am looking for this
> series to be available as baseline.

And on my side the most recent "[PATCH v15 00/45] sg: add v4 interface"
patchset [20210225] included "[PATCH v15 44/45] sg: add blk_poll support".
Jens accepted a patch to the fio sg engine to use the "hipri=1" option
to drive that new capability. This is an addition to the fio script
example that you sent using io_uring:

[global]
ioengine=io_uring
hipri=1
direct=1
runtime=3s
rw=randread
norandommap
bs=512

[seqprecon]
filename=/dev/sdb


Doug Gilbert


>> The scsi_debug driver is a simplified LLD that needs to know in advance
>> whether a request/command issued to it will be using the .mq_poll
>> callback.
>> Perhaps you have found another way but one simple way to find that out is
>> this test:
>>      if (request->cmd_flags & REQ_HIPRI)
>>
> 
> Agree. I am not very much familiar with scsi_debug code so used current code
> change as starting point and from there things can be improved.
> 
>> In the case of scsi_debug (after my patch) the delay associated with the
>> command is not wired up to generate an event which leads to completion.
>> Instead, callbacks through .mq_poll are expected and they will check if
>> that
>> delay has expired, if not the callback returns 0. When the delay has
>> expired
>> and a .mq_poll is received then completion occurs.
>>
>> Doug Gilbert
>>
>>> v3 ->
>>> - added reviewed-by tag
>>> - Fix comment provided by Hannes for below patch.
>>> https://patchwork.kernel.org/project/linux-scsi/patch/20201203034100.2
>>> 9716-3-kashyap.desai@broadcom.com/
>>> - Fix Functional issue of poll_queues settings not working in v2.
>>>
>>> v2 ->
>>> - updated feedback from v1.
>>> - added reviewed-by & tested-by tag
>>> - remove flood of prints in scsi_debug driver during iopoll
>>>     reported by Douglas Gilbert.
>>> - added new patch to support to get shost from hctx.
>>>     added new helper function "scsi_init_hctx"
>>>
>>> v1 ->
>>> Fixed warnings in scsi_debug driver.
>>> Reported-by: kernel test robot <lkp@intel.com>
>>>
>>> Kashyap Desai (4):
>>>     add io_uring with IOPOLL support in scsi layer
>>>     megaraid_sas: iouring iopoll support
>>>     scsi_debug : iouring iopoll support
>>>     scsi: set shost as hctx driver_data
>>>
>>>    drivers/scsi/megaraid/megaraid_sas.h        |   3 +
>>>    drivers/scsi/megaraid/megaraid_sas_base.c   |  87 +++++++++++--
>>>    drivers/scsi/megaraid/megaraid_sas_fusion.c |  42 ++++++-
>>>    drivers/scsi/megaraid/megaraid_sas_fusion.h |   2 +
>>>    drivers/scsi/scsi_debug.c                   | 130 ++++++++++++++++++++
>>>    drivers/scsi/scsi_lib.c                     |  29 ++++-
>>>    include/scsi/scsi_cmnd.h                    |   1 +
>>>    include/scsi/scsi_host.h                    |  11 ++
>>>    8 files changed, 291 insertions(+), 14 deletions(-)
>>>
>>>
>>> base-commit: a927ec3995427e9c47752900ad2df0755d02aba5
>>>