[1/1] NFSv4.1: add tracepoint to trunked nfs4_exchange_id calls

Commit Message

Olga Kornievskaia Jan. 26, 2024, 7:03 p.m. UTC

From: Olga Kornievskaia <[e-mail]>

Add a tracepoint to track when the client sends EXCHANGE_ID to test
a new transport for session trunking.

Signed-off-by: Olga Kornievskaia <[e-mail]>
---
 fs/nfs/nfs4proc.c  |  3 +++
 fs/nfs/nfs4trace.h | 30 ++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

Comments

Chen Hanxiao Jan. 30, 2024, 8:21 a.m. UTC | #1

> -----邮件原件-----
> 发件人: Olga Kornievskaia <[e-mail]>
> 发送时间: 2024年1月27日 3:04
> 收件人: [e-mail]; [e-mail]
> 抄送: [e-mail]
> 主题: [PATCH 1/1] NFSv4.1: add tracepoint to trunked nfs4_exchange_id calls
> 
> From: Olga Kornievskaia <[e-mail]>
> 
> Add a tracepoint to track when the client sends EXCHANGE_ID to test a new
> transport for session trunking.
> 
> Signed-off-by: Olga Kornievskaia <[e-mail]>
> ---
>  fs/nfs/nfs4proc.c  |  3 +++
>  fs/nfs/nfs4trace.h | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 

Tested-by: Chen Hanxiao <[e-mail]>

Regards,
- Chen

Benjamin Coddington Feb. 2, 2024, 1:01 p.m. UTC | #2

On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:

