[PATCH 0/6] nfsd: random cleanup and doc work

[PATCH 0/6] nfsd: random cleanup and doc work

[Jeff Layton]

Just a pile of cleanup and doc patches. None of these fix noticeable
bugs, but a couple of them should make things slightly more efficient
and (hopefully) more clear for developers.

These are probably v6.3 material.

Jeff Layton (6):
  nfsd: don't take nfsd4_copy ref for OP_OFFLOAD_STATUS
  nfsd: eliminate find_deleg_file_locked
  nfsd: simplify the delayed disposal list code
  nfsd: don't take/put an extra reference when putting a file
  nfsd: add some kerneldoc comments for stateid preprocessing functions
  nfsd: eliminate __nfs4_get_fd

 fs/nfsd/filecache.c | 68 +++++++++++++++------------------------------
 fs/nfsd/nfs4proc.c  | 35 +++++++++++++++--------
 fs/nfsd/nfs4state.c | 60 +++++++++++++++++++++------------------
 fs/nfsd/state.h     |  2 --
 4 files changed, 79 insertions(+), 86 deletions(-)

-- 
2.39.0

[PATCH 1/6] nfsd: don't take nfsd4_copy ref for OP_OFFLOAD_STATUS

[Jeff Layton]

We're not doing any blocking operations for OP_OFFLOAD_STATUS, so taking
and putting a reference is a waste of effort. Take the client lock,
search for the copy and fetch the wr_bytes_written field and return.

Also, make find_async_copy a static function.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4proc.c | 35 ++++++++++++++++++++++++-----------
 fs/nfsd/state.h    |  2 --
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 62b9d6c1b18b..731c2b22f163 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1823,23 +1823,34 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	goto out;
 }
 
-struct nfsd4_copy *
-find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
+static struct nfsd4_copy *
+find_async_copy_locked(struct nfs4_client *clp, stateid_t *stateid)
 {
 	struct nfsd4_copy *copy;
 
-	spin_lock(&clp->async_lock);
+	lockdep_assert_held(&clp->async_lock);
+
 	list_for_each_entry(copy, &clp->async_copies, copies) {
 		if (memcmp(&copy->cp_stateid.cs_stid, stateid, NFS4_STATEID_SIZE))
 			continue;
-		refcount_inc(&copy->refcount);
-		spin_unlock(&clp->async_lock);
 		return copy;
 	}
-	spin_unlock(&clp->async_lock);
 	return NULL;
 }
 
+static struct nfsd4_copy *
+find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
+{
+	struct nfsd4_copy *copy;
+
+	spin_lock(&clp->async_lock);
+	copy = find_async_copy_locked(clp, stateid);
+	if (copy)
+		refcount_inc(&copy->refcount);
+	spin_unlock(&clp->async_lock);
+	return copy;
+}
+
 static __be32
 nfsd4_offload_cancel(struct svc_rqst *rqstp,
 		     struct nfsd4_compound_state *cstate,
@@ -1924,22 +1935,24 @@ nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nfsd_file_put(nf);
 	return status;
 }
+
 static __be32
 nfsd4_offload_status(struct svc_rqst *rqstp,
 		     struct nfsd4_compound_state *cstate,
 		     union nfsd4_op_u *u)
 {
 	struct nfsd4_offload_status *os = &u->offload_status;
-	__be32 status = 0;
+	__be32 status = nfs_ok;
 	struct nfsd4_copy *copy;
 	struct nfs4_client *clp = cstate->clp;
 
-	copy = find_async_copy(clp, &os->stateid);
-	if (copy) {
+	spin_lock(&clp->async_lock);
+	copy = find_async_copy_locked(clp, &os->stateid);
+	if (copy)
 		os->count = copy->cp_res.wr_bytes_written;
-		nfs4_put_copy(copy);
-	} else
+	else
 		status = nfserr_bad_stateid;
+	spin_unlock(&clp->async_lock);
 
 	return status;
 }
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e94634d30591..d49d3060ed4f 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -705,8 +705,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name
 extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
 
 void put_nfs4_file(struct nfs4_file *fi);
-extern struct nfsd4_copy *
-find_async_copy(struct nfs4_client *clp, stateid_t *staetid);
 extern void nfs4_put_cpntf_state(struct nfsd_net *nn,
 				 struct nfs4_cpntf_state *cps);
 extern __be32 manage_cpntf_state(struct nfsd_net *nn, stateid_t *st,
-- 
2.39.0

[PATCH 2/6] nfsd: eliminate find_deleg_file_locked

[Jeff Layton]

We really don't need an accessor function here.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4dd0e8dbcbfa..57f24e3e46a0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -686,15 +686,6 @@ static struct nfsd_file *find_any_file_locked(struct nfs4_file *f)
 	return NULL;
 }
 
-static struct nfsd_file *find_deleg_file_locked(struct nfs4_file *f)
-{
-	lockdep_assert_held(&f->fi_lock);
-
-	if (f->fi_deleg_file)
-		return f->fi_deleg_file;
-	return NULL;
-}
-
 static atomic_long_t num_delegations;
 unsigned long max_delegations;
 
@@ -2703,7 +2694,7 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
 	ds = delegstateid(st);
 	nf = st->sc_file;
 	spin_lock(&nf->fi_lock);
-	file = find_deleg_file_locked(nf);
+	file = nf->fi_deleg_file;
 	if (!file)
 		goto out;
 
-- 
2.39.0

[PATCH 3/6] nfsd: simplify the delayed disposal list code

[Jeff Layton]

When queueing a dispose list to the appropriate "freeme" lists, it
pointlessly queues the objects one at a time to an intermediate list.

Remove a few helpers and just open code a list_move to make it more
clear and efficient. Better document the resulting functions with
kerneldoc comments.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 63 +++++++++++++++------------------------------
 1 file changed, 21 insertions(+), 42 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 58ac93e7e680..a2bc4bd90b9a 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head *dispose)
 	}
 }
 
-static void
-nfsd_file_list_remove_disposal(struct list_head *dst,
-		struct nfsd_fcache_disposal *l)
-{
-	spin_lock(&l->lock);
-	list_splice_init(&l->freeme, dst);
-	spin_unlock(&l->lock);
-}
-
-static void
-nfsd_file_list_add_disposal(struct list_head *files, struct net *net)
-{
-	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
-	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
-
-	spin_lock(&l->lock);
-	list_splice_tail_init(files, &l->freeme);
-	spin_unlock(&l->lock);
-	queue_work(nfsd_filecache_wq, &l->work);
-}
-
-static void
-nfsd_file_list_add_pernet(struct list_head *dst, struct list_head *src,
-		struct net *net)
-{
-	struct nfsd_file *nf, *tmp;
-
-	list_for_each_entry_safe(nf, tmp, src, nf_lru) {
-		if (nf->nf_net == net)
-			list_move_tail(&nf->nf_lru, dst);
-	}
-}
-
+/**
+ * nfsd_file_dispose_list_delayed - move list of dead files to net's freeme list
+ * @dispose: list of nfsd_files to be disposed
+ *
+ * Transfers each file to the "freeme" list for its nfsd_net, to eventually
+ * be disposed of by the per-net garbage collector.
+ */
 static void
 nfsd_file_dispose_list_delayed(struct list_head *dispose)
 {
-	LIST_HEAD(list);
-	struct nfsd_file *nf;
-
 	while(!list_empty(dispose)) {
-		nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
-		nfsd_file_list_add_pernet(&list, dispose, nf->nf_net);
-		nfsd_file_list_add_disposal(&list, nf->nf_net);
+		struct nfsd_file *nf = list_first_entry(dispose,
+						struct nfsd_file, nf_lru);
+		struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
+		struct nfsd_fcache_disposal *l = nn->fcache_disposal;
+
+		spin_lock(&l->lock);
+		list_move_tail(&nf->nf_lru, &l->freeme);
+		spin_unlock(&l->lock);
 	}
 }
 
