Received-SPF: pass (google.com: domain of linux-nfs+bounces-938-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 4 Jan 2024 10:33:16 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Benjamin Coddington <bcodding@redhat.com>
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>, Jeff Layton <jlayton@kernel.org>,
        linux-nfs@vger.kernel.org
Subject: Re: [PATCH v4 1/2] SUNRPC: Fixup v4.1 backchannel request timeouts
Message-ID: <ZZbPvP2NN3mvUtvp@tissot.1015granger.net>
References: <e28038fba1243f00b0dd66b7c5296a1e181645ea.1704379780.git.bcodding@redhat.com>
 <ZZbKRp3oe1lptDvf@tissot.1015granger.net>
 <FBF6D528-DD7C-40C3-85DC-8D48B98990A8@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <FBF6D528-DD7C-40C3-85DC-8D48B98990A8@redhat.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?vXggERX0WU/FFkRi2tPI/7zTpGNel8SN9WS/ebN3/G23FTf2Ty+uS3k7YS+Y?=
 =?us-ascii?Q?vLwdmHklkfT/VfZgEWV0juNKj01M0tFyHf2dPdJf/030wjtkytcV/EtaQ4O/?=
 =?us-ascii?Q?z4y80I0XQgaK8Pa0io8xcrq7156RTWM05O6z5M907U+LPQI5E3gdGyKJ+ZqT?=
 =?us-ascii?Q?8g2JHnaUDIr66CqfO7+3TpRQYkaJv3hZIVHwRlnG3+pbOZB4Wph6CSJ4+H9/?=
 =?us-ascii?Q?whkQpChbZvtavLVHkedSyUwlfPT0QLTsjXWxcWxLae5/K31q79NmRNzzfT0Z?=
 =?us-ascii?Q?WMA4FzqRbMI+Jiou/LSlH7/SBms2NiEfcCPkZ3hYCh1j1Pm3nHNaHTgyz8Va?=
 =?us-ascii?Q?38j5GxeKUI3c1k9a7Wqomx0pFyfFb0RTzoB6i4wnvOhWo1DJfH+6ZaqEYIOF?=
 =?us-ascii?Q?ukjh6AOOK++hFpA4JMLDZWnM6INZuVhL9yA6Y+AA+cebMbrfR2JtqJWUnMsY?=
 =?us-ascii?Q?5S1I+ogqyo11pcsBNYB28Z2HMr/uxKnYDKNlLSMhRTw98/BV1G6/uJbPQm5p?=
 =?us-ascii?Q?JoQOvzpSRNYhXCYk/xZja+ScSpK6wNrGgGkBV52FMCpa0lPu77mnLhS+xAKJ?=
 =?us-ascii?Q?loRlecNGJiFJpyjb90PIldzuu1iFmuXHICpR5P5fTteESM+34DFyfCR0URun?=
 =?us-ascii?Q?+MzLQxzidQh5LCs9+miDhh+cs747x55/lCEBgP8whvFI7tzXnBLOrWmDb0Al?=
 =?us-ascii?Q?NHYMchOzRQTdCYQdEj/csktc/JwJf7hw+FhrnQjB42P1FL/KqxZwTnwfXFBy?=
 =?us-ascii?Q?CxR2/vDuxviy/dzItDPJ997NlQ4hWa8F5Ribko282GxH+EDEE2Mg5NkS7GJ4?=
 =?us-ascii?Q?wHmrpnOjtNAUHLLKuWNPix22nreTwPxqRJ2t6jUn7XfOiuMPbbvvJAiYEpHP?=
 =?us-ascii?Q?gBYdd2fDbDkYdWETD0CXJ7eIPSXMf6jdfmy/UrNttdczvF5IAqPZYToLCt9Z?=
 =?us-ascii?Q?sATEXDKda9kGxZQxLQ60VrArX1dq4LgWCFqdJi8Sogqkn/2WUF1GnuWgoXJN?=
 =?us-ascii?Q?C+ab4CL7mHN5G7slgRXGLtP8Fo3Bkvw/brFPi9dABGojzRtSBjSlYOIFFCY4?=
 =?us-ascii?Q?fuhyIvHPNaXFeIqGe6E8cjdMN1pxq/O2AoxqWFVnbvQekt0s6uYoc4CZzh9f?=
 =?us-ascii?Q?CahcF3mz2QMNgevM3LUYJEaY1soTcnwkAnjIkuMbzuncRwUwQnswHfUAIRo+?=
 =?us-ascii?Q?m/e8JVRek/hHewLK8+vGAqxdFUlW9/pGetWcxWJK8+qMxR+yl3pbiN1v/MQU?=
 =?us-ascii?Q?neH3ZOuKiN/Vw/489mtBJxlzWO+3FHJVaRYMHd0vHhx9fFofGmgasQ0sqsfR?=
 =?us-ascii?Q?qNdiEI8rrl5owSa9TinxD2sl4/PBRwCN0ZqSJ1Nm/xWdHwM4bX0jR8CnIqoY?=
 =?us-ascii?Q?kZGVzcJmXnA6pPWnDktvcp95v71Klp+oQoS7oYSKKiHjsb12Ep6V1vmlZd3u?=
 =?us-ascii?Q?xSoAPm3Y/lBMWIADMIaqj0BYeoTG725m7YUaSsQ0JyXJdR8NCCPoz9MY+/F4?=
 =?us-ascii?Q?7trruT2ArS0MVvDMaDa+rLyV/4hKgtPjAkanuiYLHukMGUQ6EDG5l9bHirbB?=
 =?us-ascii?Q?duG7EcVT+FjxdJLZGoHw1a9oj6sQvNudr1FvhAzd6YU85pQMvuy2gFUvUrBL?=
 =?us-ascii?Q?+Q=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	BlGRQSD6xWFOZ7ydocAGPprpxn5NuOEWpaN9hrL9Vrg4sjxIPvKxCLc/JYh1mjOq6UAGdToXktp6xm3cJzRaN6yPPhz3/EW0/Ht5v7iDS7V3VF83262Ni8QDN/vkq23ib1pDRRmRyxponGTXcK+OH//VhSrZ7kRry2qlzkOmKG9FiYk7VDYzTdhckqVDls/2TprF0EUt0s92sVAyMtgQr8yR86znZ7rcTJm+MBhytLIi/F7oq/wiz6XzIEHFyPVNX6/OwUjyOFFn77jCruPgew+2t2QPdonjhh1hSqw3rhEi2OarK7qx50yIWfg5WjiXFAzLJb1XIwYTLeXlWXUEr4OmKBRrw0EokaNWtiT1109iihJbIpbfGlADOHywR4ZpKA1UZbufEZUQMRhndCoUnVw0NXVoe0FmhWU7QCHXMwpdEpwz1Zoc9xPLYHROh9jTWs9N0y/WAkypgZ7womU29IKS/sL/cHJIuDEdiGdCkl2fhYeW70GfyOT828MKFuymOrCFL+igxtCu0LYC4DMUUbLt3tgWY2y+9KMmlNSCvmpC59L/XLSjRl+CBwyGa9jsnkbkjDxcoD0H2fynw2RaxOh/bGEaBuNmfQPdFBpVX34=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 636e381a-d02c-4a83-0f6a-08dc0d3a7a73
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2024 15:33:19.4588
 (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: gF8gSEHBwBb8jynR8KDXxAGltCtzt+S+l7iQdIxzY3PyQpcHu1vpfzHqap+Nq1ESEixSpDK0WflaHAtrKGt/WQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB4961
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-04_09,2024-01-03_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxlogscore=999
 suspectscore=0 adultscore=0 phishscore=0 mlxscore=0 bulkscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2401040122
X-Proofpoint-ORIG-GUID: iBtKUhZrP7UCQ-ROl-xiqTAdhKC5t0rW
X-Proofpoint-GUID: iBtKUhZrP7UCQ-ROl-xiqTAdhKC5t0rW

On Thu, Jan 04, 2024 at 10:20:55AM -0500, Benjamin Coddington wrote:
> On 4 Jan 2024, at 10:09, Chuck Lever wrote:
> 
> > On Thu, Jan 04, 2024 at 09:58:45AM -0500, Benjamin Coddington wrote:
> >> After commit 59464b262ff5 ("SUNRPC: SOFTCONN tasks should time out when on
> >> the sending list"), any 4.1 backchannel tasks placed on the sending queue
> >                       ^^^
> >
> > "any" ? I found that this problem occurs only when the transport
> > write lock is held (ie, when the forechannel is sending a Call).
> > If the transport is idle, things work as expected. But OK, maybe
> > your reproducer is different than mine.
> 
> Any that are _placed on the sending queue_.

Ah, I misremembered: I thought all to-be-sent tasks were placed on
the sending queue. But no, only the ones that are put to sleep are.


> > One more comment below.
> >
> >
> >> would immediately return with -ETIMEDOUT since their req timers are zero.
> >>
> >> Initialize the backchannel's rpc_rqst timeout parameters from the xprt's
> >> default timeout settings.
> >>
> >> Fixes: 59464b262ff5 ("SUNRPC: SOFTCONN tasks should time out when on the sending list")
> >> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> >> ---
> >>  net/sunrpc/xprt.c | 23 ++++++++++++++---------
> >>  1 file changed, 14 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> >> index 2364c485540c..6cc9ffac962d 100644
> >> --- a/net/sunrpc/xprt.c
> >> +++ b/net/sunrpc/xprt.c
> >> @@ -651,9 +651,9 @@ static unsigned long xprt_abs_ktime_to_jiffies(ktime_t abstime)
> >>  		jiffies + nsecs_to_jiffies(-delta);
> >>  }
> >>
> >> -static unsigned long xprt_calc_majortimeo(struct rpc_rqst *req)
> >> +static unsigned long xprt_calc_majortimeo(struct rpc_rqst *req,
> >> +		const struct rpc_timeout *to)
> >>  {
> >> -	const struct rpc_timeout *to = req->rq_task->tk_client->cl_timeout;
> >>  	unsigned long majortimeo = req->rq_timeout;
> >>
> >>  	if (to->to_exponential)
> >> @@ -665,9 +665,10 @@ static unsigned long xprt_calc_majortimeo(struct rpc_rqst *req)
> >>  	return majortimeo;
> >>  }
> >>
> >> -static void xprt_reset_majortimeo(struct rpc_rqst *req)
> >> +static void xprt_reset_majortimeo(struct rpc_rqst *req,
> >> +		const struct rpc_timeout *to)
> >>  {
> >> -	req->rq_majortimeo += xprt_calc_majortimeo(req);
> >> +	req->rq_majortimeo += xprt_calc_majortimeo(req, to);
> >>  }
> >>
> >>  static void xprt_reset_minortimeo(struct rpc_rqst *req)
> >> @@ -675,7 +676,8 @@ static void xprt_reset_minortimeo(struct rpc_rqst *req)
> >>  	req->rq_minortimeo += req->rq_timeout;
> >>  }
> >>
> >> -static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
> >> +static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req,
> >> +		const struct rpc_timeout *to)
> >>  {
> >>  	unsigned long time_init;
> >>  	struct rpc_xprt *xprt = req->rq_xprt;
> >> @@ -684,8 +686,9 @@ static void xprt_init_majortimeo(struct rpc_task *task, struct rpc_rqst *req)
> >>  		time_init = jiffies;
> >>  	else
> >>  		time_init = xprt_abs_ktime_to_jiffies(task->tk_start);
> >> -	req->rq_timeout = task->tk_client->cl_timeout->to_initval;
> >> -	req->rq_majortimeo = time_init + xprt_calc_majortimeo(req);
> >> +
> >> +	req->rq_timeout = to->to_initval;
> >> +	req->rq_majortimeo = time_init + xprt_calc_majortimeo(req, to);
> >>  	req->rq_minortimeo = time_init + req->rq_timeout;
> >>  }
> >>
> >> @@ -713,7 +716,7 @@ int xprt_adjust_timeout(struct rpc_rqst *req)
> >>  	} else {
> >>  		req->rq_timeout = to->to_initval;
> >>  		req->rq_retries = 0;
> >> -		xprt_reset_majortimeo(req);
> >> +		xprt_reset_majortimeo(req, to);
> >>  		/* Reset the RTT counters == "slow start" */
> >>  		spin_lock(&xprt->transport_lock);
> >>  		rpc_init_rtt(req->rq_task->tk_client->cl_rtt, to->to_initval);
> >> @@ -1886,7 +1889,7 @@ xprt_request_init(struct rpc_task *task)
> >>  	req->rq_snd_buf.bvec = NULL;
> >>  	req->rq_rcv_buf.bvec = NULL;
> >>  	req->rq_release_snd_buf = NULL;
> >> -	xprt_init_majortimeo(task, req);
> >> +	xprt_init_majortimeo(task, req, task->tk_client->cl_timeout);
> >>
> >>  	trace_xprt_reserve(req);
> >>  }
> >> @@ -1996,6 +1999,8 @@ xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task)
> >>  	 */
> >>  	xbufp->len = xbufp->head[0].iov_len + xbufp->page_len +
> >>  		xbufp->tail[0].iov_len;
> >> +
> >
> > +	/*
> > +	 * Backchannel Replies are sent with !RPC_TASK_SOFT and
> > +	 * RPC_TASK_NO_RETRANS_TIMEOUT. The major timeout setting
> > +	 * affects only how long each Reply waits to be sent when
> > +	 * a transport connection cannot be established.
> > +	 */
> 
> I put this on 2/2 like I said in my earlier response.  I've been trying not
> to make a delta on 1/2 (yes, even though its just a comment) because there's
> a nonzero chance a maintainer is currently testing it to fix 6.7.  I
> probably should not have made these two into a series, except that the 2nd
> depends on the 1st.
> 
> If you definitely want it here instead, I will send a v5.

Got it, I didn't realize 1/2 was immutable at this point.


> I think we're probably going to be stuck with a broken 6.7 at this
> point.

Well, 6.7.0 might have the bug, but unless I've missed something,
1/2 will get backported to 6.7.y pretty quickly, even if it goes
in during the 6.8 merge window.


-- 
Chuck Lever