[PATCH 1/1] silence-call-timeout-printk

[PATCH 1/1] silence-call-timeout-printk

[Olga Kornievskaia]

When the client's callback server goes away, the server's callback client
tries to contact the server and times out. For nfsd, it is beneficial to
printout a message when the client is unable to contact the server. For
the callback server, the same message is printed yet it is really not
an error. Thus we need a way to silence the message for the callback 
and yet print it for other cases.

Signed-off-by: Olga Kornievskaia <aglo@umich.edu>
---
 fs/nfsd/nfs4callback.c      |    1 +
 include/linux/sunrpc/clnt.h |    1 +
 net/sunrpc/clnt.c           |    6 ++++--
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index aae2b29..bf7d57e 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -400,6 +400,7 @@ static int do_probe_callback(void *data)
 		status = PTR_ERR(client);
 		goto out_err;
 	}
+	client->cl_quiet = 1;
 
 	status = rpc_call_sync(client, &msg, RPC_TASK_SOFT);
 
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 129a86e..0a1141e 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -57,6 +57,7 @@ struct rpc_clnt {
 	struct rpc_timeout	cl_timeout_default;
 	struct rpc_program *	cl_program;
 	char			cl_inline_name[32];
+	int                     cl_quiet; /* silences call_timeout printk */
 };
 
 /*
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 8c6a7f1..5a2a718 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1160,8 +1160,10 @@ call_timeout(struct rpc_task *task)
 	task->tk_timeouts++;
 
 	if (RPC_IS_SOFT(task)) {
-		printk(KERN_NOTICE "%s: server %s not responding, timed out\n",
-				clnt->cl_protname, clnt->cl_server);
+		if (!clnt->cl_quiet)
+			printk(KERN_NOTICE "%s: server %s not responding, "
+				"timed out\n", clnt->cl_protname, 
+				clnt->cl_server);
 		rpc_exit(task, -EIO);
 		return;
 	}
-- 
1.5.3.3

Re: [PATCH 1/1] silence-call-timeout-printk

[J. Bruce Fields]

On Thu, Mar 27, 2008 at 02:49:12PM -0400, Olga Kornievskaia wrote:
> When the client's callback server goes away, the server's callback client
> tries to contact the server and times out. For nfsd, it is beneficial to
> printout a message when the client is unable to contact the server. For
> the callback server, the same message is printed yet it is really not
> an error. Thus we need a way to silence the message for the callback 
> and yet print it for other cases.
> 
> Signed-off-by: Olga Kornievskaia <aglo@umich.edu>
> ---
>  fs/nfsd/nfs4callback.c      |    1 +
>  include/linux/sunrpc/clnt.h |    1 +
>  net/sunrpc/clnt.c           |    6 ++++--
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index aae2b29..bf7d57e 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -400,6 +400,7 @@ static int do_probe_callback(void *data)
>  		status = PTR_ERR(client);
>  		goto out_err;
>  	}
> +	client->cl_quiet = 1;
>  
>  	status = rpc_call_sync(client, &msg, RPC_TASK_SOFT);
>  
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index 129a86e..0a1141e 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -57,6 +57,7 @@ struct rpc_clnt {
>  	struct rpc_timeout	cl_timeout_default;
>  	struct rpc_program *	cl_program;
>  	char			cl_inline_name[32];
> +	int                     cl_quiet; /* silences call_timeout printk */
>  };
>  
>  /*
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 8c6a7f1..5a2a718 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1160,8 +1160,10 @@ call_timeout(struct rpc_task *task)
>  	task->tk_timeouts++;
>  
>  	if (RPC_IS_SOFT(task)) {
> -		printk(KERN_NOTICE "%s: server %s not responding, timed out\n",
> -				clnt->cl_protname, clnt->cl_server);
> +		if (!clnt->cl_quiet)
> +			printk(KERN_NOTICE "%s: server %s not responding, "
> +				"timed out\n", clnt->cl_protname, 

Run linux/scripts/checkpath.pl on patches and it'll warn you about silly
stuff like the extra space at the end of that line.

Other than that--I'd like to do this.  Trond?

--b.

> +				clnt->cl_server);
>  		rpc_exit(task, -EIO);
>  		return;
>  	}
> -- 
> 1.5.3.3
> 

Re: [PATCH 1/1] silence-call-timeout-printk

[Trond Myklebust]

On Thu, 2008-03-27 at 15:54 -0400, J. Bruce Fields wrote:
> On Thu, Mar 27, 2008 at 02:49:12PM -0400, Olga Kornievskaia wrote:
> > When the client's callback server goes away, the server's callback client
> > tries to contact the server and times out. For nfsd, it is beneficial to
> > printout a message when the client is unable to contact the server. For
> > the callback server, the same message is printed yet it is really not
> > an error. Thus we need a way to silence the message for the callback 
> > and yet print it for other cases.

Bring, back,
Bring, back,
Oh, bring back my cl_chatty to me...

  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=f518e35aec984036903c1003e867f833747a9d79

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/1] silence-call-timeout-printk

[J. Bruce Fields]

On Thu, Mar 27, 2008 at 04:03:49PM -0400, Trond Myklebust wrote:
> On Thu, 2008-03-27 at 15:54 -0400, J. Bruce Fields wrote:
> > On Thu, Mar 27, 2008 at 02:49:12PM -0400, Olga Kornievskaia wrote:
> > > When the client's callback server goes away, the server's callback client
> > > tries to contact the server and times out. For nfsd, it is beneficial to
> > > printout a message when the client is unable to contact the server. For
> > > the callback server, the same message is printed yet it is really not
> > > an error. Thus we need a way to silence the message for the callback 
> > > and yet print it for other cases.
> 
> Bring, back,
> Bring, back,
> Oh, bring back my cl_chatty to me...
> 
>   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=f518e35aec984036903c1003e867f833747a9d79

OK, OK, so, shall we apply a patch that basically just reverts Chuck's
patch there, except that it inverts the meaning and uses the name
cl_quiet instead of cl_chatty?  (On the theory that since the verbose
case is the common case, it's best to let this thing be initialized to 0
and only make the callback code mess with it?)  Or are you nostalgic for
cl_chatty?

--b.

Re: [PATCH 1/1] silence-call-timeout-printk

[Trond Myklebust]

On Thu, 2008-03-27 at 16:23 -0400, J. Bruce Fields wrote:
> On Thu, Mar 27, 2008 at 04:03:49PM -0400, Trond Myklebust wrote:
> > On Thu, 2008-03-27 at 15:54 -0400, J. Bruce Fields wrote:
> > > On Thu, Mar 27, 2008 at 02:49:12PM -0400, Olga Kornievskaia wrote:
> > > > When the client's callback server goes away, the server's callback client
> > > > tries to contact the server and times out. For nfsd, it is beneficial to
> > > > printout a message when the client is unable to contact the server. For
> > > > the callback server, the same message is printed yet it is really not
> > > > an error. Thus we need a way to silence the message for the callback 
> > > > and yet print it for other cases.
> > 
> > Bring, back,
> > Bring, back,
> > Oh, bring back my cl_chatty to me...
> > 
> >   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=f518e35aec984036903c1003e867f833747a9d79
> 
> OK, OK, so, shall we apply a patch that basically just reverts Chuck's
> patch there, except that it inverts the meaning and uses the name
> cl_quiet instead of cl_chatty?  (On the theory that since the verbose
> case is the common case, it's best to let this thing be initialized to 0
> and only make the callback code mess with it?)  Or are you nostalgic for
> cl_chatty?
> 
> --b.

Why have two names for the same thing? That will just cause confusion
should we ever want to backport this stuff to older kernels.

In any case, Chuck has made these bit-flags private to the RPC layer, so
you can still add RPC_CLNT_CREATE_QUIET to the rpc_create_args->flags,
and have the default be cl_chatty = 1, when that create_arg flag isn't
set.

Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/1] silence-call-timeout-printk

[J. Bruce Fields]

On Thu, Mar 27, 2008 at 04:31:22PM -0400, Trond Myklebust wrote:
> 
> On Thu, 2008-03-27 at 16:23 -0400, J. Bruce Fields wrote:
> > On Thu, Mar 27, 2008 at 04:03:49PM -0400, Trond Myklebust wrote:
> > > On Thu, 2008-03-27 at 15:54 -0400, J. Bruce Fields wrote:
> > > > On Thu, Mar 27, 2008 at 02:49:12PM -0400, Olga Kornievskaia wrote:
> > > > > When the client's callback server goes away, the server's callback client
> > > > > tries to contact the server and times out. For nfsd, it is beneficial to
> > > > > printout a message when the client is unable to contact the server. For
> > > > > the callback server, the same message is printed yet it is really not
> > > > > an error. Thus we need a way to silence the message for the callback 
> > > > > and yet print it for other cases.
> > > 
> > > Bring, back,
> > > Bring, back,
> > > Oh, bring back my cl_chatty to me...
> > > 
> > >   http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=f518e35aec984036903c1003e867f833747a9d79
> > 
> > OK, OK, so, shall we apply a patch that basically just reverts Chuck's
> > patch there, except that it inverts the meaning and uses the name
> > cl_quiet instead of cl_chatty?  (On the theory that since the verbose
> > case is the common case, it's best to let this thing be initialized to 0
> > and only make the callback code mess with it?)  Or are you nostalgic for
> > cl_chatty?
> > 
> > --b.
> 
> Why have two names for the same thing? That will just cause confusion
> should we ever want to backport this stuff to older kernels.
> 
> In any case, Chuck has made these bit-flags private to the RPC layer, so
> you can still add RPC_CLNT_CREATE_QUIET to the rpc_create_args->flags,
> and have the default be cl_chatty = 1, when that create_arg flag isn't
> set.

OK, that sounds fine.

--b.

Re: [PATCH 1/1] silence-call-timeout-printk

[Talpey, Thomas]

Let me get this straight. You are suggesting that bringing back
cl_chatty as cl_quiet, in order to print messages when the
server's callback client can't talk to the client's callback server,
makes things clear? ;-)

Oh, my head does hurt. :-) :-)

Seriously, it sounds okay but printk isn't exactly the best means
for a server to tell people something doesn't work. The issue I see
is that clients will lose a delegation, data corruption may result,
and the only hint is a message in the server's log? Maybe I'm still
having trouble with the first question.

Tom.

At 04:23 PM 3/27/2008, J. Bruce Fields wrote:
>On Thu, Mar 27, 2008 at 04:03:49PM -0400, Trond Myklebust wrote:
>> On Thu, 2008-03-27 at 15:54 -0400, J. Bruce Fields wrote:
>> > On Thu, Mar 27, 2008 at 02:49:12PM -0400, Olga Kornievskaia wrote:
>> > > When the client's callback server goes away, the server's callback client
>> > > tries to contact the server and times out. For nfsd, it is beneficial to
>> > > printout a message when the client is unable to contact the server. For
>> > > the callback server, the same message is printed yet it is really not
>> > > an error. Thus we need a way to silence the message for the callback 
>> > > and yet print it for other cases.
>> 
>> Bring, back,
>> Bring, back,
>> Oh, bring back my cl_chatty to me...
>> 
>>   
>http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=com
>mitdiff;h=f518e35aec984036903c1003e867f833747a9d79
>
>OK, OK, so, shall we apply a patch that basically just reverts Chuck's
>patch there, except that it inverts the meaning and uses the name
>cl_quiet instead of cl_chatty?  (On the theory that since the verbose
>case is the common case, it's best to let this thing be initialized to 0
>and only make the callback code mess with it?)  Or are you nostalgic for
>cl_chatty?
>
>--b.
>--
>To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] silence-call-timeout-printk

[J. Bruce Fields]

On Thu, Mar 27, 2008 at 04:36:50PM -0400, Talpey, Thomas wrote:
> Let me get this straight. You are suggesting that bringing back
> cl_chatty as cl_quiet, in order to print messages when the
> server's callback client can't talk to the client's callback server,
> makes things clear? ;-)

Err, in order *not* to, actually.  (It's pretty easy for callbacks to
fail, especially the initial probe of the callback channel.  I don't
want to spam the server's logs about that.)

> 
> Oh, my head does hurt. :-) :-)

Yeah, OK ,fair enough.

> Seriously, it sounds okay but printk isn't exactly the best means
> for a server to tell people something doesn't work. The issue I see
> is that clients will lose a delegation, data corruption may result,

If the cb_path_down stuff is done conservatively enough, then in theory
there shouldn't be data corruption....

In any case, even if we think losing the callback path is worth a
printk, I still think that other cases (like failing to establish one in
the first case) aren't.

> and the only hint is a message in the server's log? Maybe I'm still
> having trouble with the first question.

So I'd rather we didn't put anything at all in the logs.  (Though
perhaps it could be useful to the administrator to have some optional
way to figure out the callback status if they were e.g. investigating a
performance problem.)

--b.

Re: [PATCH 1/1] silence-call-timeout-printk

[Trond Myklebust]

On Thu, 2008-03-27 at 16:53 -0400, J. Bruce Fields wrote:
> (Though
> perhaps it could be useful to the administrator to have some optional
> way to figure out the callback status if they were e.g. investigating a
> performance problem.)

Well, we have dprintk(), but at some point in the near future, I'd like
to check out the kernel markers functionality to see if we can't start
replacing the existing dprintk()s with something a bit more targeted.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/1] silence-call-timeout-printk

[Talpey, Thomas]

At 04:53 PM 3/27/2008, J. Bruce Fields wrote:
>  (It's pretty easy for callbacks to
>fail, especially the initial probe of the callback channel.  I don't
>want to spam the server's logs about that.)

Agree. The approach sounds good. Besides, the 4.1 session will
often avoid the whole situation, by sharing the channel.

Tom.