Date: Mon, 6 Jul 1998 07:07:20 +0000 (GMT) From: Terry Lambert <tlambert@primenet.com> To: bde@zeta.org.au (Bruce Evans) Cc: julian@whistle.com, wollman@khavrinen.lcs.mit.edu, current@FreeBSD.ORG Subject: Re: small locking patch for discussion Message-ID: <199807060707.AAA22574@usr02.primenet.com> In-Reply-To: <199807060142.LAA00552@godzilla.zeta.org.au> from "Bruce Evans" at Jul 6, 98 11:42:19 am
next in thread | previous in thread | raw e-mail | index | archive | help
> This seems to be more or less correct, but incomplete.
Yes. It does not attempt to fix everything at once; I've heard that's
one of my problems from some people... ;-)
> > /*
> > * Search vnodes associated with this mount point,
> > * deleting any references to quota file being closed.
> >+ *
> >+ * This is nearly identical to code in VFSOP_RELOAD, and
> >+ * *must* be maintained in parallel!
> > */
>
> This comment is an obfuscation. All mnt_vnodelist traversals should
> look much like this. Apart from the traversal, the code doesn't look
> much like ffs_reload(). VFSOP_RELOAD doesn't exist. Every traversal
> shouldn't be commented on.
Every traversal should be in common code; the reason the comment is there
is that the traversal mechanism changed recently.
> >! if (vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, p)) {
> > goto again;
> >+ }
>
> Style bugs: excess braces.
I think I can blame this one on Julian; he likes braces. Personally,
I could care less, unless I have to go in and put a debug printf in there,
in which case, I'd have to add them (probably in the first column, with
the printf) at that time. It may be that this had a printf in it.
> > vput(vp);
> > if (vp->v_mntvnodes.le_next != nextvp || vp->v_mount != mp)
> > goto again;
> >+ simple_lock(&mntvnode_slock);
>
> The check before locking is now bogus. It isn't valid to check
> (vp->v_mntvnodes.le_next != nextvp) before locking. This is harmless
> because the only effect of a wrong check is to "goto again" unnecessarily.
> The new consistency check on vp (= the newvp here) at the start of the
> loop handles all problems. Most other mnt_vnodelist traversals (but
> not the one in qsync()) don't bother checking vp here.
The problem is if the user quota being updated is on the group quota
file, or vice versa. This is to guard against that. The other use,
to guard against the block dev on which you are mounted being in the
list of vp's you are syncing to the block dev. This one is less of an
issue if you run devfs.
> > }
> > dqflush(qvp); qvp->v_flag &= ~VSYSTEM;
> >#########-------
>
> The above is for quotaoff(). quotaon() is also missing locking.
Yes. But you can mount with quotas. This was specifically to deal
with a shutdown unmount-all panic.
I don't know if it should be committed. I vastly prefer that the quota
code go into a stacking layer, allowing it to be shared among all FS
types. However, I'm aware that that the layering changes needed for
that code are unlikely to be commited soon, so it's OK with me if this
is committed as a temporary workaround of the panic (since the panic
is real, and the workaround is bound to stick around a long time).
Terry Lambert
terry@lambert.org
---
Any opinions in this posting are my own and not those of my present
or previous employers.
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199807060707.AAA22574>