On Fri, 2 Mar 2018 04:34:53 +0000 (UTC) Kirk McKusick <mckus...@freebsd.org> wrote:
> Author: mckusick > Date: Fri Mar 2 04:34:53 2018 > New Revision: 330264 > URL: https://svnweb.freebsd.org/changeset/base/330264 > > Log: > This change is some refactoring of Mark Johnston's changes in r329375 > to fix the memory leak that I introduced in r328426. Instead of > trying to clear up the possible memory leak in all the clients, I > ensure that it gets cleaned up in the source (e.g., ffs_sbget ensures > that memory is always freed if it returns an error). > > The original change in r328426 was a bit sparse in its description. > So I am expanding on its description here (thanks cem@ and rgrimes@ > for your encouragement for my longer commit messages). > > In preparation for adding check hashing to superblocks, r328426 is > a refactoring of the code to get the reading/writing of the superblock > into one place. Unlike the cylinder group reading/writing which > ends up in two places (ffs_getcg/ffs_geom_strategy in the kernel > and cgget/cgput in libufs), I have the core superblock functions > just in the kernel (ffs_sbfetch/ffs_sbput in ffs_subr.c which is > already imported into utilities like fsck_ffs as well as libufs to > implement sbget/sbput). The ffs_sbfetch and ffs_sbput functions > take a function pointer to do the actual I/O for which there are > four variants: > > ffs_use_bread / ffs_use_bwrite for the in-kernel filesystem > > g_use_g_read_data / g_use_g_write_data for kernel geom clients > > ufs_use_sa_read for the standalone code (stand/libsa/ufs.c > but not stand/libsa/ufsread.c which is size constrained) > > use_pread / use_pwrite for libufs > > Uses of these interfaces are in the UFS filesystem, geoms journal & > label, libsa changes, and libufs. They also permeate out into the > filesystem utilities fsck_ffs, newfs, growfs, clri, dump, quotacheck, > fsirand, fstyp, and quot. Some of these utilities should probably be > converted to directly use libufs (like dumpfs was for example), but > there does not seem to be much win in doing so. > > Tested by: Peter Holm (pho@) > > Modified: > head/lib/libufs/sblock.c > head/stand/libsa/ufs.c > head/sys/geom/geom_io.c > head/sys/geom/journal/g_journal_ufs.c > head/sys/geom/label/g_label_ufs.c > head/sys/ufs/ffs/ffs_subr.c > head/sys/ufs/ffs/ffs_vfsops.c > > Modified: head/lib/libufs/sblock.c > ============================================================================== > --- head/lib/libufs/sblock.c Fri Mar 2 03:05:36 2018 (r330263) > +++ head/lib/libufs/sblock.c Fri Mar 2 04:34:53 2018 (r330264) > @@ -150,7 +150,6 @@ use_pread(void *devfd, off_t loc, void **bufp, int siz > int fd; > > fd = *(int *)devfd; > - free(*bufp); > if ((*bufp = malloc(size)) == NULL) > return (ENOSPC); > if (pread(fd, *bufp, size, loc) != size) > > Modified: head/stand/libsa/ufs.c > ============================================================================== > --- head/stand/libsa/ufs.c Fri Mar 2 03:05:36 2018 (r330263) > +++ head/stand/libsa/ufs.c Fri Mar 2 04:34:53 2018 (r330264) > @@ -518,7 +518,7 @@ ufs_open(upath, f) > > /* read super block */ > twiddle(1); > - if ((rc = ffs_sbget(f, &fs, -1, 0, ufs_use_sa_read)) != 0) > + if ((rc = ffs_sbget(f, &fs, -1, "stand", ufs_use_sa_read)) != 0) > goto out; > fp->f_fs = fs; > /* > @@ -688,7 +688,6 @@ ufs_use_sa_read(void *devfd, off_t loc, void **bufp, i > int error; > > f = (struct open_file *)devfd; > - free(*bufp); > if ((*bufp = malloc(size)) == NULL) > return (ENOSPC); > error = (f->f_dev->dv_strategy)(f->f_devdata, F_READ, loc / > DEV_BSIZE, > > Modified: head/sys/geom/geom_io.c > ============================================================================== > --- head/sys/geom/geom_io.c Fri Mar 2 03:05:36 2018 (r330263) > +++ head/sys/geom/geom_io.c Fri Mar 2 04:34:53 2018 (r330264) > @@ -957,6 +957,9 @@ g_use_g_read_data(void *devfd, off_t loc, void **bufp, > { > struct g_consumer *cp; > > + KASSERT(*bufp == NULL, > + ("g_use_g_read_data: non-NULL *bufp %p\n", *bufp)); > + > cp = (struct g_consumer *)devfd; > /* > * Take care not to issue an invalid I/O request. The offset of > @@ -966,8 +969,6 @@ g_use_g_read_data(void *devfd, off_t loc, void **bufp, > */ > if (loc % cp->provider->sectorsize != 0) > return (ENOENT); > - if (*bufp != NULL) > - g_free(*bufp); > *bufp = g_read_data(cp, loc, size, NULL); > if (*bufp == NULL) > return (ENOENT); > > Modified: head/sys/geom/journal/g_journal_ufs.c > ============================================================================== > --- head/sys/geom/journal/g_journal_ufs.c Fri Mar 2 03:05:36 > 2018 (r330263) +++ head/sys/geom/journal/g_journal_ufs.c Fri > Mar 2 04:34:53 2018 (r330264) @@ -72,11 +72,11 @@ > g_journal_ufs_dirty(struct g_consumer *cp) > fs = NULL; > if (SBLOCKSIZE % cp->provider->sectorsize != 0 || > - ffs_sbget(cp, &fs, -1, NULL, g_use_g_read_data) != 0) { > + ffs_sbget(cp, &fs, -1, M_GEOM, g_use_g_read_data) != 0) { > GJ_DEBUG(0, "Cannot find superblock to mark file system %s " > "as dirty.", cp->provider->name); > - if (fs != NULL) > - g_free(fs); > + KASSERT(fs == NULL, > + ("g_journal_ufs_dirty: non-NULL fs %p\n", fs)); > return; > } > GJ_DEBUG(0, "clean=%d flags=0x%x", fs->fs_clean, fs->fs_flags); > > Modified: head/sys/geom/label/g_label_ufs.c > ============================================================================== > --- head/sys/geom/label/g_label_ufs.c Fri Mar 2 03:05:36 2018 > (r330263) +++ head/sys/geom/label/g_label_ufs.c Fri Mar 2 04:34:53 > 2018 (r330264) @@ -77,9 +77,9 @@ g_label_ufs_taste_common(struct > g_consumer *cp, char * > fs = NULL; > if (SBLOCKSIZE % pp->sectorsize != 0 || > - ffs_sbget(cp, &fs, -1, NULL, g_use_g_read_data) != 0) { > - if (fs != NULL) > - g_free(fs); > + ffs_sbget(cp, &fs, -1, M_GEOM, g_use_g_read_data) != 0) { > + KASSERT(fs == NULL, > + ("g_label_ufs_taste_common: non-NULL fs %p\n",fs); > return; > } > > > Modified: head/sys/ufs/ffs/ffs_subr.c > ============================================================================== > --- head/sys/ufs/ffs/ffs_subr.c Fri Mar 2 03:05:36 2018 > (r330263) +++ head/sys/ufs/ffs/ffs_subr.c Fri Mar 2 04:34:53 > 2018 (r330264) @@ -151,6 +151,7 @@ static int readsuper(void *, struct > fs **, off_t, int, > * superblock. Memory is allocated for the superblock by the readfunc and > * is returned. If filltype is non-NULL, additional memory is allocated > * of type filltype and filled in with the superblock summary information. > + * All memory is freed when any error is returned. > * > * If a superblock is found, zero is returned. Otherwise one of the > * following error values is returned: > @@ -172,16 +173,24 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsbloc > int32_t *lp; > char *buf; > > + fs = NULL; > *fsp = NULL; > if (altsblock != -1) { > - if ((error = readsuper(devfd, fsp, altsblock, 1, > - readfunc)) != 0) > + if ((error = readsuper(devfd, &fs, altsblock, 1, > + readfunc)) != 0) { > + if (fs != NULL) > + UFS_FREE(fs, filltype); > return (error); > + } > } else { > for (i = 0; sblock_try[i] != -1; i++) { > - if ((error = readsuper(devfd, fsp, sblock_try[i], 0, > + if ((error = readsuper(devfd, &fs, sblock_try[i], 0, > readfunc)) == 0) > break; > + if (fs != NULL) { > + UFS_FREE(fs, filltype); > + fs = NULL; > + } > if (error == ENOENT) > continue; > return (error); > @@ -190,20 +199,18 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsbloc > return (ENOENT); > } > /* > - * If not filling in summary information, return. > - */ > - if (filltype == NULL) > - return (0); > - /* > * Read in the superblock summary information. > */ > - fs = *fsp; > size = fs->fs_cssize; > blks = howmany(size, fs->fs_fsize); > if (fs->fs_contigsumsize > 0) > size += fs->fs_ncg * sizeof(int32_t); > size += fs->fs_ncg * sizeof(u_int8_t); > - space = UFS_MALLOC(size, filltype, M_WAITOK); > + /* When running in libufs or libsa, UFS_MALLOC may fail */ > + if ((space = UFS_MALLOC(size, filltype, M_WAITOK)) == NULL) { > + UFS_FREE(fs, filltype); > + return (ENOSPC); > + } > fs->fs_csp = (struct csum *)space; > for (i = 0; i < blks; i += fs->fs_frag) { > size = fs->fs_bsize; > @@ -213,9 +220,10 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsbloc > error = (*readfunc)(devfd, > dbtob(fsbtodb(fs, fs->fs_csaddr + i)), (void **)&buf, > size); if (error) { > - UFS_FREE(buf, filltype); > + if (buf != NULL) > + UFS_FREE(buf, filltype); > UFS_FREE(fs->fs_csp, filltype); > - fs->fs_csp = NULL; > + UFS_FREE(fs, filltype); > return (error); > } > memcpy(space, buf, size); > @@ -231,6 +239,7 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsbloc > size = fs->fs_ncg * sizeof(u_int8_t); > fs->fs_contigdirs = (u_int8_t *)space; > bzero(fs->fs_contigdirs, size); > + *fsp = fs; > return (0); > } > > @@ -246,8 +255,6 @@ readsuper(void *devfd, struct fs **fsp, off_t sblocklo > int error; > > error = (*readfunc)(devfd, sblockloc, (void **)fsp, SBLOCKSIZE); > - if (*fsp != NULL) > - (*fsp)->fs_csp = NULL; /* Not yet any summary > information */ if (error != 0) > return (error); > fs = *fsp; > @@ -263,6 +270,8 @@ readsuper(void *devfd, struct fs **fsp, off_t sblocklo > fs->fs_bsize >= roundup(sizeof(struct fs), DEV_BSIZE)) { > /* Have to set for old filesystems that predate this field */ > fs->fs_sblockactualloc = sblockloc; > + /* Not yet any summary information */ > + fs->fs_csp = NULL; > return (0); > } > return (ENOENT); > > Modified: head/sys/ufs/ffs/ffs_vfsops.c > ============================================================================== > --- head/sys/ufs/ffs/ffs_vfsops.c Fri Mar 2 03:05:36 2018 > (r330263) +++ head/sys/ufs/ffs/ffs_vfsops.c Fri Mar 2 04:34:53 > 2018 (r330264) @@ -1075,14 +1075,11 @@ ffs_use_bread(void *devfd, > off_t loc, void **bufp, int struct buf *bp; > int error; > > - free(*bufp, M_UFSMNT); > + KASSERT(*bufp == NULL, ("ffs_use_bread: non-NULL *bufp %p\n", > *bufp)); *bufp = malloc(size, M_UFSMNT, M_WAITOK); > if ((error = bread((struct vnode *)devfd, btodb(loc), size, NOCRED, > - &bp)) != 0) { > - free(*bufp, M_UFSMNT); > - *bufp = NULL; > + &bp)) != 0) > return (error); > - } > bcopy(bp->b_data, *bufp, size); > bp->b_flags |= B_INVAL | B_NOCACHE; > brelse(bp); > _______________________________________________ > svn-src-head@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/svn-src-head > To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org" Recent build of a kernel on CURRENT fails for me due to: [...] --- g_label_ufs.o --- /pool/sources/CURRENT/src/sys/geom/label/g_label_ufs.c:81:3: error: unterminated function-like macro invocation KASSERT(fs == NULL, ^ /pool/sources/CURRENT/src/sys/sys/systm.h:99:9: note: macro 'KASSERT' defined here #define KASSERT(exp,msg) do { \ ^ /pool/sources/CURRENT/src/sys/geom/label/g_label_ufs.c:155:38: error: expected '}' MODULE_DEPEND(g_label, ufs, 1, 1, 1); ^ /pool/sources/CURRENT/src/sys/geom/label/g_label_ufs.c:80:62: note: to match this '{' ffs_sbget(cp, &fs, -1, M_GEOM, g_use_g_read_data) != 0) { ^ /pool/sources/CURRENT/src/sys/geom/label/g_label_ufs.c:155:38: error: expected '}' MODULE_DEPEND(g_label, ufs, 1, 1, 1); ^ /pool/sources/CURRENT/src/sys/geom/label/g_label_ufs.c:70:1: note: to match this '{' { ^ 3 errors generated. *** [g_label_ufs.o] Error code 1 _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
