Re: vinum panic on -devel

["Simon 'corecode' Schubert" <corecode@xxxxxxxxxxxx>]

Matthew Dillon wrote:
>     That should work.  Really the correct way is probably to use getpbuf()
>     instead of geteblk(), but I'd like to make as few functional changes
>     as possible to get vinum working again.

Of course I tried to find out how the "really correct way" should look like.  Now I don't get any panics and it *seems* that it might work, but some completely different, but seemingly related (happened several times, not neccessarily directly coupled) panic occured:

dev = #ad/0x20003, block = 11896, fs = /var
panic: ffs_blkfree: freeing free block

backtrace I can't because:

(kgdb) bt
#0  0x00000000 in ?? ()

* 39 Thread 0xce2ec500 (PID=-2: syncer)  0x00000000 in ?? ()

[something is wrong with kgdb here, I will look into this later]

/var was fsck -f'ed directly before.  This time this happened when I start'ed vinum (running following patch).

What I wonder is:  is it my patch which is fucking up stuff, is it vinum doing wrong offset calculations (and thus overwriting something in /var) or is it "just" a bug in ffs?

thanks
 simon

diff -r b16b8dd8a0d1 sys/dev/raid/vinum/vinum.c
--- a/sys/dev/raid/vinum/vinum.c	Fri Jul 07 15:03:06 2006 +0200
+++ b/sys/dev/raid/vinum/vinum.c	Fri Jul 07 15:04:51 2006 +0200
@@ -97,6 +97,8 @@ vinumattach(void *dummy)
    dqend = NULL;

    cdevsw_add(&vinum_cdevsw, 0, 0);			    /* add the cdevsw entry */
+
+    vinum_conf.physbufs = nswbuf / 2 + 1;		    /* maximum amount of physical bufs */

    /* allocate space: drives... */
    DRIVE = (struct drive *) Malloc(sizeof(struct drive) * INITIAL_DRIVES);
diff -r b16b8dd8a0d1 sys/dev/raid/vinum/vinumrevive.c
--- a/sys/dev/raid/vinum/vinumrevive.c	Fri Jul 07 15:03:06 2006 +0200
+++ b/sys/dev/raid/vinum/vinumrevive.c	Fri Jul 07 15:46:07 2006 +0200
@@ -140,11 +140,10 @@ revive_block(int sdno)
	if (bp == NULL)					    /* no buffer space */
	    return ENOMEM;				    /* chicken out */
    } else {						    /* data block */
-	crit_enter();
-	bp = geteblk(size);				    /* Get a buffer */
-	crit_exit();
+	bp = trypbuf(&vinum_conf.physbufs);		    /* Get a buffer */
	if (bp == NULL)
	    return ENOMEM;
+	bp->b_data = Malloc(size);

	/*
	 * Amount to transfer: block size, unless it
@@ -164,7 +163,6 @@ revive_block(int sdno)
	else						    /* it's an unattached plex */
	    dev = VINUM_PLEX(sd->plexno);		    /* create the device number */

-	bp->b_flags = B_PAGING;		    /* either way, read it */
	bp->b_cmd = BUF_CMD_READ;
	vinumstart(dev, &bp->b_bio1, 1);
	biowait(bp);
@@ -176,7 +174,7 @@ revive_block(int sdno)
	/* Now write to the subdisk */
    {
	dev = VINUM_SD(sdno);			    /* create the device number */
-	bp->b_flags = B_ORDERED | B_PAGING;    /* and make this an ordered write */
+	bp->b_flags |= B_ORDERED;		    /* and make this an ordered write */
	bp->b_cmd = BUF_CMD_WRITE;
	bp->b_resid = bp->b_bcount;
	bp->b_bio1.bio_offset = (off_t)sd->revived << DEV_BSHIFT;		    /* write it to here */
@@ -219,11 +217,8 @@ revive_block(int sdno)
	    sd->waitlist = sd->waitlist->next;		    /* and move on to the next */
	}
    }
