Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Fri, 27 Oct 2023 11:52:49 -0400
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Oleg Nesterov <oleg@redhat.com>
Cc:     Jeff Layton <jlayton@kernel.org>, Neil Brown <neilb@suse.de>,
        Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        Ingo Molnar <mingo@redhat.com>, linux-nfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] nfsd_copy_write_verifier: use read_seqbegin() rather
 than read_seqbegin_or_lock()
Message-ID: <ZTvc0Z6DJEYXI/TL@tissot.1015granger.net>
References: <20231025163006.GA8279@redhat.com>
 <20231026145018.GA19598@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231026145018.GA19598@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?XJqabQ9zbyiIgaLo4OAJNGktKW0BKD65613buQzRcAYQ2ftDDDU5WS+8Z8NZ?=
 =?us-ascii?Q?fZ3ydj9B2BJKkopjhtJLdj3ca92wpt6fKr/pI/dY2Q4Ngb6SNVzl8Uil1Mi1?=
 =?us-ascii?Q?Ebn6R7T6K6s/iUwKGj4xvoozI/xtAhUDNYh+CthijONeh2+jeHuoPw+R1uV2?=
 =?us-ascii?Q?J3+87q9O/lUMi3K70JocJdDyLzGSFXeJTW1Msp9eoYVFji4gvdDwS0JA7jd+?=
 =?us-ascii?Q?9EMU+fwbuAVBNcEAePVQ5jSbREcZrZbAjtPp5Qq0gimDy5rZ/GNBnSeYhtZ5?=
 =?us-ascii?Q?uQc17Xn4zXZIK/ZW5Y0g4rRD9FonnjFks93xy3qF10pzLvHt2l4FlrE1TOYY?=
 =?us-ascii?Q?B+f9koCp0C3qIWvtLtIKuPlWHLxvEl+OVRY5p9YDrKQ509IXtJFMwj3belvr?=
 =?us-ascii?Q?oo+q3ds7zTDvglGJp/lqjIVycJPURLDGv6fGHBg2XUSLwRPAcXE5THco5vfw?=
 =?us-ascii?Q?W6cx3xojiKiDNnYfOVnTdBY6lTI5K0ohA3rOY/mPxZQ1jgb2KU1NFHnXqwFl?=
 =?us-ascii?Q?ukY1Pq/WpBL9nWjNY8oVqpYP5eGWHkoOQu0AcFjQW7Gt6alrcSakQEHZCeNy?=
 =?us-ascii?Q?ak+tXCdB12aSRiwsNkaMXL1Qrrp8Z5d5fHxk+u2sOLHjjPULbAtJEPoqYgOR?=
 =?us-ascii?Q?iLKCQdYAPIROgScyG+Mt/+QE0OeDKbAmTHj2XC3Dhu8HMVKvbLomwoRL/uQv?=
 =?us-ascii?Q?J7tibDwnCIJFBFq54Xbjz7n56uQZkox3Kka9D90ipnBA6E1uggHSFeMGDEnP?=
 =?us-ascii?Q?hGs7YVreNlCBppgyhvbSG30fWDyWKxzG3DqKdOEEVpwBUEK7byUwizLRNuUw?=
 =?us-ascii?Q?02qEfDmhBDbLnTD+o4d1vu2NI4EsleTxakAkt7xonAl+G6gdj9yFleRspf6R?=
 =?us-ascii?Q?62KMvWlKOALYfcBGWpnrmFmyJmNL1sfVGSGbFVb4IMh8wy4rU99JA6dfkHsk?=
 =?us-ascii?Q?PHcq8rqESm1sNtKBz2PoU68wqsHcCRKiiGovASEBRcdfRmM58lDN9zaSGmx9?=
 =?us-ascii?Q?0fhTAsxoxR30+9BXxiWTGCoKW8ANbzX5FWlgPOAjqbkkD8f5ORJH+C4gY5pi?=
 =?us-ascii?Q?hoGX11N/muRYweNZaVRtil3kk03kN/fl2HY2WCxOYm2Wn5s1hJF8EfgqAFy1?=
 =?us-ascii?Q?b+hdNlkpDmFyU9NR/Bgei1QBT4GRIvraLIDP/ZcicMB6QY7vI711jZFBUrh+?=
 =?us-ascii?Q?F6xsa8Dnlq3tRb3zvoU53YGZ6FLqQC4zx6FHaDEhuFEgwJPwijThAPIOsGTC?=
 =?us-ascii?Q?nyFkXYSRe5czr1HWAwO8pFyD9/Q7tjq3MxJHJCAdOeHcT4E0O2q23kboka0b?=
 =?us-ascii?Q?I2zkw2aD8w8CWCF1OgZ8uTlv4NYMAZAq0hIPmdlBRtZbwn49OO1dFbhAKXuy?=
 =?us-ascii?Q?NzUBluME669nmeeMtZXyd433l673ocqInXw241Vsnabl9OD8X9UWUY9cFx2S?=
 =?us-ascii?Q?MUdOejEBvMc6/vPo2GvFgpezcAkrMZs+GaDFYCCNxMOSHKAvgE4vw6C5Hh96?=
 =?us-ascii?Q?6+PeGf1s+/r1YTrZUJoXtIZhh6JP1NtFUG/Nan53E1up5bX1DMD19F67kGc0?=
 =?us-ascii?Q?mSvyPFXnIm49DB/rIhykPtoARBtuJrRA6RzeiuOq?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: JjJEER+vFI6F2tDhkCQLsjO93EIVJeX5aKh7uW/zVSjsLk6p4+d6cT9V8gX0DRYp89kzjqWNqzslqT+v3wrG6P/1F9xmqtHfP62MEK5InjiagZj2tYnOOsKtuA5HQ6+SYJx6Z03xRKgHPxp53/54m9d03oBA6fiF6tQiXCKOcHTgXv7QuPhkrRAOy3PI2JGmYVeAI+oVgbYf1dylylFcdfrSl4K+24BExDK/6jRjS8BuQVkFUqRHtYDXtUIbXVDDvM0yYppkPtepKbIGYRz1k7g64GgoDXD24adCk9jsPVo9G1fcIoS4NKBh+HYNoW9pgtZjMDqCUWnZLdualuG0MXg+nZRxshrGJn5DV/gQ0kzaVCw8Bn4zsOMuClbOiguzNJRswEVX3jR07SvT2Y+H3ErRvzt+5+3P/vdPEzRcYsALWOSZm8taKoQR1wbWh/0h+T8R70q1bEM3BhVoCJa7keqYRa41Cp8jeuiPmWNT6C0lXBSCSUtcdm2Y8nwWn5VKrsfTz5Pgbnq5NYFfcJlu0ZaY/ElCcRKdXoojGniheKdG4htHkTuV4Kql+Hl2Qw6TuA5bmJgfZnRwAIAb8MKd7nLDhBV8ydNkmQJQJudKmp2pVxraER9Sa19qNWWsxzC9bVI8/UvNqgweZy1rmeLPZZOu0dmpVQuCZxQFoj/gZB9DU5o156TiiHf5SfnfnUGwtzCUyXmRaDnCkmTtGTLyqnrpJ9LXM1VE8Nixjs0SpMRAJ4FYcKlssJCnA2HqKNEeqbZUYW1xjXRawmghnykDLBqMtMKerWyYlsF42I2uJSXJ/e7uLV0scIBd6ydSrpMr+Oi9rj2p1Wvkp0qqPi8NvO0NJBSIptlah47fAAfcS5danptyMT6Vph6M3O24EpW399kfmnBv4kVdveeZqU+HBw==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f51018bf-10ec-4aee-e3db-08dbd704c80b
