From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Vasily Averin <vvs@virtuozzo.com>
CC:     Bruce Fields <bfields@redhat.com>,
        Konstantin Khorenko <khorenko@virtuozzo.com>,
        "Denis V. Lunev" <den@virtuozzo.com>,
        Cyrill Gorcunov <gorcunov@virtuozzo.com>,
        Jeff Layton <jlayton@kernel.org>,
        "kernel@openvz.org" <kernel@openvz.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] nfsd4: add refcount for nfsd4_blocked_lock
Thread-Topic: [PATCH] nfsd4: add refcount for nfsd4_blocked_lock
Thread-Index: AQHX8xJJi0i//mNmDUy6e6NXpDYxh6w9QeOA
Date:   Tue, 21 Dec 2021 17:56:03 +0000
Message-ID: <F7169092-65F0-4D1B-A8EF-846EC0177F26@oracle.com>
References: <CAPL3RVGBN4SMCBbJcrdgpdZfsfb+AFiHiLhDOFmjkD0ua+XpNQ@mail.gmail.com>
 <943cfe47-d48a-4a55-3738-e93370160508@virtuozzo.com>
In-Reply-To: <943cfe47-d48a-4a55-3738-e93370160508@virtuozzo.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?FrOa6/fx720AuX1q6XBs5Rnc0eMSDV6cif0h5qdXTvNjlvlMZPqkM9XZF5W7?=
 =?us-ascii?Q?yUINnbceBo/1egeLpvhT3jyViI5r0Wq3oCmMV1XORE6cB9Zao1S6a1mK8D1z?=
 =?us-ascii?Q?pCEt7PvbxsNJJ3eL63LYWbWBX8jO/YdERSeeOmotSiilIC9YHRNmnMZM+r+d?=
 =?us-ascii?Q?VUtno23Y7TSqdKkRGgLXuy6OtOl3S/HZ+023JmPGHF2T3+w3vngkuABQSRGa?=
 =?us-ascii?Q?4jowitpMYICLNqpWq+V1JFn3pBK68VDwj6O6l1INU88jJxkBA7cPfJjgeE0H?=
 =?us-ascii?Q?7KTQ2JgoM93faKJ3Dj9qyzYha5DQu0PAnq2P2itJ6/b3KibSF60Mcxv8R7UG?=
 =?us-ascii?Q?WHO1pQwbWiFXmltqRl4cHOlzk28KeIuzeiVkHJe4XD/TKEGUDGg1qVfw0RcD?=
 =?us-ascii?Q?P43E0RqR41SLw+PExNHWv5uY/hlbwDmERQbGStJOPenHM8Iz1OlYuPUgCJNK?=
 =?us-ascii?Q?6XdjGdk5YF/zB3MDDK89RlfPI0RRQretRHsBlt67dc4+/HC6CHs32eHzvUh7?=
 =?us-ascii?Q?wyhgKApLjWL8RuwfeQZZh7/DKXqulSyBsJpeJR2ao3E1XX77/hwqlURr+nd3?=
 =?us-ascii?Q?spJUuOtB70I5ztlAnT2i9DYTLwP0ce7mzimMI4/xicxody2UtvTr1Q45AuDu?=
 =?us-ascii?Q?I3x7A90wO0cph+ctlzH2zpSLauNsDEGwtsamOCdWFm2OyLZIolcZieDQEqly?=
 =?us-ascii?Q?Vbs9xmiFLArcd7MLyhBmXvugqIypXYMNhiyDkSL2EeDx7dG8GExMxvpWJvJj?=
 =?us-ascii?Q?8b3LR/fmYafGc7PBOTIM4vQ3vGHeJWq7aDtmbK3WypQGCc5Yp4xh1TbDg2Q/?=
 =?us-ascii?Q?6w802hfeOyih9P3wkTpmpPxuKXaMOliLZ4NsvNnk/hXWJ0TWiDWbOHVEd2ep?=
 =?us-ascii?Q?fuPioHlUIRvMtOkv9fCRT2ZFvsBjuTNtOieHlTHGibmZgWnNVC6hv5gldHan?=
 =?us-ascii?Q?277CP0QgSkp3kIb/n7ozj1uuwdOhSM3qMydFlvoW5fz40LNcRUVWiFjocR7z?=
 =?us-ascii?Q?H9A63+dKfro1LRgcZL/ZR6g6FnLlGQFs+NKzUWZmF5Ii81nXMhf8o3dQXxzZ?=
 =?us-ascii?Q?BdCYd6JL02824D+Q2mpBcF6b6xIt0GMprHwbnZupCz0GonYEPgEU13Fxbigq?=
 =?us-ascii?Q?AFnGM9pa4FhH6cLoQ4inhgVVyjLlEKvav0exAIJfcYJw5VYsoUfdnldk2Iq+?=
 =?us-ascii?Q?apkl2LGFg+BBx4bP9T0qoX7M7pDI4s9xL3cPg2fN0BkNaHDHqSLB4YZImeCX?=
 =?us-ascii?Q?n6+R25T8WAi0qfAZJZ9eynoDq/BdrfdSW2v9RUNbMMLqkAxVTd9Sk1teboy9?=
 =?us-ascii?Q?e2Q2Y/26rx2kpE/7kMxw/H4eBITYs2eUMgE4dPDbXq2A8urKHMGMsrS+eCLR?=
 =?us-ascii?Q?JWmQvTgpsAIJ7AlwUS8DyYVn8O5/Q7LUJkp9PZ10Mnm2Y22gCdDg0TrCCQy4?=
 =?us-ascii?Q?sI5IueCAAjStqiAQ8TrvKzv70Ir8geSn1SeZzxWy1bhXJaxnmBRB7afv0xOF?=
 =?us-ascii?Q?q0GGAp6nfnoo+a5ThlpbnvVK+7otPaGLhFixdsNVQP+v9qubkZ8nIzjSt1Qn?=
 =?us-ascii?Q?7IasOJQI4f6t4hWdQ1QuVHtGCvqgSb4lVTpZ4YKVlahkafj3oMgf69t3YpRC?=
 =?us-ascii?Q?p/HDocHQnsrw4GtpNAhTGBc=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1071198104E4CE48AE46C6FBB24525A3@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: CH0PR10MB4858.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ee1f7037-a1d9-46fa-8b53-08d9c4ab27ab
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Dec 2021 17:56:03.2391
 (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: /wPqKhi02QLAVEPpOfQ6n9n9bzJMR6Hpd++RZ6upGsZ/X1LUGt6+1TRLzPkJ4tzon8Qhrvl/BURLTeVOH+xKAg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR10MB3800
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10205 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 malwarescore=0 adultscore=0
 phishscore=0 suspectscore=0 mlxscore=0 mlxlogscore=999 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112210087
X-Proofpoint-GUID: XQtExIo74lVTNag2CCyBK27u9rcWO2nm
X-Proofpoint-ORIG-GUID: XQtExIo74lVTNag2CCyBK27u9rcWO2nm
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org

Hi Vasily-

> On Dec 17, 2021, at 1:49 AM, Vasily Averin <vvs@virtuozzo.com> wrote:
>=20
> nbl allocated in nfsd4_lock can be released by a several ways:
> directly in nfsd4_lock(), via nfs4_laundromat(), via another nfs
> command RELEASE_LOCKOWNER or via nfsd4_callback.
> This structure should be refcounted to be used and released correctly
> in all these cases.
>=20
> Refcount is initialized to 1 during allocation and is incremented
> when nbl is added into nbl_list/nbl_lru lists.
>=20
> Usually nbl is linked into both lists together, so only one refcount
> is used for both lists.
>=20
> However nfsd4_lock() should keep in mind that nbl can be present
> in one of lists only. This can happen if nbl was handled already
> by nfs4_laundromat/nfsd4_callback/etc.
>=20
> Refcount is decremented if vfs_lock_file() returns FILE_LOCK_DEFERRED,
> because nbl can be handled already by nfs4_laundromat/nfsd4_callback/etc.
>=20
> Refcount is not changed in find_blocked_lock() because of it reuses count=
er
> released after removing nbl from lists.
>=20
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Thanks. Applied provisionally to the for-next branch at
git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git


> ---
> fs/nfsd/nfs4state.c | 25 ++++++++++++++++++++++---
> fs/nfsd/state.h     |  1 +
> 2 files changed, 23 insertions(+), 3 deletions(-)
>=20
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d75e1235c4f5..b74f36e9901c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -246,6 +246,7 @@ find_blocked_lock(struct nfs4_lockowner *lo, struct k=
nfsd_fh *fh,
> 	list_for_each_entry(cur, &lo->lo_blocked, nbl_list) {
> 		if (fh_match(fh, &cur->nbl_fh)) {
> 			list_del_init(&cur->nbl_list);
> +			WARN_ON(list_empty(&cur->nbl_lru));
> 			list_del_init(&cur->nbl_lru);
> 			found =3D cur;
> 			break;
> @@ -271,6 +272,7 @@ find_or_allocate_block(struct nfs4_lockowner *lo, str=
uct knfsd_fh *fh,
> 			INIT_LIST_HEAD(&nbl->nbl_lru);
> 			fh_copy_shallow(&nbl->nbl_fh, fh);
> 			locks_init_lock(&nbl->nbl_lock);
> +			kref_init(&nbl->nbl_kref);
> 			nfsd4_init_cb(&nbl->nbl_cb, lo->lo_owner.so_client,
> 					&nfsd4_cb_notify_lock_ops,
> 					NFSPROC4_CLNT_CB_NOTIFY_LOCK);
> @@ -279,12 +281,21 @@ find_or_allocate_block(struct nfs4_lockowner *lo, s=
truct knfsd_fh *fh,
> 	return nbl;
> }
>=20
> +static void
> +free_nbl(struct kref *kref)
> +{
> +	struct nfsd4_blocked_lock *nbl;
> +
> +	nbl =3D container_of(kref, struct nfsd4_blocked_lock, nbl_kref);
> +	kfree(nbl);
> +}
> +
> static void
> free_blocked_lock(struct nfsd4_blocked_lock *nbl)
> {
> 	locks_delete_block(&nbl->nbl_lock);
> 	locks_release_private(&nbl->nbl_lock);
> -	kfree(nbl);
> +	kref_put(&nbl->nbl_kref, free_nbl);
> }
>=20
> static void
> @@ -302,6 +313,7 @@ remove_blocked_locks(struct nfs4_lockowner *lo)
> 					struct nfsd4_blocked_lock,
> 					nbl_list);
> 		list_del_init(&nbl->nbl_list);
> +		WARN_ON(list_empty(&nbl->nbl_lru));
> 		list_move(&nbl->nbl_lru, &reaplist);
> 	}
> 	spin_unlock(&nn->blocked_locks_lock);
> @@ -6976,6 +6988,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_com=
pound_state *cstate,
> 		spin_lock(&nn->blocked_locks_lock);
> 		list_add_tail(&nbl->nbl_list, &lock_sop->lo_blocked);
> 		list_add_tail(&nbl->nbl_lru, &nn->blocked_locks_lru);
> +		kref_get(&nbl->nbl_kref);
> 		spin_unlock(&nn->blocked_locks_lock);
> 	}
>=20
> @@ -6988,6 +7001,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_com=
pound_state *cstate,
> 			nn->somebody_reclaimed =3D true;
> 		break;
> 	case FILE_LOCK_DEFERRED:
> +		kref_put(&nbl->nbl_kref, free_nbl);
> 		nbl =3D NULL;
> 		fallthrough;
> 	case -EAGAIN:		/* conflock holds conflicting lock */
> @@ -7008,8 +7022,13 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_co=
mpound_state *cstate,
> 		/* dequeue it if we queued it before */
> 		if (fl_flags & FL_SLEEP) {
> 			spin_lock(&nn->blocked_locks_lock);
> -			list_del_init(&nbl->nbl_list);
> -			list_del_init(&nbl->nbl_lru);
> +			if (!list_empty(&nbl->nbl_list) &&
> +			    !list_empty(&nbl->nbl_lru)) {
> +				list_del_init(&nbl->nbl_list);
> +				list_del_init(&nbl->nbl_lru);
> +				kref_put(&nbl->nbl_kref, free_nbl);
> +			}
> +			/* nbl can use one of lists to be linked to reaplist */
> 			spin_unlock(&nn->blocked_locks_lock);
> 		}
> 		free_blocked_lock(nbl);
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index e73bdbb1634a..ab61dc102300 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -629,6 +629,7 @@ struct nfsd4_blocked_lock {
> 	struct file_lock	nbl_lock;
> 	struct knfsd_fh		nbl_fh;
> 	struct nfsd4_callback	nbl_cb;
> +	struct kref		nbl_kref;
> };
>=20
> struct nfsd4_compound_state;
> --=20
> 2.25.1
>=20

--
Chuck Lever