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) {
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/lis...reebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"