From owner-freebsd-current@FreeBSD.ORG Fri Nov 16 14:38:06 2007 Return-Path: Delivered-To: freebsd-current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 66AB816A46B; Fri, 16 Nov 2007 14:38:06 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [209.31.154.42]) by mx1.freebsd.org (Postfix) with ESMTP id 09E6E13C45A; Fri, 16 Nov 2007 14:38:04 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from fledge.watson.org (fledge.watson.org [209.31.154.41]) by cyrus.watson.org (Postfix) with ESMTP id 77C88471FC; Fri, 16 Nov 2007 09:40:02 -0500 (EST) Date: Fri, 16 Nov 2007 14:37:56 +0000 (GMT) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: Mohan Srinivasan In-Reply-To: <698405.85667.qm@web31809.mail.mud.yahoo.com> Message-ID: <20071116143239.U10677@fledge.watson.org> References: <698405.85667.qm@web31809.mail.mud.yahoo.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Timo Sirainen , freebsd-current@FreeBSD.org, Adam McDougall , mohans@FreeBSD.org Subject: Re: link() not increasing link count on NFS server X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 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: Fri, 16 Nov 2007 14:38:06 -0000 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 @@ -1769,11 +1769,6 @@ 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); } @@ -1837,17 +1832,9 @@ 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; @@ -1925,15 +1912,7 @@ 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) {