From owner-freebsd-current@FreeBSD.ORG Thu Apr 24 02:41:04 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 5F90937B401 for ; Thu, 24 Apr 2003 02:41:04 -0700 (PDT) Received: from gw.catspoiler.org (217-ip-163.nccn.net [209.79.217.163]) by mx1.FreeBSD.org (Postfix) with ESMTP id AC66C43F75 for ; Thu, 24 Apr 2003 02:41:03 -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 h3O9euXB031495 for ; Thu, 24 Apr 2003 02:41:00 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Message-Id: <200304240941.h3O9euXB031495@gw.catspoiler.org> Date: Thu, 24 Apr 2003 02:40:56 -0700 (PDT) From: Don Lewis To: current@FreeBSD.org MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii Subject: 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 09:41:04 -0000 VOP_FSYNC() wants its vnode argument to be locked by the caller, but nfs_rename() doesn't do that. I've been running something similar to the patch below for months in a kernel built with the DEBUG_VFS_LOCKS option enabled. The most recent change I made was to modify my original patch to more closely resemble the way this is done in ufs_rename(). One concern that I have about all the leaf file system implementations is the possibility of spurious failures due to failure to obtain the lock. I'm pretty sure that it isn't wise to add the LK_RETRY flag because the possibility of a deadlock. I think the proper way to handle this problem is to drop all the locks, wait for the fvp lock to become available, and then retry from the beginning. Unfortunately this would be messy in the current implementation. 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. Any reason the following change to nfs_rename() shouldn't be committed? 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; + } + if ((error = vn_lock(fvp, LK_EXCLUSIVE, fcnp->cn_thread)) != 0) + goto out; /* * We have to flush B_DELWRI data prior to renaming * the file. If we don't, the delayed-write buffers @@ -1529,8 +1536,8 @@ * ( as far as I can tell ) it flushes dirty buffers more * often. */ - VOP_FSYNC(fvp, fcnp->cn_cred, MNT_WAIT, fcnp->cn_thread); + VOP_UNLOCK(fvp, 0, fcnp->cn_thread); if (tvp) VOP_FSYNC(tvp, tcnp->cn_cred, MNT_WAIT, tcnp->cn_thread);