@@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode)
  * nfsd_file_delayed_close - close unused nfsd_files
  * @work: dummy
  *
- * Walk the LRU list and destroy any entries that have not been used since
- * the last scan.
+ * Scrape the freeme list for this nfsd_net, and then dispose of them
+ * all.
  */
 static void
 nfsd_file_delayed_close(struct work_struct *work)
@@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct *work)
 	struct nfsd_fcache_disposal *l = container_of(work,
 			struct nfsd_fcache_disposal, work);
 
-	nfsd_file_list_remove_disposal(&head, l);
+	spin_lock(&l->lock);
+	list_splice_init(&l->freeme, &head);
+	spin_unlock(&l->lock);
+
 	nfsd_file_dispose_list(&head);
 }
 
-- 
2.39.0

[PATCH 4/6] nfsd: don't take/put an extra reference when putting a file

[Jeff Layton]

The last thing that filp_close does is an fput, so don't bother taking
and putting the extra reference.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index a2bc4bd90b9a..f604dd8109e4 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -401,11 +401,8 @@ nfsd_file_free(struct nfsd_file *nf)
 
 	if (nf->nf_mark)
 		nfsd_file_mark_put(nf->nf_mark);
-	if (nf->nf_file) {
-		get_file(nf->nf_file);
+	if (nf->nf_file)
 		filp_close(nf->nf_file, NULL);
-		fput(nf->nf_file);
-	}
 
 	/*
 	 * If this item is still linked via nf_lru, that's a bug.
-- 
2.39.0

[PATCH 5/6] nfsd: add some kerneldoc comments for stateid preprocessing functions

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 57f24e3e46a0..06a95f25c522 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6539,8 +6539,19 @@ void nfs4_put_cpntf_state(struct nfsd_net *nn, struct nfs4_cpntf_state *cps)
 	spin_unlock(&nn->s2s_cp_lock);
 }
 
-/*
- * Checks for stateid operations
+/**
+ * nfs4_preprocess_stateid_op - find and prep stateid for an operation
+ * @rqstp: incoming request from client
+ * @cstate: current compound state
+ * @fhp: filehandle associated with requested stateid
+ * @stateid: stateid (provided by client)
+ * @flags: flags describing type of operation to be done
+ * @nfp: optional nfsd_file return pointer (may be NULL)
+ * @cstid: optional returned nfs4_stid pointer (may be NULL)
+ *
+ * Given info from the client, look up a nfs4_stid for the operation. On
+ * success, it returns a reference to the nfs4_stid and/or the nfsd_file
+ * associated with it.
  */
 __be32
 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
@@ -6729,8 +6740,18 @@ static __be32 nfs4_seqid_op_checks(struct nfsd4_compound_state *cstate, stateid_
 	return status;
 }
 
-/* 
- * Checks for sequence id mutating operations. 
+/**
+ * nfs4_preprocess_seqid_op - find and prep an ol_stateid for a seqid-morphing op
+ * @cstate: compund state
+ * @seqid: seqid (provided by client)
+ * @stateid: stateid (provided by client)
+ * @typemask: mask of allowable types for this operation
+ * @stpp: return pointer for the stateid found
+ * @nn: net namespace for request
+ *
+ * Given a stateid+seqid from a client, look up an nfs4_ol_stateid and
+ * return it in @stpp. On a nfs_ok return, the returned stateid will
+ * have its st_mutex locked.
  */
 static __be32
 nfs4_preprocess_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
-- 
2.39.0

[PATCH 6/6] nfsd: eliminate __nfs4_get_fd

[Jeff Layton]

This is wrapper is pointless, and just obscures what's going on.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 06a95f25c522..e61b878a4b45 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -599,12 +599,6 @@ put_nfs4_file(struct nfs4_file *fi)
 	}
 }
 
-static struct nfsd_file *
-__nfs4_get_fd(struct nfs4_file *f, int oflag)
-{
-	return nfsd_file_get(f->fi_fds[oflag]);
-}
-
 static struct nfsd_file *
 find_writeable_file_locked(struct nfs4_file *f)
 {
@@ -612,9 +606,9 @@ find_writeable_file_locked(struct nfs4_file *f)
 
 	lockdep_assert_held(&f->fi_lock);
 
-	ret = __nfs4_get_fd(f, O_WRONLY);
+	ret = nfsd_file_get(f->fi_fds[O_WRONLY]);
 	if (!ret)
-		ret = __nfs4_get_fd(f, O_RDWR);
+		ret = nfsd_file_get(f->fi_fds[O_RDWR]);
 	return ret;
 }
 
@@ -637,9 +631,9 @@ find_readable_file_locked(struct nfs4_file *f)
 
 	lockdep_assert_held(&f->fi_lock);
 
-	ret = __nfs4_get_fd(f, O_RDONLY);
+	ret = nfsd_file_get(f->fi_fds[O_RDONLY]);
 	if (!ret)
-		ret = __nfs4_get_fd(f, O_RDWR);
+		ret = nfsd_file_get(f->fi_fds[O_RDWR]);
 	return ret;
 }
 
@@ -663,11 +657,11 @@ find_any_file(struct nfs4_file *f)
 	if (!f)
 		return NULL;
 	spin_lock(&f->fi_lock);
-	ret = __nfs4_get_fd(f, O_RDWR);
+	ret = nfsd_file_get(f->fi_fds[O_RDWR]);
 	if (!ret) {
-		ret = __nfs4_get_fd(f, O_WRONLY);
+		ret = nfsd_file_get(f->fi_fds[O_WRONLY]);
 		if (!ret)
-			ret = __nfs4_get_fd(f, O_RDONLY);
+			ret = nfsd_file_get(f->fi_fds[O_RDONLY]);
 	}
 	spin_unlock(&f->fi_lock);
 	return ret;
-- 
2.39.0

Re: [PATCH 1/6] nfsd: don't take nfsd4_copy ref for OP_OFFLOAD_STATUS

[Olga Kornievskaia]

On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> We're not doing any blocking operations for OP_OFFLOAD_STATUS, so taking
> and putting a reference is a waste of effort. Take the client lock,
> search for the copy and fetch the wr_bytes_written field and return.
>
> Also, make find_async_copy a static function.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4proc.c | 35 ++++++++++++++++++++++++-----------
>  fs/nfsd/state.h    |  2 --
>  2 files changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 62b9d6c1b18b..731c2b22f163 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1823,23 +1823,34 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>         goto out;
>  }
>
> -struct nfsd4_copy *
> -find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
> +static struct nfsd4_copy *
> +find_async_copy_locked(struct nfs4_client *clp, stateid_t *stateid)
>  {
>         struct nfsd4_copy *copy;
>
> -       spin_lock(&clp->async_lock);
> +       lockdep_assert_held(&clp->async_lock);
> +
>         list_for_each_entry(copy, &clp->async_copies, copies) {
>                 if (memcmp(&copy->cp_stateid.cs_stid, stateid, NFS4_STATEID_SIZE))
>                         continue;
> -               refcount_inc(&copy->refcount);

If we don't take a refcount on the copy, this copy could be removed
between the time we found it in the list of copies and when we then
look inside to check the amount written so far. This would lead to a
null (or bad) pointer dereference?

> -               spin_unlock(&clp->async_lock);
>                 return copy;
>         }
> -       spin_unlock(&clp->async_lock);
>         return NULL;
>  }
>
> +static struct nfsd4_copy *
> +find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
> +{
> +       struct nfsd4_copy *copy;
> +
> +       spin_lock(&clp->async_lock);
> +       copy = find_async_copy_locked(clp, stateid);
> +       if (copy)
> +               refcount_inc(&copy->refcount);
> +       spin_unlock(&clp->async_lock);
> +       return copy;
> +}
> +
>  static __be32
>  nfsd4_offload_cancel(struct svc_rqst *rqstp,
>                      struct nfsd4_compound_state *cstate,
> @@ -1924,22 +1935,24 @@ nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>         nfsd_file_put(nf);
>         return status;
>  }
> +
>  static __be32
>  nfsd4_offload_status(struct svc_rqst *rqstp,
>                      struct nfsd4_compound_state *cstate,
>                      union nfsd4_op_u *u)
>  {
>         struct nfsd4_offload_status *os = &u->offload_status;
> -       __be32 status = 0;
> +       __be32 status = nfs_ok;
>         struct nfsd4_copy *copy;
>         struct nfs4_client *clp = cstate->clp;
>
> -       copy = find_async_copy(clp, &os->stateid);
> -       if (copy) {
> +       spin_lock(&clp->async_lock);
> +       copy = find_async_copy_locked(clp, &os->stateid);
> +       if (copy)
>                 os->count = copy->cp_res.wr_bytes_written;
> -               nfs4_put_copy(copy);
> -       } else
> +       else
>                 status = nfserr_bad_stateid;
> +       spin_unlock(&clp->async_lock);
>
>         return status;
>  }
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index e94634d30591..d49d3060ed4f 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -705,8 +705,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name
>  extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
>
>  void put_nfs4_file(struct nfs4_file *fi);
> -extern struct nfsd4_copy *
> -find_async_copy(struct nfs4_client *clp, stateid_t *staetid);
>  extern void nfs4_put_cpntf_state(struct nfsd_net *nn,
>                                  struct nfs4_cpntf_state *cps);
>  extern __be32 manage_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> --
> 2.39.0
>

