Re: [PATCH RFC v7 10/12] megaraid_sas: switch fusion adapters to MQ

[Ming Lei <ming.lei@redhat.com>]

On Thu, Jul 30, 2020 at 12:01:22AM +0530, Kashyap Desai wrote:
> > > >
> > > > Another update is that V4 of 'scsi: core: only re-run queue in
> > > > scsi_end_request() if device queue is busy' is quite hard to
> > > > implement
> > > since
> > > > commit b4fd63f42647110c9 ("Revert "scsi: core: run queue if SCSI
> > > > device queue isn't ready and queue is idle").
> > >
> > > Ming -
> > >
> > > Update from my testing. I found only one case of IO stall. I can
> > > discuss this specific topic if you like to send separate patch. It is
> > > too much interleaved discussion in this thread.
> > >
> > > I noted you mentioned that V4 of 'scsi: core: only re-run queue in
> > > scsi_end_request() if device queue is busy' need underlying support of
> > > "scsi: core: run queue if SCSI device queue isn't ready and queue is
> idle"
> > > patch which is already reverted in mainline.
> >
> > Right.
> >
> > > Overall idea of running h/w queues conditionally in your patch " scsi:
> > > core: only re-run queue in scsi_end_request" is still worth. There can
> > > be
> >
> > I agree.
> >
> > > some race if we use this patch and for that you have concern. Am I
> > > correct. ?
> >
> > If the patch of "scsi: core: run queue if SCSI device queue isn't ready
> and queue
> > is idle" is re-added, the approach should work.
> I could not find issue in " scsi: core: only re-run queue in
> scsi_end_request" even though above mentioned patch is reverted.
> There may be some corner cases/race condition in submission path which can
> be fixed doing self-restart of h/w queue.

It is because two corner cases are handled in other ways:

    Now that we have the patches ("blk-mq: In blk_mq_dispatch_rq_list()
    "no budget" is a reason to kick") and ("blk-mq: Rerun dispatching in
    the case of budget contention") we should no longer need the fix in
    the SCSI code.  Revert it, resolving conflicts with other patches that
    have touched this code.

> 
> > However, it looks a bit
> > complicated, and I was thinking if one simpler approach can be figured
> out.
> 
> I was thinking your original approach is simple, but if you think some
> other simple approach I can test as part of these series.
> BTW, I am still not getting why you think your original approach is not
> good design.

It is still not straightforward enough or simple enough for proving its
correctness, even though the implementation isn't complicated.

> 
> >
> > >
> > > One of the race I found in my testing is fixed by below patch -
> > >
> > > diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c index
> > > 54f9015..bcfd33a 100644
> > > --- a/block/blk-mq-sched.c
> > > +++ b/block/blk-mq-sched.c
> > > @@ -173,8 +173,10 @@ static int blk_mq_do_dispatch_ctx(struct
> > > blk_mq_hw_ctx *hctx)
> > >                 if (!sbitmap_any_bit_set(&hctx->ctx_map))
> > >                         break;
> > >
> > > -               if (!blk_mq_get_dispatch_budget(hctx))
> > > +               if (!blk_mq_get_dispatch_budget(hctx)) {
> > > +                       blk_mq_delay_run_hw_queue(hctx,
> > > BLK_MQ_BUDGET_DELAY);
> > >                         break;
> > > +               }
> >
> > Actually all hw queues need to be run, instead of this hctx, cause the
> budget
> > stuff is request queue wide.
> 
> 
> OK. But I thought all the hctx will see issue independently, if they are
> active and they will restart its own hctx queue.
> BTW, do you think above handling in block layer code make sense
> irrespective of current h/w queue restart logic OR it is just relative
> stuffs ?

You are right, it is correct to just run this hctx.

> 
> >
> > >
> > >                 rq = blk_mq_dequeue_from_ctx(hctx, ctx);
> > >                 if (!rq) {
> > >
> > >
> > > In my test setup, I have your V3 'scsi: core: only re-run queue in
> > > scsi_end_request() if device queue is busy' rebased on 5.8 which does
> > > not have
> > > "scsi: core: run queue if SCSI device queue isn't ready and queue is
> idle"
> > > since it is already reverted in mainline.
> >
> > If you added the above patch, I believe you can remove the run queue in
> > scsi_end_request() unconditionally. However, the delay run queue may
> > degrade io performance.
> 
> I understood.  But that performance issue is due to budget contention and
> may impact some old HBA(less queue depth) or emulation HBA.

Your patch for delay running hw queue causes delay once one request
is completed, and the queue should have been run immediately after one
request is finished.

> That is why I thought your patch of conditional h/w run from completion
> would improve performance.

Yeah, we all think that way is correct thing to do, and now the problem
is how to run hw queue just in case of budget contention.


thanks, 
Ming