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
