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

On Fri, 16 Nov 2007, Mohan Srinivasan wrote:

> Your changes look good to me.

Adam, Timo, Mohan,

I've gone ahead and committed these changes to the NFS2/3 client in HEAD, and 
will MFC them in a week or so assuming things don't go horribly wrong.

Robert N M Watson
Computer Laboratory
University of Cambridge

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