svn commit: r326643 - head/sys/cam

[Warner Losh]

On Mon, Dec 18, 2017 at 7:59 AM, Mark Johnston <markj at freebsd.org> wrote:

> On Wed, Dec 06, 2017 at 11:05:07PM +0000, Warner Losh wrote:
> > Author: imp
> > Date: Wed Dec  6 23:05:07 2017
> > New Revision: 326643
> > URL: https://svnweb.freebsd.org/changeset/base/326643
> >
> > Log:
> >   Make cam_periph_runccb be safe to call when we can only do polling.
> >
> >   Sponsored by: Netflix
> >   Differential Revision: https://reviews.freebsd.org/D13388
> >
> > Modified:
> >   head/sys/cam/cam_periph.c
> >   head/sys/cam/cam_xpt.c
> >   head/sys/cam/cam_xpt.h
> >
> > Modified: head/sys/cam/cam_periph.c
> > ============================================================
> ==================
> > --- head/sys/cam/cam_periph.c Wed Dec  6 23:03:34 2017        (r326642)
> > +++ head/sys/cam/cam_periph.c Wed Dec  6 23:05:07 2017        (r326643)
> > @@ -1160,7 +1160,11 @@ cam_periph_runccb(union ccb *ccb,
> >       struct bintime *starttime;
> >       struct bintime ltime;
> >       int error;
> > -
> > +     bool sched_stopped;
> > +     struct mtx *periph_mtx;
> > +     struct cam_periph *periph;
> > +     uint32_t timeout = 1;
> > +
> >       starttime = NULL;
> >       xpt_path_assert(ccb->ccb_h.path, MA_OWNED);
> >       KASSERT((ccb->ccb_h.flags & CAM_UNLOCKED) == 0,
> > @@ -1180,21 +1184,47 @@ cam_periph_runccb(union ccb *ccb,
> >               devstat_start_transaction(ds, starttime);
> >       }
> >
> > +     sched_stopped = SCHEDULER_STOPPED();
>
> It looks like this regresses DDB's "dump" command: while
> SCHEDULER_STOPPED() will be true after a panic, it is not true after
> breaking into DDB from the console. pho@ reported the following
> issue:
>
> db:0:allt>  call doadump
> Dumping 2234 out of 65426 MB:panic: sleepq_add: td 0xfffff80003a48000 to
> sleep on wchan 0xfffffe0000b36ce8 with sleeping prohibited
> cpuid = 18
> time = 1513582125
> KDB: stack backtrace:
> db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame
> 0xfffffe0000b36940
> vpanic() at vpanic+0x19c/frame 0xfffffe0000b369c0
> kassert_panic() at kassert_panic+0x126/frame 0xfffffe0000b36a30
> sleepq_add() at sleepq_add+0x34d/frame 0xfffffe0000b36a80
> _sleep() at _sleep+0x26c/frame 0xfffffe0000b36b20
> cam_periph_runccb() at cam_periph_runccb+0x17d/frame 0xfffffe0000b36c80
> dadump() at dadump+0x12a/frame 0xfffffe0000b36ef0
> dump_append() at dump_append+0xa5/frame 0xfffffe0000b36f10
> blk_write() at blk_write+0x28b/frame 0xfffffe0000b36f50
> minidumpsys() at minidumpsys+0x959/frame 0xfffffe0000b37010
> ...
>
> Wouldn't it be more correct to predicate on "dumping" rather than
> SCHEDULER_STOPPED()?
>

I debated between a number of different alternatives, but didn't have a use
case for when SCHEDUELR_STOPPED() would be wrong. Now I do. I think that
you are right. I'll make that change. Sorry for the hassle this may have
caused.  I'll submit a review and add you as a reviewer today.

Warner