Date: Fri, 16 Nov 2007 08:58:11 -0800 (PST) From: Mohan Srinivasan <mohan_srinivasan@yahoo.com> To: Robert Watson <rwatson@FreeBSD.org> Cc: Timo Sirainen <tss@iki.fi>, freebsd-current@FreeBSD.org, Adam McDougall <mcdouga9@egr.msu.edu>, mohans@FreeBSD.org Subject: Re: link() not increasing link count on NFS server Message-ID: <673204.48075.qm@web31813.mail.mud.yahoo.com> In-Reply-To: <20071116143239.U10677@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Robert
Your changes look good to me.
mohan
--- On Fri, 11/16/07, Robert Watson <rwatson@FreeBSD.org> wrote:
> From: Robert Watson <rwatson@FreeBSD.org>
> Subject: Re: link() not increasing link count on NFS server
> To: "Mohan Srinivasan" <mohan_srinivasan@yahoo.com>
> Cc: "Timo Sirainen" <tss@iki.fi>, "Adam McDougall" <mcdouga9@egr.msu.edu>, freebsd-current@FreeBSD.org, mohans@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
> @@ -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) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?673204.48075.qm>