From owner-freebsd-current@FreeBSD.ORG Thu Apr 24 13:26:45 2003 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 7AB0537B401 for ; Thu, 24 Apr 2003 13:26:45 -0700 (PDT) Received: from gw.catspoiler.org (217-ip-163.nccn.net [209.79.217.163]) by mx1.FreeBSD.org (Postfix) with ESMTP id 67AE843FA3 for ; Thu, 24 Apr 2003 13:26:42 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Received: from FreeBSD.org (scratch.catspoiler.org [192.168.101.3]) by gw.catspoiler.org (8.12.6/8.12.6) with ESMTP id h3OKQXXB032968; Thu, 24 Apr 2003 13:26:38 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Message-Id: <200304242026.h3OKQXXB032968@gw.catspoiler.org> Date: Thu, 24 Apr 2003 13:26:33 -0700 (PDT) From: Don Lewis To: bde@zeta.org.au In-Reply-To: <20030424231352.B25240@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii cc: current@FreeBSD.org Subject: Re: locking vnode pointer fvp in nfs_rename() X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Apr 2003 20:26:45 -0000 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.