kern/138109: Minor cleanups to the sys/gnu/fs/ext2fs based on
BSD Lite2
Bruce Evans
brde at optusnet.com.au
Mon Aug 24 14:40:04 UTC 2009
The following reply was made to PR kern/138109; it has been noted by GNATS.
From: Bruce Evans <brde at optusnet.com.au>
To: "Pedro F. Giffuni" <giffunip at tutopia.com>
Cc: freebsd-gnats-submit at FreeBSD.org
Subject: Re: kern/138109: Minor cleanups to the sys/gnu/fs/ext2fs based on
BSD Lite2
Date: Mon, 24 Aug 2009 23:14:47 +1000 (EST)
On Sun, 23 Aug 2009, Pedro F. Giffuni wrote:
>> Description:
> I have been looking at some of the FFS BSD-lite2 fixes to apply them to our ext2fs (based on an older FFS1 from BSD lites). This is helping getting some of the code more in sync with the NetBSD implementation.
Please don't format mail for 200+ column terminals.
> I am still missing some bigger changes but for now here are pretty simple cleanups, based on these FFS changes:
>
> ffs_inode.c
> ------------
> Use the correct flags (IO_SYNC -> B_SYNC) when deciding to do a sync or
> async write in the section that changes the filesize. The bug resulted
> in the updates always being async.
I tested this a bit after you told me about it a few months ago.
> ffs_vfsops.c
> -------------
> Speed up for vfs_bio -- addition of a routine bqrelse to greatly diminish
> overhead for merged cache.
Interesting. I've used this for many years, but didn't notice it reducing
overheads.
The changes involving vnode_pager_setsize() seem to be bugs:
>> How-To-Repeat:
>
>> Fix:
> diff -ruN ext2fs.orig/ext2_inode.c ext2fs/ext2_inode.c
> --- ext2fs.orig/ext2_inode.c 2009-08-18 20:32:13.000000000 -0500
> +++ ext2fs/ext2_inode.c 2009-08-23 12:37:18.000000000 -0500
> @@ -126,16 +126,16 @@
> long count, nblocks, blocksreleased = 0;
> int aflags, error, i, allerror;
> off_t osize;
> -/*
-printf("ext2_truncate called %d to %d\n", VTOI(ovp)->i_number, length);
> -*/ /*
> +
> + /*
> * negative file sizes will totally break the code below and
> * are not meaningful anyways.
> + * XXX: We should check for max file size here too.
> */
> + oip = VTOI(ovp);
> if (length < 0)
> - return EFBIG;
> + return EINVAL;
Should also fix the style bugs (indentation, and missing parentheses).
These style bugs are missing in the ffs version.
>
> - oip = VTOI(ovp);
> if (ovp->v_type == VLNK &&
> oip->i_size < ovp->v_mount->mnt_maxsymlinklen) {
> #ifdef DIAGNOSTIC
This code should be almost identical with that in ffs, and now almost is.
Did you get if from FreeBSD or NetBSD?
The comments in it don't seem necessary. ffs doesn't have them. Both
check for maxfilesize, but only ffs does it at this point, before the
VLNK code, but after the extended attributes code). It may be technically
correct to not check maxfilesize for either symlinks or extended attributes,
since maxfilesize only applies to regular files, but maxfilesize should be
larger so checking it first is harmless. ffs is now inconsisent about this
for symlinks versus extended attributes.
FreeBSD history shows that it is ffs that has the misplaced check for
maxfilesize. Lite1 was just missing the check. Lite2 added it where
ext2fs has it now. FreeBSD already had it when Lite2 was merged in
ffs_inode.c 1.24. FreeBSD had it up-front, and FreeBSD had the wrong
error code value for the (length < 0) case (like ext2fs has now).
Rev.1.24 merged Lite2 incompletely by fixing the error code but not
moving the maxfilesize check.
> ...
> @@ -167,12 +167,13 @@
> aflags = B_CLRBUF;
> if (flags & IO_SYNC)
> aflags |= B_SYNC;
> - vnode_pager_setsize(ovp, length);
Moving this is dangerous. It is inconsistent with ffs and seems wrong.
Don't copy NetBSD for this. dyson!@ had to fix many misplaced calls
to vnode_pager_setsize().
> - if ((error = ext2_balloc(oip, lbn, offset + 1, cred, &bp,
> - aflags)) != 0)
> + error = ext2_balloc(oip, lbn, offset + 1, cred,
> + &bp, aflags);
This line no longer needs splitting.
> + if (error)
> return (error);
Another old bug is not restoring the size on error. This was fixed
in ffs (but not here :-() just this year (ffs_inode.c 1.115). Presumably
the old code delayed the setting until the space was allocated to avoid
having to clean up and/or to avoid having an inconsistent setting while
allocating, but in FreeBSD it is necessary to increase the vm size while
allocating.
> oip->i_size = length;
> - if (aflags & IO_SYNC)
> + vnode_pager_setsize(ovp, length);
Don't move this -- see above.
> + if (aflags & B_SYNC)
> bwrite(bp);
> else
> bawrite(bp);
> @@ -195,18 +196,20 @@
> aflags = B_CLRBUF;
> if (flags & IO_SYNC)
> aflags |= B_SYNC;
> - if ((error = ext2_balloc(oip, lbn, offset, cred, &bp,
> - aflags)) != 0)
> + ext2_balloc(oip, lbn, offset, cred, &bp,
> + aflags)
This line shouldn't be split, much more so than the one above.
> + if (error)
> return (error);
Another place that is missing restoring the vm size on error.
> oip->i_size = length;
> size = blksize(fs, oip, lbn);
> bzero((char *)bp->b_data + offset, (u_int)(size - offset));
> allocbuf(bp, size);
> - if (aflags & IO_SYNC)
> + if (aflags & B_SYNC)
> bwrite(bp);
> else
> bawrite(bp);
> }
> + vnode_pager_setsize(ovp, length);
Don't move this...
> /*
> * Calculate index into inode's block list of
> * last direct and indirect blocks (if any)
> diff -ruN ext2fs.orig/ext2_vfsops.c ext2fs/ext2_vfsops.c
> --- ext2fs.orig/ext2_vfsops.c 2009-08-18 20:32:13.000000000 -0500
> +++ ext2fs/ext2_vfsops.c 2009-08-23 12:40:27.000000000 -0500
> @@ -171,10 +171,7 @@
> flags = WRITECLOSE;
> if (mp->mnt_flag & MNT_FORCE)
> flags |= FORCECLOSE;
> - if (vfs_busy(mp, LK_NOWAIT, 0, td))
> - return (EBUSY);
> error = ext2_flushfiles(mp, flags, td);
> - vfs_unbusy(mp, td);
> if (!error && fs->s_wasvalid) {
> fs->s_es->s_state |= EXT2_VALID_FS;
> ext2_sbupdate(ump, MNT_WAIT);
Consistent with ffs, but I don't understand it.
> @@ -496,10 +493,10 @@
> * Things to do to update the mount:
> * 1) invalidate all cached meta-data.
> * 2) re-read superblock from disk.
> - * 3) re-read summary information from disk.
> - * 4) invalidate all inactive vnodes.
> - * 5) invalidate all cached file data.
> - * 6) re-read inode data for all active vnodes.
> + * 3) (re-read summary information from disk.)
> + * - (invalidate all inactive vnodes.)
> + * 4) invalidate all cached file data.
> + * 5) re-read inode data for all active vnodes.
> */
> static int
> ext2_reload(struct mount *mp, struct thread *td)
Don't renumber this. The meaning of the parentheses is unclear. IIRC,
ext2fs doesn't do all the steps here, but it should. Use descriptive
comments instead of parentheses to say which ones.
> @@ -1007,8 +1004,8 @@
> * still zero, it will be unlinked and returned to the free
> * list by vput().
> */
> - vput(vp);
> brelse(bp);
> + vput(vp);
> *vpp = NULL;
> return (error);
> }
Consistent with ffs, and seems to be needed (not just a style fix).
> @@ -1032,7 +1029,7 @@
> /*
> ext2_print_inode(ip);
> */
> - brelse(bp);
> + bqrelse(bp);
>
> /*
> * Initialize the vnode from the inode, check for aliases.
Though I use it, I'm not sure about the safety of this.
Bruce
More information about the freebsd-fs
mailing list