Date: Tue, 20 Nov 2012 17:20:38 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Attilio Rao <attilio@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs Message-ID: <20121120162708.I924@besplex.bde.org> In-Reply-To: <201211192243.qAJMhjFF055708@svn.freebsd.org> References: <201211192243.qAJMhjFF055708@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 19 Nov 2012, Attilio Rao wrote:
> Log:
> r16312 is not any longer real since many years (likely since when VFS
> received granular locking) but the comment present in UFS has been
> copied all over other filesystems code incorrectly for several times.
>
> Removes comments that makes no sense now.
It still made sense (except for bitrot in the function name), but might not
be true). The code made sense with it. Now the code makes no sense.
> Modified: head/sys/ufs/ffs/ffs_vfsops.c
> ==============================================================================
> --- head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 21:58:14 2012 (r243310)
> +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 22:43:45 2012 (r243311)
> @@ -1676,14 +1676,6 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags
> ump = VFSTOUFS(mp);
> dev = ump->um_dev;
> fs = ump->um_fs;
> -
> - /*
> - * If this malloc() is performed after the getnewvnode()
This malloc() didn't match the code, which uses uma_zalloc(). Old
versions used MALLOC() in both the comment and the code. ffs's comment
was updated to say malloc() when the code was changed to use malloc(),
then rotted when the code was changed to use uma_zalloc(). In some
other file systems, the comment still said MALLOC().
> - * it might block, leaving a vnode with a NULL v_data to be
> - * found by ffs_sync() if a sync happens to fire right then,
> - * which will cause a panic because ffs_sync() blindly
> - * dereferences vp->v_data (as well it should).
> - */
> ip = uma_zalloc(uma_inode, M_WAITOK | M_ZERO);
>
> /* Allocate a new vnode/inode. */
>
The code makes no sense now. The comment explains why ip is allocated
before vp, instead of in the natural, opposite order like it used to
be. Allocating things in an unnatural order requires extra code to
free ip when the allocation of vp fails.
I use the following comment fixes for this and nearby comments in an old
version:
% Index: ffs_vfsops.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_vfsops.c,v
% retrieving revision 1.233
% diff -u -2 -r1.233 ffs_vfsops.c
% --- ffs_vfsops.c 16 Jun 2004 09:47:25 -0000 1.233
% +++ ffs_vfsops.c 1 Oct 2007 12:41:25 -0000
% @@ -1210,4 +1275,10 @@
% }
%
% +/*
% + * Look up a FFS dinode number to find its incore vnode, otherwise read it
% + * in from disk. If it is in core, wait for the lock bit to clear, then
% + * return the inode locked. Detection and handling of mount points must be
% + * done by the calling routine.
% + */
% int
% ffs_vget(mp, ino, flags, vpp)
Previous bitrot removed this comment, leaving no comment about what the
function does. The above just restores it from rev.1.1. I didn't fix
its grammar error (comma splice before "otherwise") or style bugs (it
uses KNF style with 2-space sentence breaks, but other parts of ffs use
non-KNF style with 1-space sentence breaks).
This fix is still relevant.
% @@ -1230,8 +1301,9 @@
%
% /*
% - * We do not lock vnode creation as it is believed to be too
% - * expensive for such rare case as simultaneous creation of vnode
% - * for same ino by different processes. We just allow them to race
% - * and check later to decide who wins. Let the race begin!
% + * We do not lock vnode creation since it is believed to be too
% + * expensive to prevent the rare operation of simultaneous creation
% + * of a vnode for the same ino by different processes. We just allow
% + * the processes to race and check later to decide who wins. Let the
% + * race begin!
% */
% if ((error = ufs_ihashget(dev, ino, flags, vpp)) != 0)
Improve grammar.
This fix is still relevant, although ufs_ihashget() is no longer used.
Now the comment is attached to null code, where previously it was only
attached to the hashget call. It should be attached to the (ip, vp)
allocation code. Your commit removes just 1 of the blank lines that
obfuscated the scope of the above comment by splitting up related
sections.
% @@ -1241,5 +1313,5 @@
%
% /*
% - * If this MALLOC() is performed after the getnewvnode()
% + * If this uma_zalloc() is performed after the getnewvnode()
% * it might block, leaving a vnode with a NULL v_data to be
% * found by ffs_sync() if a sync happens to fire right then,
Perhaps still relevant. The vnode is still not locked here. If this
is not relevant, then please fix the code by moving the ip allocation
back to after the vp allocation and removing error recovering. It
would only not be still relevant if ffs_sync() has been changed to
either not see or to ignore vnodes with a NULL v_data.
% @@ -1275,14 +1347,14 @@
% #endif
% /*
% - * Exclusively lock the vnode before adding to hash. Note, that we
% - * must not release nor downgrade the lock (despite flags argument
% - * says) till it is fully initialized.
% + * Exclusively lock the vnode before hashing it. Note that we must
% + * not release nor downgrade the lock (no matter what the flags
% + * argument says) until it is fully initialized.
% */
% lockmgr(vp->v_vnlock, LK_EXCLUSIVE, (struct mtx *)0, td);
Improve grammar.
This has been replaced by a much shorter comment. I like shortness, but
it is less descriptive now. I don't see any problems in the above except
that it is too specific about hashing. The lock covers much more than
hashing in preemptible kernels.
This and the corresponding current comment are not separated from
previous code by blank lines, while the previous code has bogus
subsections created by blank lines, so the scope of the previous
comments is especially unclear.
%
% /*
% - * Atomicaly (in terms of ufs_hash operations) check the hash for
% - * duplicate of vnode being created and add it to the hash. If a
% - * duplicate vnode was found, it will be vget()ed from hash for us.
% + * Atomically (in terms of ufs_hash operations) check the hash table
% + * for a duplicate of our vnode. If a duplicate is found, it will be
% + * vget()ed from the hash table for us.
% */
% if ((error = ufs_ihashins(ip, flags, vpp)) != 0) {
Fix spelling and improve wording.
This is mostly no longer relevant, at least here. Comments about how
the hashing function works belong in the function.
% @@ -1292,5 +1364,8 @@
% }
%
% - /* We lost the race, then throw away our vnode and return existing */
% + /*
% + * If we lost the race, throw away our vnode and return the
% + * duplicate.
% + */
% if (*vpp != NULL) {
% vput(vp);
Fix grammar and punctuation.
This code has been merged with the error checking for the hash function
and its comment and the comment about the error checking both removed.
This seems to be a regression. An earlier comment still says "Let the
race begin!" This was where the race ended. It is less clear where
it ends now. I think it ends when we acquire the lock, but we still
have to check if it was lost while we didn't hold the lock. The above
seems to be such a check, except it seems to be placed too far from the
lock acquisition for just that. Maybe there were multiple races.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121120162708.I924>