From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Neil Brown <neilb@suse.de>
CC:     Trond Myklebust <trondmy@hammerspace.com>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2] NFSD: Fix RCU-related sparse splat
Thread-Topic: [PATCH v2] NFSD: Fix RCU-related sparse splat
Thread-Index: AQHX5XDFTrw4hMDSskyK33okWYrHZawcrx4AgAAMIoCAAAzJgIAAFXYAgAJ7f4A=
Date:   Thu, 2 Dec 2021 15:33:22 +0000
Message-ID: <CA08837F-AE63-492F-865D-CCA71AFB0B2C@oracle.com>
References: <163821156142.90770.4019362941494014139.stgit@bazille.1015granger.net>
 <20211130225250.GC641268@paulmck-ThinkPad-P17-Gen-1>
 <163831537509.26075.12859361728901873347@noble.neil.brown.name>
 <2ABC02B1-CE76-422A-B64F-64B108B12C0B@oracle.com>
 <163832273051.26075.5034720991945492453@noble.neil.brown.name>
In-Reply-To: <163832273051.26075.5034720991945492453@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?E2UGeAcd8uHszlyy8uTV5NYBRvjKlpWsZNzyt4cqsJIzpHLTcxY84t6WPD/z?=
 =?us-ascii?Q?mQ7o6F8yOsRj1ldInJdT+j5DfqVqbQb/ruevV43WSvuHLoiZw8PIX99zyRBa?=
 =?us-ascii?Q?rer+xvzggcjyr4MNID5XHGt0urAQbQSouD8mI0Lu3dATlbLF4HcpR1gw5MzL?=
 =?us-ascii?Q?+EiKQnI98JYDf4D/4pPvFMs6gCWF61vhDP5tziSGk2N0pEGWubXb2XfG/Gcc?=
 =?us-ascii?Q?3AOqPfJi02Hu/WWXdJJ/whw+LIpHyTonaexv0buuXoX3Dpb+wnP11PkU4VTa?=
 =?us-ascii?Q?DQy7/Ff7Vw9QVY9pZu1U485SxfBS9atvwmsEl07E27IMgK9qqfT0a2K1LYv4?=
 =?us-ascii?Q?Yx0KrPwFH4a55IovLQuEYv8Ju4u/PmTKEheQ1aRGic1d9O2pypmXY0lGcq9t?=
 =?us-ascii?Q?Xwd49XhmjIDIraCtM7pwrMoMLDtq2SO22Fxa7Ee9sWxSbXpGJmlcBRc0GaZo?=
 =?us-ascii?Q?e4Lo8GJhI44rv/2a1XlhNWGj53Zmhot6cqZIMChAPtqrjxQMcbI24JxF0/Ra?=
 =?us-ascii?Q?qOc7DCO234iBg5hDDupoaIl0pNJzq5amVtKHqhrTF8R6ps1hcHs4gEkG4wsC?=
 =?us-ascii?Q?lY4BvnrUlarX4Mu4TFCz82PBIXz0XZtVSWT1uEb6q8gGx7QQveQEIrVnjaR8?=
 =?us-ascii?Q?tZQsmZ4GLGK8Psn/AlDKYCj7ryzzQdUzU0DKs+RjWm6J0wztxidv7a7XHgE9?=
 =?us-ascii?Q?Xn8Ie2F9yupnUdkZGGy360whDcsT43Frkv4x9HJaNgdV2HHt4vQBq0NiBMfo?=
 =?us-ascii?Q?4UuIjG9GVFQf3qZuNvacpSmZ2DpdJgu1j1GRSuywi9wb/pHpB+dH9L4xRTS6?=
 =?us-ascii?Q?A6whcGvMHaC0SoEa/JJaDMU/7+Soy6vOoDI4q3swpQlIT0amPIAONIlk6hC3?=
 =?us-ascii?Q?SUsRWUsYe9RBel8BWJKWbrNt0UXJD9uFlhDJSFDtIeTpGH7fPWHP91hxvwZH?=
 =?us-ascii?Q?rJOYHaS26E/NMY1cEywaSM9x6fmPFYtYi4oFU6iF0RO6fao2jRm2VzPElkiQ?=
 =?us-ascii?Q?SQ2DX6QsfyhajZ8+kJ/YTfBZi+9G0euzF1oEoDKCz/T2ie7tCf767Yod2OX7?=
 =?us-ascii?Q?4mErvxlt8uPGIvQ8wFGa4hL1VddtVzuGLTncYSjlbBKPoefmizJggZXjvyu4?=
 =?us-ascii?Q?xUEQ7Glil80aEi2jRIXPFrhWl4YH1vBpxA1rmOd7QnVjyaIbnbwauKTXiODX?=
 =?us-ascii?Q?Ja9aJ7xfHK5iNe73ZU/zn1Y94f6RoyeSMmPCxHZjcF2DC7lqmflOiygIFzo6?=
 =?us-ascii?Q?tBFPfAUD2ESwaOB7riIyUpccZNEB40lKhU1S1GAB+EcRH4fLTt3JeHvFNOny?=
 =?us-ascii?Q?Ib/UXlgxqzT11OTM6OU5QzgQOWDxT5QymZAMiSGFySaqJDaNMM/KHT/NiyWD?=
 =?us-ascii?Q?ZBaoQ8G7HyWRvOdZnM7+1Effzx3T+g+9Uly1w+1UwW+lFRVcrXSqvSoZnG2x?=
 =?us-ascii?Q?/fQhoqSllySTA36iyRVdQR7890CCTrZ1xYFjVBGnvqTAIY0O/pTqomG2wwWa?=
 =?us-ascii?Q?0nyLNuPzdc4v+Iv0RE3U3EIJ+PQLuR1s+WjaDJAhKMn+1b7rzVVAmsFku3VL?=
 =?us-ascii?Q?2yyFXDJ7e9bOiql+IMALYyjF9FBDHLHFpCld32EVv+okiX1aL3IGO30BJ1wU?=
 =?us-ascii?Q?VmoelV39CUOAbMVyBc0Ix/Q=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C0A2D3C639D4594B8717D843047DB8F3@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4688.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1b5a83db-3c8c-4656-62e0-08d9b5a91328
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Dec 2021 15:33:22.3875
 (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: hqCQBwMAWR/GF/YjCR8p44Cd/C+1tzQwaX5esAav2J9V21miH4EEpRR4bN48URjsKv5y+t9eIA19hZVinZjnDg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2568
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10185 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 malwarescore=0 mlxscore=0
 suspectscore=0 mlxlogscore=999 spamscore=0 phishscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112020102
X-Proofpoint-ORIG-GUID: V5vCdFagETgHfdk-m6KFTO9WEUXy8gzV
X-Proofpoint-GUID: V5vCdFagETgHfdk-m6KFTO9WEUXy8gzV
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Nov 30, 2021, at 8:38 PM, NeilBrown <neilb@suse.de> wrote:
>=20
> On Wed, 01 Dec 2021, Chuck Lever III wrote:
>>=20
>> By way of further explanation:
>>=20
>> The Documentation/ for RCU (ie, "RCU for Dummies") suggests that
>> rcu_assign_pointer() is adequate for preventing undesirable
>> compiler optimizations or CPU load/store reordering.
>>=20
>> rcu_assign_pointer() uses WRITE_ONCE for constants like NULL and
>> smp_store_release() when assigning variable values. I copied the
>> use of WRITE_ONCE() from rcu_assign_pointer(). So I expect exactly
>> zero change in behavior or compiled code... (but I should have
>> checked the generated object to verify that is the case).
>=20
> True, there would be no change in behaviour - but we should at least ask
> if that behaviour is correct, and why.
>=20
> If any barriers are needed here, they would have to be between the
> assignment of NULL here, and the tests of l->net in
>   nfsd_file_list_add_disposal()
>   nfsd_free_fcache_disposal_net()
> (because they are the only places ->net is used)
> The only conceivable race is that they will see a value in ->net "after"
> NULL has been assigned.
> If there were a race there, it would be between different cpus, so
> smp_store_relase() and smp_load_acquire()
> would be the correct tools to avoid that race.

smp_load_acquire() is not used in those functions, so one might
draw the conclusion that either the RCU annotations are incorrect,
or that degree of concurrence paranoia (including using
rcu_assign_pointer() to assign the NULL) is unnecessary.


> If, on the other hand, there is no chance of a race, then there is no
> need to assign NULL to ->net at all.  I believe this is actually the
> case.
> As the 'net' is freed using kfree_rcu, there is no possibility for a
> search that started before something was removed from the list to get a
> false-positive when testing if l->net =3D=3D net.
>=20
> So while your change is safe from a behavioural perspective, I don't
> think it is safe from a maintenance perspective because it leaves in
> place code that doesn't really make sense, but removes the warning that
> helps us to find that nonsense.

Well I agree that the code is either hard to follow, incomplete,
or incorrect, and therefore needs to be cleaned up so it does not
accrue further technical debt.


>> Sure. If it makes sense to use nfsd_net instead of a separate data
>> structure, then that can be made to happen. I would like to hear
>> from Trond regarding why he felt a separate data structure was
>> necessary for fcache_disposal.
>=20
> That would be best, yes.

Since Trond was active recently, I think we can conclude from his
silence he does not care and we are free to act as we see fit.

I'm comfortable starting with https://lore.kernel.org/linux-nfs/16383166945=
5.26075.14420575954675785122@noble.neil.brown.name/raw

--
Chuck Lever