Re: getiobuf(x, false) can sleep ?

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Fri, Apr 02, 2010 at 12:49:56PM +0000, Andrew Doran wrote:
> On Fri, Apr 02, 2010 at 06:30:30PM +0900, enami tsugutomo wrote:
> > > When called this way getiobuf() will do pool_cache_get(bufio_cache,
> > > PR_NOWAIT).  Does anyone see if this can sleep somewhere despite the
> > > PR_NOWAIT ?
> > 
> > The mutex_enter() in the pool_cache_get_slow()?
> 
> This cache won't sleep if PR_NOWAIT, it's interrupt safe.
> 
> You could verify the context switch theory using something like:
> 
>       uint64_t pctr;
> 
>       kpreempt_disable();
>       pctr = lwp_pctr();
>       /* do stuff */
>       KASSERT(pctr == lwp_pctr());    /* check for context switch */
>       kpreempt_enable();

I've been running with the attached patch for a day now (the workaround
for bp changing under us was already there for a couple days), and the
"dkstart: preemption occured" printf fired twice. I didn't do the
kpreempt_disable()/kpreempt_enable() because the code is running
with the kernel_lock and at splbio(), so kernel preemption should already
be disabled (if it's not it's a problem). 
The "tbp != bp" has not fired today, but I guess it's because the
load is sighly smaller than a working day.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: dk.c
===================================================================
RCS file: /cvsroot/src/sys/dev/dkwedge/dk.c,v
retrieving revision 1.42.6.2
diff -u -r1.42.6.2 dk.c
--- dk.c        30 Jan 2010 19:00:46 -0000      1.42.6.2
+++ dk.c        4 Apr 2010 21:58:30 -0000
@@ -1021,6 +1021,8 @@
 
        /* Place it in the queue and start I/O on the unit. */
        s = splbio();
+       KASSERT(curcpu()->ci_biglock_count > 0);
+       KASSERT(curlwp->l_blcnt > 0);
        sc->sc_iopend++;
        BUFQ_PUT(sc->sc_bufq, bp);
        dkstart(sc);
@@ -1042,10 +1044,14 @@
 dkstart(struct dkwedge_softc *sc)
 {
        struct vnode *vp;
-       struct buf *bp, *nbp;
+       struct buf *bp, *nbp, *tbp;
+       uint64_t pctr;
 
        /* Do as much work as has been enqueued. */
        while ((bp = BUFQ_PEEK(sc->sc_bufq)) != NULL) {
+               KASSERT(curcpu()->ci_biglock_count > 0);
+               KASSERT(curcpu()->ci_ilevel >= IPL_BIO);
+               KASSERT(curlwp->l_blcnt > 0);
                if (sc->sc_state != DKW_STATE_RUNNING) {
                        (void) BUFQ_GET(sc->sc_bufq);
                        if (sc->sc_iopend-- == 1 &&
@@ -1056,24 +1062,39 @@
                        bp->b_error = ENXIO;
                        bp->b_resid = bp->b_bcount;
                        biodone(bp);
+                       continue;
                }
 
-               /* Instrumentation. */
-               disk_busy(&sc->sc_dk);
-
+               pctr = curlwp->l_ncsw;
                nbp = getiobuf(sc->sc_parent->dk_rawvp, false);
+               if (pctr != curlwp->l_ncsw) {
+                       printf("dkstart: preemption occured\n");
+               }
                if (nbp == NULL) {
                        /*
                         * No resources to run this request; leave the
                         * buffer queued up, and schedule a timer to
                         * restart the queue in 1/2 a second.
                         */
-                       disk_unbusy(&sc->sc_dk, 0, bp->b_flags & B_READ);
                        callout_schedule(&sc->sc_restart_ch, hz / 2);
                        return;
                }
 
-               (void) BUFQ_GET(sc->sc_bufq);
+               KASSERT(curcpu()->ci_biglock_count > 0);
+               KASSERT(curcpu()->ci_ilevel >= IPL_BIO);
+               KASSERT(curlwp->l_blcnt > 0);
+               tbp = BUFQ_GET(sc->sc_bufq);
+               if (tbp != bp) {
+                       printf("dkstart tbp %p != bp %p\n", tbp, bp);
+                       bp = tbp;
+                       if (bp == NULL) {
+                               putiobuf(nbp);
+                               return;
+                       }
+               }
+                               
+               /* Instrumentation. */
+               disk_busy(&sc->sc_dk);
 
                nbp->b_data = bp->b_data;
                nbp->b_flags = bp->b_flags;