linux-kernel - Re: [NFS] [PATCH] NFS: supress warnings about NFS4ERR_OLD_STATEID in nfs4_handle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20070501145835.GB24354@dantu.rdu.redhat.com>
Date:	Tue, 1 May 2007 10:58:36 -0400
From:	Jeff Layton <jlayton@...hat.com>
To:	Trond Myklebust <trond.myklebust@....uio.no>
Cc:	Jeff Layton <jlayton@...hat.com>, nfs@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org, ffilz@...ibm.com
Subject: Re: [NFS] [PATCH] NFS: supress warnings about NFS4ERR_OLD_STATEID
	in nfs4_handle_exception

On Tue, May 01, 2007 at 07:42:29AM -0700, Trond Myklebust wrote:
> On Thu, 2007-04-26 at 15:13 -0400, Jeff Layton wrote:
> > I recently posted this patch to the nfs mailing list, but got no comment.
> > Reposting with an actual "[PATCH]" subject...
> > 
> > In certain situations, a NFSv4 client can end up with a set of racing calls
> > that cause the server to return a NFS4ERR_OLD_STATEID error. When this occurs,
> > the only real recourse that the client has is to retry the call. In the
> > current code, when this occurs, it triggers a useless printk (in
> > nfs4_map_errors):
> > 
> >     nfs4_map_errors could not handle NFSv4 error 10024
> > 
> > Trond recently mentioned that we really don't need to worry users with that
> > info. The following patch is based on a suggestion by Frank Filz and supresses
> > this error. Changing the return code here to 0 should be safe since all callers
> > of nfs4_handle_exception are expected to retry the call in this situation
> > anyway.
> > 
> > Signed-off-by: Jeff Layton <jlayton@...hat.com>
> > Signed-off-by: Frank Filz <ffilz@...ibm.com>
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index f52cf5c..b456783 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -2774,6 +2774,7 @@ int nfs4_handle_exception(const struct nfs_server *server, int errorcode, struct
> >  				break;
> >  		case -NFS4ERR_OLD_STATEID:
> >  			exception->retry = 1;
> > +			ret = 0;
> >  	}
> >  	/* We failed to handle the error */
> >  	return nfs4_map_errors(ret);
> 
> Surely "return 0;" is more efficient.
> 
> ...and a minor nit about the comment: it doesn't trigger a printk(). It
> rather triggers a dprintk(), (which should be turned off by default
> unless you happen to be debugging the code).
> 
> The other nit is that I'm not sure the dprintk() is totally useless. It
> is always nice to know _why_ the thing looped when you are debugging.
> That said, we should perhaps rather put the dprintk() for that in
> nfs4_handle_exception instead...
> 
> Trond
> 
> 

Argh. My mistake, that's what I get for looking at old code and new code at
the same time. Since this is a dprintk, I agree that we should probably
just toss this patch. If you're debugging, then there's good reason to want to
see this.

As for moving the dprintk, I'm not so sure that's a great idea either.
nfs4_map_error is called from a couple of different places and I'd think
leaving the dprintk the a more "general" location would be good.

-- Jeff

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/