From b5d7061d1ace7a0b0583af8ceae683dbd3e9bc81 Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Thu, 23 Sep 2010 17:15:07 -0700 Subject: [PATCH] kernel - Work through some memory leaks in dsched * Add a uninitbufbio() function to complement initbufbio(). Also move BUF_LOCKINIT() into initbufbio() and BUF_LOCKFREE() into uninitbufbio(). * There are several device drivers and other places where the struct buf is still being allocated manually (verses using getpbuf()). These were kfree()ing the buffer without dealing with e.g. dsched_exit_buf(). Have uninitbufbio() call dsched_exit_buf() and adjust the various manual allocations/frees of struct buf to call uninitbufbio() before kfree()ing. Also remove various manual calls to BUF_LOCKFREE() (which is now done inside uninitbufbio()). * This should hopefully deal with the memory leaks but there could be a few left. Reported-by: "Steve O'Hara-Smith" --- sys/dev/disk/ata/ata-raid.c | 25 +++++++++++++++++-------- sys/dev/disk/ccd/ccd.c | 3 +-- sys/dev/disk/nata/ata-raid.c | 4 ++-- sys/dev/raid/vinum/vinuminterrupt.c | 4 ++-- sys/dev/raid/vinum/vinumrequest.c | 8 +++----- sys/kern/kern_dsched.c | 2 +- sys/kern/vfs_bio.c | 16 ++++++++++++++-- sys/sys/buf.h | 1 + sys/vfs/devfs/devfs_vnops.c | 19 ++++++++++--------- 9 files changed, 51 insertions(+), 31 deletions(-) diff --git a/sys/dev/disk/ata/ata-raid.c b/sys/dev/disk/ata/ata-raid.c index bf986c0757..c89e962f09 100644 --- a/sys/dev/disk/ata/ata-raid.c +++ b/sys/dev/disk/ata/ata-raid.c @@ -559,9 +559,8 @@ arstrategy(struct dev_strategy_args *ap) } buf1 = kmalloc(sizeof(struct ar_buf), M_AR, M_INTWAIT | M_ZERO); - BUF_LOCKINIT(&buf1->bp); - BUF_LOCK(&buf1->bp, LK_EXCLUSIVE); initbufbio(&buf1->bp); + BUF_LOCK(&buf1->bp, LK_EXCLUSIVE); buf1->bp.b_bio1.bio_offset = (off_t)lba << DEV_BSHIFT; if ((buf1->drive = drv) > 0) buf1->bp.b_bio1.bio_offset += (off_t)rdp->offset << DEV_BSHIFT; @@ -582,6 +581,8 @@ arstrategy(struct dev_strategy_args *ap) !AD_SOFTC(rdp->disks[buf1->drive])->dev) { rdp->disks[buf1->drive].flags &= ~AR_DF_ONLINE; ar_config_changed(rdp, 1); + BUF_UNLOCK(&buf1->bp); + uninitbufbio(&buf1->bp); kfree(buf1, M_AR); bp->b_flags |= B_ERROR; bp->b_error = EIO; @@ -618,6 +619,8 @@ arstrategy(struct dev_strategy_args *ap) ar_config_changed(rdp, 1); if (!(rdp->flags & AR_F_READY)) { + BUF_UNLOCK(&buf1->bp); + uninitbufbio(&buf1->bp); kfree(buf1, M_AR); bp->b_flags |= B_ERROR; bp->b_error = EIO; @@ -643,9 +646,8 @@ arstrategy(struct dev_strategy_args *ap) buf1_blkno < rdp->lock_start)) { buf2 = kmalloc(sizeof(struct ar_buf), M_AR, M_INTWAIT); bcopy(buf1, buf2, sizeof(struct ar_buf)); - BUF_LOCKINIT(&buf2->bp); - BUF_LOCK(&buf2->bp, LK_EXCLUSIVE); initbufbio(&buf2->bp); + BUF_LOCK(&buf2->bp, LK_EXCLUSIVE); buf2->bp.b_bio1.bio_offset = buf1->bp.b_bio1.bio_offset; buf1->mirror = buf2; buf2->mirror = buf1; @@ -653,6 +655,7 @@ arstrategy(struct dev_strategy_args *ap) dev_dstrategy(AD_SOFTC(rdp->disks[buf2->drive])->dev, &buf2->bp.b_bio1); rdp->disks[buf2->drive].last_lba = buf1_blkno + chunk; + /* XXX free buf2? */ } else buf1->drive = buf1->drive + rdp->width; @@ -745,6 +748,8 @@ ar_done(struct bio *bio) default: kprintf("ar%d: unknown array type in ar_done\n", rdp->lun); } + BUF_UNLOCK(&buf->bp); + uninitbufbio(&buf->bp); kfree(buf, M_AR); rel_mplock(); } @@ -1396,6 +1401,8 @@ ar_rw_done(struct bio *bio) { struct buf *bp = bio->bio_buf; + BUF_UNLOCK(bp); + uninitbufbio(bp); kfree(bp->b_data, M_AR); kfree(bp, M_AR); } @@ -1407,9 +1414,8 @@ ar_rw(struct ad_softc *adp, u_int32_t lba, int count, caddr_t data, int flags) int retry = 0, error = 0; bp = kmalloc(sizeof(struct buf), M_AR, M_INTWAIT|M_ZERO); - BUF_LOCKINIT(bp); - BUF_LOCK(bp, LK_EXCLUSIVE); initbufbio(bp); + BUF_LOCK(bp, LK_EXCLUSIVE); bp->b_data = data; bp->b_bio1.bio_offset = (off_t)lba << DEV_BSHIFT; bp->b_bcount = count; @@ -1432,10 +1438,13 @@ ar_rw(struct ad_softc *adp, u_int32_t lba, int count, caddr_t data, int flags) error = biowait_timeout(&bp->b_bio1, "arrw", 10); if (!error && (bp->b_flags & B_ERROR)) error = bp->b_error; - if (error == EWOULDBLOCK) + if (error == EWOULDBLOCK) { bp->b_bio1.bio_done = ar_rw_done; - else + } else { + BUF_UNLOCK(bp); + uninitbufbio(bp); kfree(bp, M_AR); + } } return error; } diff --git a/sys/dev/disk/ccd/ccd.c b/sys/dev/disk/ccd/ccd.c index bea11e18cf..fc6b6f412f 100644 --- a/sys/dev/disk/ccd/ccd.c +++ b/sys/dev/disk/ccd/ccd.c @@ -277,7 +277,6 @@ getccdbuf(void) * independant struct buf initialization */ buf_dep_init(&cbp->cb_buf); - BUF_LOCKINIT(&cbp->cb_buf); BUF_LOCK(&cbp->cb_buf, LK_EXCLUSIVE); BUF_KERNPROC(&cbp->cb_buf); cbp->cb_buf.b_flags = B_PAGING | B_BNOCLIP; @@ -296,13 +295,13 @@ void putccdbuf(struct ccdbuf *cbp) { BUF_UNLOCK(&cbp->cb_buf); - BUF_LOCKFREE(&cbp->cb_buf); if (numccdfreebufs < NCCDFREEHIWAT) { cbp->cb_freenext = ccdfreebufs; ccdfreebufs = cbp; ++numccdfreebufs; } else { + uninitbufbio(&cbp->cb_buf); kfree((caddr_t)cbp, M_DEVBUF); } } diff --git a/sys/dev/disk/nata/ata-raid.c b/sys/dev/disk/nata/ata-raid.c index d64831993c..10d17211f5 100644 --- a/sys/dev/disk/nata/ata-raid.c +++ b/sys/dev/disk/nata/ata-raid.c @@ -885,9 +885,8 @@ ata_raid_dump(struct dev_dump_args *ap) } bzero(&dbuf, sizeof(struct buf)); - BUF_LOCKINIT(&dbuf); - BUF_LOCK(&dbuf, LK_EXCLUSIVE); initbufbio(&dbuf); + BUF_LOCK(&dbuf, LK_EXCLUSIVE); /* bio_offset is byte granularity, convert block granularity a_blkno */ dbuf.b_bio1.bio_offset = ap->a_offset; dbuf.b_bio1.bio_caller_info1.ptr = (void *)rdp; @@ -903,6 +902,7 @@ ata_raid_dump(struct dev_dump_args *ap) return(dbuf.b_error ? dbuf.b_error : EIO); } BUF_UNLOCK(&dbuf); + uninitbufbio(&dbuf); return 0; } diff --git a/sys/dev/raid/vinum/vinuminterrupt.c b/sys/dev/raid/vinum/vinuminterrupt.c index a209d121d0..812e1e1d07 100644 --- a/sys/dev/raid/vinum/vinuminterrupt.c +++ b/sys/dev/raid/vinum/vinuminterrupt.c @@ -250,7 +250,7 @@ freerq(struct request *rq) Free(rqg->rqe[rqno].b.b_data); /* free it */ if (rqg->rqe[rqno].flags & XFR_BUFLOCKED) { /* locked this buffer, */ BUF_UNLOCK(&rqg->rqe[rqno].b); /* unlock it again */ - BUF_LOCKFREE(&rqg->rqe[rqno].b); + uninitbufbio(&rqg->rqe[rqno].b); } } nrqg = rqg->next; /* note the next one */ @@ -292,7 +292,7 @@ sdio_done(struct bio *bio) biodone_sync(bio); biodone(sbp->bio); /* complete the caller's I/O */ BUF_UNLOCK(&sbp->b); - BUF_LOCKFREE(&sbp->b); + uninitbufbio(&sbp->b); Free(sbp); rel_mplock(); } diff --git a/sys/dev/raid/vinum/vinumrequest.c b/sys/dev/raid/vinum/vinumrequest.c index a9b7f58456..f035828c70 100644 --- a/sys/dev/raid/vinum/vinumrequest.c +++ b/sys/dev/raid/vinum/vinumrequest.c @@ -818,10 +818,9 @@ build_rq_buffer(struct rqelement *rqe, struct plex *plex) if (rqe->flags & XFR_BUFLOCKED) /* paranoia */ panic("build_rq_buffer: rqe already locked"); /* XXX remove this when we're sure */ #endif - BUF_LOCKINIT(bp); /* get a lock for the buffer */ + initbufbio(bp); BUF_LOCK(bp, LK_EXCLUSIVE); /* and lock it */ BUF_KERNPROC(bp); - initbufbio(bp); rqe->flags |= XFR_BUFLOCKED; bp->b_bio1.bio_done = complete_rqe; /* @@ -949,10 +948,9 @@ sdio(struct bio *bio) sbp->b.b_bcount = bp->b_bcount; /* number of bytes to transfer */ sbp->b.b_resid = bp->b_resid; /* and amount waiting */ sbp->b.b_data = bp->b_data; /* data buffer */ - BUF_LOCKINIT(&sbp->b); /* get a lock for the buffer */ + initbufbio(&sbp->b); BUF_LOCK(&sbp->b, LK_EXCLUSIVE); /* and lock it */ BUF_KERNPROC(&sbp->b); - initbufbio(&sbp->b); sbp->b.b_bio1.bio_offset = bio->bio_offset + ((off_t)sd->driveoffset << DEV_BSHIFT); sbp->b.b_bio1.bio_done = sdio_done; /* come here on completion */ sbp->b.b_bio1.bio_flags |= BIO_SYNC; @@ -966,7 +964,7 @@ sdio(struct bio *bio) bp->b_resid = bp->b_bcount; /* nothing transferred */ biodone(bio); BUF_UNLOCK(&sbp->b); - BUF_LOCKFREE(&sbp->b); + uninitbufbio(&sbp->b); Free(sbp); return; } diff --git a/sys/kern/kern_dsched.c b/sys/kern/kern_dsched.c index 3b09494392..580a3dd70f 100644 --- a/sys/kern/kern_dsched.c +++ b/sys/kern/kern_dsched.c @@ -904,7 +904,7 @@ dsched_exit_proc(struct proc *p) KKASSERT(tdctx != NULL); tdctx->dead = 0xDEAD; - dsched_set_proc_priv(p, 0); + dsched_set_proc_priv(p, NULL); dsched_thread_ctx_unref(tdctx); /* one for alloc, */ dsched_thread_ctx_unref(tdctx); /* one for ref */ diff --git a/sys/kern/vfs_bio.c b/sys/kern/vfs_bio.c index 4e13a5694a..3f480dc7f3 100644 --- a/sys/kern/vfs_bio.c +++ b/sys/kern/vfs_bio.c @@ -634,7 +634,6 @@ bufinit(void) initbufbio(bp); xio_init(&bp->b_xio); buf_dep_init(bp); - BUF_LOCKINIT(bp); TAILQ_INSERT_TAIL(&bufqueues[BQUEUE_EMPTY], bp, b_freelist); } @@ -701,7 +700,8 @@ bufinit(void) } /* - * Initialize the embedded bio structures + * Initialize the embedded bio structures, typically used by + * deprecated code which tries to allocate its own struct bufs. */ void initbufbio(struct buf *bp) @@ -719,6 +719,8 @@ initbufbio(struct buf *bp) bp->b_bio2.bio_next = NULL; bp->b_bio2.bio_done = NULL; bp->b_bio2.bio_flags = 0; + + BUF_LOCKINIT(bp); } /* @@ -737,6 +739,16 @@ reinitbufbio(struct buf *bp) } /* + * Undo the effects of an initbufbio(). + */ +void +uninitbufbio(struct buf *bp) +{ + dsched_exit_buf(bp); + BUF_LOCKFREE(bp); +} + +/* * Push another BIO layer onto an existing BIO and return it. The new * BIO layer may already exist, holding cached translation data. */ diff --git a/sys/sys/buf.h b/sys/sys/buf.h index 21b69bb568..d57acb76dc 100644 --- a/sys/sys/buf.h +++ b/sys/sys/buf.h @@ -398,6 +398,7 @@ void waitrunningbufspace(void); int buf_dirty_count_severe (void); int buf_runningbufspace_severe (void); void initbufbio(struct buf *); +void uninitbufbio(struct buf *); void reinitbufbio(struct buf *); void clearbiocache(struct bio *); void bremfree (struct buf *); diff --git a/sys/vfs/devfs/devfs_vnops.c b/sys/vfs/devfs/devfs_vnops.c index 7bc79ee262..4916632642 100644 --- a/sys/vfs/devfs/devfs_vnops.c +++ b/sys/vfs/devfs/devfs_vnops.c @@ -1688,7 +1688,6 @@ devfs_spec_strategy(struct vop_strategy_args *ap) nbp = kmalloc(sizeof(*bp), M_DEVBUF, M_INTWAIT|M_ZERO); initbufbio(nbp); buf_dep_init(nbp); - BUF_LOCKINIT(nbp); BUF_LOCK(nbp, LK_EXCLUSIVE); BUF_KERNPROC(nbp); nbp->b_vp = vp; @@ -1762,8 +1761,6 @@ devfs_spec_strategy_done(struct bio *nbio) bp, bp->b_error, bp->b_bcount, bp->b_bcount - bp->b_resid); #endif - kfree(nbp, M_DEVBUF); - biodone(bio); } else if (nbp->b_resid) { /* * A short read or write terminates the chain @@ -1777,8 +1774,6 @@ devfs_spec_strategy_done(struct bio *nbio) "bcount %d/%d\n", bp, bp->b_bcount - bp->b_resid, bp->b_bcount); #endif - kfree(nbp, M_DEVBUF); - biodone(bio); } else if (nbp->b_bcount != nbp->b_bufsize) { /* * A short read or write can also occur by truncating b_bcount @@ -1792,8 +1787,6 @@ devfs_spec_strategy_done(struct bio *nbio) bp->b_error = 0; bp->b_bcount = nbp->b_bcount + boffset; bp->b_resid = nbp->b_resid; - kfree(nbp, M_DEVBUF); - biodone(bio); } else if (nbp->b_bcount + boffset == bp->b_bcount) { /* * No more data terminates the chain @@ -1805,8 +1798,6 @@ devfs_spec_strategy_done(struct bio *nbio) #endif bp->b_error = 0; bp->b_resid = 0; - kfree(nbp, M_DEVBUF); - biodone(bio); } else { /* * Continue the chain @@ -1826,7 +1817,17 @@ devfs_spec_strategy_done(struct bio *nbio) #endif dev_dstrategy(nbp->b_vp->v_rdev, &nbp->b_bio1); + return; } + + /* + * Fall through to here on termination. biodone(bp) and + * clean up and free nbp. + */ + biodone(bio); + BUF_UNLOCK(nbp); + uninitbufbio(nbp); + kfree(nbp, M_DEVBUF); } /* -- 2.11.4.GIT