Date: Sat, 28 Feb 1998 10:33:12 +0100 From: Eivind Eklund <eivind@yes.no> To: Bruce Evans <bde@zeta.org.au>, eivind@yes.no, jlemon@americantv.com Cc: fs@FreeBSD.ORG Subject: Re: syncer / SMP question Message-ID: <19980228103312.01592@follo.net> In-Reply-To: <199802280454.PAA20951@godzilla.zeta.org.au>; from Bruce Evans on Sat, Feb 28, 1998 at 03:54:10PM %2B1100 References: <199802280454.PAA20951@godzilla.zeta.org.au>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Feb 28, 1998 at 03:54:10PM +1100, Bruce Evans wrote:
> >> It appears to be released. The vfs_busy() routine makes a call to
> >> lockmgr(), which sets a lock on mountpoint and releases the lock on
> >> the mountlist.
> >
> >But... But... What about the fetch of the next-pointer? Looks like
> >a potential race-condition to me, but this includes some lock-calls I
> >don't really understand what do (interlocks? What are they?).
>
> Interlocks are for very short-term locks and for locking the aquisition
> of full locks.
Thanks.
> There is no race, because vfs_busy(), unlike lockmgr(), never releases
> the interlock in the LK_NOWAIT case. This non-release is surprising and
> is actually commented on at the beginning of vfs_busy() (bug: the comment
> says that the interlock is not released on failure, but it always
> temporarily released on failure in the !LK_NOWAIT case). Release of the
> interlock on success is normal and is not commented on.
Are you saying the release of the mountlist lock on success normal?
It looks very much abnormal to me; it is at the wrong layer. If this
is normal, it means I'll have to remember 200 locking calls instead of
5, as any call that mess with the locks must be considered a locking
call. Ouch.
> >Perhaps a WALK_MOUNTLIST macro would be better? I use
> >#define LIST_WALK_FORWARD(list, node, extra_node) \
> > for ((node) = (list)->pHead; \
> > (node) = (extra_node); \
> > (extra_node) = (node)->pNext)
>
> No. Macros are never better :-). Here they would obfuscate the locking.
The macro above is for walking 'normal lists' (actually, tail-node
terminated lists with the tail-node marked by having a NULL
next-pointer). They don't contain locking.
The other macro I showed would hide the locking, as you say. This is
intentional, as it is part of abstracting the datastructure. The
interface for manipulating the datastructure is assumed to take care
of locking as necessary; 'obfuscating' the locking by hiding it from
the client.
IMO, macros are better if they can consistently provide a set of
functionality. Due to C lacking late binding, anynoymous functions,
and user-definable control-structures, we have to resort to textual
trickery to reach the same goals ;-)
Using a macro allow me to take a fairly complex piece of code that
need to use user variables and centralize it. Afterwards, I don't
have to read things that carefully; if I see that macro, I know
exactly what code it is. Readability improved.
Now, here is what sync() would look like if I had free reign (I've cut
the comment at the end here, for readbility):
int
sync(p, uap)
struct proc *p;
struct sync_args *uap;
{
struct mount *mp; /* mountpoint - walked through the list */
struct mount *nmp; /* next-pointer (internal use in WALK_M_L) */
int asyncflag;
WALK_MOUNT_LIST(&mountlist, mp, nmp, p) {
if ((mp->mnt_flag & MNT_RDONLY) == 0) {
asyncflag = mp->mnt_flag & MNT_ASYNC;
mp->mnt_flag &= ~MNT_ASYNC;
vfs_msync(mp, MNT_NOWAIT);
VFS_SYNC(mp, MNT_NOWAIT,
p != NULL ? p->p_ucred : NOCRED, p);
mp->mnt_flag |= asyncflag;
}
}
}
This is about half the code of the previous version, and to me it is
much clearer than before.
Eivind.
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-fs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?19980228103312.01592>