Received-SPF: pass (google.com: domain of linux-kernel+bounces-133497-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 5 Apr 2024 15:03:32 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Neil Brown <neilb@suse.de>, Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] nfsd: new tracepoint for check_slot_seqid
Message-ID: <ZhBLBEIgKxChNBAY@tissot.1015granger.net>
References: <20240405-nfsd-fixes-v1-0-e017bfe9a783@kernel.org>
 <20240405-nfsd-fixes-v1-2-e017bfe9a783@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240405-nfsd-fixes-v1-2-e017bfe9a783@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?9SmxOK7MblMZ/bGm9jx0bOGKJwWg4pCpy5vliBV7A+BOh1FmZPpH9rdrrsra?=
 =?us-ascii?Q?V0+hOSWa9EXinsiNHxH7ZjrnnXsC33YgJccf0M/CykxjgMzAhuvawlNrrQTN?=
 =?us-ascii?Q?owl1WktMy1vnoxt3u7sHJoxUpEsDyz5PSkXu0rDqwsQxYfeU1Da74qP4iZ8T?=
 =?us-ascii?Q?5hl74W9s5ZwUVjRTXwoIrQt8Xl2LpYT5UIv7sE4xA40+JCqSHBHDY95PpKRx?=
 =?us-ascii?Q?wwhubsUqSOpBP6H+LQBrPx3zYYq2NZTIOyaUgOb0ogowsxhvaP1PwYvU8Wof?=
 =?us-ascii?Q?bVQ5bG7n54/4GUwjRgvjNVDHRwpBbILJOzgULw98BM6Xl4b08o/f5Yz0DLYN?=
 =?us-ascii?Q?HrqDCYBkD4gIcsElbnONgw7gwlxfEfL7EJ0u4+aRYEGFCU7XCQjlPvB9Z/5N?=
 =?us-ascii?Q?QH2wvnA1d90bsoNH9dNzzi2zhyVUfakdfHw5z/BXaGjErkixMPBKeCu1MA1i?=
 =?us-ascii?Q?zIhw/xzalYvjKajEj2djLZrZOVFQrPwn8x4iPbeBpjM1PMBZxz7R/lCDa8mt?=
 =?us-ascii?Q?fEIe3gyNJi5SIUuxOK98+GEHF/FQb0xqHItGPreVgUPwmz67VjV5pL5Ur8ze?=
 =?us-ascii?Q?gIDmL8Y6R9YFX1vlaZm9GoxlxMuy6YGALWE2fSF3bSVVSE2bLDLIZpFUn1eA?=
 =?us-ascii?Q?kZdcuF2VtH+pPsOuVS6A+lnCsoCOKikoyLk8qPGGaMtvkfczTYJTVpuALdSP?=
 =?us-ascii?Q?Bc1RiCFXHheAROf1BqtxICUzcZ5/HUda0adI01FHCIbgQ5BwruJiHZ56Pkjk?=
 =?us-ascii?Q?4nfZc5TFWHpOrOvdrcKTDtUufsV2K8QaGbsRVGZga5ZWEUvlYG8YeohgyUjt?=
 =?us-ascii?Q?0WMvUcBlliaFpYZK8fX83FeiPV89p165G5aVuUM8DTNIzofOaEVXYBsheb8w?=
 =?us-ascii?Q?46yrqlR2oYoelEEsSM4+Gg4JeOItyXK1in3E77xrUGbdQxv7m9m3E4aHhh2c?=
 =?us-ascii?Q?MqMbzAy3ge49Qq5znHkBuGLDolApVdsn4Cfi7hFKnIfpTOekp7TBKUUOfGMC?=
 =?us-ascii?Q?1GSqZPdB91M7TpqSCjFuFQeOQnP8wUjcG3sN6/+z9UU0iuSSvWbl35NtMMRA?=
 =?us-ascii?Q?yK1iD3jzmaxDI23M07ta+ZXP0FvEjSFmpoDNUaANnbMzPPWXNp1VIx6uLJDP?=
 =?us-ascii?Q?63AAZm2zrQ/PdjEARjGkj+2/tCQ38VAyok38un406agT00mj/Mb/cBFj9C10?=
 =?us-ascii?Q?8tsg4M+xhiDdYQSjb9cW7oAhg2XyQ15bVT/vbhbPOpk964SJVBRXF2ylIl11?=
 =?us-ascii?Q?Cw95wR3CP2c0YeWdNYd24PYuUMNhiK5XVgwsBUV4W+bfjQJquRUbuBJNq+Di?=
 =?us-ascii?Q?ARdIkdyt+85ZF4c9VL1MmQgziDV/Pm4/GIOVlErONvGwEjs367Z+uCek9P+s?=
 =?us-ascii?Q?X3IVtkWqpslJcXVDyBb8FK5Ewwf79J+IDsuIZTJGNVPLP6Y8ugN1oaemhpYd?=
 =?us-ascii?Q?fnVGD+63Ezrc+di1uPbY+/kO7vHXDci2ey6rGPUceOSAVIvamYfNG4iZ8x+6?=
 =?us-ascii?Q?ZeufaveIVaZoL0Zc6Yw5A9uUyJsFqmM1Gu8AtjaOFUOr0Mgye2VmSN0heQC1?=
 =?us-ascii?Q?CAVS0eizc+95CtaHfBXDQFB6Jj/nmcTtJsOIgVAlAh3xB+WZU2FPGoTXVxnE?=
 =?us-ascii?Q?kQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	MKeHj/vb5zlyouNUv1SVf4k5viCp76qxhW/bcZsTodPS2dx6R7+7v4pBKoPsg+eyLJYpunhwMhDf3bMbDchA97fjTsrtbGwc3lRkXJxACD9nG//dr7jkk42dkbVMcYRvJN6cmvq/dsJPZP+NPit9r7bN/0ZnuL0zFdNZJGS1sII4mb9Z/OUFtags99AdDIFO1Hx37gvVORpMHfnNr9jaE8UkbA5xXXojXOu1S6iUkVkkg8SGXss0t2/yPdAJopGYerdERPkOnkwzgR+FsPNPRCzE0hJPrJB//IsdQtzLKB8iI2eA42aMfX0oLR10ghVmDeF4GIUrI2mDsSXQw3I0T5q7YRJTnd+4F3dFWfOn79VzwpD0EBQCBgZRdkaJ23aH3jErHLFGC7nEREATWJOUq8/Cbv3L7ctmM008AKWZJrDOrnHJcYhMVtUoWPyquFtCu7a7f7/bZiw5HprO0SSdFN8iNIf9fsSX0F4MrYZVZdiDq3qE0KfmcU2Nv8VXI9lOPcyCi1a50Tez/1WfgZP8HoKPiDC4CzAzTpWWDZSeg146l3klJdT4ozaa6bu6avZQlN1ZbqBxpBzU8my9WU9AELgXnTtOSDmJvAzu4YsPlxM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 40856b4c-5570-4773-48da-08dc55a31807
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Apr 2024 19:03:35.2029
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: sEyQVvjSrGzT7K3B9v6k3MBJimEfJqwn85IfxtVBKxcfGyvvTeZzMKKNA5EeRAU5R4fesRIvoW2sQwo7gx1/vg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4736
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-05_21,2024-04-05_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 spamscore=0 adultscore=0
 malwarescore=0 phishscore=0 mlxlogscore=890 bulkscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2404050135
