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

Robert

Your changes look good to me.

mohan

--- On Fri, 11/16/07, Robert Watson <rwatson_at_FreeBSD.org> wrote:

> From: Robert Watson <rwatson_at_FreeBSD.org>
> Subject: Re: link() not increasing link count on NFS server
> To: "Mohan Srinivasan" <mohan_srinivasan_at_yahoo.com>
> Cc: "Timo Sirainen" <tss_at_iki.fi>, "Adam McDougall" <mcdouga9_at_egr.msu.edu>, freebsd-current_at_FreeBSD.org, mohans_at_FreeBSD.org
> Date: Friday, November 16, 2007, 6:37 AM
> 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) {