Re: locking vnode pointer fvp in nfs_rename()

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
>> _at__at_ -1521,6 +1521,13 _at__at_
>>  		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.