Re: [PATCH 1/6] nfsd: don't take nfsd4_copy ref for OP_OFFLOAD_STATUS

[Jeff Layton]

On Wed, 2023-01-18 at 12:43 -0500, Olga Kornievskaia wrote:
> On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > We're not doing any blocking operations for OP_OFFLOAD_STATUS, so taking
> > and putting a reference is a waste of effort. Take the client lock,
> > search for the copy and fetch the wr_bytes_written field and return.
> > 
> > Also, make find_async_copy a static function.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfs4proc.c | 35 ++++++++++++++++++++++++-----------
> >  fs/nfsd/state.h    |  2 --
> >  2 files changed, 24 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 62b9d6c1b18b..731c2b22f163 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1823,23 +1823,34 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >         goto out;
> >  }
> > 
> > -struct nfsd4_copy *
> > -find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
> > +static struct nfsd4_copy *
> > +find_async_copy_locked(struct nfs4_client *clp, stateid_t *stateid)
> >  {
> >         struct nfsd4_copy *copy;
> > 
> > -       spin_lock(&clp->async_lock);
> > +       lockdep_assert_held(&clp->async_lock);
> > +
> >         list_for_each_entry(copy, &clp->async_copies, copies) {
> >                 if (memcmp(&copy->cp_stateid.cs_stid, stateid, NFS4_STATEID_SIZE))
> >                         continue;
> > -               refcount_inc(&copy->refcount);
> 
> If we don't take a refcount on the copy, this copy could be removed
> between the time we found it in the list of copies and when we then
> look inside to check the amount written so far. This would lead to a
> null (or bad) pointer dereference?
> 

No. The existing code finds this object, takes a reference to it,
fetches a single integer out of it and then puts the reference. This
patch just has it avoid the reference altogether and fetch the value
while we still hold the spinlock. This should be completely safe
(assuming the locking around the existing list handling is correct,
which it does seem to be).


> > -               spin_unlock(&clp->async_lock);
> >                 return copy;
> >         }
> > -       spin_unlock(&clp->async_lock);
> >         return NULL;
> >  }
> > 
> > +static struct nfsd4_copy *
> > +find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
> > +{
> > +       struct nfsd4_copy *copy;
> > +
> > +       spin_lock(&clp->async_lock);
> > +       copy = find_async_copy_locked(clp, stateid);
> > +       if (copy)
> > +               refcount_inc(&copy->refcount);
> > +       spin_unlock(&clp->async_lock);
> > +       return copy;
> > +}
> > +
> >  static __be32
> >  nfsd4_offload_cancel(struct svc_rqst *rqstp,
> >                      struct nfsd4_compound_state *cstate,
> > @@ -1924,22 +1935,24 @@ nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >         nfsd_file_put(nf);
> >         return status;
> >  }
> > +
> >  static __be32
> >  nfsd4_offload_status(struct svc_rqst *rqstp,
> >                      struct nfsd4_compound_state *cstate,
> >                      union nfsd4_op_u *u)
> >  {
> >         struct nfsd4_offload_status *os = &u->offload_status;
> > -       __be32 status = 0;
> > +       __be32 status = nfs_ok;
> >         struct nfsd4_copy *copy;
> >         struct nfs4_client *clp = cstate->clp;
> > 
> > -       copy = find_async_copy(clp, &os->stateid);
> > -       if (copy) {
> > +       spin_lock(&clp->async_lock);
> > +       copy = find_async_copy_locked(clp, &os->stateid);
> > +       if (copy)
> >                 os->count = copy->cp_res.wr_bytes_written;
> > -               nfs4_put_copy(copy);
> > -       } else
> > +       else
> >                 status = nfserr_bad_stateid;
> > +       spin_unlock(&clp->async_lock);
> > 
> >         return status;
> >  }
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index e94634d30591..d49d3060ed4f 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -705,8 +705,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name
> >  extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
> > 
> >  void put_nfs4_file(struct nfs4_file *fi);
> > -extern struct nfsd4_copy *
> > -find_async_copy(struct nfs4_client *clp, stateid_t *staetid);
> >  extern void nfs4_put_cpntf_state(struct nfsd_net *nn,
> >                                  struct nfs4_cpntf_state *cps);
> >  extern __be32 manage_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> > --
> > 2.39.0
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/6] nfsd: don't take nfsd4_copy ref for OP_OFFLOAD_STATUS

[Olga Kornievskaia]

