Date: Thu, 24 Apr 2003 02:40:56 -0700 (PDT) From: Don Lewis <truckman@FreeBSD.org> To: current@FreeBSD.org Subject: locking vnode pointer fvp in nfs_rename() Message-ID: <200304240941.h3O9euXB031495@gw.catspoiler.org>
next in thread | raw e-mail | index | archive | help
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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200304240941.h3O9euXB031495>