svn commit: r330264 - in head: lib/libufs stand/libsa sys/geom sys/geom/journal sys/geom/label sys/ufs/ffs
O. Hartmann
o.hartmann at walstatt.org
Fri Mar 2 05:46:06 UTC 2018
On Fri, 2 Mar 2018 04:34:53 +0000 (UTC)
Kirk McKusick <mckusick at 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 at freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/svn-src-head
> To unsubscribe, send any mail to "svn-src-head-unsubscribe at 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
More information about the svn-src-all
mailing list