Re: [PATCH 2/2] scsi: core: Fix stall if two threads request budget at the same time

[Doug Anderson]

Hi,

On Mon, Mar 30, 2020 at 6:41 PM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
>
> On Mon, Mar 30, 2020 at 07:49:06AM -0700, Douglas Anderson wrote:
> > It is possible for two threads to be running
> > blk_mq_do_dispatch_sched() at the same time with the same "hctx".
> > This is because there can be more than one caller to
> > __blk_mq_run_hw_queue() with the same "hctx" and hctx_lock() doesn't
> > prevent more than one thread from entering.
> >
> > If more than one thread is running blk_mq_do_dispatch_sched() at the
> > same time with the same "hctx", they may have contention acquiring
> > budget.  The blk_mq_get_dispatch_budget() can eventually translate
> > into scsi_mq_get_budget().  If the device's "queue_depth" is 1 (not
> > uncommon) then only one of the two threads will be the one to
> > increment "device_busy" to 1 and get the budget.
> >
> > The losing thread will break out of blk_mq_do_dispatch_sched() and
> > will stop dispatching requests.  The assumption is that when more
> > budget is available later (when existing transactions finish) the
> > queue will be kicked again, perhaps in scsi_end_request().
> >
> > The winning thread now has budget and can go on to call
> > dispatch_request().  If dispatch_request() returns NULL here then we
> > have a potential problem.  Specifically we'll now call
>
> I guess this problem should be BFQ specific. Now there is definitely
> requests in BFQ queue wrt. this hctx. However, looks this request is
> only available from another loser thread, and it won't be retrieved in
> the winning thread via e->type->ops.dispatch_request().
>
> Just wondering why BFQ is implemented in this way?

Paolo can maybe comment why.

...but even if BFQ wanted to try to change this, I think it's
impossible to fully close the race.  There is no locking between the
call to has_work() and dispatch_request() and there can be two (or
more) threads running the code at the same time.  Without some type of
locking I think it will always be possible for dispatch_request() to
return NULL.  Are we OK with code that works most of the time but
still has a race?  ...or did I misunderstand how this all works?


> > blk_mq_put_dispatch_budget() which translates into
> > scsi_mq_put_budget().  That will mark the device as no longer busy but
> > doesn't do anything to kick the queue.  This violates the assumption
> > that the queue would be kicked when more budget was available.
> >
> > Pictorially:
> >
> > Thread A                          Thread B
> > ================================= ==================================
> > blk_mq_get_dispatch_budget() => 1
> > dispatch_request() => NULL
> >                                   blk_mq_get_dispatch_budget() => 0
> >                                   // because Thread A marked
> >                                   // "device_busy" in scsi_device
> > blk_mq_put_dispatch_budget()
> >
> > The above case was observed in reboot tests and caused a task to hang
> > forever waiting for IO to complete.  Traces showed that in fact two
> > tasks were running blk_mq_do_dispatch_sched() at the same time with
> > the same "hctx".  The task that got the budget did in fact see
> > dispatch_request() return NULL.  Both tasks returned and the system
> > went on for several minutes (until the hung task delay kicked in)
> > without the given "hctx" showing up again in traces.
> >
> > Let's attempt to fix this problem by detecting budget contention.  If
> > we're in the SCSI code's put_budget() function and we saw that someone
> > else might have wanted the budget we got then we'll kick the queue.
> >
> > The mechanism of kicking due to budget contention has the potential to
> > overcompensate and kick the queue more than strictly necessary, but it
> > shouldn't hurt.
> >
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > ---
> >
> >  drivers/scsi/scsi_lib.c    | 27 ++++++++++++++++++++++++---
> >  drivers/scsi/scsi_scan.c   |  1 +
> >  include/scsi/scsi_device.h |  2 ++
> >  3 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 610ee41fa54c..0530da909995 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -344,6 +344,21 @@ static void scsi_dec_host_busy(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
> >       rcu_read_unlock();
> >  }
> >
> > +static void scsi_device_dec_busy(struct scsi_device *sdev)
> > +{
> > +     bool was_contention;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&sdev->budget_lock, flags);
> > +     atomic_dec(&sdev->device_busy);
> > +     was_contention = sdev->budget_contention;
> > +     sdev->budget_contention = false;
> > +     spin_unlock_irqrestore(&sdev->budget_lock, flags);
> > +
> > +     if (was_contention)
> > +             blk_mq_run_hw_queues(sdev->request_queue, true);
> > +}
> > +
> >  void scsi_device_unbusy(struct scsi_device *sdev, struct scsi_cmnd *cmd)
> >  {
> >       struct Scsi_Host *shost = sdev->host;
> > @@ -354,7 +369,7 @@ void scsi_device_unbusy(struct scsi_device *sdev, struct scsi_cmnd *cmd)
> >       if (starget->can_queue > 0)
> >               atomic_dec(&starget->target_busy);
> >
> > -     atomic_dec(&sdev->device_busy);
> > +     scsi_device_dec_busy(sdev);
> >  }
> >
> >  static void scsi_kick_queue(struct request_queue *q)
> > @@ -1624,16 +1639,22 @@ static void scsi_mq_put_budget(struct blk_mq_hw_ctx *hctx)
> >       struct request_queue *q = hctx->queue;
> >       struct scsi_device *sdev = q->queuedata;
> >
> > -     atomic_dec(&sdev->device_busy);
> > +     scsi_device_dec_busy(sdev);
> >  }
> >
> >  static bool scsi_mq_get_budget(struct blk_mq_hw_ctx *hctx)
> >  {
> >       struct request_queue *q = hctx->queue;
> >       struct scsi_device *sdev = q->queuedata;
> > +     unsigned long flags;
> >
> > -     if (scsi_dev_queue_ready(q, sdev))
> > +     spin_lock_irqsave(&sdev->budget_lock, flags);
> > +     if (scsi_dev_queue_ready(q, sdev)) {
> > +             spin_unlock_irqrestore(&sdev->budget_lock, flags);
> >               return true;
> > +     }
> > +     sdev->budget_contention = true;
> > +     spin_unlock_irqrestore(&sdev->budget_lock, flags);
>
> No, it really hurts performance by adding one per-sdev spinlock in fast path,
> and we actually tried to kill the atomic variable of 'sdev->device_busy'
> for high performance HBA.

It might be slow, but correctness trumps speed, right?  I tried to do
this with a 2nd atomic and without the spinlock but I kept having a
hole one way or the other.  I ended up just trying to keep the
spinlock section as small as possible.

If you know of a way to get rid of the spinlock that still makes the
code correct, I'd be super interested!  :-)  I certainly won't claim
that it's impossible to do, only that I didn't manage to come up with
a way.

-Doug