-    if (bp->b_qindex == 0) {				    /* not on a queue, */
-	bp->b_flags |= B_INVAL;
-	bp->b_flags &= ~B_ERROR;
-	brelse(bp);					    /* is this kosher? */
-    }
+    Free(bp->b_data);
+    relpbuf(bp, &vinum_conf.physbufs);
    return error;
}

@@ -321,9 +316,8 @@ parityops(struct vinum_ioctl_msg *data)
    }
    if (pbp->b_flags & B_ERROR)
	reply->error = pbp->b_error;
-    pbp->b_flags |= B_INVAL;
-    pbp->b_flags &= ~B_ERROR;
-    brelse(pbp);
+    Free(pbp->b_data);
+    relpbuf(pbp, &vinum_conf.physbufs);
    unlockrange(plexno, lock);
}

@@ -397,18 +391,16 @@ parityrebuild(struct plex *plex,
    for (sdno = 0; sdno < bufcount; sdno++) {		    /* for each subdisk */
	if ((sdno != psd) || (op != rebuildparity)) {
	    /* Get a buffer header and initialize it. */
-	    crit_enter();
-	    bpp[sdno] = geteblk(mysize);		    /* Get a buffer */
+	    bpp[sdno] = trypbuf(&vinum_conf.physbufs);	    /* Get a buffer */
	    if (bpp[sdno] == NULL) {
		while (sdno-- > 0) {			    /* release the ones we got */
-		    bpp[sdno]->b_flags |= B_INVAL;
-		    brelse(bpp[sdno]);			    /* give back our resources */
+		    Free(bpp[sdno]->b_data);
+		    relpbuf(bpp[sdno], &vinum_conf.physbufs); /* give back our resources */
		}
-		crit_exit();
		printf("vinum: can't allocate buffer space for parity op.\n");
		return NULL;				    /* no bpps */
	    }
-	    crit_exit();
+	    bpp[sdno]->b_data = Malloc(mysize);
	    if (sdno == psd)
		parity_buf = (int *) bpp[sdno]->b_data;
	    if (sdno == newpsd)				    /* the new one? */
@@ -416,7 +408,6 @@ parityrebuild(struct plex *plex,
	    else
		bpp[sdno]->b_bio1.bio_driver_info = VINUM_SD(plex->sdnos[sdno]);	/* device number */
	    bpp[sdno]->b_cmd = BUF_CMD_READ;	    /* either way, read it */
-	    bpp[sdno]->b_flags = B_PAGING;
	    bpp[sdno]->b_bcount = mysize;
	    bpp[sdno]->b_resid = bpp[sdno]->b_bcount;
	    bpp[sdno]->b_bio1.bio_offset = (off_t)pstripe << DEV_BSHIFT;	    /* transfer from here */
@@ -468,8 +459,8 @@ parityrebuild(struct plex *plex,
	    }
	}
	if (sdno != psd) {				    /* release all bps except parity */
-	    bpp[sdno]->b_flags |= B_INVAL;
-	    brelse(bpp[sdno]);				    /* give back our resources */
+	    Free(bpp[sdno]->b_data);
+	    relpbuf(bpp[sdno], &vinum_conf.physbufs);	    /* give back our resources */
	}
    }

@@ -489,8 +480,8 @@ parityrebuild(struct plex *plex,
		break;
	    }
	}
-	bpp[psd]->b_flags |= B_INVAL;
-	brelse(bpp[psd]);				    /* give back our resources */
+	Free(bpp[psd]->b_data);
+	relpbuf(bpp[psd], &vinum_conf.physbufs);	    /* give back our resources */
    }
    /* release our resources */
    Free(bpp);
@@ -543,14 +534,13 @@ initsd(int sdno, int verify)

    size = min(sd->init_blocksize >> DEV_BSHIFT, sd->sectors - sd->initialized) << DEV_BSHIFT;

