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:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH RFC] NFSD: Fix possible sleep during
 nfsd4_release_lockowner()
Thread-Topic: [PATCH RFC] NFSD: Fix possible sleep during
 nfsd4_release_lockowner()
Thread-Index: AQHYbfIMzaj7dJY/skSZB97V0kxLh60seYcAgAAWLQCAAAdIAIAABFwAgAAPm4CAAA1wgA==
Date:   Mon, 23 May 2022 17:25:48 +0000
Message-ID: <FF7F2939-C3DE-4584-BFFA-13B554706B9C@oracle.com>
References: <165323344948.2381.7808135229977810927.stgit@bazille.1015granger.net>
 <fe3f9ece807e1433631ee3e0bd6b78238305cb87.camel@kernel.org>
 <510282CB-38D3-438A-AF8A-9AC2519FCEF7@oracle.com>
 <c3d053dc36dd5e7dee1267f1c7107bbf911e4d53.camel@kernel.org>
 <1A37E2B5-8113-48D6-AF7C-5381F364D99E@oracle.com>
 <c357e63272de96f9e7595bf6688680879d83dc83.camel@kernel.org>
In-Reply-To: <c357e63272de96f9e7595bf6688680879d83dc83.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?2y88tTb5EOKpoO3erdEjAYpptWqAitF3PziJqA9UOnHgZQyFzCKDc+tOFgIG?=
 =?us-ascii?Q?h3dMQ1dPY/L1EdeLHe6fZVEi+hs8NL5xkDK0NO5jhPhxxFfge5K/xAg1ndA3?=
 =?us-ascii?Q?njxuriP1O/afuMCHqGjkob5HMcJi6AsAbWuuSIbeLFeULqWnMkOTg2nB/VcS?=
 =?us-ascii?Q?7BQw735uIFAEvEZewIe45XoG13qPisQPX2u8B+jIaYfYXl+deSfBdUHl8+xZ?=
 =?us-ascii?Q?YajP5IiMFKspUHPVR9mx9H6pK1pMUEPrPPIztAX5ECip1qSeb/qCrw3dWccW?=
 =?us-ascii?Q?BgbZ42ebJFkLDLBmXKe9iKF7rxhBJaoMK1x92knk2xfboEnh+ECcUNKhATT7?=
 =?us-ascii?Q?EZp9fnMSHEKGsGzD39KWJCe7Plrkjlk2i2wPsgMtZwBKJ+Wg//IM/v7D4PSR?=
 =?us-ascii?Q?wOfq5HrHeJqfdRm230NeOT0YHhpNzCf4VkTEqmVfe5lSudP7QtYBXNl3MV+e?=
 =?us-ascii?Q?orp6FlI6i6aqFKGxsf+6j4jRaGYIYHck9XmPMpeYBTP9sIYsMK3DjLYdujMu?=
 =?us-ascii?Q?ZMHa2Bumt5x1nVkelHdyWIhfnAYkLcpjMxV7m4eJ+wryVaiwN5IfHE1QezPP?=
 =?us-ascii?Q?zeSMSibSENWp6HkiY/k4G4uzKLqGxkqoMFneQffL3G8B3mkRNTU9Ynl8yQwJ?=
 =?us-ascii?Q?5rf90PIEQe5AoUsWFmeamzAcgXNkjb4MtFUyH4S66pcxSUftZSpobPiGa0fD?=
 =?us-ascii?Q?MfZQpjGnCTtBXG4VZbV09sQeWppqASYBjcaIvNGPU8QDlB/OsngdCHrfy3gv?=
 =?us-ascii?Q?SqZe+WVPW02J+cts4S7TRIqvliYPzTQMj2g8fZ31I8bvIXhdej/5mEh+aqyZ?=
 =?us-ascii?Q?6iP64hyjZgCkaaokQEmYete2rJ99qFWjbD4++w9lnjL6Ef8zjg1xSV+ERzxB?=
 =?us-ascii?Q?s0InpWJo0r8+i7ZgT4Qjg5kU7TLvruUIipC/iin4TvXhl1Y6EfDJ5RkWz0j2?=
 =?us-ascii?Q?94KI0CmysleBNxZXXi0fum3P5o4ki+DdIjwyJZp7UsjHGnjuCQEyrLTAEHhr?=
 =?us-ascii?Q?i6mvz9Ixydq/Lq/Of4Sumby9DqADtS3mwavVmgIz4ppwhZre/AHliJc6Y4k6?=
 =?us-ascii?Q?4NNV6agAULVfK0FuSaVGmCpLIWwBhEG7prvgBSDrzXOHTApai1xvULJlZ6jJ?=
 =?us-ascii?Q?7tAOpBohODTfEPKBUBaMZPsDICDH8U1tc3aj9AisoqC5/WtqhtysFF0lq2v0?=
 =?us-ascii?Q?f/Ck0kl6oaU9u2VXsXc6qJL1Dss7zSiXG0LmOOv4obfnWgdz9LP3l08CS+rD?=
 =?us-ascii?Q?XOT046eZYgQLeXgeHysVLpIGcsPCujat7mwsYb/YisrrNLDqrPtWplSBGt78?=
 =?us-ascii?Q?WBSV5a1KzIiFWoJQtQV/U4C9UZpJG/Z4nqbq0oy5O6CuLNkU9fmObWvUuosr?=
 =?us-ascii?Q?/uBpy4mizbKR814XF6k99CB78nmu85x1ZsA3oJTPkseYWD5bNLjQKAVFHekI?=
 =?us-ascii?Q?qCGPRTx6iR4RcHvjzonTQQX/6YS2qlp4VlOAN82PAkZgGMzwNzYbM1G67ow0?=
 =?us-ascii?Q?h61y3qIRF75NuLDtQayIdXBjtCX7/AdS2v2f0alnXIWOk56fQS3sJJ173lO9?=
 =?us-ascii?Q?XxvPdq2JPXpfJFUFbTACozT5adGRQVuo8eAFHeqYJF7iYocDaKzKK7Aoutrv?=
 =?us-ascii?Q?BgLcg+4yYoekuvQskgbHZAyrWAp8wvBfB753VG9EG4nnxTobJJ11nFtoBkTT?=
 =?us-ascii?Q?cZiqdef7j9JC9FjaGzlTC3/5hUXkI7/qj4IwJN0TbLRjX3UPM50GHFqF3V+6?=
 =?us-ascii?Q?8A07XEtcNhBb0hbQOi3vIpgu2CviUZs=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <FB9B1A7EDBE5004284633219445EB806@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: 7c9394a9-a898-4902-712d-08da3ce146fc
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 May 2022 17:25:48.2291
 (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: 4QhFXWvKbO8F1Ul0zzNRJmanojaQwDZYJ5ojjX7V2rPrG1XSg+xJh3+JVU4DSNF2IKH9ZxrdLEWBgElaoXCqjw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5486
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.874
 definitions=2022-05-23_07:2022-05-23,2022-05-23 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 bulkscore=0
 mlxscore=0 mlxlogscore=999 suspectscore=0 adultscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2205230098
X-Proofpoint-ORIG-GUID: TXDgN5NcEXQtSh5AOiNS5nUik1yRbmGJ
X-Proofpoint-GUID: TXDgN5NcEXQtSh5AOiNS5nUik1yRbmGJ
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,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE 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 May 23, 2022, at 12:37 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Mon, 2022-05-23 at 15:41 +0000, Chuck Lever III wrote:
>>=20
>>> On May 23, 2022, at 11:26 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>>=20
>>> What I was going to suggest is a nfsd_file_put variant that takes a
>>> list_head. If the refcount goes to zero and the thing ends up being
>>> unhashed, then you put it on the dispose list rather than doing the
>>> blocking operations, and then clean it up later.
>>=20
>> Trond doesn't like that approach; see the e-mail thread.
>=20
> I didn't see him saying that that would be wrong, per-se, but the
> initial implementation was racy.

I proposed this for check_for_locks() to use:

> void nfsd_file_put_async(struct nfsd_file *nf)
> {
> 	if (refcount_dec_and_test(&nf->nf_ref))
> 		nfsd_file_close_inode(nf->nf_inode);
> }


Trond's response was:

> That approach moves the sync to the garbage collector, which was
> exactly what we're trying to avoid in the first place.

Basically he's opposed to any flush work being done by
the garbage collector because that has a known negative
performance impact.


> His suggestion was just to keep a counter in the lockowner of how many
> locks are associated with it. That seems like a good suggestion, though
> you'd probably need to add a parameter to lm_get_owner to indicate
> whether you were adding a new lock or just doing a conflock copy.

locks_copy_conflock() would need to take a boolean parameter
that callers would set when they actually manipulate a lock.

I would feel more comfortable with that approach if
locks_copy_conflock() was a private API used only in fs/locks.c
so we can audit its usage to ensure that callers are managing
the lock count correctly.


--
Chuck Lever