Received-SPF: pass (google.com: domain of linux-nfs+bounces-1465-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 26 Jan 2024 09:12:43 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Benjamin Coddington <bcodding@redhat.com>
Cc: Chuck Lever <cel@kernel.org>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH RFC 07/13] NFSD: Add callback operation lifetime trace
 points
Message-ID: <ZbO92zZkNpRomQqq@tissot.1015granger.net>
References: <170619984210.2833.7173004255003914651.stgit@manet.1015granger.net>
 <170620016455.2833.5426224225062159088.stgit@manet.1015granger.net>
 <86789B68-0271-4AEB-9941-CFDB956E84EE@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <86789B68-0271-4AEB-9941-CFDB956E84EE@redhat.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?QlAKyKYgG36sItbW5BBrQ2gE3U7CjIWwBYMjqa7EzJb/SCcccpbz/zg8Rxtj?=
 =?us-ascii?Q?0Qy7rMkHFkxG1wkeoCmFD/HBshpvko1BuXuL5lmY6VuPs/XzCacMAudjLhT8?=
 =?us-ascii?Q?8I8SRDa/pNhI+cmG9Qs+e8e1d9/Q3kD9bGtCjjpFjYldegd3r2dRqqE31HjF?=
 =?us-ascii?Q?s7Anq0HlfIkSZOSwZ7yvg5xSdOvI+dDUH46dN/1NkZoJPNn1QqP3cobd0Idh?=
 =?us-ascii?Q?bk8xQxnC8R/s5IxPXPkrZQcY1cvBkfTtu/ZUy71sebWR8amH48fKtgQqssDN?=
 =?us-ascii?Q?NWpELB8RRJ0NZGv514v2FKpje2rQuxy3kZpG+Y95mopKU9JRmMfg4ri9Ca80?=
 =?us-ascii?Q?YCdHhcutb3V7Pco4QRVH/t58p9EII6kuZPGkHTM5p1LgdLL4FgNuKnzWMN2B?=
 =?us-ascii?Q?x3r2rgyVH3P003vEbtZHJ7e5u7cCM7VS5/qqgo+S+xpYLhRFnLmSc3oYC+0K?=
 =?us-ascii?Q?UvcKxUqMkKT8RJSpLVO4rBEWCCscpijoXeWdNuKUkh7geeELxGBt5WUys+g/?=
 =?us-ascii?Q?/2vjPaYE88ZbPKVM6rrMpFB+hbPjjnmxcJe+EmkI6pY0SQii3xN8tn+uqr4M?=
 =?us-ascii?Q?pN+YTsFw9JckCg6HEfe6Z7YWfuR7HExzTeBUbaB/b4gCL/rlOqtf7T7kM8IC?=
 =?us-ascii?Q?j1TmIx25c0ttXX7J7+wMi7mq/dwAYWmzyVSaVScZ8YgzrVqxpsiDwZm71ntO?=
 =?us-ascii?Q?QnQXA2hXT0FucjVDWz0zq7i+cW9xkMQtDJxsT4coaXvC2y3mNqjAGkajpt2i?=
 =?us-ascii?Q?e9KvufTttETWL3LswtwP3H5IScZ1lWj9QYBlqhe1RinB53G4p5CDqmg87Rlv?=
 =?us-ascii?Q?Fx0S7SF98O5qKv+vJ9jI856WgzIQaNJXPweOO7OMkqp5SU9OSIvPuHhM7Wls?=
 =?us-ascii?Q?cYC+DrZXqyTxDcIC+UdSkTyCPPRh5GbiPwrGs+TTirOfawdXL6LtwlzHqJV6?=
 =?us-ascii?Q?bVLTb5uDOfvvOAhGI4h/0GtWKKPu+/jsXe5JtypybITF1IxvAVQPi17v71YW?=
 =?us-ascii?Q?VJeyCp68DuqqSdoRkka92m6+iQ/A/yGtY+h6OMXoWIFFdmPblt+rjmkFbppY?=
 =?us-ascii?Q?SCIQnrhZBkG9dC5JJ8SnWgudiL3AaBsSHHQ2OsLvwjxaimFkaQPXEexCZdFd?=
 =?us-ascii?Q?ii6+wKLBnMx19KdPE6TMF8WjGjJLwOc1QNyMudo9DYVKbdX193lh8GbGXYwa?=
 =?us-ascii?Q?fewnVftwnR1538RzX2PX0TCAZNAxaFPQPd8X0V4Jp/69UTvvkvDCzZTMOmb5?=
 =?us-ascii?Q?7UCz5wvwHzvQY+RJxJ+EIIssijeUKV5oPqdJfP4T4f9fWVXshcvSHbcd94IT?=
 =?us-ascii?Q?0wdXfwZQfJo3fbEsjiTMq5fH3f7FH4yx+Tt1gYf5UuSzggMpdgepANLFIUS6?=
 =?us-ascii?Q?Zf9myikSNk1IX6yv30xW80vvTQ/IeVlHfZoRiNSME7Hi/rvIiS6mYmxZZs6N?=
 =?us-ascii?Q?bTpB9taGCLKULRx4RsD7hPRKAcN7AYnzXFExzeJ/G832htAtlrZMZlMQ+FFz?=
 =?us-ascii?Q?Pla3Wh3nyCof9gSsoPy38kUohFmS9NCwVJfXC6AekjI+OEvigGIvGN0olBEQ?=
 =?us-ascii?Q?nweKmJUb7shXN85KyQ/WJsCrEZVNxVjUffeJ3e6Oo2FxLxDIfdwyJPfMviJ9?=
 =?us-ascii?Q?Qg=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	jHimZjoS/V1iyiVmKUyB8ViVpu/+ud3XZkA1cNNHTlcpKIr0RUgAF5JvjiRmuqzp6Z4NytLM40d4r5FuNisrIxvtvmfcfbGBlslRfPfmhakIL2loxfKlzrpdu4dNJt+AN0kxPf7pgpxsnbZuDjKxxhd5WisrGZG7CF5067+brXMZ2qqg+a5ea21WFRWTqD3jSpA4aMkMnPOuagUEcK1R77c5V/tvDdcU4iofs3OuzGHoSpq2/U9K3Hgmwb+bBbUnmbVjnxWCUBNXD2I2Zb+Q4yu8xRu0izT5No35OExPHJGL4Uj7uNfaGSjHf+XHkmH7fQdPSshgCnyUp2LHDkTBT8ADL14uISfF3juaUU5LEiuyIqHPRH/JrWOfj7F/w8Lyq0G95ZKwiQaMh0cC0zLbUhxJlN2fbWUZW6bYW7riKaqodMOYYFXTfgXpAkE8kUGeQHA4pDOHSpMttNNPTlllCDRKr0A45EQpEdmkalcWVdCwfakqd4Hjx2CAMZa+D8J0kp6bhtvGe8n18ZPYXGJcxrpqhhBX21CPINdTtkbwqiWqjxOaHfoL/HEl3QJ8g4odIv+dG85gN0MrWhwh391A4ASvSWX7qceRNpHVMHsP6ZA=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 84ff6e40-afaf-4e71-2dda-08dc1e78de80
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2024 14:12:45.8342
 (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: p4D0M5KokeJ6A6yXbkAb7h5l5XLmV2hIe/lAgYfDJ0KcqX/LP4dqSjvkvcUaSlxEwpvCKRVuCa2KHMYH1b22xQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB7117
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-01-25_14,2024-01-25_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=684
 malwarescore=0 mlxscore=0 phishscore=0 bulkscore=0 suspectscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2401260104
X-Proofpoint-ORIG-GUID: gix2UHX3JoUPUaV9CpBj6-9-SKBqRPc3
X-Proofpoint-GUID: gix2UHX3JoUPUaV9CpBj6-9-SKBqRPc3

On Thu, Jan 25, 2024 at 04:49:17PM -0500, Benjamin Coddington wrote:
> On 25 Jan 2024, at 11:29, Chuck Lever wrote:
> 
> > From: Chuck Lever <chuck.lever@oracle.com>
> >
> > Help observe the flow of callback operations.
> >
> > bc_shutdown() records exactly when the backchannel RPC client is
> > destroyed and cl_cb_client is replaced with NULL.
> >
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfs4callback.c   |    7 +++++++
> >  fs/nfsd/trace.h          |   42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/trace/misc/nfs.h |   34 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 83 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index 1c85426830b1..4d5a6370b92c 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -887,6 +887,7 @@ static struct workqueue_struct *callback_wq;
> >
> >  static bool nfsd4_queue_cb(struct nfsd4_callback *cb)
> >  {
> > +	trace_nfsd_cb_queue(cb->cb_clp, cb);
> >  	return queue_work(callback_wq, &cb->cb_work);
> >  }
> >
> > @@ -1106,6 +1107,7 @@ static void nfsd41_destroy_cb(struct nfsd4_callback *cb)
> >  {
> >  	struct nfs4_client *clp = cb->cb_clp;
> >
> > +	trace_nfsd_cb_destroy(clp, cb);
> >  	nfsd41_cb_release_slot(cb);
> >  	if (cb->cb_ops && cb->cb_ops->release)
> >  		cb->cb_ops->release(cb);
> > @@ -1220,6 +1222,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
> >  	goto out;
> >  need_restart:
> >  	if (!test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) {
> > +		trace_nfsd_cb_restart(clp, cb);
> >  		task->tk_status = 0;
> >  		cb->cb_need_restart = true;
> 
> I think you want to call the tracepoint /after/ setting cb_need_restart here..

Maybe?

The trace point currently captures the value of cb_need_restart
before this logic overwrites it. Is that beneficial for
troubleshooting?


> >  	}
> > @@ -1333,11 +1336,14 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
> >  	struct nfsd4_conn *c;
> >  	int err;
> >
> > +	trace_nfsd_cb_bc_update(clp, cb);
> > +
> >  	/*
> >  	 * This is either an update, or the client dying; in either case,
> >  	 * kill the old client:
> >  	 */
> >  	if (clp->cl_cb_client) {
> > +		trace_nfsd_cb_bc_shutdown(clp, cb);
> >  		rpc_shutdown_client(clp->cl_cb_client);
> >  		clp->cl_cb_client = NULL;
> >  		put_cred(clp->cl_cb_cred);
> > @@ -1349,6 +1355,7 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
> >  	}
> >  	if (test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags))
> >  		return;
> > +
> 
> I'm in favor of this whitespace change, but did you mean to include it?

Yes, but it might be better off in one of the other patches.


-- 
Chuck Lever