From: Doug Anderson <dianders@chromium.org>
To: Ming Lei <ming.lei@redhat.com>
Cc: Jens Axboe <axboe@kernel.dk>,
"James E.J. Bottomley" <jejb@linux.ibm.com>,
"Martin K. Petersen" <martin.petersen@oracle.com>,
linux-block@vger.kernel.org, Guenter Roeck <groeck@chromium.org>,
Paolo Valente <paolo.valente@linaro.org>,
linux-scsi@vger.kernel.org, Salman Qazi <sqazi@google.com>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] scsi: core: Fix stall if two threads request budget at the same time
Date: Mon, 30 Mar 2020 19:15:54 -0700 [thread overview]
Message-ID: <CAD=FV=V-6kFD2Nso+8YGpx5atDpkegBH+7JH9YZ70gPAs84FOw@mail.gmail.com> (raw)
In-Reply-To: <20200331014109.GA20230@ming.t460p>
Hi,
On Mon, Mar 30, 2020 at 6:41 PM Ming Lei <ming.lei@redhat.com> 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@chromium.org>
> > ---
> >
> > 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
next prev parent reply other threads:[~2020-03-31 2:16 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-30 14:49 [PATCH 0/2] blk-mq: Fix two causes of IO stalls found in reboot testing Douglas Anderson
2020-03-30 14:49 ` [PATCH 1/2] blk-mq: In blk_mq_dispatch_rq_list() "no budget" is a reason to kick Douglas Anderson
2020-03-30 14:49 ` [PATCH 2/2] scsi: core: Fix stall if two threads request budget at the same time Douglas Anderson
2020-03-31 1:41 ` Ming Lei
2020-03-31 2:15 ` Doug Anderson [this message]
2020-03-31 2:58 ` Ming Lei
2020-03-31 4:05 ` Doug Anderson
2020-03-31 18:07 ` Paolo Valente
2020-03-31 18:26 ` Jens Axboe
2020-03-31 23:51 ` Doug Anderson
2020-04-01 1:21 ` Jens Axboe
2020-04-01 7:49 ` Paolo Valente
2020-04-02 15:52 ` Doug Anderson
2020-04-01 2:04 ` Ming Lei
2020-04-01 2:32 ` Doug Anderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD=FV=V-6kFD2Nso+8YGpx5atDpkegBH+7JH9YZ70gPAs84FOw@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=axboe@kernel.dk \
--cc=groeck@chromium.org \
--cc=jejb@linux.ibm.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=ming.lei@redhat.com \
--cc=paolo.valente@linaro.org \
--cc=sqazi@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).