svn commit: r326643 - head/sys/cam

[Warner Losh]

Sorry for the top post, but https://reviews.freebsd.org/D13531 should have
a fix.

Warner

On Mon, Dec 18, 2017 at 8:03 AM, Warner Losh <imp at bsdimp.com> wrote:

>
>
> 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
>