On Thu, 15 Nov 2007, Mohan Srinivasan wrote:
[color=blue]
> 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.[/color]

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.

/*
- * 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) {
_______________________________________________
[email]freebsd-current@freebsd.org[/email] mailing list
[url]http://lists.freebsd.org/mailman/listinfo/freebsd-current[/url]
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"