Re: REQ_HIPRI and SCSI mid-level

On Tue, Jun 01, 2021 at 02:19:10PM +0200, Hannes Reinecke wrote:

On 5/28/21 3:32 AM, Ming Lei wrote:

quoted

On Thu, May 27, 2021 at 05:43:07PM +0200, Hannes Reinecke wrote:

quoted

On 5/25/21 6:03 PM, Douglas Gilbert wrote:

quoted

On 2021-05-21 5:56 p.m., Douglas Gilbert wrote:

quoted

The REQ_HIPRI flag on requests is associated with blk_poll() (aka iopoll)
and assumes the user space (or some higher level) will be calling
blk_poll() on requests marked with REQ_HIPRI and that will lead to their
completion.

In lk 5.13-rc1 the megaraid and scsi_debug LLDs support blk_poll() [seen
by searching for 'mq_poll'] with more to follow, I assume. I have tested
blk_poll() on the scsi_debug driver using both fio and the new sg driver.
It works well with one caveat: as long as there isn't an error.
After fighting with that error processing from the ULD side (i.e. the
new sg driver) and the LLD side I am concluding that the glue that
holds them together, that is, the mid-level is not as REQ_HIPRI aware
as it should be.

Yes REQ_HIPRI is there in scsi_lib.c but it is missing from scsi_error.c
How can scsi_error.c re-issue requests _without_ taking into account
that the original was issued with REQ_HIPRI ? Well I don't know but I'm
pretty sure that is close to the area that I see causing problems
(mainly lockups).

As an example the scsi_debug driver has an in-use bitmap that when a new
request arrives the code looks for an empty slot. Due to (incorrect)
parameter setup that may fail. If the driver returns:
     device_qfull_result = (DID_OK << 16) | SAM_STAT_TASK_SET_FULL;
then I see lock-ups if the request in question has REQ_HIPRI set.

If that is changed to:
     device_qfull_result = (DID_ABORT << 16) | SAM_STAT_TASK_SET_FULL;
then my user space test program sees that error and aborts showing the
TASK SET FULL SCSI status. That is much better than a lockup ...

That's because with the first result the command is requeued (due to
DID_OK), whereas in the latter result the command is aborted (due to
DID_ABORT).

So the question really is whether we should retry the commands which have
REQ_HIPRI set, or whether we shouldn't rather complete them with appropriate
error code.
A bit like enhanced BLOCK_PC requests, if you will.

quoted

quoted

Having played around with variants of the above for a few weeks, I'd
like to throw this problem into the open :-)


Suggestion: perhaps the eh could give up immediately on any request
with REQ_HIPRI set (i.e. make it a higher level layer's problem).

Like I said above: it's not only scsi EH which would need to be modified,
but possibly also the result evaluation in scsi_decide_disposition(); it's
questionable whether a HIPRI command should be requeued at all.

Why can't HIPRI req be requeued?

Oh, it can.
As I said: it's questionable; HIPRI / polled completions are just that,
polling for completions. And a completion indicating a requeue is
_still_ a completion.
So one could argue that we should return here (as it's a completion, and
we're polling for completion).

No, blk_poll() actually poll for bio's completion instead of request's
completion. If the submitted HIPRI bio isn't completed, the polling won't be
stopped.

quoted

quoted

But this might even affect the NVMe folks; they do return commands with
BLK_STS_RESOURCE, too.

Block layer will be responsible for re-queueing BLK_STS_RESOURCE requests,
so still not understand why it is one issue for HIPRI req. Also
rq->mq_hctx won't be changed since its allocation, blk_poll()
will keep polling on the correct hw queue for reaping the IO.

As mentioned above, I was talking about completions indicating a requeue.
Requeues due to resource shortage on the initiator side would of course
be requeued.

blk_poll() doesn't care request requeue, what matters is that if the HIPRI
bio is completed or not.

So either timeout or EH codes needn't to handle HIPRI IO specially.

Thanks,
Ming