> From: Olga Kornievskaia <[e-mail]>
>
> Add a tracepoint to track when the client sends EXCHANGE_ID to test
> a new transport for session trunking.
>
> Signed-off-by: Olga Kornievskaia <[e-mail]>
> ---
>  fs/nfs/nfs4proc.c  |  3 +++
>  fs/nfs/nfs4trace.h | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 23819a756508..cdda7971c945 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -8974,6 +8974,9 @@ void nfs4_test_session_trunk(struct rpc_clnt *clnt, struct rpc_xprt *xprt,
>  		status = nfs4_detect_session_trunking(adata->clp,
>  				task->tk_msg.rpc_resp, xprt);
>
> +	trace_nfs4_trunked_exchange_id(adata->clp,
> +			xprt->address_strings[RPC_DISPLAY_ADDR], status);
> +

Any worry about the ambiguity of whether "status" comes from tk_status or
from nfs4_detect_session_trunking() here?  The latter can return -EINVAL
which isn't in show_nfs4_status().

Otherwise, looks good to me.

Reviewed-by: Benjamin Coddington <[e-mail]>

Ben

Olga Kornievskaia Feb. 2, 2024, 2:42 p.m. UTC | #3

On Fri, Feb 2, 2024 at 8:01 AM Benjamin Coddington <[e-mail]> wrote:
>
> On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:
>
> > From: Olga Kornievskaia <[e-mail]>
> >
> > Add a tracepoint to track when the client sends EXCHANGE_ID to test
> > a new transport for session trunking.
> >
> > Signed-off-by: Olga Kornievskaia <[e-mail]>
> > ---
> >  fs/nfs/nfs4proc.c  |  3 +++
> >  fs/nfs/nfs4trace.h | 30 ++++++++++++++++++++++++++++++
> >  2 files changed, 33 insertions(+)
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 23819a756508..cdda7971c945 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -8974,6 +8974,9 @@ void nfs4_test_session_trunk(struct rpc_clnt *clnt, struct rpc_xprt *xprt,
> >               status = nfs4_detect_session_trunking(adata->clp,
> >                               task->tk_msg.rpc_resp, xprt);
> >
> > +     trace_nfs4_trunked_exchange_id(adata->clp,
> > +                     xprt->address_strings[RPC_DISPLAY_ADDR], status);
> > +
>
> Any worry about the ambiguity of whether "status" comes from tk_status or
> from nfs4_detect_session_trunking() here?  The latter can return -EINVAL
> which isn't in show_nfs4_status().

Good catch, I didn't realize there wasn't an EINVAL mapping. I was
focusing on capturing the fact that exchangeid was happening and ip
info of the trunking connection that I didn't pay attention to the
status. I'll send a v2 with EINVAL added to show_nfs4_status.

> Otherwise, looks good to me.
>
> Reviewed-by: Benjamin Coddington <[e-mail]>
>
> Ben
>

Benjamin Coddington Feb. 2, 2024, 3:36 p.m. UTC | #4

On 2 Feb 2024, at 9:42, Olga Kornievskaia wrote:

> On Fri, Feb 2, 2024 at 8:01 AM Benjamin Coddington <[e-mail]> wrote:
>>
>> On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:
>>
>>> From: Olga Kornievskaia <[e-mail]>
>>>
>>> Add a tracepoint to track when the client sends EXCHANGE_ID to test
>>> a new transport for session trunking.
>>>
>>> Signed-off-by: Olga Kornievskaia <[e-mail]>
>>> ---
>>>  fs/nfs/nfs4proc.c  |  3 +++
>>>  fs/nfs/nfs4trace.h | 30 ++++++++++++++++++++++++++++++
>>>  2 files changed, 33 insertions(+)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 23819a756508..cdda7971c945 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -8974,6 +8974,9 @@ void nfs4_test_session_trunk(struct rpc_clnt *clnt, struct rpc_xprt *xprt,
>>>               status = nfs4_detect_session_trunking(adata->clp,
>>>                               task->tk_msg.rpc_resp, xprt);
>>>
>>> +     trace_nfs4_trunked_exchange_id(adata->clp,
>>> +                     xprt->address_strings[RPC_DISPLAY_ADDR], status);
>>> +
>>
>> Any worry about the ambiguity of whether "status" comes from tk_status or
>> from nfs4_detect_session_trunking() here?  The latter can return -EINVAL
>> which isn't in show_nfs4_status().
>
> Good catch, I didn't realize there wasn't an EINVAL mapping. I was
> focusing on capturing the fact that exchangeid was happening and ip
> info of the trunking connection that I didn't pay attention to the
> status. I'll send a v2 with EINVAL added to show_nfs4_status.

If you're only interested in tk_status, you could just move the tracepoint.
That would resolve the conditional branch that changes the source of
"status".

Ben

Benjamin Coddington Feb. 2, 2024, 4:56 p.m. UTC | #5

On 2 Feb 2024, at 11:47, Olga Kornievskaia wrote:

> On Fri, Feb 2, 2024 at 10:36 AM Benjamin Coddington <[e-mail]>
> wrote:
>
>> On 2 Feb 2024, at 9:42, Olga Kornievskaia wrote:
>>
>>> On Fri, Feb 2, 2024 at 8:01 AM Benjamin Coddington <[e-mail]>
>> wrote:
>>>>
>>>> On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:
>>>>
>>>>> From: Olga Kornievskaia <[e-mail]>
>>>>>
>>>>> Add a tracepoint to track when the client sends EXCHANGE_ID to test
>>>>> a new transport for session trunking.
>>>>>
>>>>> Signed-off-by: Olga Kornievskaia <[e-mail]>
>>>>> ---
>>>>>  fs/nfs/nfs4proc.c  |  3 +++
>>>>>  fs/nfs/nfs4trace.h | 30 ++++++++++++++++++++++++++++++
>>>>>  2 files changed, 33 insertions(+)
>>>>>
>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>> index 23819a756508..cdda7971c945 100644
>>>>> --- a/fs/nfs/nfs4proc.c
>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>> @@ -8974,6 +8974,9 @@ void nfs4_test_session_trunk(struct rpc_clnt
>> *clnt, struct rpc_xprt *xprt,
>>>>>               status = nfs4_detect_session_trunking(adata->clp,
>>>>>                               task->tk_msg.rpc_resp, xprt);
>>>>>
>>>>> +     trace_nfs4_trunked_exchange_id(adata->clp,
>>>>> +                     xprt->address_strings[RPC_DISPLAY_ADDR], status);
>>>>> +
>>>>
>>>> Any worry about the ambiguity of whether "status" comes from tk_status
>> or
>>>> from nfs4_detect_session_trunking() here?  The latter can return -EINVAL
>>>> which isn't in show_nfs4_status().
>>>
>>> Good catch, I didn't realize there wasn't an EINVAL mapping. I was
>>> focusing on capturing the fact that exchangeid was happening and ip
>>> info of the trunking connection that I didn't pay attention to the
>>> status. I'll send a v2 with EINVAL added to show_nfs4_status.
>>
>> If you're only interested in tk_status, you could just move the tracepoint.
>> That would resolve the conditional branch that changes the source of
>> "status".
>>
>
> We are not interested in tk_status that can be gotten from the
> nfs4_xdr_status tracepoint. We are interested in the results of the
> trunking decision.

Gotcha, ok, I understand now.  Tucking into the conditional or moving it into
nfs4_detect_session_trunking() would make that clearer, but no big objection
from me.

If the task returns an error, this tracepoint will still be called with
tk_status instead of any result from the trunking decision.

Ben

Benjamin Coddington Feb. 2, 2024, 6:24 p.m. UTC | #6

On 2 Feb 2024, at 12:45, Olga Kornievskaia wrote:

> On Fri, Feb 2, 2024 at 12:05 PM Olga Kornievskaia <
> [e-mail]> wrote:
>
>>
>>
>> On Fri, Feb 2, 2024 at 11:56 AM Benjamin Coddington <[e-mail]>
>> wrote:
>>
>>> On 2 Feb 2024, at 11:47, Olga Kornievskaia wrote:
>>>
>>>> On Fri, Feb 2, 2024 at 10:36 AM Benjamin Coddington <
>>> [e-mail]>
>>>> wrote:
>>>>
>>>>> On 2 Feb 2024, at 9:42, Olga Kornievskaia wrote:
>>>>>
>>>>>> On Fri, Feb 2, 2024 at 8:01 AM Benjamin Coddington <
>>> [e-mail]>
>>>>> wrote:
>>>>>>>
>>>>>>> On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:
>>>>>>>
>>>>>>>> From: Olga Kornievskaia <[e-mail]>
>>>>>>>>
>>>>>>>> Add a tracepoint to track when the client sends EXCHANGE_ID to test
>>>>>>>> a new transport for session trunking.
>>>>>>>>
>>>>>>>> Signed-off-by: Olga Kornievskaia <[e-mail]>
>>>>>>>> ---
>>>>>>>>  fs/nfs/nfs4proc.c  |  3 +++
>>>>>>>>  fs/nfs/nfs4trace.h | 30 ++++++++++++++++++++++++++++++
>>>>>>>>  2 files changed, 33 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>>>>> index 23819a756508..cdda7971c945 100644
>>>>>>>> --- a/fs/nfs/nfs4proc.c
>>>>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>>>>> @@ -8974,6 +8974,9 @@ void nfs4_test_session_trunk(struct rpc_clnt
>>>>> *clnt, struct rpc_xprt *xprt,
>>>>>>>>               status = nfs4_detect_session_trunking(adata->clp,
>>>>>>>>                               task->tk_msg.rpc_resp, xprt);
>>>>>>>>
>>>>>>>> +     trace_nfs4_trunked_exchange_id(adata->clp,
>>>>>>>> +                     xprt->address_strings[RPC_DISPLAY_ADDR],
>>> status);
>>>>>>>> +
>>>>>>>
>>>>>>> Any worry about the ambiguity of whether "status" comes from
>>> tk_status
>>>>> or
>>>>>>> from nfs4_detect_session_trunking() here?  The latter can return
>>> -EINVAL
>>>>>>> which isn't in show_nfs4_status().
>>>>>>
>>>>>> Good catch, I didn't realize there wasn't an EINVAL mapping. I was
>>>>>> focusing on capturing the fact that exchangeid was happening and ip
>>>>>> info of the trunking connection that I didn't pay attention to the
>>>>>> status. I'll send a v2 with EINVAL added to show_nfs4_status.
>>>>>
>>>>> If you're only interested in tk_status, you could just move the
>>> tracepoint.
>>>>> That would resolve the conditional branch that changes the source of
>>>>> "status".
>>>>>
>>>>
>>>> We are not interested in tk_status that can be gotten from the
>>>> nfs4_xdr_status tracepoint. We are interested in the results of the
>>>> trunking decision.
>>>
>>> Gotcha, ok, I understand now.  Tucking into the conditional or moving it
>>> into
>>> nfs4_detect_session_trunking() would make that clearer, but no big
>>> objection
>>> from me.
>>>
>>> If the task returns an error, this tracepoint will still be called with
>>> tk_status instead of any result from the trunking decision.
>>>
>>
>> :) I'll send a v3. though I don't see my v2 posted but i did send it
>> already.
>>
>
> Well looks like I'm not longer able to submit patches because of something
> new fancy policies. I have used the same gitemail from mac as v1 and now my
> emails are rejected. I use Mac's git sendmail command.
>
> Ben, how do you submit to your patches, do you use Mac git's or submit
> through something else? Thank you for the help.

I use git send-email from linux and I relay through redhat.com's smtp
servers, which are generally well-trusted.

Your v2 made it to me and the archives:
https://lore.kernel.org/linux-nfs/[e-mail]/T/#u


Ben

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 23819a756508..cdda7971c945 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -8974,6 +8974,9 @@  void nfs4_test_session_trunk(struct rpc_clnt *clnt, struct rpc_xprt *xprt,
 		status = nfs4_detect_session_trunking(adata->clp,
 				task->tk_msg.rpc_resp, xprt);
 
+	trace_nfs4_trunked_exchange_id(adata->clp,
+			xprt->address_strings[RPC_DISPLAY_ADDR], status);
+
 	if (status == 0)
 		rpc_clnt_xprt_switch_add_xprt(clnt, xprt);
 	else if (status != -NFS4ERR_DELAY && rpc_clnt_xprt_switch_has_addr(clnt,
diff --git a/fs/nfs/nfs4trace.h b/fs/nfs/nfs4trace.h
index 713d080fd268..7d9cb980865d 100644
--- a/fs/nfs/nfs4trace.h
+++ b/fs/nfs/nfs4trace.h
@@ -77,6 +77,36 @@  DEFINE_NFS4_CLIENTID_EVENT(nfs4_bind_conn_to_session);
 DEFINE_NFS4_CLIENTID_EVENT(nfs4_sequence);
 DEFINE_NFS4_CLIENTID_EVENT(nfs4_reclaim_complete);
 
+TRACE_EVENT(nfs4_trunked_exchange_id,
+		TP_PROTO(
+			const struct nfs_client *clp,
+			const char *addr,
+			int error
+		),
+
+		TP_ARGS(clp, addr, error),
+
+		TP_STRUCT__entry(
+			__string(main_addr, clp->cl_hostname)
+			__string(trunk_addr, addr)
+			__field(unsigned long, error)
+		),
+
+		TP_fast_assign(
+			__entry->error = error < 0 ? -error : 0;
+			__assign_str(main_addr, clp->cl_hostname);
+			__assign_str(trunk_addr, addr);
+		),
+
+		TP_printk(
+			"error=%ld (%s) main_addr=%s trunk_addr=%s",
+			-__entry->error,
+			show_nfs4_status(__entry->error),
+			__get_str(main_addr),
+			__get_str(trunk_addr)
+		)
+);
+
 TRACE_EVENT(nfs4_sequence_done,
 		TP_PROTO(
 			const struct nfs4_session *session,