Received-SPF: pass (google.com: domain of linux-nfs+bounces-1717-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Benjamin Coddington <bcodding@redhat.com>
To: Olga Kornievskaia <olga.kornievskaia@gmail.com>
Cc: trond.myklebust@hammerspace.com, anna.schumaker@netapp.com,
 linux-nfs@vger.kernel.org
Subject: Re: [PATCH 1/1] NFSv4.1: add tracepoint to trunked nfs4_exchange_id
 calls
Date: Fri, 02 Feb 2024 11:56:29 -0500
Message-ID: <A60265EE-8564-4772-A035-916F88C82107@redhat.com>
In-Reply-To: <CAN-5tyHjkWsJPteVvLh83X3YTsvvS8vDfHBny-LMaAVbx0ww1g@mail.gmail.com>
References: <20240126190333.13528-1-olga.kornievskaia@gmail.com>
 <1DBC4AE1-E253-454F-9E7E-12DFBA14EBA6@redhat.com>
 <CAN-5tyGDQaaud7emd-Sgx_0E31bLx0k6EgrONSp0SbfoMTwY9A@mail.gmail.com>
 <D23CBB93-3883-4E41-A1E5-848BCB2D477E@redhat.com>
 <CAN-5tyHjkWsJPteVvLh83X3YTsvvS8vDfHBny-LMaAVbx0ww1g@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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

> On Fri, Feb 2, 2024 at 10:36 AM Benjamin Coddington <bcodding@redhat.com>
> wrote:
>
>> On 2 Feb 2024, at 9:42, Olga Kornievskaia wrote:
>>
>>> On Fri, Feb 2, 2024 at 8:01 AM Benjamin Coddington <bcodding@redhat.com>
>> wrote:
>>>>
>>>> On 26 Jan 2024, at 14:03, Olga Kornievskaia wrote:
>>>>
>>>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>>>
>>>>> Add a tracepoint to track when the client sends EXCHANGE_ID to test
>>>>> a new transport for session trunking.
>>>>>
>>>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>>>> ---
>>>>>  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