Date: Thu, 24 Apr 2003 13:26:33 -0700 (PDT) From: Don Lewis <truckman@FreeBSD.org> To: bde@zeta.org.au Cc: current@FreeBSD.org Subject: Re: locking vnode pointer fvp in nfs_rename() Message-ID: <200304242026.h3OKQXXB032968@gw.catspoiler.org> In-Reply-To: <20030424231352.B25240@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 24 Apr, Bruce Evans wrote:
> On Thu, 24 Apr 2003, Don Lewis wrote:
>> I think it would make a lot of sense to move the preliminary code that
>> is common to each of the leaf file system implementations, including
>> locking fvp, back into kern_rename(). This would make the
>> implementation of the lock retry code a lot easier and would clean up a
>> lot of cut-and-paste code in the leaf file systems. Unfortunately it's
>> probably too late to do this for 5.x.
>
> Was it ever in kern_rename() (or rename()?). This doesn't seem to save
> much (just one vn_lock() call). More below.
No. On re-reading what I wrote, I see that "back" is ambiguous. I
meant to move the common code into the caller (earlier in the execution
path) rather than reverting to an earlier implementation.
It saves more than just the vn_lock() call. All the leaf file system
implementations of the rename method start off with something like:
/* Check for cross-device rename */
if ((fvp->v_mount != tdvp->v_mount) ||
(tvp && (fvp->v_mount != tvp->v_mount))) {
error = EXDEV;
goto out;
}
if (fvp == tvp) {
printf("nfs_rename: fvp == tvp (can't happen)\n");
error = 0;
goto out;
}
I was looking at this change last fall, but ran out of time before I
could look at how this might affect fun things like nullfs and other
potential callers of VOP_RENAME(). VOP_RENAME() is also called by
unionfs, lomacfs, and the NFS server code.
>> Any reason the following change to nfs_rename() shouldn't be committed?
>
> It has some style bugs :-).
Right, a violation of the Fourth Law of Thermodyamics, the conservation
of white space :-) Easily fixed.
>> Index: sys/nfsclient/nfs_vnops.c
>> ===================================================================
>> RCS file: /home/ncvs/src/sys/nfsclient/nfs_vnops.c,v
>> retrieving revision 1.203
>> diff -u -r1.203 nfs_vnops.c
>> --- sys/nfsclient/nfs_vnops.c 23 Apr 2003 02:58:26 -0000 1.203
>> +++ sys/nfsclient/nfs_vnops.c 24 Apr 2003 03:06:21 -0000
>> @@ -1521,6 +1521,13 @@
>> goto out;
>> }
>>
>> + if (fvp == tvp) {
>> + printf("nfs_rename: fvp == tvp (can't happen)\n");
>> + error = 0;
>> + goto out;
>> + }
>
> Maybe the lock was delayed because of the (deleted) complications for the
> (fvp == tvp) case. Exclusive locking would have to be avoided in this
> case just to avoid deadlock.
I believe that is correct. It would also be possible to deadlock with
another process that was doing the locking in the opposite order.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200304242026.h3OKQXXB032968>