X-Proofpoint-GUID: aQoGyFOx-UbkGnWJDp3Q9LTiX7Sfzev1
X-Proofpoint-ORIG-GUID: aQoGyFOx-UbkGnWJDp3Q9LTiX7Sfzev1

On Fri, Apr 05, 2024 at 02:40:50PM -0400, Jeff Layton wrote:
> Replace a dprintk in check_slot_seqid with a new tracepoint.  Add a
> nfs4_client argument to check_slot_seqid so that we can pass the
> appropriate info to the tracepoint.
> 
> Signed-off-by: Jeffrey Layton <jlayton@redhat.com>
> ---
>  fs/nfsd/nfs4state.c | 12 ++++++------
>  fs/nfsd/trace.h     | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3cef81e196c6..5891bc3e2b0b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3642,10 +3642,9 @@ nfsd4_exchange_id(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  }
>  
>  static __be32
> -check_slot_seqid(u32 seqid, u32 slot_seqid, int slot_inuse)
> +check_slot_seqid(struct nfs4_client *clp, u32 seqid, u32 slot_seqid, bool slot_inuse)
>  {
> -	dprintk("%s enter. seqid %d slot_seqid %d\n", __func__, seqid,
> -		slot_seqid);
> +	trace_check_slot_seqid(clp, seqid, slot_seqid, slot_inuse);

Getting rid of the dprintk: +1

Tracing slot seqid checks: +1

But I'd like something a little different for the tracepoint
itself. I can make these changes if you like to just hand this off.

Let's make this trace point into three separate trace events, one
at the nfsd4_sequence check_slot_seqid() call site, and two in
nfsd4_create_session(), like below.

Two reasons for this change:

1. Separate tracepoints in nfsd4_create_session will show whether
   the client is confirmed or not

2. The tracepoint in nfsd4_sequence will normally be noisy, so
   having a separate tracepoint for that case makes it easy to
   disable that one while leaving the create_session tracepoints
   enabled.

And, bonus: you won't have to change the synopsis of
check_slot_seqid().


>  	/* The slot is in use, and no response has been sent. */
>  	if (slot_inuse) {
> @@ -3827,7 +3826,8 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>  		cs_slot = &conf->cl_cs_slot;
		trace_nfsd_slot_seqid_confirmed
>  	else
>  		cs_slot = &unconf->cl_cs_slot;
		trace_nfsd_slot_seqid_unconfirmed
> -	status = check_slot_seqid(cr_ses->seqid, cs_slot->sl_seqid, 0);
> +	status = check_slot_seqid(conf ? conf : unconf, cr_ses->seqid,
> +				  cs_slot->sl_seqid, false);
>  	switch (status) {
>  	case nfs_ok:
>  		cs_slot->sl_seqid++;
> @@ -4221,8 +4221,8 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	 * sr_highest_slotid and the sr_target_slot id to maxslots */
>  	seq->maxslots = session->se_fchannel.maxreqs;
>  
	trace_nfsd_slot_seqid_sequence
> -	status = check_slot_seqid(seq->seqid, slot->sl_seqid,
> -					slot->sl_flags & NFSD4_SLOT_INUSE);
> +	status = check_slot_seqid(clp, seq->seqid, slot->sl_seqid,
> +				  slot->sl_flags & NFSD4_SLOT_INUSE);
>  	if (status == nfserr_replay_cache) {
>  		status = nfserr_seq_misordered;
>  		if (!(slot->sl_flags & NFSD4_SLOT_INITIALIZED))
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 7f1a6d568bdb..ec00ca7ecfc8 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -1542,6 +1542,40 @@ TRACE_EVENT(nfsd_cb_seq_status,
>  	)
>  );
>  
> +TRACE_EVENT(check_slot_seqid,

Nit: NFSD tracepoint names should start with "nfsd_".


> +	TP_PROTO(
> +		const struct nfs4_client *clp,
> +		u32 seqid,
> +		u32 slot_seqid,
> +		bool inuse
> +	),
> +	TP_ARGS(clp, seqid, slot_seqid, inuse),
> +	TP_STRUCT__entry(
> +		__field(u32, seqid)
> +		__field(u32, slot_seqid)
> +		__field(u32, cl_boot)
> +		__field(u32, cl_id)
> +		__sockaddr(addr, clp->cl_cb_conn.cb_addrlen)
> +		__field(bool, conf)
> +		__field(bool, inuse)
> +	),
> +	TP_fast_assign(
> +		__entry->cl_boot = clp->cl_clientid.cl_boot;
> +		__entry->cl_id = clp->cl_clientid.cl_id;
> +		__assign_sockaddr(addr, &clp->cl_cb_conn.cb_addr,
> +				  clp->cl_cb_conn.cb_addrlen);
> +		__entry->seqid = seqid;
> +		__entry->slot_seqid = slot_seqid;
> +		__entry->conf = test_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags);
> +		__entry->inuse = inuse;
> +	),
> +	TP_printk("addr=%pISpc %s client %08x:%08x seqid=%u slot_seqid=%u inuse=%d",
> +		__get_sockaddr(addr), __entry->conf ? "conf" : "unconf",
> +		__entry->cl_boot, __entry->cl_id,
> +		__entry->seqid, __entry->slot_seqid, __entry->inuse

Nit: How about: __entry->in_use ? "(in use)" : "(not in use)"

Since TP_printk is for human readers.


> +	)
> +);
> +
>  TRACE_EVENT(nfsd_cb_free_slot,
>  	TP_PROTO(
>  		const struct rpc_task *task,
> 
> -- 
> 2.44.0
> 

-- 
Chuck Lever