Re: link() not increasing link count on NFS server

On Thu, 15 Nov 2007, Mohan Srinivasan wrote:

> The code you cite, which launches a lookup on the receipt of an EEXIST in 
> nfs_link() is a horrible hack that needs to be removed. I always wanted to 
> remove it but did not want to stir up controversy.
>
> The logic predates the NFS/UDP duplicate request cache, which all NFS 
> servers will support. The NFS dupreq cache caches the replies for 
> non-idempotent operations and will replay the cached response if a 
> non-idenpotent operation is retransmitted. This works around spurious errors 
> in the event the NFS response was lost, of course. The dupreq cache appeared 
> in most NFS server implementations in late 1989.
>
> There is no justification for the logic that the FreeBSD NFS client has at 
> the end of these ops. In fact it breaks more things that it fixes. At 
> Yahoo!, we had a group that was doing locking by creating lockfiles and 
> checking for the existence of these lockfiles. As you can imagine, that 
> application broke over FreeBSD NFS. I worked around this in FreeBSD's Yahoo! 
> implementation.
>
> I have not looked at the original link bug reported, but I would 
> wholeheartedly endorse ripping out the "launch a lookup on a an error in 
> these ops" in all of the NFS ops and just return the error/or success 
> returned by the original NFS op.

OK, I've attached an initial patch that does this -- we still need to keep the 
lookup code for NFSv2, where the file handle of the new node isn't returned 
with the reply, but I drop the EEXIST handling cases.  Does this look 
reasonable to you?  I'm not set up to easily test this scenario, however.

Robert N M Watson
Computer Laboratory
University of Cambridge

Index: nfs_vnops.c
===================================================================
RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/nfsclient/nfs_vnops.c,v
retrieving revision 1.276
diff -u -r1.276 nfs_vnops.c
--- nfs_vnops.c	1 Jun 2007 01:12:44 -0000	1.276
+++ nfs_vnops.c	16 Nov 2007 14:35:59 -0000
_at__at_ -1769,11 +1769,6 _at__at_
  		VTONFS(vp)->n_attrstamp = 0;
  	if (!wccflag)
  		VTONFS(tdvp)->n_attrstamp = 0;
-	/*
-	 * Kludge: Map EEXIST => 0 assuming that it is a reply to a retry.
-	 */
-	if (error == EEXIST)
-		error = 0;
  	return (error);
  }

_at__at_ -1837,17 +1832,9 _at__at_
  nfsmout:

  	/*
-	 * If we get an EEXIST error, silently convert it to no-error
-	 * in case of an NFS retry.
-	 */
-	if (error == EEXIST)
-		error = 0;
-
-	/*
-	 * If we do not have (or no longer have) an error, and we could
-	 * not extract the newvp from the response due to the request being
-	 * NFSv2 or the error being EEXIST.  We have to do a lookup in order
-	 * to obtain a newvp to return.
+	 * If we do not have an error and we could not extract the newvp from
+	 * the response due to the request being NFSv2, we have to do a
+	 * lookup in order to obtain a newvp to return.
  	 */
  	if (error == 0 && newvp == NULL) {
  		struct nfsnode *np = NULL;
_at__at_ -1925,15 +1912,7 _at__at_
  	mtx_unlock(&(VTONFS(dvp))->n_mtx);
  	if (!wccflag)
  		VTONFS(dvp)->n_attrstamp = 0;
-	/*
-	 * Kludge: Map EEXIST => 0 assuming that you have a reply to a retry
-	 * if we can succeed in looking up the directory.
-	 */
-	if (error == EEXIST || (!error && !gotvp)) {
-		if (newvp) {
-			vput(newvp);
-			newvp = NULL;
-		}
+	if (error == 0 && newvp == NULL) {
  		error = nfs_lookitup(dvp, cnp->cn_nameptr, len, cnp->cn_cred,
  			cnp->cn_thread, &np);
  		if (!error) {