Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Neil Brown <neilb@suse.de>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v4 5/7] NFSD: Use rhashtable for managing nfs4_file
 objects
Thread-Topic: [PATCH v4 5/7] NFSD: Use rhashtable for managing nfs4_file
 objects
Thread-Index: AQHY4ywcCv9nfMlOd0+Wa/Y/m9MOsq4VmJcAgAAuS4CABw1rAIAAlEuAgABtAQA=
Date:   Mon, 24 Oct 2022 17:28:05 +0000
Message-ID: <1ACC29A8-B45F-4DF3-BA90-4791EE8A3A04@oracle.com>
References: <166612295223.1291.11761205673682408148.stgit@manet.1015granger.net>
 <166612313084.1291.5764156173845222109.stgit@manet.1015granger.net>
 <cdad5af44b1c62c8e2ceae27cb2b646f7b2ec0bd.camel@poochiereds.net>
 <1DE04392-E8C9-4D39-BF23-BD1A59DB4FE3@oracle.com>
 <166657723034.12462.8422170607830380805@noble.neil.brown.name>
 <6af5e6b60fcac271b4bb37dec2ffe25adfef80df.camel@kernel.org>
In-Reply-To: <6af5e6b60fcac271b4bb37dec2ffe25adfef80df.camel@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?uqdtvy0H7kg/FzqbMCMsYkBIi/6PF6zatOmz6VP5JII2sxGXdWsldLOxBn3T?=
 =?us-ascii?Q?LRPTLvMK60oy07jPhP0LYrlCg5bsM6+RUhBtN4jUiLgmOsaiHFdNKggTmhUv?=
 =?us-ascii?Q?HgXsPVMV2NixHLMp/v874+3ZBcqWRMTSrdmiIp+5gqCCMZdxMAUNEt0EC0KX?=
 =?us-ascii?Q?vcJWuxGpYncfcwnz1XPYQrPVtmUNq4Pi5uCreEq1MNy3eO8c4qqeE6ouuJEF?=
 =?us-ascii?Q?w+mtjzpo1WCHjRdOScn5ltRP8UYL+qcLS2UlewpAfsQD6Ws233Ew1PvwDu9P?=
 =?us-ascii?Q?2tUHVNlPXDJgSKxJOag5Ln7sFyYQ9UwXIi8FNtezQV2NY8+yrpw2kd8tzSbr?=
 =?us-ascii?Q?1YoaHMkWjEpnEOPTVPyJCQIacWwRuG+ip9O5WFfqaqondb1TzhZ+ouogU7kx?=
 =?us-ascii?Q?98FWWjYK07+G4k3PZ2Ud98M/HaEwTX7LBzepKf9fdfbqnryA5FxsMXBvQCT9?=
 =?us-ascii?Q?XByFF7V+oxjKrAX7FQJeDZCJ2lW/ZJEUsH7HgiKp0UE+NKry3fLkcqXy0k2l?=
 =?us-ascii?Q?t7DXUIj+S0bjxiDfwmVhGBVeCmY3gH02BfORvbR+UY8LEU+vthtTdRcoEI0H?=
 =?us-ascii?Q?4ff5dURiOdSDURM0W/O5ow8LoII2x5aE94bKRY6TtPBnKNIj9jlDE0j+TqZ4?=
 =?us-ascii?Q?2oVn4L9oWJufTcnxsslgJ/Z+t7QXevPCTqTVwhN/aC+bXf3tLCE69VWcOPpO?=
 =?us-ascii?Q?wTjAjmz9H0dxMmB+IcnkCutww60kQkig7xmblluoFHbC9IEe6r7XoS3H3Pbr?=
 =?us-ascii?Q?F837QTfSwxK/bkVkmDAYTtptyawdvGOJzsvkWweRf5X+uatSrElvhm6Cuarm?=
 =?us-ascii?Q?BACTLcWhxcOLqREZ5IyMp7MyEZRMQVvUSRokr0wu/fQ9vjHKeFH9C0PPOlGO?=
 =?us-ascii?Q?Z/6HDDfwZT0hDFBBVkYwjiJxqD/oQZ51kWk4o/uR/p8UQuQErvHiyxo5Ja92?=
 =?us-ascii?Q?72/soVqTn1XfsxGJX5uSWjxRsNFRWGRkVy7XpS3gMLYILOM36mpdtWz06fN+?=
 =?us-ascii?Q?VYjl4b0D/3hUlnBF4FhRMH2ytGV00hzjhPjz9vjMGlQAOSup+9ory6CFyJxl?=
 =?us-ascii?Q?L20yw/P3qAFZI2fLquZUtbZMTBclVziqkn0LCDL7gI2cF0nm5RK16P2WMgvE?=
 =?us-ascii?Q?2EBQMIdsEEcyPRXvkWBh2+bfY7npN0bMKed8zCMLjhQpIhJf78BDQXl6/UjI?=
 =?us-ascii?Q?B0DG5CTbdWwpVSf7T7OSQIhmLuccx+XE4FEL1tBakVaCVqlBMyi9BcXLXans?=
 =?us-ascii?Q?T+pzyXzxMohG//o1Oz/q2AcYlRsC4x8crHO1Y6xELXlnxbRhL1xQ5i4ciogj?=
 =?us-ascii?Q?tYvWT8fBj+QxdAnrscAuqRqofFhf034ccyROCIu0IS0oCEnuJj3auv7rqlg2?=
 =?us-ascii?Q?Qq/lVMg+cpXHDU3SG8L/uwWvSLcVzxE4ookaUURHmh/xWqZ39iU2tF7Hfwf5?=
 =?us-ascii?Q?nzOqbKtHVPIU69bMeTM85qQ83GNeamOchsicYKQ67XwqdNIIfrqd1nwPd2R2?=
 =?us-ascii?Q?u02vB1UZxNOZ0gdPQJEuNiiPsfKNOIpxNuVQOos3Z+cIEdDE05G4LUevAjz3?=
 =?us-ascii?Q?nffbwrKvg7yE3y/ywApD4fb+0mmnE72ctXVExkHDT8N3snX3BoHzk6Bxyk/l?=
 =?us-ascii?Q?Qg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <EB2F7BEC0CF3D44D95B08B0CF8A1AF90@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: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: eab330a8-801d-4042-4703-08dab5e51c50
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Oct 2022 17:28:05.3027
 (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: SNMm1Hfi/ZCKT8uZ6awdQxPs7ckvpgZbwmSMghArfdFoswsIfMjUbqd7g2oA8zFbiwOQn3CA3aEzL7RuO4oIBQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR10MB6188
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-24_05,2022-10-21_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=373 bulkscore=0
 suspectscore=0 malwarescore=0 phishscore=0 mlxscore=0 spamscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2210240106
X-Proofpoint-GUID: vsj8rulcL0j6-gY_K7vyBnNw73LA5xUN
X-Proofpoint-ORIG-GUID: vsj8rulcL0j6-gY_K7vyBnNw73LA5xUN
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Oct 24, 2022, at 6:57 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Mon, 2022-10-24 at 13:07 +1100, NeilBrown wrote:
>> On Thu, 20 Oct 2022, Chuck Lever III wrote:
>>>=20
>>>> On Oct 19, 2022, at 7:39 AM, Jeff Layton <jlayton@poochiereds.net> wro=
te:
>>>>=20
>>>>> -	fp =3D find_or_add_file(open->op_file, current_fh);
>>>>> +	rcu_read_lock();
>>>>> +	fp =3D insert_nfs4_file(open->op_file, current_fh);
>>>>> +	rcu_read_unlock();
>>>>=20
>>>> It'd probably be better to push this rcu_read_lock down into
>>>> insert_nfs4_file. You don't need to hold it over the actual insertion,
>>>> since that requires the state_lock.
>>>=20
>>> I used this arrangement because:
>>>=20
>>> insert_nfs4_file() invokes only find_nfs4_file() and the
>>> insert_file() helper. Both find_nfs4_file() and the
>>> insert_file() helper invoke rhltable_lookup(), which
>>> must be called with the RCU read lock held.
>>>=20
>>> And this is the reason why put_nfs4_file() no longer takes
>>> the state_lock: it would take the state_lock first and
>>> then the RCU read lock (which is implicitly taken in
>>> rhltable_remove()), which results in a lock inversion
>>> relative to insert_nfs4_file(), which takes the RCU read
>>> lock first, then the state_lock.
>>=20
>> It doesn't make any sense to talk about lock inversion with
>> rcu_read_lock().  It isn't really a lock in any traditional sense in
>> that it can never block (which is what cause lock-inversion problems).
>> I prefer to think for rcu_read_lock() as taking a reference on some
>> global state.
>>=20
>=20
> Right. To be clear, you can deadlock with synchronize_rcu if you use it
> improperly, but the rcu_read_lock itself never blocks.
>=20
>>>=20
>>>=20
>>> I'm certainly not an expert, so I'm willing to listen to
>>> alternative approaches. Can we rely on only the RCU read
>>> lock for exclusion on hash insertion?
>>=20
>> Probably we can.  I'll read through all the patches now and provide some
>> review.
>>=20
>=20
> The rcu_read_lock provides _no_ exclusion whatsoever, so it's not
> usually suitable for things that need exclusive access (like a hash
> insertion). If each rhl hash chain has its own lock though, then we may
> not require other locking to serialize insertions.

rhash does have per-bucket serialization using bit-spin-locks,
as far as I can tell.


--
Chuck Lever