X-MS-Exchange-CrossTenant-AuthSource: DS7PR10MB5134.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Oct 2023 15:52:54.1174
 (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: NtY/9XJFow06IUN3JHAtbdvZbBcoM291QcNbNr7e2u4oAa5FYPMtCy4ooGDS6pjxlRZuLoDDiURxQjBTjKAFqg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB5675
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-27_14,2023-10-27_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxlogscore=984 mlxscore=0
 spamscore=0 suspectscore=0 adultscore=0 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310240000
 definitions=main-2310270137
X-Proofpoint-GUID: gK2B_khZnKORSP-9wp-blQRyeEMWvt2p
X-Proofpoint-ORIG-GUID: gK2B_khZnKORSP-9wp-blQRyeEMWvt2p
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Fri, 27 Oct 2023 08:53:51 -0700 (PDT)

On Thu, Oct 26, 2023 at 04:50:18PM +0200, Oleg Nesterov wrote:
> The usage of read_seqbegin_or_lock() in nfsd_copy_write_verifier()
> is wrong. "seq" is always even and thus "or_lock" has no effect,
> this code can never take ->writeverf_lock for writing.
> 
> I guess this is fine, nfsd_copy_write_verifier() just copies 8 bytes
> and nfsd_reset_write_verifier() is supposed to be very rare operation
> so we do not need the adaptive locking in this case.
> 
> Yet the code looks wrong and sub-optimal, it can use read_seqbegin()
> without changing the behaviour.

I was debating whether to add Fixes/Cc-stable, but if the behavior
doesn't change, this doesn't need a backport.

Just waiting now on R-b.


> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  fs/nfsd/nfssvc.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index c7af1095f6b5..094b765c5397 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -359,13 +359,12 @@ static bool nfsd_needs_lockd(struct nfsd_net *nn)
>   */
>  void nfsd_copy_write_verifier(__be32 verf[2], struct nfsd_net *nn)
>  {
> -	int seq = 0;
> +	unsigned seq;
>  
>  	do {
> -		read_seqbegin_or_lock(&nn->writeverf_lock, &seq);
> +		seq = read_seqbegin(&nn->writeverf_lock);
>  		memcpy(verf, nn->writeverf, sizeof(nn->writeverf));
> -	} while (need_seqretry(&nn->writeverf_lock, seq));
> -	done_seqretry(&nn->writeverf_lock, seq);
> +	} while (read_seqretry(&nn->writeverf_lock, seq));
>  }
>  
>  static void nfsd_reset_write_verifier_locked(struct nfsd_net *nn)
> -- 
> 2.25.1.362.g51ebf55
> 
> 

-- 
Chuck Lever