Received-SPF: pass (google.com: domain of linux-nfs+bounces-1274-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 22 Jan 2024 17:53:13 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Benjamin Coddington <bcodding@redhat.com>
Cc: samasth.norway.ananda@oracle.com, linux-nfs@vger.kernel.org
Subject: Re: [External] : Re: [PATCH 1/1] NFSv4.1: Assign retries to
 timeout.to_retries instead of timeout.to_initval
Message-ID: <Za7x2aYNRqc3nIbl@tissot.1015granger.net>
References: <20240122172353.2859254-1-samasth.norway.ananda@oracle.com>
 <995F0BB9-C709-4C3A-92F3-5EB9710A47F5@redhat.com>
 <bdd760d9-9900-43ef-8000-0e9ce0aa8b3e@oracle.com>
 <8B835117-498D-484E-B1DE-912FBBBCC74E@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <8B835117-498D-484E-B1DE-912FBBBCC74E@redhat.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?elpkcktwTzdJT3NSOHE2UXhVRU5RTzBJQU9JSG9aZlNWRGJoa3YrZExpWmQ5?=
 =?utf-8?B?ZC9nNTVYRjZtQXlGeUJvc2dXZndldVB3OHZhTm5jUi9xeHpaZUpQVUdocXNw?=
 =?utf-8?B?QTBFdGdDYUhBNHFsV29Cbmk4TDlXOWhTMHRKZkZpVkZHSlZDeG1rVWFqSGx3?=
 =?utf-8?B?TzRBSEwwVDE1MldSNkNDTVBjRWFKTHQ0Mlk1RU9URjBtUE5aR3ZYN0dmZTV1?=
 =?utf-8?B?MTRMSEFDR0tKUGdnOTF2dUVGQ0xYKzkrQmNNdU5nRWVkMy9QdXM0K1V4VkRR?=
 =?utf-8?B?bTQ3bmZXcUJhdXRRVlhhRFNVbFcwSVlOWFd4c2RlT3RUeUg3UWxBeXJHQWJV?=
 =?utf-8?B?NFFVa25lc0ZSSzA4cU0zOWs5eXZOc2RUWmpLVDdBanBwSVVwWWRLS0tYOHdv?=
 =?utf-8?B?VWhMNXhaNXhGZFpsVkxWQUg2R0pyYnZIZ2lSYTlXVS9qMzcybkdEUkFGMHdL?=
 =?utf-8?B?SWdRVTIzdXFtclVBdWFORUtlblZvcWNoMlRBM1ArMUpTSTE3VC9aY3dZMUlk?=
 =?utf-8?B?U25YZ0g4YWtSU0FpTDRVZ1pZTXpIY2ZITVY5QS92cWFBdmM4SXcwb1ZmdnBO?=
 =?utf-8?B?K0xyOVo2OFNFVUxCNzRqbHFpbzZManFybzBkUnYzL2k1TGZGUmVaRE9QTFhn?=
 =?utf-8?B?TS82bG5LQ1NydDdCemxpWHFCUTlkWE5xa3YzRVVwUFBOcVZJMnNnckJUUFFr?=
 =?utf-8?B?ZHNzblU5Tm5ZUTN6S1M1cUZlWHVuVW40Rk1rOXdwcWtBS1QrNDY5a1JvZmVN?=
 =?utf-8?B?N1MxaVU2a3dqUDFQMHV3WWlRcmZucnUrdUtqNkN2Y05ndDdzeHlqVVNrM2lm?=
 =?utf-8?B?VlUwNFVKbVZBVGphS3QyZDZOUWM4b0pzbWpHSkMwdjgvWm8yUXgrMGsvOExp?=
 =?utf-8?B?UDhVZndqdEFmbm1MaVFBSm5oWFpYSmxoNFVpQ2k1VHBGWm1yM0hPZ2dNWTZ4?=
 =?utf-8?B?b2pzSmpWVWU1TXZFeWhLYnVOUTRpaThwNnlweEhCejRSZnJ2MTB1aVBpcjd3?=
 =?utf-8?B?OTVuaDZZV2l2cTMvS3lPSlB1c2VKNHVKUVVSQ1d4OU9GVUlBRXVSdjdKR3dq?=
 =?utf-8?B?MUR3U3VOdThpM3I5YzlZWUpUd3pXZDVLVldLL0dtSEdSVFNTQ0VLNWh3VTZv?=
 =?utf-8?B?TC9JVm8wSm8vZGpsM0pMblJ0TGdvaE9NaDk1NjR1bU1UWTJYSTQ4emtBNGtt?=
 =?utf-8?B?K2FTaEtINkRqOE9zak5tdUE1c09uUGpOOHBjeVBEOTZVYnpmRnN3YTZ2MTBt?=
 =?utf-8?B?Y0toNVNnZFdselc3UnZSYTZFUURFVnpaSjZzUEVhbDR5R1FCajRqYjlyeGlh?=
 =?utf-8?B?RWdMejlwYjdUVnp2dm1lKzg5SGpZejJmSFEyS3RISUJqNUk4UHJBWkdYaEow?=
 =?utf-8?B?OXZZclZTNG5kL09ac1Q3bVBzZm5vUjUyNnEvNmFsVFlrYUFXdnE0YmpBaVoy?=
 =?utf-8?B?QXJEaytud1Y1NEpBUkswU3k1dVBLNVkxZU84eGpDRGZvdE1Ta2FMdDFURG1p?=
 =?utf-8?B?YkxMMkRCUHBOSnVTemhtYmJHcjl3RUFuVEJpNEQ5Yjl2aU0wMjBiOWMwSk9x?=
 =?utf-8?B?YlFPQ2hlZ0ZBM1Z2WThFSC9wQitYR3RrR2dIV244VGRBUkZUY2puTDZWeWhp?=
 =?utf-8?B?NlhxWW9sYWZ2anlOSVhnQno5R2hNdTJlT2l2V0xSOFduSDgwK0JqbVJxUkZG?=
 =?utf-8?B?QWp6b2pHNTBEZUttd0lVNjFpS2lJUGJ5cGYxdFV1RDJRbDJoVnhBd3FnQ2JF?=
 =?utf-8?B?Q2dzWFllYWV1aXhqNjVVSXUvMVhhL3gwOTZwSWZxSTBUYUdRZG1nWmxZdUxF?=
 =?utf-8?B?OE10bnpNM3AxU294aThpb2lTZXA3bjZOcUZvTWgyYWhYTXlNRGZBM0d6WFN5?=
 =?utf-8?B?RnJqay90QUNUSjJ4bUxHb1lxL1IvSW9lMmZ2M3R2S2VyOTg1NGNQOGZ4cVl3?=
 =?utf-8?B?Z25ENnBCMkF3bUYwQTBVdm5meGI3blBvb2pSUmVBN1ZscC9TYmlnV1VWaWNu?=
 =?utf-8?B?REZjMzhIdWZ4Q2Rpb2s4L1BEUTNTaWo2MlIvc1ZMb0JXakRLWW1odmp6NGRE?=
 =?utf-8?B?VEkyRW9xallqSU5OQ2JaTDBqK0hreG1VNEtHRnRWKzl5SFF1OXdSYlFGeGwy?=
 =?utf-8?B?Ujg4VmJjb0czZ0lINEFpYnhjUSthclh2a1ZPMUZLS0N1KytoSWl2ZlR4dzlk?=
 =?utf-8?B?WkE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	FhVazp2KXBC+Fs7d4Xpw6kYAlWhNaXzn5+ytRF9kxeBW2mEQzXpUBvIHiQEPiHmUGeW5J859aOQr3J3aRQyUd5odtLCyiO8bEKosW4kUGao6kpxHf/k/fSoDry+S9UekASye/p3cTxJoVdqB2OzrEkVHm3O6SlMUP77rVb1MZ0rPn8FDPvsEeDKtBR22D6tYgvaUC896B8zk2v+1LnT362GxSSgYtYTLANP9awfSl5cvBiwCgTMpHelVYxvz127KYnlnoF97VCxOT0V8Rr4ZzGC7cbdsS/IL9F+ITQ/2b5lAmMGOLZUtFN6Ca5yV4lkWL4ahZJntwnzc6HtDzBxpQMy7zM1+UTOQpIqZ28xPG6Mx+qKI4n8RThBThTxI2YqDLz7+acsO4pnTobFxctJNGftOxOlMs9D5LVmaX+isDW/jPjACqAWN2OH59nQGnQl54lMU6K8WUqKisXbACeJQo7GUgFO8PYvi2NSe00FymZpt1QgYbLNDGrlZxi9CKlwiP7ZlYPqph0P/XCRSzZD0FBSDjzjMHtke1QKBrYWSKFqlltR0GmJG4F9hDpnbEFrFSyy2BRzHPaYLBGjxJ0Esvah/4HsVTmjgo22WLoH8Elo=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ae3b1e1f-8a1e-4c32-f9d0-08dc1b9ceb6c
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jan 2024 22:53:15.9186
 (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: 1PuzMFcxbK49FWomBXoQvFUGDsi65JIRsKgSe3Ho2YghYMfWJimYG2Y53AUJVfgXxrMyUCNS1fqnw3xayH6rqw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB4272
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-22_12,2024-01-22_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0
 mlxlogscore=999 mlxscore=0 phishscore=0 adultscore=0 spamscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2401220163
X-Proofpoint-GUID: QIRxbZQQAEdLxVx-2iD1j1mIi6_OU-KC
X-Proofpoint-ORIG-GUID: QIRxbZQQAEdLxVx-2iD1j1mIi6_OU-KC

On Mon, Jan 22, 2024 at 05:46:56PM -0500, Benjamin Coddington wrote:
> On 22 Jan 2024, at 17:44, samasth.norway.ananda@oracle.com wrote:
> 
> > On 1/22/24 2:41 PM, Benjamin Coddington wrote:
> >> On 22 Jan 2024, at 12:23, Samasth Norway Ananda wrote:
> >>
> >>> In the else block we are assigning the req->rq_xprt->timeout->to_retries
> >>> value to timeout.to_initval, whereas it should have been assigned to
> >>> timeout.to_retries instead.
> >>>
> >>> Fixes: 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc timeouts for backchannel")
> >>> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
> >>> ---
> >>> Hi,
> >>>
> >>> I came across the patch 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc
> >>> timeouts for backchannel") which assigns value to same variable in the
> >>> else block.  Can I please get your input on the patch?
> >>
> >> Oh yes, this a good fix.  Usually the maintainers won't pick up a patch
> >> that's only sent to the list, rather the patch should be addressed to them
> >> directly and copied to the list.  Can you re-send this patch to:
> >>
> >> Trond Myklebust <trond.myklebust@hammerspace.com>,Anna Schumaker <anna@kernel.org>
> >>
> >> and copy linux-nfs@vger.kernel.org?  You can also add my:
> >>
> >> Reviewed-by: Benjamin Coddington <bcodding@redhat.com>
> >
> > Sure, I will do that. Thanks for the review.
> 
> Can you also fixup the block above the hunk you posted?  Its backwards there too!

It's backwards in a different way.

And should you set to_maxval in both places as well?


-- 
Chuck Lever