Date: Mon, 6 Jul 1998 11:42:19 +1000 From: Bruce Evans <bde@zeta.org.au> To: julian@whistle.com, wollman@khavrinen.lcs.mit.edu Cc: current@FreeBSD.ORG Subject: Re: small locking patch for discussion Message-ID: <199807060142.LAA00552@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
>here's the patch
>Index: ufs/ufs/ufs_quota.c
This seems to be more or less correct, but incomplete.
>===================================================================
>RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_quota.c,v
>retrieving revision 1.23
>diff -c -r1.23 ufs_quota.c
>*** ufs_quota.c 1998/06/21 14:53:40 1.23
>--- ufs_quota.c 1998/07/04 20:39:16
>***************
>*** 465,478 ****
> /*
> * Search vnodes associated with this mount point,
> * deleting any references to quota file being closed.
> */
> again:
> for (vp = mp->mnt_vnodelist.lh_first; vp != NULL; vp = nextvp) {
> nextvp = vp->v_mntvnodes.le_next;
> if (vp->v_type == VNON)
> continue;
>! if (vget(vp, LK_EXCLUSIVE, p))
> goto again;
> ip = VTOI(vp);
> dq = ip->i_dquot[type];
> ip->i_dquot[type] = NODQUOT;
>--- 465,489 ----
> /*
> * 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.
> again:
>+ simple_lock(&mntvnode_slock);
> for (vp = mp->mnt_vnodelist.lh_first; vp != NULL; vp = nextvp) {
>+ if (vp->v_mount != mp) {
>+ simple_unlock(&mntvnode_slock);
>+ goto again;
>+ }
> nextvp = vp->v_mntvnodes.le_next;
> if (vp->v_type == VNON)
> continue;
>! simple_lock(&vp->v_interlock);
>! simple_unlock(&mntvnode_slock);
>! if (vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, p)) {
> goto again;
>+ }
Style bugs: excess braces.
> ip = VTOI(vp);
> dq = ip->i_dquot[type];
> ip->i_dquot[type] = NODQUOT;
>***************
>*** 480,485 ****
>--- 491,497 ----
> 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.
> }
> dqflush(qvp); qvp->v_flag &= ~VSYSTEM;
>#########-------
The above is for quotaoff(). quotaon() is also missing locking.
Bruce
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?199807060142.LAA00552>