On Wed, Jan 18, 2023 at 12:49 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2023-01-18 at 12:43 -0500, Olga Kornievskaia wrote:
> > On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > We're not doing any blocking operations for OP_OFFLOAD_STATUS, so taking
> > > and putting a reference is a waste of effort. Take the client lock,
> > > search for the copy and fetch the wr_bytes_written field and return.
> > >
> > > Also, make find_async_copy a static function.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/nfsd/nfs4proc.c | 35 ++++++++++++++++++++++++-----------
> > >  fs/nfsd/state.h    |  2 --
> > >  2 files changed, 24 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index 62b9d6c1b18b..731c2b22f163 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -1823,23 +1823,34 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >         goto out;
> > >  }
> > >
> > > -struct nfsd4_copy *
> > > -find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
> > > +static struct nfsd4_copy *
> > > +find_async_copy_locked(struct nfs4_client *clp, stateid_t *stateid)
> > >  {
> > >         struct nfsd4_copy *copy;
> > >
> > > -       spin_lock(&clp->async_lock);
> > > +       lockdep_assert_held(&clp->async_lock);
> > > +
> > >         list_for_each_entry(copy, &clp->async_copies, copies) {
> > >                 if (memcmp(&copy->cp_stateid.cs_stid, stateid, NFS4_STATEID_SIZE))
> > >                         continue;
> > > -               refcount_inc(&copy->refcount);
> >
> > If we don't take a refcount on the copy, this copy could be removed
> > between the time we found it in the list of copies and when we then
> > look inside to check the amount written so far. This would lead to a
> > null (or bad) pointer dereference?
> >
>
> No. The existing code finds this object, takes a reference to it,
> fetches a single integer out of it and then puts the reference. This
> patch just has it avoid the reference altogether and fetch the value
> while we still hold the spinlock. This should be completely safe
> (assuming the locking around the existing list handling is correct,
> which it does seem to be).

Thank you for the explanation. I see it now.

>
>
> > > -               spin_unlock(&clp->async_lock);
> > >                 return copy;
> > >         }
> > > -       spin_unlock(&clp->async_lock);
> > >         return NULL;
> > >  }
> > >
> > > +static struct nfsd4_copy *
> > > +find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
> > > +{
> > > +       struct nfsd4_copy *copy;
> > > +
> > > +       spin_lock(&clp->async_lock);
> > > +       copy = find_async_copy_locked(clp, stateid);
> > > +       if (copy)
> > > +               refcount_inc(&copy->refcount);
> > > +       spin_unlock(&clp->async_lock);
> > > +       return copy;
> > > +}
> > > +
> > >  static __be32
> > >  nfsd4_offload_cancel(struct svc_rqst *rqstp,
> > >                      struct nfsd4_compound_state *cstate,
> > > @@ -1924,22 +1935,24 @@ nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >         nfsd_file_put(nf);
> > >         return status;
> > >  }
> > > +
> > >  static __be32
> > >  nfsd4_offload_status(struct svc_rqst *rqstp,
> > >                      struct nfsd4_compound_state *cstate,
> > >                      union nfsd4_op_u *u)
> > >  {
> > >         struct nfsd4_offload_status *os = &u->offload_status;
> > > -       __be32 status = 0;
> > > +       __be32 status = nfs_ok;
> > >         struct nfsd4_copy *copy;
> > >         struct nfs4_client *clp = cstate->clp;
> > >
> > > -       copy = find_async_copy(clp, &os->stateid);
> > > -       if (copy) {
> > > +       spin_lock(&clp->async_lock);
> > > +       copy = find_async_copy_locked(clp, &os->stateid);
> > > +       if (copy)
> > >                 os->count = copy->cp_res.wr_bytes_written;
> > > -               nfs4_put_copy(copy);
> > > -       } else
> > > +       else
> > >                 status = nfserr_bad_stateid;
> > > +       spin_unlock(&clp->async_lock);
> > >
> > >         return status;
> > >  }
> > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > > index e94634d30591..d49d3060ed4f 100644
> > > --- a/fs/nfsd/state.h
> > > +++ b/fs/nfsd/state.h
> > > @@ -705,8 +705,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name
> > >  extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
> > >
> > >  void put_nfs4_file(struct nfs4_file *fi);
> > > -extern struct nfsd4_copy *
> > > -find_async_copy(struct nfs4_client *clp, stateid_t *staetid);
> > >  extern void nfs4_put_cpntf_state(struct nfsd_net *nn,
> > >                                  struct nfs4_cpntf_state *cps);
> > >  extern __be32 manage_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> > > --
> > > 2.39.0
> > >
>
> --
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/6] nfsd: don't take nfsd4_copy ref for OP_OFFLOAD_STATUS

[Chuck Lever III]

> On Jan 18, 2023, at 1:08 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> 
> On Wed, Jan 18, 2023 at 12:49 PM Jeff Layton <jlayton@kernel.org> wrote:
>> 
>> On Wed, 2023-01-18 at 12:43 -0500, Olga Kornievskaia wrote:
>>> On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <jlayton@kernel.org> wrote:
>>>> 
>>>> We're not doing any blocking operations for OP_OFFLOAD_STATUS, so taking
>>>> and putting a reference is a waste of effort. Take the client lock,
>>>> search for the copy and fetch the wr_bytes_written field and return.
>>>> 
>>>> Also, make find_async_copy a static function.
>>>> 
>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>> ---
>>>> fs/nfsd/nfs4proc.c | 35 ++++++++++++++++++++++++-----------
>>>> fs/nfsd/state.h    |  2 --
>>>> 2 files changed, 24 insertions(+), 13 deletions(-)
>>>> 
>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>> index 62b9d6c1b18b..731c2b22f163 100644
>>>> --- a/fs/nfsd/nfs4proc.c
>>>> +++ b/fs/nfsd/nfs4proc.c
>>>> @@ -1823,23 +1823,34 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>        goto out;
>>>> }
>>>> 
>>>> -struct nfsd4_copy *
>>>> -find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
>>>> +static struct nfsd4_copy *
>>>> +find_async_copy_locked(struct nfs4_client *clp, stateid_t *stateid)
>>>> {
>>>>        struct nfsd4_copy *copy;
>>>> 
>>>> -       spin_lock(&clp->async_lock);
>>>> +       lockdep_assert_held(&clp->async_lock);
>>>> +
>>>>        list_for_each_entry(copy, &clp->async_copies, copies) {
>>>>                if (memcmp(&copy->cp_stateid.cs_stid, stateid, NFS4_STATEID_SIZE))
>>>>                        continue;
>>>> -               refcount_inc(&copy->refcount);
>>> 
>>> If we don't take a refcount on the copy, this copy could be removed
>>> between the time we found it in the list of copies and when we then
>>> look inside to check the amount written so far. This would lead to a
>>> null (or bad) pointer dereference?
>>> 
>> 
>> No. The existing code finds this object, takes a reference to it,
>> fetches a single integer out of it and then puts the reference. This
>> patch just has it avoid the reference altogether and fetch the value
>> while we still hold the spinlock. This should be completely safe
>> (assuming the locking around the existing list handling is correct,
>> which it does seem to be).
> 
> Thank you for the explanation. I see it now.

May I add Reviewed-by: Olga Kornievskaia <kolga@netapp.com> ?


>>>> -               spin_unlock(&clp->async_lock);
>>>>                return copy;
>>>>        }
>>>> -       spin_unlock(&clp->async_lock);
>>>>        return NULL;
>>>> }
>>>> 
>>>> +static struct nfsd4_copy *
>>>> +find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
>>>> +{
>>>> +       struct nfsd4_copy *copy;
>>>> +
>>>> +       spin_lock(&clp->async_lock);
>>>> +       copy = find_async_copy_locked(clp, stateid);
>>>> +       if (copy)
>>>> +               refcount_inc(&copy->refcount);
>>>> +       spin_unlock(&clp->async_lock);
>>>> +       return copy;
>>>> +}
>>>> +
>>>> static __be32
>>>> nfsd4_offload_cancel(struct svc_rqst *rqstp,
>>>>                     struct nfsd4_compound_state *cstate,
>>>> @@ -1924,22 +1935,24 @@ nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>        nfsd_file_put(nf);
>>>>        return status;
>>>> }
>>>> +
>>>> static __be32
>>>> nfsd4_offload_status(struct svc_rqst *rqstp,
>>>>                     struct nfsd4_compound_state *cstate,
>>>>                     union nfsd4_op_u *u)
>>>> {
>>>>        struct nfsd4_offload_status *os = &u->offload_status;
>>>> -       __be32 status = 0;
>>>> +       __be32 status = nfs_ok;
>>>>        struct nfsd4_copy *copy;
>>>>        struct nfs4_client *clp = cstate->clp;
>>>> 
>>>> -       copy = find_async_copy(clp, &os->stateid);
>>>> -       if (copy) {
>>>> +       spin_lock(&clp->async_lock);
>>>> +       copy = find_async_copy_locked(clp, &os->stateid);
>>>> +       if (copy)
>>>>                os->count = copy->cp_res.wr_bytes_written;
>>>> -               nfs4_put_copy(copy);
>>>> -       } else
>>>> +       else
>>>>                status = nfserr_bad_stateid;
>>>> +       spin_unlock(&clp->async_lock);
>>>> 
>>>>        return status;
>>>> }
>>>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>>>> index e94634d30591..d49d3060ed4f 100644
>>>> --- a/fs/nfsd/state.h
>>>> +++ b/fs/nfsd/state.h
>>>> @@ -705,8 +705,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name
>>>> extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
>>>> 
>>>> void put_nfs4_file(struct nfs4_file *fi);
>>>> -extern struct nfsd4_copy *
>>>> -find_async_copy(struct nfs4_client *clp, stateid_t *staetid);
>>>> extern void nfs4_put_cpntf_state(struct nfsd_net *nn,
>>>>                                 struct nfs4_cpntf_state *cps);
>>>> extern __be32 manage_cpntf_state(struct nfsd_net *nn, stateid_t *st,
>>>> --
>>>> 2.39.0
>>>> 
>> 
>> --
>> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever

Re: [PATCH 3/6] nfsd: simplify the delayed disposal list code

[Chuck Lever III]

> On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> When queueing a dispose list to the appropriate "freeme" lists, it
> pointlessly queues the objects one at a time to an intermediate list.
> 
> Remove a few helpers and just open code a list_move to make it more
> clear and efficient. Better document the resulting functions with
> kerneldoc comments.

I'd like to freeze the filecache code until we've sorted out the
destroy_deleg_unhashed crashes. Shall I simply maintain 3/6 and
4/6 and any subsequent filecache changes (like my rhltable
rewrite) on a topic branch?

One good reason to do that is to enable an eventual fix to be
backported to stable kernels without also needing to pull in
intervening clean-up patches.

I've already applied a couple small changes that I would rather
wait on for this reason. I might move those over to the topic
branch as well... I promise to keep it based on nfsd-next so it
makes sense to continue developing filecache work on top of the
topic branch.

The other patches in this series are sensible clean-ups that I
plan to apply for v6.3 if there are no other objections.


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 63 +++++++++++++++------------------------------
> 1 file changed, 21 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 58ac93e7e680..a2bc4bd90b9a 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head *dispose)
> 	}
> }
> 
> -static void
> -nfsd_file_list_remove_disposal(struct list_head *dst,
> -		struct nfsd_fcache_disposal *l)
> -{
> -	spin_lock(&l->lock);
> -	list_splice_init(&l->freeme, dst);
> -	spin_unlock(&l->lock);
> -}
> -
> -static void
> -nfsd_file_list_add_disposal(struct list_head *files, struct net *net)
> -{
> -	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> -	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> -
> -	spin_lock(&l->lock);
> -	list_splice_tail_init(files, &l->freeme);
> -	spin_unlock(&l->lock);
> -	queue_work(nfsd_filecache_wq, &l->work);
> -}
> -
> -static void
> -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head *src,
> -		struct net *net)
> -{
> -	struct nfsd_file *nf, *tmp;
> -
> -	list_for_each_entry_safe(nf, tmp, src, nf_lru) {
> -		if (nf->nf_net == net)
> -			list_move_tail(&nf->nf_lru, dst);
> -	}
> -}
> -
> +/**
> + * nfsd_file_dispose_list_delayed - move list of dead files to net's freeme list
> + * @dispose: list of nfsd_files to be disposed
> + *
> + * Transfers each file to the "freeme" list for its nfsd_net, to eventually
> + * be disposed of by the per-net garbage collector.
> + */
> static void
> nfsd_file_dispose_list_delayed(struct list_head *dispose)
> {
> -	LIST_HEAD(list);
> -	struct nfsd_file *nf;
> -
> 	while(!list_empty(dispose)) {
> -		nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
> -		nfsd_file_list_add_pernet(&list, dispose, nf->nf_net);
> -		nfsd_file_list_add_disposal(&list, nf->nf_net);
> +		struct nfsd_file *nf = list_first_entry(dispose,
> +						struct nfsd_file, nf_lru);
> +		struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> +		struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> +
> +		spin_lock(&l->lock);
> +		list_move_tail(&nf->nf_lru, &l->freeme);
> +		spin_unlock(&l->lock);
> 	}
> }
> 
> @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode)
>  * nfsd_file_delayed_close - close unused nfsd_files
>  * @work: dummy
>  *
> - * Walk the LRU list and destroy any entries that have not been used since
> - * the last scan.
> + * Scrape the freeme list for this nfsd_net, and then dispose of them
> + * all.
>  */
> static void
> nfsd_file_delayed_close(struct work_struct *work)
> @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct *work)
> 	struct nfsd_fcache_disposal *l = container_of(work,
> 			struct nfsd_fcache_disposal, work);
> 
> -	nfsd_file_list_remove_disposal(&head, l);
> +	spin_lock(&l->lock);
> +	list_splice_init(&l->freeme, &head);
> +	spin_unlock(&l->lock);
> +
> 	nfsd_file_dispose_list(&head);
> }
> 
> -- 
> 2.39.0
> 

