Date: Mon, 12 May 1997 09:54:50 +0100 (BST) From: Doug Rabson <dfr@nlsystems.com> To: Sean Eric Fagan <sef@Kithrup.COM> Cc: current@freebsd.org Subject: Re: PATCHES: NFS server locking support Message-ID: <Pine.BSF.3.95q.970512094126.310G-100000@herring.nlsystems.com> In-Reply-To: <199705090309.UAA04692@kithrup.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 8 May 1997, Sean Eric Fagan wrote: > In article <199705090127.SAA29328.kithrup.freebsd.current@phaeton.artisoft.com> terry writes: > >I have just uploaded patches for kernel support of NFS server locking. > > I've looked at a previous version of these patches before (and am going over > the current ones now). > > > o The NAMEI layering changes > > o The namei() EXCLUDE flag changes > > > > Note: If you don't like this, fix CVS, or apply > > the inverse of the NAMEI patch, which is > > located in: > > > > ~terry/NAMEI.DIFF.FREEZE > > > > Otherwise, I think this should be committed > > as a unit. > > I am still, as I have told Terry, not sure about these changes. I don't > object to them in principal (and, for what they want to do, the code looks > correct, based on my manual examination), but they are a change in the API, > and that bothers me. I have just read through these changes and as far as I am concerned, they are fine. The modifications to existing filesystems are trivial (basically stubbing out VOP_ABORTOP and returning 0 from VOP_ADVLOCK) and will not cause any merge problems for possibly external filesystem code imports. The syscall changes are (understandably) a bit more extensive, more from the use of the new EXCLUDE flag to namei rather than the abortop changes. I have read though the changes once and they look fine so far. I need to take another pass over them, especially rename() which has changed the most but I think they are sound. I don't expect merging external code here will be much of a problem either. > > > o The necessary changes to the fcntl(2) man page > > o Support for the additional fcntl(2) operands: > [...] > >Notes: * F_CNVT requires a covenant between the NFS lockd in > > user space, and the kernel, based on the wire > > representation of NFS file handles propagated to > > the lockd process. Because I don't know what this > > is from the incomplete user space rpc.lockd code, > > this function is stubbed to return ENOSYS. Once > > this information is documented, it will be a simple > > matter to call FHTOVP on the user's behalf. > > While I think this is a nice thing, and the code seems okay (I wonder about > the "backward compatibility hack" though). However... I would prefer to see > some explanation of the programming model (although it's pretty easy to > figure out, I think, based on the code) *AND* a working lockd. (But, again: > not a terribly strong requirement, as long as lockd is being worked on. But > without it, these changes do nothing, which means just another source of > potential bitrot.) The 'backward compatibility hack' is slightly unpleasant (for those who haven't read the code, Terry has added two fields to struct flock, which affects old binaries and has implications for recompiling old source). Given that it is carefully documented that the new fields are ignored (set to default values) for existing lock calls and only used for the new calls, I think it is OK. What I would really like is to go back in time to when struct flock was created and add a version field to it :-). > The namei changes to concern me. The other people working on filesystem > code *MUST* buy into this. If they're willing to do the changes, I don't > object (if anyone really cares, I mean ;)). Like I said, on first reading, I would take the namei changes along with the flock changes. The old system of SAVENAME, VOP_ABORTOP always made me shudder and IMHO was complicated and prone to errors and systematic memory leaks. -- Doug Rabson Mail: dfr@nlsystems.com Nonlinear Systems Ltd. Phone: +44 181 951 1891
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.3.95q.970512094126.310G-100000>