Date:   Mon, 20 Apr 2020 09:31:22 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Jens Axboe <axboe@...nel.dk>
Cc:     Paolo Valente <paolo.valente@...aro.org>,
        Guenter Roeck <groeck@...omium.org>,
        Gwendal Grignou <gwendal@...omium.org>,
        linux-scsi@...r.kernel.org,
        linux-block <linux-block@...r.kernel.org>,
        Ming Lei <ming.lei@...hat.com>, Salman Qazi <sqazi@...gle.com>,
        André Almeida <andrealmeid@...labora.com>,
        Bart Van Assche <bvanassche@....org>,
        Damien Le Moal <damien.lemoal@....com>,
        John Garry <john.garry@...wei.com>,
        Pavel Begunkov <asml.silence@...il.com>,
        Sagi Grimberg <sagi@...mberg.me>,
        LKML <linux-kernel@...r.kernel.org>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>
Subject: Re: [PATCH v4 0/4] blk-mq: Fix two causes of IO stalls found in
 reboot testing

Hi,

On Mon, Apr 20, 2020 at 8:49 AM Jens Axboe <axboe@...nel.dk> wrote:
>
> On 4/20/20 8:45 AM, Doug Anderson wrote:
> > Hi Jens,
> >
> > On Wed, Apr 8, 2020 at 8:35 AM Douglas Anderson <dianders@...omium.org> wrote:
> >>
> >> While doing reboot testing, I found that occasionally my device would
> >> trigger the hung task detector.  Many tasks were stuck waiting for the
> >> a blkdev mutex, but at least one task in the system was always sitting
> >> waiting for IO to complete (and holding the blkdev mutex).  One
> >> example of a task that was just waiting for its IO to complete on one
> >> reboot:
> >>
> >>  udevd           D    0  2177    306 0x00400209
> >>  Call trace:
> >>   __switch_to+0x15c/0x17c
> >>   __schedule+0x6e0/0x928
> >>   schedule+0x8c/0xbc
> >>   schedule_timeout+0x9c/0xfc
> >>   io_schedule_timeout+0x24/0x48
> >>   do_wait_for_common+0xd0/0x160
> >>   wait_for_completion_io_timeout+0x54/0x74
> >>   blk_execute_rq+0x9c/0xd8
> >>   __scsi_execute+0x104/0x198
> >>   scsi_test_unit_ready+0xa0/0x154
> >>   sd_check_events+0xb4/0x164
> >>   disk_check_events+0x58/0x154
> >>   disk_clear_events+0x74/0x110
> >>   check_disk_change+0x28/0x6c
> >>   sd_open+0x5c/0x130
> >>   __blkdev_get+0x20c/0x3d4
> >>   blkdev_get+0x74/0x170
> >>   blkdev_open+0x94/0xa8
> >>   do_dentry_open+0x268/0x3a0
> >>   vfs_open+0x34/0x40
> >>   path_openat+0x39c/0xdf4
> >>   do_filp_open+0x90/0x10c
> >>   do_sys_open+0x150/0x3c8
> >>   ...
> >>
> >> I've reproduced this on two systems: one boots from an internal UFS
> >> disk and one from eMMC.  Each has a card reader attached via USB with
> >> an SD card plugged in.  On the USB-attached SD card is a disk with 12
> >> partitions (a Chrome OS test image), if it matters.  The system
> >> doesn't do much with the USB disk other than probe it (it's plugged in
> >> my system to help me recover).
> >>
> >> From digging, I believe that there are two separate but related
> >> issues.  Both issues relate to the SCSI code saying that there is no
> >> budget.
> >>
> >> I have done testing with only one or the other of the two patches in
> >> this series and found that I could still encounter hung tasks if only
> >> one of the two patches was applied.  This deserves a bit of
> >> explanation.  To me, it's fairly obvious that the first fix wouldn't
> >> fix the problems talked about in the second patch.  However, it's less
> >> obvious why the second patch doesn't fix the problems in
> >> blk_mq_dispatch_rq_list().  It turns out that it _almost_ does
> >> (problems become much more rare), but I did manage to get a single
> >> trace where the "kick" scheduled by the second patch happened really
> >> quickly.  The scheduled kick then ran and found nothing to do.  This
> >> happened in parallel to a task running in blk_mq_dispatch_rq_list()
> >> which hadn't gotten around to splicing the list back into
> >> hctx->dispatch.  This is why we need both fixes.
> >>
> >> Most of my testing has been atop Chrome OS 5.4's kernel tree which
> >> currently has v5.4.30 merged in.  The Chrome OS 5.4 tree also has a
> >> patch by Salman Qazi, namely ("block: Limit number of items taken from
> >> the I/O scheduler in one go").  Reverting that patch didn't make the
> >> hung tasks go away, so I kept it in for most of my testing.
> >>
> >> I have also done some testing on mainline Linux (most on what git
> >> describe calls v5.6-rc7-227-gf3e69428b5e2) even without Salman's
> >> patch.  I found that I could reproduce the problems there and that
> >> traces looked about the same as I saw on the downstream branch.  These
> >> patches were also confirmed to fix the problems on mainline.
> >>
> >> Chrome OS is currently setup to use the BFQ scheduler and I found that
> >> I couldn't reproduce the problems without BFQ.  As discussed in the
> >> second patch this is believed to be because BFQ sometimes returns
> >> "true" from has_work() but then NULL from dispatch_request().
> >>
> >> I'll insert my usual caveat that I'm sending patches to code that I
> >> know very little about.  If I'm making a total bozo patch here, please
> >> help me figure out how I should fix the problems I found in a better
> >> way.
> >>
> >> If you want to see a total ridiculous amount of chatter where I
> >> stumbled around a whole bunch trying to figure out what was wrong and
> >> how to fix it, feel free to read <https://crbug.com/1061950>.  I
> >> promise it will make your eyes glaze over right away if this cover
> >> letter didn't already do that.  Specifically comment 79 in that bug
> >> includes a link to my ugly prototype of making BFQ's has_work() more
> >> exact (I only managed it by actually defining _both_ an exact and
> >> inexact function to avoid circular locking problems when it was called
> >> directly from blk_mq_hctx_has_pending()).  Comment 79 also has more
> >> thoughts about alternatives considered.
> >>
> >> I don't know if these fixes represent a regression of some sort or are
> >> new.  As per above I could only reproduce with BFQ enabled which makes
> >> it nearly impossible to go too far back with this.  I haven't listed
> >> any "Fixes" tags here, but if someone felt it was appropriate to
> >> backport this to some stable trees that seems like it'd be nice.
> >> Presumably at least 5.4 stable would make sense.
> >>
> >> Thanks to Salman Qazi, Paolo Valente, and Guenter Roeck who spent a
> >> bunch of time helping me trawl through some of this code and reviewing
> >> early versions of this patch.
> >>
> >> Changes in v4:
> >> - Only kick in blk_mq_do_dispatch_ctx() / blk_mq_do_dispatch_sched().
> >>
> >> Changes in v3:
> >> - Note why blk_mq_dispatch_rq_list() change is needed.
> >> - ("blk-mq: Add blk_mq_delay_run_hw_queues() API call") new for v3
> >> - Always kick when putting the budget.
> >> - Delay blk_mq_do_dispatch_sched() kick by 3 ms for inexact has_work().
> >> - Totally rewrote commit message.
> >> - ("Revert "scsi: core: run queue...") new for v3.
> >>
> >> Changes in v2:
> >> - Replace ("scsi: core: Fix stall...") w/ ("blk-mq: Rerun dispatch...")
> >>
> >> Douglas Anderson (4):
> >>   blk-mq: In blk_mq_dispatch_rq_list() "no budget" is a reason to kick
> >>   blk-mq: Add blk_mq_delay_run_hw_queues() API call
> >>   blk-mq: Rerun dispatching in the case of budget contention
> >>   Revert "scsi: core: run queue if SCSI device queue isn't ready and
> >>     queue is idle"
> >>
> >>  block/blk-mq-sched.c    | 18 ++++++++++++++++++
> >>  block/blk-mq.c          | 30 +++++++++++++++++++++++++++---
> >>  drivers/scsi/scsi_lib.c |  7 +------
> >>  include/linux/blk-mq.h  |  1 +
> >>  4 files changed, 47 insertions(+), 9 deletions(-)
> >
> > Is there anything blocking this series from landing?  All has been
> > quiet for a while.  All the patches have Ming's review and the SCSI
> > patch has Martin's Ack.  This seems like a great time to get it into
> > linux-next so it can get a whole bunch of testing before the next
> > merge window.
>
> Current series doesn't apply - can you resend it?

Of course.  I've sent v5 based on 'linux_blk/block-5.7' and brought
the collected tags forward.  The conflict I found was with
5fe56de799ad ("blk-mq: Put driver tag in blk_mq_dispatch_rq_list()
when no budget").  I built and booted with my rebased series but I
didn't run a stress test since the resolution was easy.

Please yell if there's anything else you need from me.

Thanks!

-Doug

Powered by blists - more mailing lists