> On May 13, 2022, at 12:34 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> wrote:
>
> Problem caused by check_for_locks() calling nfsd_file_put while
> holding the cl_lock.
>
> Fix by moving nfsd_file_put to callers of check_for_locks().
> nfsd4_release_lockowner delays calling nfsd_file_put until after
> releasing the cl_lock.
>
> Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
> ---
> fs/nfsd/filecache.c | 13 +++++++++++++
> fs/nfsd/filecache.h | 2 ++
> fs/nfsd/nfs4state.c | 41 +++++++++++++++++++++++------------------
> 3 files changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 2c1b027774d4..4a8ae1e562d2 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -175,6 +175,7 @@ nfsd_file_alloc(struct inode *inode, unsigned int may, unsigned int hashval,
> if (nf) {
> INIT_HLIST_NODE(&nf->nf_node);
> INIT_LIST_HEAD(&nf->nf_lru);
> + INIT_LIST_HEAD(&nf->nf_putfile);
> nf->nf_file = NULL;
> nf->nf_cred = get_current_cred();
> nf->nf_net = net;
> @@ -315,6 +316,18 @@ nfsd_file_put(struct nfsd_file *nf)
> nfsd_file_gc();
> }
>
> +void
> +nfsd_file_bulk_put(struct list_head *putlist)
> +{
> + struct nfsd_file *nf;
> +
> + while (!list_empty(putlist)) {
> + nf = list_first_entry(putlist, struct nfsd_file, nf_putfile);
> + list_del_init(&nf->nf_putfile);
> + nfsd_file_put(nf);
> + }
> +}
> +
> struct nfsd_file *
> nfsd_file_get(struct nfsd_file *nf)
> {
> diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
> index 435ceab27897..2d775fea431a 100644
> --- a/fs/nfsd/filecache.h
> +++ b/fs/nfsd/filecache.h
> @@ -46,6 +46,7 @@ struct nfsd_file {
> refcount_t nf_ref;
> unsigned char nf_may;
> struct nfsd_file_mark *nf_mark;
> + struct list_head nf_putfile;
> };
>
> int nfsd_file_cache_init(void);
> @@ -54,6 +55,7 @@ void nfsd_file_cache_shutdown(void);
> int nfsd_file_cache_start_net(struct net *net);
> void nfsd_file_cache_shutdown_net(struct net *net);
> void nfsd_file_put(struct nfsd_file *nf);
> +void nfsd_file_bulk_put(struct list_head *putlist);
> struct nfsd_file *nfsd_file_get(struct nfsd_file *nf);
> void nfsd_file_close_inode_sync(struct inode *inode);
> bool nfsd_file_is_cached(struct inode *inode);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 234e852fcdfa..1486f77541fe 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -80,7 +80,7 @@ static u64 current_sessionid = 1;
> #define CLOSE_STATEID(stateid) (!memcmp((stateid), &close_stateid, sizeof(stateid_t)))
>
> /* forward declarations */
> -static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
> +static bool check_for_locks(struct nfsd_file *nf, struct nfs4_lockowner *lowner);
> static void nfs4_free_ol_stateid(struct nfs4_stid *stid);
> void nfsd4_end_grace(struct nfsd_net *nn);
> static void _free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps);
> @@ -6139,6 +6139,7 @@ static __be32
> nfsd4_free_lock_stateid(stateid_t *stateid, struct nfs4_stid *s)
> {
> struct nfs4_ol_stateid *stp = openlockstateid(s);
> + struct nfsd_file *nf;
> __be32 ret;
>
> ret = nfsd4_lock_ol_stateid(stp);
> @@ -6150,9 +6151,14 @@ nfsd4_free_lock_stateid(stateid_t *stateid, struct nfs4_stid *s)
> goto out;
>
> ret = nfserr_locks_held;
> - if (check_for_locks(stp->st_stid.sc_file,
> - lockowner(stp->st_stateowner)))
> - goto out;
> + nf = find_any_file(stp->st_stid.sc_file);
> + if (nf) {
> + if (check_for_locks(nf, lockowner(stp->st_stateowner))) {
> + nfsd_file_put(nf);
> + goto out;
> + }
> + nfsd_file_put(nf);
> + }
>
> release_lock_stateid(stp);
> ret = nfs_ok;
> @@ -7266,20 +7272,13 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> * false: no locks held by lockowner
> */
> static bool
> -check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner)
> +check_for_locks(struct nfsd_file *nf, struct nfs4_lockowner *lowner)
> {
> struct file_lock *fl;
> int status = false;
> - struct nfsd_file *nf = find_any_file(fp);
> struct inode *inode;
> struct file_lock_context *flctx;
>
> - if (!nf) {
> - /* Any valid lock stateid should have some sort of access */
> - WARN_ON_ONCE(1);
> - return status;
> - }
> -
> inode = locks_inode(nf->nf_file);
> flctx = inode->i_flctx;
>
> @@ -7293,7 +7292,6 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner)
> }
> spin_unlock(&flctx->flc_lock);
> }
> - nfsd_file_put(nf);
> return status;
> }
>
> @@ -7313,6 +7311,8 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> struct nfs4_client *clp;
> LIST_HEAD (reaplist);
> + LIST_HEAD(putlist);
> + struct nfsd_file *nf;
>
> dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
> clid->cl_boot, clid->cl_id);
> @@ -7333,13 +7333,17 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> /* see if there are still any locks associated with it */
> lo = lockowner(sop);
> list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
> - if (check_for_locks(stp->st_stid.sc_file, lo)) {
> - status = nfserr_locks_held;
> - spin_unlock(&clp->cl_lock);
> - return status;
> + nf = find_any_file(stp->st_stid.sc_file);
> + if (nf) {
> + list_add(&nf->nf_putfile, &putlist);
> + if (check_for_locks(nf, lo)) {
> + status = nfserr_locks_held;
> + spin_unlock(&clp->cl_lock);
> + nfsd_file_bulk_put(&putlist);
> + return status;
> + }
> }
> }
> -
> nfs4_get_stateowner(sop);
> break;
> }
> @@ -7357,6 +7361,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> put_ol_stateid_locked(stp, &reaplist);
> }
> spin_unlock(&clp->cl_lock);
> + nfsd_file_bulk_put(&putlist);
> free_ol_stateid_reaplist(&reaplist);
> remove_blocked_locks(lo);
> nfs4_put_stateowner(&lo->lo_owner);
> --
> 2.9.5
This seems like a reasonable solution to me. Still teetering on
whether it should go into 5.18-rc instead of 5.19:
- The performance regression fix merged in 18-rc3 does make the
existing "sleeping function" problem worse, so maybe this fix
should go into 18-rc as well?
- But it's pretty late in the 18-rc cycle, and this is a sizable
code change.
- Greg and Sasha can take this patch in 5.18.1 quickly if it goes
only into v5.19.
--
Chuck Lever
