Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
From:   Chuck Lever III <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 Mailing List <linux-nfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] nfsd_copy_write_verifier: use read_seqbegin() rather than
 read_seqbegin_or_lock()
Thread-Topic: [PATCH] nfsd_copy_write_verifier: use read_seqbegin() rather
 than read_seqbegin_or_lock()
Thread-Index: AQHaCBvpT1YyJkmg50Gbg8cXyhTQ77Bdyw2AgAA9zACAAAHLgA==
Date:   Fri, 27 Oct 2023 19:40:35 +0000
Message-ID: <8FAB33A8-CB9F-41EB-8CD5-B558B993E912@oracle.com>
References: <20231025163006.GA8279@redhat.com>
 <20231026145018.GA19598@redhat.com> <ZTvc0Z6DJEYXI/TL@tissot.1015granger.net>
 <20231027193359.GB24128@redhat.com>
In-Reply-To: <20231027193359.GB24128@redhat.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ELW3XIjC5V5DWUI6Gyg+cwKmsAV/d5pqy4Do8LUDXlw/FkOrikp6mngLLHYe?=
 =?us-ascii?Q?HQf05bULWEsHwKuuIXLIvEaxr5cWKMibib/c90HW9ca8tktx8ZvITOUC3ayR?=
 =?us-ascii?Q?0rq0c0d+wH4pQzXef+hYvcEvDQU+RqqmXSD7qXWl3cHeQ2N2nKF8kmKonsrX?=
 =?us-ascii?Q?DLMiPbj4E4sxeRjr75AJCFyRPiqkVTMcpdFpUgzSLgg/8jMiwcKTi/+KYpAj?=
 =?us-ascii?Q?BVV7kmPifQqkEeO2lLkZ4mCszM9K1cMr8NQdg94L5cg2Co8rxM8PZOefIlg7?=
 =?us-ascii?Q?M99pi+wNYK87mAVN11RjxPv78NngGdJyBsRDU/9ynsXBFTAcLhg2FjKO1JTv?=
 =?us-ascii?Q?zU0aoomMb42sN3/fJb+T5Dz2wH1On57Y+ajtWWe0hfWr6w8aVGPIvkt3C1D2?=
 =?us-ascii?Q?4sumomnqXQ75RnxMm8pIut0NlrX1qvt+SKzwxNcrbWsvy+RtMx4GtclmDHDs?=
 =?us-ascii?Q?+o1nUFUCf4kfDfQDqlThJPZLachjf1WXeeepBthrarinc6F28xe1aR1xCMyO?=
 =?us-ascii?Q?F+ATQxZMr5GEADqrfGLhjTzQA8QZgag9rsWQ94MI/2/6UPurfSXKknnIcnk5?=
 =?us-ascii?Q?Lsc+zgML7FcTFjCMTsjhrJ4xnCZOGya9aXSqa5ekysA7WnrmKfu8VzUk0pfk?=
 =?us-ascii?Q?zvAFbyNW8gOD6ry3/V4ohUOrVeHylvBVL5JeFfwRcWjmivj8tp1XpXYVQdYN?=
 =?us-ascii?Q?IH8B2LxBjgvmzZ9uJ55JDPI6mHxYzs7Y1FESxB+yGlx9oAxjCeXiHWwkZBja?=
 =?us-ascii?Q?66QMmKpSxUDwdRKTH/uEzSZ2KOjDLQLTWBjc2R6WEC5R9kmxhq4d9M2HLcLC?=
 =?us-ascii?Q?8O4jgEwCN9yMTSxVBovZVXHUhPA5i8djoBLjwan87J5b2OhmKloa1+yWTwt4?=
 =?us-ascii?Q?+MlCZy5rQz4gJvpTb91Ml5TiZvRbao/nAaR3+sNsq4sMf12Xbreg1b1j6fbc?=
 =?us-ascii?Q?3nqOkb3LhxMCa6St7n4sjAIy6CHL38UM2JKC91iQ2TcPAiQED/JDfLy4HbGI?=
 =?us-ascii?Q?9lmd/Xj0B85QKsnR8f4cDqo/Bshriv44C/8hZ0xGbC4Wxb3GlFygfXxGJ9UK?=
 =?us-ascii?Q?lLVXLg5dyYmsp1jdomGUrMBGxludFyN5wO0asuh2G3RwrtOMMGV7YJhOccS+?=
 =?us-ascii?Q?srro2udXjYamOyfkRivJMgOhcL4Vw3l2VJNhoJ6HrhlyIhHELBgr5yaEd/AN?=
 =?us-ascii?Q?BYSulEnctKZzeo085X2ccrinAiTpMQrLeL3Pgh4Nrh8YuhTczkePBQUuAHDn?=
 =?us-ascii?Q?q/7guJPldzvunYmjpb2GxtNyIwZzCIR5yMPRW47x8ez+r2ZXXzis2mjyxS4y?=
 =?us-ascii?Q?/TcSqTzXBq8uP/SX5GW1CKcLWRJsFo4/I+6oLetInq1xdI8YD27EmD3WMVet?=
 =?us-ascii?Q?kjYcf7SQJGEIfm9AwgvhVp9JSNCgcM838ZPtHA5zcqkRUGwVD9HZgnI/9h07?=
 =?us-ascii?Q?SxsZ3TTxpACfT8JW/rMbNoJJyOmsBQVW6BPcVvcgPKo1npHQ8l2R4B4gZXtr?=
 =?us-ascii?Q?4wBBu6T6WWuUmK7BRIWDd8552RnhGyUF6CIJmukUGJgums4yJXVbz7cUiErE?=
 =?us-ascii?Q?H+3s4pa99zdRsVsv/G60eo2Pd4tpPTwm7ddmUMBwRUlqWrORFlZc9Okxqigf?=
 =?us-ascii?Q?tA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <35F9FC619519594FBA78A3FB72E7D6E5@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: UGP0JXPmNQLIRLs/HR1x6aFs7HUs3UAKZbGIsgdWYRNK8B6T5HvDNie4FaQrBqh/37r4Y4YYxdvRgNdxVydkxb45htHUlvjMZAYHu3yjDLTUh708lOWzxyksJq7WHRaoXgr30SmJ7+LmlH0QYevJ3djCqk1I5qd7vi8RsAFrTB6FlyLfs+x4sXuiJ4XHBN2ilsqTrqtxo710qG543buE9zKhRxWjPCOQbypUNnVdAkfVzYzCyxq7dpEYzUXMSfVWyVOmyJlwAFFHuLFdQIxUXBYlOWttGn9DeMJrmGmMtkfprgZWLCxKBmfT3LJHoSw2acsStSxsxYsymHmCdKBXA7WlNCCNBt0+Pgg08j83AZA/uIr8c45HTpK+ZGSiRVJYkuiPpYpyv8KAtonmoi2kZy8N5C78sixPJ6GQceYKYGhHOirF3CcSsrWvZFOX6YM9hdstBoJhDmIRJtve8I7P706WEJ7yompwmxtaxXcVEY+HKqRe8AlngvxiFq99Me0Ae8Ds8L09VclTLdB4zKDVhOfETQyLkS8uGRAkp+UFDZstDTvExbhS0LDWL6uk7wnkocZVVaNWSUMKH/KNkm4xkeU02aNY33kxc1NZ0aUK6r5RN5HzXckF5RtpsfnFA73Qa7ncnKAJLMh8EYMuTlOrET4z8hnIhqqoOnallzvdBXUHi2wW1MxJ9CjO81is3ie0FkWhR9wQdtDl7LjFcMswzI3buLip4K0cpwA3P7l9nAcERQgjZRWA/uihtpkfKVauueQPg3w7WoGPRtIKSE7EzHjR1wUGAqO3l5BPOJ2rOIxzenfZ7qO+F7kztBs8tXAj7e4i73uhYy0SddVq/lBGvHjyjOfKJmcc6b9vkf2U40rBKdQWk9vZ3IBQ+sf/2IAXYVKL+YjWKvMRUyy7EQ811A==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DS7PR10MB5134.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 11f8bff2-c099-4025-d0cc-08dbd7249740
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Oct 2023 19:40:35.9219
 (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: KoIbow2A+1snWaLSsg9Kc2cpmapwCLYFWEwPLmH4Ps+GZV2wa1nC5trIPYolAFyTDYTx/E1v4L01vEH+9DIQOg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB6460
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_18,2023-10-27_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0 adultscore=0
 phishscore=0 spamscore=0 mlxlogscore=987 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310240000
 definitions=main-2310270170
X-Proofpoint-ORIG-GUID: QbtPrnWQrTsgg6fvJK0GZITBrsQL7TgH
X-Proofpoint-GUID: QbtPrnWQrTsgg6fvJK0GZITBrsQL7TgH
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 pete.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 (pete.vger.email [0.0.0.0]); Fri, 27 Oct 2023 12:41:05 -0700 (PDT)


> On Oct 27, 2023, at 12:34 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>=20
> On 10/27, Chuck Lever wrote:
>>=20
>> 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.
>>>=20
>>> 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.
>>>=20
>>> Yet the code looks wrong and sub-optimal, it can use read_seqbegin()
>>> without changing the behaviour.
>>=20
>> I was debating whether to add Fixes/Cc-stable, but if the behavior
>> doesn't change, this doesn't need a backport.
>=20
> Yes, yes, sorry for confusion. This code is not buggy. Just a) it looks
> confusing because read_seqbegin_or_lock() doesn't do what it is supposed
> to do, and b) I am going to change the semantics of done_seqretry() to
> enforce the locking on the 2nd pass.
>=20
> Chuck, I can reword the changelog to make it more clear and send V2 if
> you think this makes sense.

No confusion, the changelog is clear to me. I'm simply stating
my intention for other reviewers and the lore archive that I
will leave off Fixes/Cc-stable when I commit your patch.

So far there has been no review comment that suggests we need a v2.


--
Chuck Lever