--
Chuck Lever

Re: [PATCH 3/6] nfsd: simplify the delayed disposal list code

[Jeff Layton]

On Wed, 2023-01-18 at 19:08 +0000, Chuck Lever III wrote:
> 
> > On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > When queueing a dispose list to the appropriate "freeme" lists, it
> > pointlessly queues the objects one at a time to an intermediate list.
> > 
> > Remove a few helpers and just open code a list_move to make it more
> > clear and efficient. Better document the resulting functions with
> > kerneldoc comments.
> 
> I'd like to freeze the filecache code until we've sorted out the
> destroy_deleg_unhashed crashes. Shall I simply maintain 3/6 and
> 4/6 and any subsequent filecache changes (like my rhltable
> rewrite) on a topic branch?
> 
> One good reason to do that is to enable an eventual fix to be
> backported to stable kernels without also needing to pull in
> intervening clean-up patches.
> 
> I've already applied a couple small changes that I would rather
> wait on for this reason. I might move those over to the topic
> branch as well... I promise to keep it based on nfsd-next so it
> makes sense to continue developing filecache work on top of the
> topic branch.
> 
> The other patches in this series are sensible clean-ups that I
> plan to apply for v6.3 if there are no other objections.
> 

So that means you won't take patches 3 and 4, but the rest are ok?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/6] nfsd: simplify the delayed disposal list code

[Chuck Lever III]

> On Jan 18, 2023, at 2:42 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Wed, 2023-01-18 at 19:08 +0000, Chuck Lever III wrote:
>> 
>>> On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>> 
>>> When queueing a dispose list to the appropriate "freeme" lists, it
>>> pointlessly queues the objects one at a time to an intermediate list.
>>> 
>>> Remove a few helpers and just open code a list_move to make it more
>>> clear and efficient. Better document the resulting functions with
>>> kerneldoc comments.
>> 
>> I'd like to freeze the filecache code until we've sorted out the
>> destroy_deleg_unhashed crashes. Shall I simply maintain 3/6 and
>> 4/6 and any subsequent filecache changes (like my rhltable
>> rewrite) on a topic branch?
>> 
>> One good reason to do that is to enable an eventual fix to be
>> backported to stable kernels without also needing to pull in
>> intervening clean-up patches.
>> 
>> I've already applied a couple small changes that I would rather
>> wait on for this reason. I might move those over to the topic
>> branch as well... I promise to keep it based on nfsd-next so it
>> makes sense to continue developing filecache work on top of the
>> topic branch.
>> 
>> The other patches in this series are sensible clean-ups that I
>> plan to apply for v6.3 if there are no other objections.
>> 
> 
> So that means you won't take patches 3 and 4, but the rest are ok?

They all look fine to me. I've applied 1, 2, 5, and 6 to nfsd-next,
and 3 and 4 along with several others have been applied to a
branch called topic-filecache-cleanups, which is published now so
you can continue developing against that and so it will get pulled
into the 0-day test harness.

I will merge the stuff in that topic branch at some point, I'm just
not committing yet to applying it specifically to v6.3.

Yes, you can call me "too conservative." :-) But I am sensitive to
addressing the destroy_unhashed_deleg crashers in v6.1, which is
to become an LTS if I understand correctly. That's the main reason
for adding this extra step for filecache-related clean-ups. Narrow
bug fixes still go right into nfsd-next or nfsd-fixes, as usual.

If this arrangement becomes onerous, I will mix those commits back
into the general nfsd-next population and we will never speak of it
again.


