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>
index | next in thread | previous in thread | raw e-mail
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.
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200304242026.h3OKQXXB032968>