Date: Sun, 26 Jan 1997 13:24:13 -0700 (MST) From: Terry Lambert <terry@lambert.org> To: michaelh@cet.co.jp Cc: bde@freefall.freebsd.org, Hackers@FreeBSD.ORG Subject: Re: cvs commit: src/sys/kern kern_lockf.c Message-ID: <199701262024.NAA02217@phaeton.artisoft.com> In-Reply-To: <Pine.SV4.3.95.970126125611.20304B-100000@parkplace.cet.co.jp> from "Michael Hancock" at Jan 26, 97 01:15:56 pm
next in thread | previous in thread | raw e-mail | index | archive | help
> All of the argument checking seems out of place here. The call trace is
> like this:
>
> fcntl => VOP_ADVLOCK => lf_advlock
>
> or
>
> open => VOP_ADVLOCK => lf_advlock
>
> Garbage input should be stopped at the source and lf_advlock should be
> completely free from arg checking. The original coder wanted to factor
> error checking into lf_advlock, but it seems incorrect to allow garbage to
> come in so far.
>
> A consistent division of arg checking responsibilities would make it
> easier for people to decide where to do the checks. We would need some
> comments or preconditions specified in lf_advlock to communicate what was
> expected so that we would know what to do in fcntl and open.
>
> Any comments?
Yes. The syntactic checking should be in the system call layer, and
the grammatic checking should be in the lf_advlock layer, which should
be called from the system call layer.
A system call interface is a consumer of VFS services, just as an NFS
or NetWare or SMB or Lantastic or AppleTalk server may be a consumer
of VFS services without being a consumer of the system call layer.
This is one of the things I am always on about.
The call trace should be:
fcntl(lock) <- check call syntax here
lf_advlock(lock) <- check arg values here
if( !VOP_ADVLOCK(lock))
lf_advlock(unlock)
And the FS specific VOP_ADVLOCK should simply return 0 in all cases for
most FS's.
For fan-out layers (like union mount, where 1 VOP_ADVLOCK needs to
apply to potentially more than one underlying stack),
union.VOP_ADVLOCK(lock)
if( rv = lf_advlock(lock)) <- underlying vnode 1
return( rv); <- FAIL
if( underlying1.VOP_ADVLOCK(lock)) {
lf_advlock(unlock); <- underlying vnode 1
return( 1); <- FAIL
}
if( rv = lf_advlock(lock)) { <- underlying vnode 2
underlying1.VOP_ADVLOCK(unlock);
lf_advlock(unlock); <- underlying vnode 1
return( rv); <-- FAIL
}
if( underlying2.VOP_ADVLOCK(lock)) {
lf_advlock(unlock); <- underlying vnode 2
underlying1.VOP_ADVLOCK(unlock);
lf_advlock(unlock); <- underlying vnode 1
return( 1); <- FAIL
}
return( 0);
With unwind in case the locking really needs to do something. Probably
the easiest way to do this would be a list + recursion, and let a union
only union 2 FS's (union does this, but make it a rule for others).
Clearly, this would benefit from negative logic to enable a single point
of exit from the unionfs code. It's as shown to make control flow
obvious, but negative logic is best for SMP and kernel reentrancy
of the routine to enable clean tracking of entry/exit lock state. All
functions should be single-entry/single-exit, if possible, so as to
not damage the ability to later implement SMP reentrancy and kernel
multithreading as quickly and error-free as humanly possible.
The veto architecture is needed to make union FS's not accidently
lf_advlock() the same vnode twice. For example, say I have a quota
and a non-quota "view" onto the same FS. The quota implementation
is a stacking layer so that all FS's can have quotas. The vnode
is passed through the quota layer, because all the quota layer does
is a name space incursion to subtract out the quota file on the FS
root. A lock on the quota vnode and the non-quota vnode would be
a lock on the same vnode.
For NFS,
nfs.VOP_ADVLOCK(lock)
#ifdef NFS_CLIENT_LOCKING
return( nfs_client_lock(lock));
#else /* !NFS_CLIENT_LOCKING*/
return( 0); /* always succeed; current implementation*/
#endif /* !NFS_CLIENT_LOCKING*/
This has the advantage of a local lock on the same file failing before
NFS has to hit actually the wire.
The place for the checking is either fcntl + open, or lf_advlock,
depending on who pulls the arguments in.
The Lite2 locking implementation is slightly different, but can be
handled the same way, since it's just a lock manager issue, not a
hierarchy issue.
Regards,
Terry Lambert
terry@lambert.org
---
Any opinions in this posting are my own and not those of my present
or previous employers.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199701262024.NAA02217>