--
Chuck Lever

Re: [PATCH 1/6] nfsd: don't take nfsd4_copy ref for OP_OFFLOAD_STATUS

[Olga Kornievskaia]

On Wed, Jan 18, 2023 at 1:53 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Jan 18, 2023, at 1:08 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > On Wed, Jan 18, 2023 at 12:49 PM Jeff Layton <jlayton@kernel.org> wrote:
> >>
> >> On Wed, 2023-01-18 at 12:43 -0500, Olga Kornievskaia wrote:
> >>> On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <jlayton@kernel.org> wrote:
> >>>>
> >>>> We're not doing any blocking operations for OP_OFFLOAD_STATUS, so taking
> >>>> and putting a reference is a waste of effort. Take the client lock,
> >>>> search for the copy and fetch the wr_bytes_written field and return.
> >>>>
> >>>> Also, make find_async_copy a static function.
> >>>>
> >>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >>>> ---
> >>>> fs/nfsd/nfs4proc.c | 35 ++++++++++++++++++++++++-----------
> >>>> fs/nfsd/state.h    |  2 --
> >>>> 2 files changed, 24 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>>> index 62b9d6c1b18b..731c2b22f163 100644
> >>>> --- a/fs/nfsd/nfs4proc.c
> >>>> +++ b/fs/nfsd/nfs4proc.c
> >>>> @@ -1823,23 +1823,34 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>>>        goto out;
> >>>> }
> >>>>
> >>>> -struct nfsd4_copy *
> >>>> -find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
> >>>> +static struct nfsd4_copy *
> >>>> +find_async_copy_locked(struct nfs4_client *clp, stateid_t *stateid)
> >>>> {
> >>>>        struct nfsd4_copy *copy;
> >>>>
> >>>> -       spin_lock(&clp->async_lock);
> >>>> +       lockdep_assert_held(&clp->async_lock);
> >>>> +
> >>>>        list_for_each_entry(copy, &clp->async_copies, copies) {
> >>>>                if (memcmp(&copy->cp_stateid.cs_stid, stateid, NFS4_STATEID_SIZE))
> >>>>                        continue;
> >>>> -               refcount_inc(&copy->refcount);
> >>>
> >>> If we don't take a refcount on the copy, this copy could be removed
> >>> between the time we found it in the list of copies and when we then
> >>> look inside to check the amount written so far. This would lead to a
> >>> null (or bad) pointer dereference?
> >>>
> >>
> >> No. The existing code finds this object, takes a reference to it,
> >> fetches a single integer out of it and then puts the reference. This
> >> patch just has it avoid the reference altogether and fetch the value
> >> while we still hold the spinlock. This should be completely safe
> >> (assuming the locking around the existing list handling is correct,
> >> which it does seem to be).
> >
> > Thank you for the explanation. I see it now.
>
> May I add Reviewed-by: Olga Kornievskaia <kolga@netapp.com> ?

Of course.

> >>>> -               spin_unlock(&clp->async_lock);
> >>>>                return copy;
> >>>>        }
> >>>> -       spin_unlock(&clp->async_lock);
> >>>>        return NULL;
> >>>> }
> >>>>
> >>>> +static struct nfsd4_copy *
> >>>> +find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
> >>>> +{
> >>>> +       struct nfsd4_copy *copy;
> >>>> +
> >>>> +       spin_lock(&clp->async_lock);
> >>>> +       copy = find_async_copy_locked(clp, stateid);
> >>>> +       if (copy)
> >>>> +               refcount_inc(&copy->refcount);
> >>>> +       spin_unlock(&clp->async_lock);
> >>>> +       return copy;
> >>>> +}
> >>>> +
> >>>> static __be32
> >>>> nfsd4_offload_cancel(struct svc_rqst *rqstp,
> >>>>                     struct nfsd4_compound_state *cstate,
> >>>> @@ -1924,22 +1935,24 @@ nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>>>        nfsd_file_put(nf);
> >>>>        return status;
> >>>> }
> >>>> +
> >>>> static __be32
> >>>> nfsd4_offload_status(struct svc_rqst *rqstp,
> >>>>                     struct nfsd4_compound_state *cstate,
> >>>>                     union nfsd4_op_u *u)
> >>>> {
> >>>>        struct nfsd4_offload_status *os = &u->offload_status;
> >>>> -       __be32 status = 0;
> >>>> +       __be32 status = nfs_ok;
> >>>>        struct nfsd4_copy *copy;
> >>>>        struct nfs4_client *clp = cstate->clp;
> >>>>
> >>>> -       copy = find_async_copy(clp, &os->stateid);
> >>>> -       if (copy) {
> >>>> +       spin_lock(&clp->async_lock);
> >>>> +       copy = find_async_copy_locked(clp, &os->stateid);
> >>>> +       if (copy)
> >>>>                os->count = copy->cp_res.wr_bytes_written;
> >>>> -               nfs4_put_copy(copy);
> >>>> -       } else
> >>>> +       else
> >>>>                status = nfserr_bad_stateid;
> >>>> +       spin_unlock(&clp->async_lock);
> >>>>
> >>>>        return status;
> >>>> }
> >>>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> >>>> index e94634d30591..d49d3060ed4f 100644
> >>>> --- a/fs/nfsd/state.h
> >>>> +++ b/fs/nfsd/state.h
> >>>> @@ -705,8 +705,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name
> >>>> extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
> >>>>
> >>>> void put_nfs4_file(struct nfs4_file *fi);
> >>>> -extern struct nfsd4_copy *
> >>>> -find_async_copy(struct nfs4_client *clp, stateid_t *staetid);
> >>>> extern void nfs4_put_cpntf_state(struct nfsd_net *nn,
> >>>>                                 struct nfs4_cpntf_state *cps);
> >>>> extern __be32 manage_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> >>>> --
> >>>> 2.39.0
> >>>>
> >>
> >> --
> >> Jeff Layton <jlayton@kernel.org>
>
> --
> Chuck Lever
>
>
>

Re: [PATCH 3/6] nfsd: simplify the delayed disposal list code

[Chuck Lever III]

> On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> When queueing a dispose list to the appropriate "freeme" lists, it
> pointlessly queues the objects one at a time to an intermediate list.
> 
> Remove a few helpers and just open code a list_move to make it more
> clear and efficient. Better document the resulting functions with
> kerneldoc comments.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 63 +++++++++++++++------------------------------
> 1 file changed, 21 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 58ac93e7e680..a2bc4bd90b9a 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head *dispose)
> }
> }
> 
> -static void
> -nfsd_file_list_remove_disposal(struct list_head *dst,
> - struct nfsd_fcache_disposal *l)
> -{
> - spin_lock(&l->lock);
> - list_splice_init(&l->freeme, dst);
> - spin_unlock(&l->lock);
> -}
> -
> -static void
> -nfsd_file_list_add_disposal(struct list_head *files, struct net *net)
> -{
> - struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> - struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> -
> - spin_lock(&l->lock);
> - list_splice_tail_init(files, &l->freeme);
> - spin_unlock(&l->lock);
> - queue_work(nfsd_filecache_wq, &l->work);
> -}
> -
> -static void
> -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head *src,
> - struct net *net)
> -{
> - struct nfsd_file *nf, *tmp;
> -
> - list_for_each_entry_safe(nf, tmp, src, nf_lru) {
> - if (nf->nf_net == net)
> - list_move_tail(&nf->nf_lru, dst);
> - }
> -}
> -
> +/**
> + * nfsd_file_dispose_list_delayed - move list of dead files to net's freeme list
> + * @dispose: list of nfsd_files to be disposed
> + *
> + * Transfers each file to the "freeme" list for its nfsd_net, to eventually
> + * be disposed of by the per-net garbage collector.
> + */
> static void
> nfsd_file_dispose_list_delayed(struct list_head *dispose)
> {
> - LIST_HEAD(list);
> - struct nfsd_file *nf;
> -
> while(!list_empty(dispose)) {
> - nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
> - nfsd_file_list_add_pernet(&list, dispose, nf->nf_net);
> - nfsd_file_list_add_disposal(&list, nf->nf_net);
> + struct nfsd_file *nf = list_first_entry(dispose,
> + struct nfsd_file, nf_lru);
> + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> + struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> +
> + spin_lock(&l->lock);
> + list_move_tail(&nf->nf_lru, &l->freeme);
> + spin_unlock(&l->lock);
> }
> }
> 
> @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode)
>  * nfsd_file_delayed_close - close unused nfsd_files
>  * @work: dummy
>  *
> - * Walk the LRU list and destroy any entries that have not been used since
> - * the last scan.
> + * Scrape the freeme list for this nfsd_net, and then dispose of them
> + * all.
>  */
> static void
> nfsd_file_delayed_close(struct work_struct *work)
> @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct *work)
> struct nfsd_fcache_disposal *l = container_of(work,
> struct nfsd_fcache_disposal, work);
> 
> - nfsd_file_list_remove_disposal(&head, l);
> + spin_lock(&l->lock);
> + list_splice_init(&l->freeme, &head);
> + spin_unlock(&l->lock);
> +
> nfsd_file_dispose_list(&head);
> }

Hey Jeff -

After applying this one, tmpfs exports appear to leak space,
even after all files and directories are deleted. Eventually
the filesystem is "full" -- modifying operations return ENOSPC
but removing files doesn't recover the used space.

Can you have a look at this?


--
Chuck Lever

Re: [PATCH 3/6] nfsd: simplify the delayed disposal list code

[Jeff Layton]

On Fri, 2023-04-14 at 18:20 +0000, Chuck Lever III wrote:
> > On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org>
> > wrote:
> > 
> > When queueing a dispose list to the appropriate "freeme" lists, it
> > pointlessly queues the objects one at a time to an intermediate
> > list.
> > 
> > Remove a few helpers and just open code a list_move to make it more
> > clear and efficient. Better document the resulting functions with
> > kerneldoc comments.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > fs/nfsd/filecache.c | 63 +++++++++++++++----------------------------
> > --
> > 1 file changed, 21 insertions(+), 42 deletions(-)
> > 
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index 58ac93e7e680..a2bc4bd90b9a 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head
> > *dispose)
> > }
> > }
> > 
> > -static void
> > -nfsd_file_list_remove_disposal(struct list_head *dst,
> > - struct nfsd_fcache_disposal *l)
> > -{
> > - spin_lock(&l->lock);
> > - list_splice_init(&l->freeme, dst);
> > - spin_unlock(&l->lock);
> > -}
> > -
> > -static void
> > -nfsd_file_list_add_disposal(struct list_head *files, struct net
> > *net)
> > -{
> > - struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > - struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> > -
> > - spin_lock(&l->lock);
> > - list_splice_tail_init(files, &l->freeme);
> > - spin_unlock(&l->lock);
> > - queue_work(nfsd_filecache_wq, &l->work);
> > -}
> > -
> > -static void
> > -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head
> > *src,
> > - struct net *net)
> > -{
> > - struct nfsd_file *nf, *tmp;
> > -
> > - list_for_each_entry_safe(nf, tmp, src, nf_lru) {
> > - if (nf->nf_net == net)
> > - list_move_tail(&nf->nf_lru, dst);
> > - }
> > -}
> > -
> > +/**
> > + * nfsd_file_dispose_list_delayed - move list of dead files to
> > net's freeme list
> > + * @dispose: list of nfsd_files to be disposed
> > + *
> > + * Transfers each file to the "freeme" list for its nfsd_net, to
> > eventually
> > + * be disposed of by the per-net garbage collector.
> > + */
> > static void
> > nfsd_file_dispose_list_delayed(struct list_head *dispose)
> > {
> > - LIST_HEAD(list);
> > - struct nfsd_file *nf;
> > -
> > while(!list_empty(dispose)) {
> > - nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
> > - nfsd_file_list_add_pernet(&list, dispose, nf->nf_net);
> > - nfsd_file_list_add_disposal(&list, nf->nf_net);
> > + struct nfsd_file *nf = list_first_entry(dispose,
> > + struct nfsd_file, nf_lru);
> > + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> > + struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> > +
> > + spin_lock(&l->lock);
> > + list_move_tail(&nf->nf_lru, &l->freeme);
> > + spin_unlock(&l->lock);
> > }
> > }
> > 
> > @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode)
> >  * nfsd_file_delayed_close - close unused nfsd_files
> >  * @work: dummy
> >  *
> > - * Walk the LRU list and destroy any entries that have not been
> > used since
> > - * the last scan.
> > + * Scrape the freeme list for this nfsd_net, and then dispose of
> > them
> > + * all.
> >  */
> > static void
> > nfsd_file_delayed_close(struct work_struct *work)
> > @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct
> > *work)
> > struct nfsd_fcache_disposal *l = container_of(work,
> > struct nfsd_fcache_disposal, work);
> > 
> > - nfsd_file_list_remove_disposal(&head, l);
> > + spin_lock(&l->lock);
> > + list_splice_init(&l->freeme, &head);
> > + spin_unlock(&l->lock);
> > +
> > nfsd_file_dispose_list(&head);
> > }
> 
> Hey Jeff -
> 
> After applying this one, tmpfs exports appear to leak space,
> even after all files and directories are deleted. Eventually
> the filesystem is "full" -- modifying operations return ENOSPC
> but removing files doesn't recover the used space.
> 
> Can you have a look at this?
> 

Hrm, ok. Do you have a reproducer?

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/6] nfsd: simplify the delayed disposal list code

[Jeff Layton]

On Fri, 2023-04-14 at 18:20 +0000, Chuck Lever III wrote:
> > On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org>
> > wrote:
> > 
> > When queueing a dispose list to the appropriate "freeme" lists, it
> > pointlessly queues the objects one at a time to an intermediate
> > list.
> > 
> > Remove a few helpers and just open code a list_move to make it more
> > clear and efficient. Better document the resulting functions with
> > kerneldoc comments.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > fs/nfsd/filecache.c | 63 +++++++++++++++----------------------------
> > --
> > 1 file changed, 21 insertions(+), 42 deletions(-)
> > 
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index 58ac93e7e680..a2bc4bd90b9a 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head
> > *dispose)
> > }
> > }
> > 
> > -static void
> > -nfsd_file_list_remove_disposal(struct list_head *dst,
> > - struct nfsd_fcache_disposal *l)
> > -{
> > - spin_lock(&l->lock);
> > - list_splice_init(&l->freeme, dst);
> > - spin_unlock(&l->lock);
> > -}
> > -
> > -static void
> > -nfsd_file_list_add_disposal(struct list_head *files, struct net
> > *net)
> > -{
> > - struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > - struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> > -
> > - spin_lock(&l->lock);
> > - list_splice_tail_init(files, &l->freeme);
> > - spin_unlock(&l->lock);
> > - queue_work(nfsd_filecache_wq, &l->work);
> > -}
> > -
> > -static void
> > -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head
> > *src,
> > - struct net *net)
> > -{
> > - struct nfsd_file *nf, *tmp;
> > -
> > - list_for_each_entry_safe(nf, tmp, src, nf_lru) {
> > - if (nf->nf_net == net)
> > - list_move_tail(&nf->nf_lru, dst);
> > - }
> > -}
> > -
> > +/**
> > + * nfsd_file_dispose_list_delayed - move list of dead files to
> > net's freeme list
> > + * @dispose: list of nfsd_files to be disposed
> > + *
> > + * Transfers each file to the "freeme" list for its nfsd_net, to
> > eventually
> > + * be disposed of by the per-net garbage collector.
> > + */
> > static void
> > nfsd_file_dispose_list_delayed(struct list_head *dispose)
> > {
> > - LIST_HEAD(list);
> > - struct nfsd_file *nf;
> > -
> > while(!list_empty(dispose)) {
> > - nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
> > - nfsd_file_list_add_pernet(&list, dispose, nf->nf_net);
> > - nfsd_file_list_add_disposal(&list, nf->nf_net);
> > + struct nfsd_file *nf = list_first_entry(dispose,
> > + struct nfsd_file, nf_lru);
> > + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> > + struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> > +
> > + spin_lock(&l->lock);
> > + list_move_tail(&nf->nf_lru, &l->freeme);
> > + spin_unlock(&l->lock);
> > }
> > }
> > 
> > @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode)
> >  * nfsd_file_delayed_close - close unused nfsd_files
> >  * @work: dummy
> >  *
> > - * Walk the LRU list and destroy any entries that have not been
> > used since
> > - * the last scan.
> > + * Scrape the freeme list for this nfsd_net, and then dispose of
> > them
> > + * all.
> >  */
> > static void
> > nfsd_file_delayed_close(struct work_struct *work)
> > @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct
> > *work)
> > struct nfsd_fcache_disposal *l = container_of(work,
> > struct nfsd_fcache_disposal, work);
> > 
> > - nfsd_file_list_remove_disposal(&head, l);
> > + spin_lock(&l->lock);
> > + list_splice_init(&l->freeme, &head);
> > + spin_unlock(&l->lock);
> > +
> > nfsd_file_dispose_list(&head);
> > }
> 
> Hey Jeff -
> 
> After applying this one, tmpfs exports appear to leak space,
> even after all files and directories are deleted. Eventually
> the filesystem is "full" -- modifying operations return ENOSPC
> but removing files doesn't recover the used space.
> 
> Can you have a look at this?
> 

Actually, I may see the bug. Does this fix it?

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index c173d460b17d..f40d8f3b35a4 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -421,6 +421,7 @@ nfsd_file_dispose_list_delayed(struct list_head
*dispose)
 		spin_lock(&l->lock);
 		list_move_tail(&nf->nf_lru, &l->freeme);
 		spin_unlock(&l->lock);
+		queue_work(nfsd_filecache_wq, &l->work);
 	}
 }
 

Re: [PATCH 3/6] nfsd: simplify the delayed disposal list code

[Chuck Lever III]

> On Apr 14, 2023, at 3:17 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Fri, 2023-04-14 at 18:20 +0000, Chuck Lever III wrote:
>>> On Jan 18, 2023, at 12:31 PM, Jeff Layton <jlayton@kernel.org>
>>> wrote:
>>> 
>>> When queueing a dispose list to the appropriate "freeme" lists, it
>>> pointlessly queues the objects one at a time to an intermediate
>>> list.
>>> 
>>> Remove a few helpers and just open code a list_move to make it more
>>> clear and efficient. Better document the resulting functions with
>>> kerneldoc comments.
>>> 
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>> fs/nfsd/filecache.c | 63 +++++++++++++++----------------------------
>>> --
>>> 1 file changed, 21 insertions(+), 42 deletions(-)
>>> 
>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>> index 58ac93e7e680..a2bc4bd90b9a 100644
>>> --- a/fs/nfsd/filecache.c
>>> +++ b/fs/nfsd/filecache.c
>>> @@ -513,49 +513,25 @@ nfsd_file_dispose_list(struct list_head
>>> *dispose)
>>> }
>>> }
>>> 
>>> -static void
>>> -nfsd_file_list_remove_disposal(struct list_head *dst,
>>> - struct nfsd_fcache_disposal *l)
>>> -{
>>> - spin_lock(&l->lock);
>>> - list_splice_init(&l->freeme, dst);
>>> - spin_unlock(&l->lock);
>>> -}
>>> -
>>> -static void
>>> -nfsd_file_list_add_disposal(struct list_head *files, struct net
>>> *net)
>>> -{
>>> - struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>> - struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>>> -
>>> - spin_lock(&l->lock);
>>> - list_splice_tail_init(files, &l->freeme);
>>> - spin_unlock(&l->lock);
>>> - queue_work(nfsd_filecache_wq, &l->work);
>>> -}
>>> -
>>> -static void
>>> -nfsd_file_list_add_pernet(struct list_head *dst, struct list_head
>>> *src,
>>> - struct net *net)
>>> -{
>>> - struct nfsd_file *nf, *tmp;
>>> -
>>> - list_for_each_entry_safe(nf, tmp, src, nf_lru) {
>>> - if (nf->nf_net == net)
>>> - list_move_tail(&nf->nf_lru, dst);
>>> - }
>>> -}
>>> -
>>> +/**
>>> + * nfsd_file_dispose_list_delayed - move list of dead files to
>>> net's freeme list
>>> + * @dispose: list of nfsd_files to be disposed
>>> + *
>>> + * Transfers each file to the "freeme" list for its nfsd_net, to
>>> eventually
>>> + * be disposed of by the per-net garbage collector.
>>> + */
>>> static void
>>> nfsd_file_dispose_list_delayed(struct list_head *dispose)
>>> {
>>> - LIST_HEAD(list);
>>> - struct nfsd_file *nf;
>>> -
>>> while(!list_empty(dispose)) {
>>> - nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
>>> - nfsd_file_list_add_pernet(&list, dispose, nf->nf_net);
>>> - nfsd_file_list_add_disposal(&list, nf->nf_net);
>>> + struct nfsd_file *nf = list_first_entry(dispose,
>>> + struct nfsd_file, nf_lru);
>>> + struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
>>> + struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>>> +
>>> + spin_lock(&l->lock);
>>> + list_move_tail(&nf->nf_lru, &l->freeme);
>>> + spin_unlock(&l->lock);
>>> }
>>> }
>>> 
>>> @@ -765,8 +741,8 @@ nfsd_file_close_inode_sync(struct inode *inode)
>>>  * nfsd_file_delayed_close - close unused nfsd_files
>>>  * @work: dummy
>>>  *
>>> - * Walk the LRU list and destroy any entries that have not been
>>> used since
>>> - * the last scan.
>>> + * Scrape the freeme list for this nfsd_net, and then dispose of
>>> them
>>> + * all.
>>>  */
>>> static void
>>> nfsd_file_delayed_close(struct work_struct *work)
>>> @@ -775,7 +751,10 @@ nfsd_file_delayed_close(struct work_struct
>>> *work)
>>> struct nfsd_fcache_disposal *l = container_of(work,
>>> struct nfsd_fcache_disposal, work);
>>> 
>>> - nfsd_file_list_remove_disposal(&head, l);
>>> + spin_lock(&l->lock);
>>> + list_splice_init(&l->freeme, &head);
>>> + spin_unlock(&l->lock);
>>> +
>>> nfsd_file_dispose_list(&head);
>>> }
>> 
>> Hey Jeff -
>> 
>> After applying this one, tmpfs exports appear to leak space,
>> even after all files and directories are deleted. Eventually
>> the filesystem is "full" -- modifying operations return ENOSPC
>> but removing files doesn't recover the used space.
>> 
>> Can you have a look at this?

> Hrm, ok. Do you have a reproducer?


Nothing special. Any workload that cleans up after itself
should leave the "df -k" %Used column at zero percent when
it finishes. What I'm seeing is that %Used never goes down.


> Actually, I may see the bug. Does this fix it?
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index c173d460b17d..f40d8f3b35a4 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -421,6 +421,7 @@ nfsd_file_dispose_list_delayed(struct list_head
> *dispose)
> spin_lock(&l->lock);
> list_move_tail(&nf->nf_lru, &l->freeme);
> spin_unlock(&l->lock);
> + queue_work(nfsd_filecache_wq, &l->work);
> }
> }

Yes, that addresses the symptom.

I'll drop this version of the patch, send me a refresh?


--
Chuck Lever