+    bp = trypbuf(&vinum_conf.physbufs);		    /* Get a buffer */
+    if (bp == NULL)
+	return ENOMEM;
+    bp->b_data = Malloc(size);
+
    verified = 0;
    while (!verified) {					    /* until we're happy with it, */
-	crit_enter();
-	bp = geteblk(size);				    /* Get a buffer */
-	crit_exit();
-	if (bp == NULL)
-	    return ENOMEM;
-
	bp->b_bcount = size;
	bp->b_resid = bp->b_bcount;
	bp->b_bio1.bio_offset = (off_t)sd->initialized << DEV_BSHIFT;		    /* write it to here */
@@ -561,49 +551,33 @@ initsd(int sdno, int verify)
	biowait(bp);
	if (bp->b_flags & B_ERROR)
	    error = bp->b_error;
-	if (bp->b_qindex == 0) {			    /* not on a queue, */
-	    bp->b_flags |= B_INVAL;
-	    bp->b_flags &= ~B_ERROR;
-	    brelse(bp);					    /* is this kosher? */
-	}
	if ((error == 0) && verify) {			    /* check that it got there */
-	    crit_enter();
-	    bp = geteblk(size);				    /* get a buffer */
-	    if (bp == NULL) {
-		crit_exit();
-		error = ENOMEM;
-	    } else {
-		bp->b_bcount = size;
-		bp->b_resid = bp->b_bcount;
-		bp->b_bio1.bio_offset = (off_t)sd->initialized << DEV_BSHIFT;	    /* read from here */
-		bp->b_bio1.bio_driver_info = VINUM_SD(sdno);
-		bp->b_cmd = BUF_CMD_READ;		    /* read it back */
-		crit_exit();
-		sdio(&bp->b_bio1);
-		biowait(bp);
-		/*
-		 * XXX Bug fix code.  This is hopefully no
-		 * longer needed (21 February 2000).
-		 */
-		if (bp->b_flags & B_ERROR)
-		    error = bp->b_error;
-		else if ((*bp->b_data != 0)		    /* first word spammed */
-		||(bcmp(bp->b_data, &bp->b_data[1], bp->b_bcount - 1))) { /* or one of the others */
-		    printf("vinum: init error on %s, offset 0x%llx sectors\n",
-			sd->name,
-			(long long) sd->initialized);
-		    verified = 0;
-		} else
-		    verified = 1;
-		if (bp->b_qindex == 0) {		    /* not on a queue, */
-		    bp->b_flags |= B_INVAL;
-		    bp->b_flags &= ~B_ERROR;
-		    brelse(bp);				    /* is this kosher? */
-		}
-	    }
+	    bp->b_bcount = size;
+	    bp->b_resid = bp->b_bcount;
+	    bp->b_bio1.bio_offset = (off_t)sd->initialized << DEV_BSHIFT;	    /* read from here */
+	    bp->b_bio1.bio_driver_info = VINUM_SD(sdno);
+	    bp->b_cmd = BUF_CMD_READ;		    /* read it back */
+	    sdio(&bp->b_bio1);
+	    biowait(bp);
+	    /*
+	     * XXX Bug fix code.  This is hopefully no
+	     * longer needed (21 February 2000).
+	     */
+	    if (bp->b_flags & B_ERROR)
+		error = bp->b_error;
+	    else if ((*bp->b_data != 0)		    /* first word spammed */
+	    ||(bcmp(bp->b_data, &bp->b_data[1], bp->b_bcount - 1))) { /* or one of the others */
+		printf("vinum: init error on %s, offset 0x%llx sectors\n",
+		    sd->name,
+		    (long long) sd->initialized);
+		verified = 0;
+	    } else
+		verified = 1;
	} else
	    verified = 1;
    }
+    Free(bp->b_data);
+    relpbuf(bp, &vinum_conf.physbufs);
    if (error == 0) {					    /* did it, */
	sd->initialized += size >> DEV_BSHIFT;		    /* moved this much further down */
	if (sd->initialized >= sd->sectors) {		    /* finished */
diff -r b16b8dd8a0d1 sys/dev/raid/vinum/vinumvar.h
--- a/sys/dev/raid/vinum/vinumvar.h	Fri Jul 07 15:03:06 2006 +0200
+++ b/sys/dev/raid/vinum/vinumvar.h	Fri Jul 07 15:04:51 2006 +0200
@@ -313,6 +313,7 @@ struct _vinum_conf {
    struct request *lastrq;
    struct bio *lastbio;
#endif
+    int physbufs;
};

/* Use these defines to simplify code */


--
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \