From: Olga Kornievskaia <[email protected]>
Add a tracepoint to track when the client sends EXCHANGE_ID to test
a new transport for session trunking.
Signed-off-by: Olga Kornievskaia <[email protected]>
---
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);
+
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,
--
2.39.1
> -----?ʼ?ԭ??-----
> ??????: Olga Kornievskaia <[email protected]>
> ????ʱ??: 2024??1??27?? 3:04
> ?ռ???: [email protected]; [email protected]
> ????: [email protected]
> ????: [PATCH 1/1] NFSv4.1: add tracepoint to trunked nfs4_exchange_id calls
>
> From: Olga Kornievskaia <[email protected]>
>
> Add a tracepoint to track when the client sends EXCHANGE_ID to test a new
> transport for session trunking.
>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 3 +++
> fs/nfs/nfs4trace.h | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 33 insertions(+)
>
Tested-by: Chen Hanxiao <[email protected]>
Regards,
- Chen
On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> Add a tracepoint to track when the client sends EXCHANGE_ID to test
> a new transport for session trunking.
>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
> 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 <[email protected]>
Ben
On Fri, Feb 2, 2024 at 8:01 AM Benjamin Coddington <[email protected]> wrote:
>
> On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:
>
> > From: Olga Kornievskaia <[email protected]>
> >
> > Add a tracepoint to track when the client sends EXCHANGE_ID to test
> > a new transport for session trunking.
> >
> > Signed-off-by: Olga Kornievskaia <[email protected]>
> > ---
> > 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 <[email protected]>
>
> Ben
>
On 2 Feb 2024, at 9:42, Olga Kornievskaia wrote:
> On Fri, Feb 2, 2024 at 8:01 AM Benjamin Coddington <[email protected]> wrote:
>>
>> On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:
>>
>>> From: Olga Kornievskaia <[email protected]>
>>>
>>> Add a tracepoint to track when the client sends EXCHANGE_ID to test
>>> a new transport for session trunking.
>>>
>>> Signed-off-by: Olga Kornievskaia <[email protected]>
>>> ---
>>> 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
On 2 Feb 2024, at 11:47, Olga Kornievskaia wrote:
> On Fri, Feb 2, 2024 at 10:36 AM Benjamin Coddington <[email protected]>
> wrote:
>
>> On 2 Feb 2024, at 9:42, Olga Kornievskaia wrote:
>>
>>> On Fri, Feb 2, 2024 at 8:01 AM Benjamin Coddington <[email protected]>
>> wrote:
>>>>
>>>> On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:
>>>>
>>>>> From: Olga Kornievskaia <[email protected]>
>>>>>
>>>>> Add a tracepoint to track when the client sends EXCHANGE_ID to test
>>>>> a new transport for session trunking.
>>>>>
>>>>> Signed-off-by: Olga Kornievskaia <[email protected]>
>>>>> ---
>>>>> 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
On 2 Feb 2024, at 12:45, Olga Kornievskaia wrote:
> On Fri, Feb 2, 2024 at 12:05 PM Olga Kornievskaia <
> [email protected]> wrote:
>
>>
>>
>> On Fri, Feb 2, 2024 at 11:56 AM Benjamin Coddington <[email protected]>
>> wrote:
>>
>>> On 2 Feb 2024, at 11:47, Olga Kornievskaia wrote:
>>>
>>>> On Fri, Feb 2, 2024 at 10:36 AM Benjamin Coddington <
>>> [email protected]>
>>>> wrote:
>>>>
>>>>> On 2 Feb 2024, at 9:42, Olga Kornievskaia wrote:
>>>>>
>>>>>> On Fri, Feb 2, 2024 at 8:01 AM Benjamin Coddington <
>>> [email protected]>
>>>>> wrote:
>>>>>>>
>>>>>>> On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:
>>>>>>>
>>>>>>>> From: Olga Kornievskaia <[email protected]>
>>>>>>>>
>>>>>>>> Add a tracepoint to track when the client sends EXCHANGE_ID to test
>>>>>>>> a new transport for session trunking.
>>>>>>>>
>>>>>>>> Signed-off-by: Olga Kornievskaia <[email protected]>
>>>>>>>> ---
>>>>>>>> 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/[email protected]/T/#u
Ben