[PATCH v3 0/8] async COPY fixes for NFSD

[PATCH v3 0/8] async COPY fixes for NFSD

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Extend the life of async COPY state IDs so that clients get
actionable OFFLOAD_STATUS results after COPY operations complete.
This lifetime extension comports with RFC 7862, although does not
bring NFSD fully into compliance.

There are a number of other small fixes to improve observability,
behavior during temporary resource shortages, and behavior during
client shutdown.

Async COPY remains disabled in NFSD until the Linux client has grown
support for OFFLOAD_STATUS. Patches for that are available in the
"fix-async-copy" branch of:

  https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git

Changes since previous versions:
- Loads of testing, improvements, and re-organization

Chuck Lever (8):
  NFSD: Add a tracepoint to record canceled async COPY operations
  NFSD: Fix nfsd4_shutdown_copy()
  NFSD: Free async copy information in nfsd4_cb_offload_release()
  NFSD: Handle an NFS4ERR_DELAY response to CB_OFFLOAD
  NFSD: Block DESTROY_CLIENTID only when there are ongoing async COPY
    operations
  NFSD: Add a laundromat reaper for async copy state
  NFSD: Add nfsd4_copy time-to-live
  NFSD: Send CB_OFFLOAD on graceful shutdown

 fs/nfsd/nfs4proc.c  | 101 ++++++++++++++++++++++++++++++++++++++++----
 fs/nfsd/nfs4state.c |   3 +-
 fs/nfsd/state.h     |  17 ++++++++
 fs/nfsd/trace.h     |  11 ++++-
 fs/nfsd/xdr4.h      |   6 +++
 5 files changed, 128 insertions(+), 10 deletions(-)

-- 
2.47.0

[PATCH v3 1/8] NFSD: Add a tracepoint to record canceled async COPY operations

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4proc.c |  1 +
 fs/nfsd/trace.h    | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index d32f2dfd148f..9f6617fa5412 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1287,6 +1287,7 @@ static void nfs4_put_copy(struct nfsd4_copy *copy)
 
 static void nfsd4_stop_copy(struct nfsd4_copy *copy)
 {
+	trace_nfsd_copy_async_cancel(copy);
 	if (!test_and_set_bit(NFSD4_COPY_F_STOPPED, &copy->cp_flags))
 		kthread_stop(copy->copy_task);
 	nfs4_put_copy(copy);
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index b8470d4cbe99..acbc7f37cdc5 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -2232,7 +2232,7 @@ TRACE_EVENT(nfsd_copy_done,
 	)
 );
 
-TRACE_EVENT(nfsd_copy_async_done,
+DECLARE_EVENT_CLASS(nfsd_copy_async_done_class,
 	TP_PROTO(
 		const struct nfsd4_copy *copy
 	),
@@ -2301,6 +2301,15 @@ TRACE_EVENT(nfsd_copy_async_done,
 	)
 );
 
+#define DEFINE_COPY_ASYNC_DONE_EVENT(name)		\
+DEFINE_EVENT(nfsd_copy_async_done_class,		\
+	nfsd_copy_async_##name,				\
+	TP_PROTO(const struct nfsd4_copy *copy),	\
+	TP_ARGS(copy))
+
+DEFINE_COPY_ASYNC_DONE_EVENT(done);
+DEFINE_COPY_ASYNC_DONE_EVENT(cancel);
+
 #endif /* _NFSD_TRACE_H */
 
 #undef TRACE_INCLUDE_PATH
-- 
2.47.0

[PATCH v3 2/8] NFSD: Fix nfsd4_shutdown_copy()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

nfsd4_shutdown_copy() is just this:

	while ((copy = nfsd4_get_copy(clp)) != NULL)
		nfsd4_stop_copy(copy);

nfsd4_get_copy() bumps @copy's reference count, preventing
nfsd4_stop_copy() from releasing @copy.

A while loop like this usually works by removing the first element
of the list, but neither nfsd4_get_copy() nor nfsd4_stop_copy()
alters the async_copies list.

Best I can tell, then, is that nfsd4_shutdown_copy() continues to
loop until other threads manage to remove all the items from this
list. The spinning loop blocks shutdown until these items are gone.

Possibly the reason we haven't seen this issue in the field is
because client_has_state() prevents __destroy_client() from calling
nfsd4_shutdown_copy() if there are any items on this list. In a
subsequent patch I plan to remove that restriction.

Fixes: e0639dc5805a ("NFSD introduce async copy feature")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4proc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 9f6617fa5412..8229bbfdd735 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1302,6 +1302,9 @@ static struct nfsd4_copy *nfsd4_get_copy(struct nfs4_client *clp)
 		copy = list_first_entry(&clp->async_copies, struct nfsd4_copy,
 					copies);
 		refcount_inc(&copy->refcount);
+		copy->cp_clp = NULL;
+		if (!list_empty(&copy->copies))
+			list_del_init(&copy->copies);
 	}
 	spin_unlock(&clp->async_lock);
 	return copy;
-- 
2.47.0

Re: [PATCH v3 2/8] NFSD: Fix nfsd4_shutdown_copy()

[Jeff Layton]

On Thu, 2024-10-31 at 09:40 -0400, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> nfsd4_shutdown_copy() is just this:
> 
> 	while ((copy = nfsd4_get_copy(clp)) != NULL)
> 		nfsd4_stop_copy(copy);
> 
> nfsd4_get_copy() bumps @copy's reference count, preventing
> nfsd4_stop_copy() from releasing @copy.
> 
> A while loop like this usually works by removing the first element
> of the list, but neither nfsd4_get_copy() nor nfsd4_stop_copy()
> alters the async_copies list.
> 
> Best I can tell, then, is that nfsd4_shutdown_copy() continues to
> loop until other threads manage to remove all the items from this
> list. The spinning loop blocks shutdown until these items are gone.
> 
> Possibly the reason we haven't seen this issue in the field is
> because client_has_state() prevents __destroy_client() from calling
> nfsd4_shutdown_copy() if there are any items on this list. In a
> subsequent patch I plan to remove that restriction.
> 
> Fixes: e0639dc5805a ("NFSD introduce async copy feature")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4proc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 9f6617fa5412..8229bbfdd735 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1302,6 +1302,9 @@ static struct nfsd4_copy *nfsd4_get_copy(struct nfs4_client *clp)
>  		copy = list_first_entry(&clp->async_copies, struct nfsd4_copy,
>  					copies);
>  		refcount_inc(&copy->refcount);
> +		copy->cp_clp = NULL;
> +		if (!list_empty(&copy->copies))
> +			list_del_init(&copy->copies);
>  	}
>  	spin_unlock(&clp->async_lock);
>  	return copy;

My initial thought was:

The problem sounds real, but is nfsd4_get_copy() the place for the
above logic?

But then I noticed that nfsd4_get_copy() is only called from
nfsd4_shutdown_copy(). Maybe we should be rename nfsd4_get_copy() to
nfsd4_find_and_unhash_copy() ?

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 2/8] NFSD: Fix nfsd4_shutdown_copy()

[Chuck Lever]

On Fri, Nov 01, 2024 at 08:28:42AM -0400, Jeff Layton wrote:
> On Thu, 2024-10-31 at 09:40 -0400, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > nfsd4_shutdown_copy() is just this:
> > 
> > 	while ((copy = nfsd4_get_copy(clp)) != NULL)
> > 		nfsd4_stop_copy(copy);
> > 
> > nfsd4_get_copy() bumps @copy's reference count, preventing
> > nfsd4_stop_copy() from releasing @copy.
> > 
> > A while loop like this usually works by removing the first element
> > of the list, but neither nfsd4_get_copy() nor nfsd4_stop_copy()
> > alters the async_copies list.
> > 
> > Best I can tell, then, is that nfsd4_shutdown_copy() continues to
> > loop until other threads manage to remove all the items from this
> > list. The spinning loop blocks shutdown until these items are gone.
> > 
> > Possibly the reason we haven't seen this issue in the field is
> > because client_has_state() prevents __destroy_client() from calling
> > nfsd4_shutdown_copy() if there are any items on this list. In a
> > subsequent patch I plan to remove that restriction.
> > 
> > Fixes: e0639dc5805a ("NFSD introduce async copy feature")
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfs4proc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 9f6617fa5412..8229bbfdd735 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1302,6 +1302,9 @@ static struct nfsd4_copy *nfsd4_get_copy(struct nfs4_client *clp)
> >  		copy = list_first_entry(&clp->async_copies, struct nfsd4_copy,
> >  					copies);
> >  		refcount_inc(&copy->refcount);
> > +		copy->cp_clp = NULL;
> > +		if (!list_empty(&copy->copies))
> > +			list_del_init(&copy->copies);
> >  	}
> >  	spin_unlock(&clp->async_lock);
> >  	return copy;
> 
> My initial thought was:
> 
> The problem sounds real, but is nfsd4_get_copy() the place for the
> above logic?
> 
> But then I noticed that nfsd4_get_copy() is only called from
> nfsd4_shutdown_copy(). Maybe we should be rename nfsd4_get_copy() to
> nfsd4_find_and_unhash_copy() ?

nfsd4_get_copy() book-ends nfsd4_put_copy(). But nfsd4_unhash_copy()
would work too, I think.

-- 
Chuck Lever

[PATCH v3 3/8] NFSD: Free async copy information in nfsd4_cb_offload_release()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

RFC 7862 Section 4.8 states:

> A copy offload stateid will be valid until either (A) the client
> or server restarts or (B) the client returns the resource by
> issuing an OFFLOAD_CANCEL operation or the client replies to a
> CB_OFFLOAD operation.

Currently, NFSD purges the metadata for an async COPY operation as
soon as the CB_OFFLOAD callback has been sent. It does not wait even
for the client's CB_OFFLOAD response, as the paragraph above
suggests that it should.

This makes the OFFLOAD_STATUS operation ineffective during the
window between the completion of an asynchronous COPY and the
server's receipt of the corresponding CB_OFFLOAD response. This is
important if, for example, the client responds with NFS4ERR_DELAY,
or the transport is lost before the server receives the response. A
client might use OFFLOAD_STATUS to query the server about the still
pending asynchronous COPY, but NFSD will respond to OFFLOAD_STATUS
as if it had never heard of the presented copy stateid.

This patch starts to address this issue by extending the lifetime of
struct nfsd4_copy at least until the server has seen the client's
CB_OFFLOAD response, or the CB_OFFLOAD has timed out.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4proc.c | 17 ++++++++++-------
 fs/nfsd/xdr4.h     |  3 +++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 8229bbfdd735..39e90391bce2 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -57,6 +57,8 @@ module_param(inter_copy_offload_enable, bool, 0644);
 MODULE_PARM_DESC(inter_copy_offload_enable,
 		 "Enable inter server to server copy offload. Default: false");
 
+static void cleanup_async_copy(struct nfsd4_copy *copy);
+
 #ifdef CONFIG_NFSD_V4_2_INTER_SSC
 static int nfsd4_ssc_umount_timeout = 900000;		/* default to 15 mins */
 module_param(nfsd4_ssc_umount_timeout, int, 0644);
@@ -1602,8 +1604,10 @@ static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
 {
 	struct nfsd4_cb_offload *cbo =
 		container_of(cb, struct nfsd4_cb_offload, co_cb);
+	struct nfsd4_copy *copy =
+		container_of(cbo, struct nfsd4_copy, cp_cb_offload);
 
-	kfree(cbo);
+	cleanup_async_copy(copy);
 }
 
 static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
@@ -1736,11 +1740,7 @@ static void cleanup_async_copy(struct nfsd4_copy *copy)
 
 static void nfsd4_send_cb_offload(struct nfsd4_copy *copy)
 {
-	struct nfsd4_cb_offload *cbo;
-
-	cbo = kzalloc(sizeof(*cbo), GFP_KERNEL);
-	if (!cbo)
-		return;
+	struct nfsd4_cb_offload *cbo = &copy->cp_cb_offload;
 
 	memcpy(&cbo->co_res, &copy->cp_res, sizeof(copy->cp_res));
 	memcpy(&cbo->co_fh, &copy->fh, sizeof(copy->fh));
@@ -1790,10 +1790,13 @@ static int nfsd4_do_async_copy(void *data)
 	}
 
 do_callback:
+	/* The kthread exits forthwith. Ensure that a subsequent
+	 * OFFLOAD_CANCEL won't try to kill it again. */
+	set_bit(NFSD4_COPY_F_STOPPED, &copy->cp_flags);
+
 	set_bit(NFSD4_COPY_F_COMPLETED, &copy->cp_flags);
 	trace_nfsd_copy_async_done(copy);
 	nfsd4_send_cb_offload(copy);
-	cleanup_async_copy(copy);
 	return 0;
 }
 
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 2a21a7662e03..dec29afa43f3 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -699,6 +699,9 @@ struct nfsd4_copy {
 	struct nfsd42_write_res	cp_res;
 	struct knfsd_fh		fh;
 
+	/* offload callback */
+	struct nfsd4_cb_offload	cp_cb_offload;
+
 	struct nfs4_client      *cp_clp;
 
 	struct nfsd_file        *nf_src;
-- 
2.47.0

[PATCH v3 4/8] NFSD: Handle an NFS4ERR_DELAY response to CB_OFFLOAD

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

RFC 7862 permits callback services to respond to CB_OFFLOAD with
NFS4ERR_DELAY. Currently NFSD drops the CB_OFFLOAD in that case.

To improve the reliability of COPY offload, NFSD should rather send
another CB_OFFLOAD completion notification.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4proc.c | 8 ++++++++
 fs/nfsd/xdr4.h     | 1 +
 2 files changed, 9 insertions(+)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 39e90391bce2..0918d05c54a1 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1617,6 +1617,13 @@ static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
 		container_of(cb, struct nfsd4_cb_offload, co_cb);
 
 	trace_nfsd_cb_offload_done(&cbo->co_res.cb_stateid, task);
+	switch (task->tk_status) {
+	case -NFS4ERR_DELAY:
+		if (cbo->co_retries--) {
+			rpc_delay(task, 1 * HZ);
+			return 0;
+		}
+	}
 	return 1;
 }
 
@@ -1745,6 +1752,7 @@ static void nfsd4_send_cb_offload(struct nfsd4_copy *copy)
 	memcpy(&cbo->co_res, &copy->cp_res, sizeof(copy->cp_res));
 	memcpy(&cbo->co_fh, &copy->fh, sizeof(copy->fh));
 	cbo->co_nfserr = copy->nfserr;
+	cbo->co_retries = 5;
 
 	nfsd4_init_cb(&cbo->co_cb, copy->cp_clp, &nfsd4_cb_offload_ops,
 		      NFSPROC4_CLNT_CB_OFFLOAD);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index dec29afa43f3..cd2bf63651e3 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -675,6 +675,7 @@ struct nfsd4_cb_offload {
 	struct nfsd4_callback	co_cb;
 	struct nfsd42_write_res	co_res;
 	__be32			co_nfserr;
+	unsigned int		co_retries;
 	struct knfsd_fh		co_fh;
 };
 
-- 
2.47.0

Re: [PATCH v3 4/8] NFSD: Handle an NFS4ERR_DELAY response to CB_OFFLOAD

[Jeff Layton]

On Thu, 2024-10-31 at 09:40 -0400, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> RFC 7862 permits callback services to respond to CB_OFFLOAD with
> NFS4ERR_DELAY. Currently NFSD drops the CB_OFFLOAD in that case.
> 
> To improve the reliability of COPY offload, NFSD should rather send
> another CB_OFFLOAD completion notification.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4proc.c | 8 ++++++++
>  fs/nfsd/xdr4.h     | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 39e90391bce2..0918d05c54a1 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1617,6 +1617,13 @@ static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
>  		container_of(cb, struct nfsd4_cb_offload, co_cb);
>  
>  	trace_nfsd_cb_offload_done(&cbo->co_res.cb_stateid, task);
> +	switch (task->tk_status) {
> +	case -NFS4ERR_DELAY:
> +		if (cbo->co_retries--) {
> +			rpc_delay(task, 1 * HZ);
> +			return 0;
> +		}
> +	}
>  	return 1;

Not a comment on your patch specifically, but when we can't send a
callback, should we be trying to log something? A pr_notice() warning?
Conditional tracepoint? I'm not sure of the best way to communicate
this, but it seems like something that admins might want to know.

Maybe nfsd needs its own trace buffer that could be scraped with
nfsdctl?

>  }
>  
> @@ -1745,6 +1752,7 @@ static void nfsd4_send_cb_offload(struct nfsd4_copy *copy)
>  	memcpy(&cbo->co_res, &copy->cp_res, sizeof(copy->cp_res));
>  	memcpy(&cbo->co_fh, &copy->fh, sizeof(copy->fh));
>  	cbo->co_nfserr = copy->nfserr;
> +	cbo->co_retries = 5;
>  
>  	nfsd4_init_cb(&cbo->co_cb, copy->cp_clp, &nfsd4_cb_offload_ops,
>  		      NFSPROC4_CLNT_CB_OFFLOAD);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index dec29afa43f3..cd2bf63651e3 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -675,6 +675,7 @@ struct nfsd4_cb_offload {
>  	struct nfsd4_callback	co_cb;
>  	struct nfsd42_write_res	co_res;
>  	__be32			co_nfserr;
> +	unsigned int		co_retries;
>  	struct knfsd_fh		co_fh;
>  };
>  

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 4/8] NFSD: Handle an NFS4ERR_DELAY response to CB_OFFLOAD

[Chuck Lever]

On Fri, Nov 01, 2024 at 08:41:33AM -0400, Jeff Layton wrote:
> On Thu, 2024-10-31 at 09:40 -0400, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > RFC 7862 permits callback services to respond to CB_OFFLOAD with
> > NFS4ERR_DELAY. Currently NFSD drops the CB_OFFLOAD in that case.
> > 
> > To improve the reliability of COPY offload, NFSD should rather send
> > another CB_OFFLOAD completion notification.
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfs4proc.c | 8 ++++++++
> >  fs/nfsd/xdr4.h     | 1 +
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 39e90391bce2..0918d05c54a1 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1617,6 +1617,13 @@ static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
> >  		container_of(cb, struct nfsd4_cb_offload, co_cb);
> >  
> >  	trace_nfsd_cb_offload_done(&cbo->co_res.cb_stateid, task);
> > +	switch (task->tk_status) {
> > +	case -NFS4ERR_DELAY:
> > +		if (cbo->co_retries--) {
> > +			rpc_delay(task, 1 * HZ);
> > +			return 0;
> > +		}
> > +	}
> >  	return 1;
> 
> Not a comment on your patch specifically, but when we can't send a
> callback, should we be trying to log something? A pr_notice() warning?
> Conditional tracepoint? I'm not sure of the best way to communicate
> this, but it seems like something that admins might want to know.

There is a tracepoint here and in every other callback completion
handler.

I can't think of anything actionable to report -- what would an
admin need or want to do in response to a callback failure? There
isn't much to do if, for instance, the client doesn't recognize the
copy stateid.

Also, DELAY is not infrequent and is a common temporary condition.

My sense is that the only time you want to see callback failures is
when you're looking for something specific. Otherwise, it will
generate a lot of low-value noise.


> Maybe nfsd needs its own trace buffer that could be scraped with
> nfsdctl?

The Flight Data Recorder can do that if needed.


> >  }
> >  
> > @@ -1745,6 +1752,7 @@ static void nfsd4_send_cb_offload(struct nfsd4_copy *copy)
> >  	memcpy(&cbo->co_res, &copy->cp_res, sizeof(copy->cp_res));
> >  	memcpy(&cbo->co_fh, &copy->fh, sizeof(copy->fh));
> >  	cbo->co_nfserr = copy->nfserr;
> > +	cbo->co_retries = 5;
> >  
> >  	nfsd4_init_cb(&cbo->co_cb, copy->cp_clp, &nfsd4_cb_offload_ops,
> >  		      NFSPROC4_CLNT_CB_OFFLOAD);
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index dec29afa43f3..cd2bf63651e3 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -675,6 +675,7 @@ struct nfsd4_cb_offload {
> >  	struct nfsd4_callback	co_cb;
> >  	struct nfsd42_write_res	co_res;
> >  	__be32			co_nfserr;
> > +	unsigned int		co_retries;
> >  	struct knfsd_fh		co_fh;
> >  };
> >  
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

-- 
Chuck Lever

[PATCH v3 5/8] NFSD: Block DESTROY_CLIENTID only when there are ongoing async COPY operations

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Currently __destroy_client() consults the nfs4_client's async_copies
list to determine whether there are ongoing async COPY operations.
However, NFSD now keeps copy state in that list even when the
async copy has completed, to enable OFFLOAD_STATUS to find the
COPY results for a while after the COPY has completed.

DESTROY_CLIENTID should not be blocked if the client's async_copies
list contains state for only completed copy operations.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4proc.c  | 30 ++++++++++++++++++++++++++++++
 fs/nfsd/nfs4state.c |  2 +-
 fs/nfsd/state.h     |  1 +
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 0918d05c54a1..4d44b785a580 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1278,6 +1278,36 @@ nfsd4_clone(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	return status;
 }
 
+/**
+ * nfsd4_has_active_async_copies - Check for ongoing copy operations
+ * @clp: Client to be checked
+ *
+ * NFSD maintains state for async COPY operations after they complete,
+ * and this state remains in the nfs4_client's async_copies list.
+ * Ongoing copies should block the destruction of the nfs4_client, but
+ * completed copies should not.
+ *
+ * Return values:
+ *   %true: At least one active async COPY is ongoing
+ *   %false: No active async COPY operations were found
+ */
+bool nfsd4_has_active_async_copies(struct nfs4_client *clp)
+{
+	struct nfsd4_copy *copy;
+	bool result = false;
+
+	spin_lock(&clp->async_lock);
+	list_for_each_entry(copy, &clp->async_copies, copies) {
+		if (!test_bit(NFSD4_COPY_F_COMPLETED, &copy->cp_flags) &&
+		    !test_bit(NFSD4_COPY_F_STOPPED, &copy->cp_flags)) {
+			result = true;
+			break;
+		}
+	}
+	spin_unlock(&clp->async_lock);
+	return result;
+}
+
 static void nfs4_put_copy(struct nfsd4_copy *copy)
 {
 	if (!refcount_dec_and_test(&copy->refcount))
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 551d2958ec29..cde5ba69d7a5 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3487,7 +3487,7 @@ static bool client_has_state(struct nfs4_client *clp)
 #endif
 		|| !list_empty(&clp->cl_delegations)
 		|| !list_empty(&clp->cl_sessions)
-		|| !list_empty(&clp->async_copies);
+		|| nfsd4_has_active_async_copies(clp);
 }
 
 static __be32 copy_impl_id(struct nfs4_client *clp,
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 35b3564c065f..6c84c0900ec4 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -742,6 +742,7 @@ extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
 extern bool nfsd4_run_cb(struct nfsd4_callback *cb);
 extern void nfsd4_shutdown_callback(struct nfs4_client *);
 extern void nfsd4_shutdown_copy(struct nfs4_client *clp);
+bool nfsd4_has_active_async_copies(struct nfs4_client *clp);
 extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name,
 				struct xdr_netobj princhash, struct nfsd_net *nn);
 extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
-- 
2.47.0

[PATCH v3 6/8] NFSD: Add a laundromat reaper for async copy state

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

RFC 7862 Section 4.8 states:

> A copy offload stateid will be valid until either (A) the client
> or server restarts or (B) the client returns the resource by
> issuing an OFFLOAD_CANCEL operation or the client replies to a
> CB_OFFLOAD operation.

Instead of releasing async copy state when the CB_OFFLOAD callback
completes, now let it live until the next laundromat run after the
callback completes.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4proc.c  | 35 ++++++++++++++++++++++++++++++++++-
 fs/nfsd/nfs4state.c |  1 +
 fs/nfsd/state.h     |  1 +
 fs/nfsd/xdr4.h      |  1 +
 4 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 4d44b785a580..7061db2f33b0 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1308,6 +1308,39 @@ bool nfsd4_has_active_async_copies(struct nfs4_client *clp)
 	return result;
 }
 
+/**
+ * nfsd4_async_copy_reaper - Purge completed copies
+ * @nn: Network namespace with possible active copy information
+ */
+void nfsd4_async_copy_reaper(struct nfsd_net *nn)
+{
+	struct nfs4_client *clp;
+	struct nfsd4_copy *copy;
+	LIST_HEAD(reaplist);
+
+	spin_lock(&nn->client_lock);
+	list_for_each_entry(clp, &nn->client_lru, cl_lru) {
+		struct list_head *pos, *next;
+
+		spin_lock(&clp->async_lock);
+		list_for_each_safe(pos, next, &clp->async_copies) {
+			copy = list_entry(pos, struct nfsd4_copy, copies);
+			if (test_bit(NFSD4_COPY_F_OFFLOAD_DONE, &copy->cp_flags)) {
+				list_del_init(&copy->copies);
+				list_add(&copy->copies, &reaplist);
+			}
+		}
+		spin_unlock(&clp->async_lock);
+	}
+	spin_unlock(&nn->client_lock);
+
+	while (!list_empty(&reaplist)) {
+		copy = list_first_entry(&reaplist, struct nfsd4_copy, copies);
+		list_del_init(&copy->copies);
+		cleanup_async_copy(copy);
+	}
+}
+
 static void nfs4_put_copy(struct nfsd4_copy *copy)
 {
 	if (!refcount_dec_and_test(&copy->refcount))
@@ -1637,7 +1670,7 @@ static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
 	struct nfsd4_copy *copy =
 		container_of(cbo, struct nfsd4_copy, cp_cb_offload);
 
-	cleanup_async_copy(copy);
+	set_bit(NFSD4_COPY_F_OFFLOAD_DONE, &copy->cp_flags);
 }
 
 static int nfsd4_cb_offload_done(struct nfsd4_callback *cb,
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index cde5ba69d7a5..ea2b5ab9a05c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6562,6 +6562,7 @@ nfs4_laundromat(struct nfsd_net *nn)
 			_free_cpntf_state_locked(nn, cps);
 	}
 	spin_unlock(&nn->s2s_cp_lock);
+	nfsd4_async_copy_reaper(nn);
 	nfs4_get_client_reaplist(nn, &reaplist, &lt);
 	nfs4_process_client_reaplist(&reaplist);
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 6c84c0900ec4..0e7f0dd960c1 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -742,6 +742,7 @@ extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client *clp,
 extern bool nfsd4_run_cb(struct nfsd4_callback *cb);
 extern void nfsd4_shutdown_callback(struct nfs4_client *);
 extern void nfsd4_shutdown_copy(struct nfs4_client *clp);
+void nfsd4_async_copy_reaper(struct nfsd_net *nn);
 bool nfsd4_has_active_async_copies(struct nfs4_client *clp);
 extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name,
 				struct xdr_netobj princhash, struct nfsd_net *nn);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index cd2bf63651e3..a3a59fce33b5 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -694,6 +694,7 @@ struct nfsd4_copy {
 #define NFSD4_COPY_F_SYNCHRONOUS	(2)
 #define NFSD4_COPY_F_COMMITTED		(3)
 #define NFSD4_COPY_F_COMPLETED		(4)
+#define NFSD4_COPY_F_OFFLOAD_DONE	(5)
 
 	/* response */
 	__be32			nfserr;
-- 
2.47.0

[PATCH v3 7/8] NFSD: Add nfsd4_copy time-to-live

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Keep async copy state alive for a few lease cycles after the copy
completes so that OFFLOAD_STATUS returns something meaningful.

This means that NFSD's client shutdown processing needs to purge
any of this state that happens to be waiting to die.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4proc.c |  7 +++++--
 fs/nfsd/state.h    | 15 +++++++++++++++
 fs/nfsd/xdr4.h     |  1 +
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 7061db2f33b0..4c964bce6bd7 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1326,8 +1326,10 @@ void nfsd4_async_copy_reaper(struct nfsd_net *nn)
 		list_for_each_safe(pos, next, &clp->async_copies) {
 			copy = list_entry(pos, struct nfsd4_copy, copies);
 			if (test_bit(NFSD4_COPY_F_OFFLOAD_DONE, &copy->cp_flags)) {
-				list_del_init(&copy->copies);
-				list_add(&copy->copies, &reaplist);
+				if (--copy->cp_ttl) {
+					list_del_init(&copy->copies);
+					list_add(&copy->copies, &reaplist);
+				}
 			}
 		}
 		spin_unlock(&clp->async_lock);
@@ -1921,6 +1923,7 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		async_copy->cp_nn = nn;
 		INIT_LIST_HEAD(&async_copy->copies);
 		refcount_set(&async_copy->refcount, 1);
+		async_copy->cp_ttl = NFSD_COPY_INITIAL_TTL;
 		/* Arbitrary cap on number of pending async copy operations */
 		if (atomic_inc_return(&nn->pending_async_copies) >
 				(int)rqstp->rq_pool->sp_nrthreads)
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 0e7f0dd960c1..5242729e38ec 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -137,6 +137,21 @@ struct nfs4_cpntf_state {
 	time64_t		cpntf_time;	/* last time stateid used */
 };
 
+/*
+ * RFC 7862 Section 4.8 states:
+ *
+ * | A copy offload stateid will be valid until either (A) the client
+ * | or server restarts or (B) the client returns the resource by
+ * | issuing an OFFLOAD_CANCEL operation or the client replies to a
+ * | CB_OFFLOAD operation.
+ *
+ * Because a client might not reply to a CB_OFFLOAD, or a reply
+ * might get lost due to connection loss, NFSD purges async copy
+ * state after a short period to prevent it from accumulating
+ * over time.
+ */
+#define NFSD_COPY_INITIAL_TTL 10
+
 struct nfs4_cb_fattr {
 	struct nfsd4_callback ncf_getattr;
 	u32 ncf_cb_status;
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index a3a59fce33b5..9a0c6c107463 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -714,6 +714,7 @@ struct nfsd4_copy {
 	struct list_head	copies;
 	struct task_struct	*copy_task;
 	refcount_t		refcount;
+	unsigned int		cp_ttl;
 
 	struct nfsd4_ssc_umount_item *ss_nsui;
 	struct nfs_fh		c_fh;
-- 
2.47.0

[PATCH v3 8/8] NFSD: Send CB_OFFLOAD on graceful shutdown

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

If an async COPY operation happens to be running when the server is
shut down, notify the requesting client that the copy has completed.

Since the nfs4_client is going away, seems like this could introduce
some UAFs.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4proc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 4c964bce6bd7..51b3f85f3791 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -68,6 +68,8 @@ MODULE_PARM_DESC(nfsd4_ssc_umount_timeout,
 
 #define NFSDDBG_FACILITY		NFSDDBG_PROC
 
+static void nfsd4_send_cb_offload(struct nfsd4_copy *copy);
+
 static u32 nfsd_attrmask[] = {
 	NFSD_WRITEABLE_ATTRS_WORD0,
 	NFSD_WRITEABLE_ATTRS_WORD1,
@@ -1381,8 +1383,10 @@ void nfsd4_shutdown_copy(struct nfs4_client *clp)
 {
 	struct nfsd4_copy *copy;
 
-	while ((copy = nfsd4_get_copy(clp)) != NULL)
+	while ((copy = nfsd4_get_copy(clp)) != NULL) {
 		nfsd4_stop_copy(copy);
+		nfsd4_send_cb_offload(copy);
+	}
 }
 #ifdef CONFIG_NFSD_V4_2_INTER_SSC
 
-- 
2.47.0

Re: [PATCH v3 8/8] NFSD: Send CB_OFFLOAD on graceful shutdown

[Jeff Layton]

On Thu, 2024-10-31 at 09:40 -0400, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> If an async COPY operation happens to be running when the server is
> shut down, notify the requesting client that the copy has completed.
> 
> Since the nfs4_client is going away, seems like this could introduce
> some UAFs.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4proc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 4c964bce6bd7..51b3f85f3791 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -68,6 +68,8 @@ MODULE_PARM_DESC(nfsd4_ssc_umount_timeout,
>  
>  #define NFSDDBG_FACILITY		NFSDDBG_PROC
>  
> +static void nfsd4_send_cb_offload(struct nfsd4_copy *copy);
> +
>  static u32 nfsd_attrmask[] = {
>  	NFSD_WRITEABLE_ATTRS_WORD0,
>  	NFSD_WRITEABLE_ATTRS_WORD1,
> @@ -1381,8 +1383,10 @@ void nfsd4_shutdown_copy(struct nfs4_client *clp)
>  {
>  	struct nfsd4_copy *copy;
>  
> -	while ((copy = nfsd4_get_copy(clp)) != NULL)
> +	while ((copy = nfsd4_get_copy(clp)) != NULL) {
>  		nfsd4_stop_copy(copy);
> +		nfsd4_send_cb_offload(copy);
> +	}

Not sure about a UAF, but  it seems like NFS4ERR_DELAY returns might
delay the client destruction for quite a while. Maybe this CB_OFFLOAD
shouldn't retry on DELAY?


>  }
>  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
>  


-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 8/8] NFSD: Send CB_OFFLOAD on graceful shutdown

[Chuck Lever]

On Fri, Nov 01, 2024 at 09:05:07AM -0400, Jeff Layton wrote:
> On Thu, 2024-10-31 at 09:40 -0400, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > If an async COPY operation happens to be running when the server is
> > shut down, notify the requesting client that the copy has completed.
> > 
> > Since the nfs4_client is going away, seems like this could introduce
> > some UAFs.
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfs4proc.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 4c964bce6bd7..51b3f85f3791 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -68,6 +68,8 @@ MODULE_PARM_DESC(nfsd4_ssc_umount_timeout,
> >  
> >  #define NFSDDBG_FACILITY		NFSDDBG_PROC
> >  
> > +static void nfsd4_send_cb_offload(struct nfsd4_copy *copy);
> > +
> >  static u32 nfsd_attrmask[] = {
> >  	NFSD_WRITEABLE_ATTRS_WORD0,
> >  	NFSD_WRITEABLE_ATTRS_WORD1,
> > @@ -1381,8 +1383,10 @@ void nfsd4_shutdown_copy(struct nfs4_client *clp)
> >  {
> >  	struct nfsd4_copy *copy;
> >  
> > -	while ((copy = nfsd4_get_copy(clp)) != NULL)
> > +	while ((copy = nfsd4_get_copy(clp)) != NULL) {
> >  		nfsd4_stop_copy(copy);
> > +		nfsd4_send_cb_offload(copy);
> > +	}
> 
> Not sure about a UAF, but it seems like NFS4ERR_DELAY returns might
> delay the client destruction for quite a while.

The nfsd4_copy() is removed from the nfs4_client's async_copies
list, and the RPC proceeds in the background. It doesn't block the
destruction of the CLIENTID.

But it might be a problem for the RPC logic to transmit the call
when there's no nfs4_client to dereference. I should probably
drop this patch and try a different approach.


> Maybe this CB_OFFLOAD shouldn't retry on DELAY?
> 
> 
> >  }
> >  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> >  
> 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

-- 
Chuck Lever

Re: [PATCH v3 8/8] NFSD: Send CB_OFFLOAD on graceful shutdown

[Jeff Layton]

On Fri, 2024-11-01 at 09:18 -0400, Chuck Lever wrote:
> On Fri, Nov 01, 2024 at 09:05:07AM -0400, Jeff Layton wrote:
> > On Thu, 2024-10-31 at 09:40 -0400, cel@kernel.org wrote:
> > > From: Chuck Lever <chuck.lever@oracle.com>
> > > 
> > > If an async COPY operation happens to be running when the server is
> > > shut down, notify the requesting client that the copy has completed.
> > > 
> > > Since the nfs4_client is going away, seems like this could introduce
> > > some UAFs.
> > > 
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > ---
> > >  fs/nfsd/nfs4proc.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index 4c964bce6bd7..51b3f85f3791 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -68,6 +68,8 @@ MODULE_PARM_DESC(nfsd4_ssc_umount_timeout,
> > >  
> > >  #define NFSDDBG_FACILITY		NFSDDBG_PROC
> > >  
> > > +static void nfsd4_send_cb_offload(struct nfsd4_copy *copy);
> > > +
> > >  static u32 nfsd_attrmask[] = {
> > >  	NFSD_WRITEABLE_ATTRS_WORD0,
> > >  	NFSD_WRITEABLE_ATTRS_WORD1,
> > > @@ -1381,8 +1383,10 @@ void nfsd4_shutdown_copy(struct nfs4_client *clp)
> > >  {
> > >  	struct nfsd4_copy *copy;
> > >  
> > > -	while ((copy = nfsd4_get_copy(clp)) != NULL)
> > > +	while ((copy = nfsd4_get_copy(clp)) != NULL) {
> > >  		nfsd4_stop_copy(copy);
> > > +		nfsd4_send_cb_offload(copy);
> > > +	}
> > 
> > Not sure about a UAF, but it seems like NFS4ERR_DELAY returns might
> > delay the client destruction for quite a while.
> 
> The nfsd4_copy() is removed from the nfs4_client's async_copies
> list, and the RPC proceeds in the background. It doesn't block the
> destruction of the CLIENTID.
> 
> But it might be a problem for the RPC logic to transmit the call
> when there's no nfs4_client to dereference. I should probably
> drop this patch and try a different approach.


No, I think this will block the client shutdown. After
nfsd4_shutdown_copy(), it calls nfsd4_shutdown_callback(), which does:

        flush_workqueue(clp->cl_callback_wq);                         
        nfsd41_cb_inflight_wait_complete(clp);  

So the CB_OFFLOAD workqueue jobs will run, and everything will wait for
clp->cl_cb_inflight to go to 0. That won't happen until the CB_OFFLOAD
jobs complete.

No objection from me though if you see a better approach. Dropping this
one for now seems reasonable.

> 
> > Maybe this CB_OFFLOAD shouldn't retry on DELAY?
> > 
> > 
> > >  }
> > >  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > >  
> > 
> > 
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 8/8] NFSD: Send CB_OFFLOAD on graceful shutdown

[Chuck Lever]

On Fri, Nov 01, 2024 at 09:30:44AM -0400, Jeff Layton wrote:
> On Fri, 2024-11-01 at 09:18 -0400, Chuck Lever wrote:
> > On Fri, Nov 01, 2024 at 09:05:07AM -0400, Jeff Layton wrote:
> > > On Thu, 2024-10-31 at 09:40 -0400, cel@kernel.org wrote:
> > > > From: Chuck Lever <chuck.lever@oracle.com>
> > > > 
> > > > If an async COPY operation happens to be running when the server is
> > > > shut down, notify the requesting client that the copy has completed.
> > > > 
> > > > Since the nfs4_client is going away, seems like this could introduce
> > > > some UAFs.
> > > > 
> > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > > ---
> > > >  fs/nfsd/nfs4proc.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > index 4c964bce6bd7..51b3f85f3791 100644
> > > > --- a/fs/nfsd/nfs4proc.c
> > > > +++ b/fs/nfsd/nfs4proc.c
> > > > @@ -68,6 +68,8 @@ MODULE_PARM_DESC(nfsd4_ssc_umount_timeout,
> > > >  
> > > >  #define NFSDDBG_FACILITY		NFSDDBG_PROC
> > > >  
> > > > +static void nfsd4_send_cb_offload(struct nfsd4_copy *copy);
> > > > +
> > > >  static u32 nfsd_attrmask[] = {
> > > >  	NFSD_WRITEABLE_ATTRS_WORD0,
> > > >  	NFSD_WRITEABLE_ATTRS_WORD1,
> > > > @@ -1381,8 +1383,10 @@ void nfsd4_shutdown_copy(struct nfs4_client *clp)
> > > >  {
> > > >  	struct nfsd4_copy *copy;
> > > >  
> > > > -	while ((copy = nfsd4_get_copy(clp)) != NULL)
> > > > +	while ((copy = nfsd4_get_copy(clp)) != NULL) {
> > > >  		nfsd4_stop_copy(copy);
> > > > +		nfsd4_send_cb_offload(copy);
> > > > +	}
> > > 
> > > Not sure about a UAF, but it seems like NFS4ERR_DELAY returns might
> > > delay the client destruction for quite a while.
> > 
> > The nfsd4_copy() is removed from the nfs4_client's async_copies
> > list, and the RPC proceeds in the background. It doesn't block the
> > destruction of the CLIENTID.
> > 
> > But it might be a problem for the RPC logic to transmit the call
> > when there's no nfs4_client to dereference. I should probably
> > drop this patch and try a different approach.
> 
> 
> No, I think this will block the client shutdown. After
> nfsd4_shutdown_copy(), it calls nfsd4_shutdown_callback(), which does:
> 
>         flush_workqueue(clp->cl_callback_wq);                         
>         nfsd41_cb_inflight_wait_complete(clp);  
> 
> So the CB_OFFLOAD workqueue jobs will run, and everything will wait for
> clp->cl_cb_inflight to go to 0. That won't happen until the CB_OFFLOAD
> jobs complete.
> 
> No objection from me though if you see a better approach. Dropping this
> one for now seems reasonable.

There is no spec mandate (yet) for CB_OFFLOAD notification on
DESTROY_CLIENTID, so I'm dropping the patch for now.


> > > Maybe this CB_OFFLOAD shouldn't retry on DELAY?

Some logic can be added such that no retry is done. However, if the
client does not respond at all to the CB_OFFLOAD, NFSD's RPC timeout
is still 7 seconds.

We expect this will be exceptionally rare, though -- clients are
typically aware of ongoing COPY operations and generally won't try
to destroy a client ID where there is pending work that they care
about.


> > > >  }
> > > >  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > > >  
> > > 
> > > 
> > > -- 
> > > Jeff Layton <jlayton@kernel.org>
> > > 
> > 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

-- 
Chuck Lever

Re: [PATCH v3 0/8] async COPY fixes for NFSD

[Jeff Layton]

On Thu, 2024-10-31 at 09:40 -0400, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Extend the life of async COPY state IDs so that clients get
> actionable OFFLOAD_STATUS results after COPY operations complete.
> This lifetime extension comports with RFC 7862, although does not
> bring NFSD fully into compliance.
> 
> There are a number of other small fixes to improve observability,
> behavior during temporary resource shortages, and behavior during
> client shutdown.
> 
> Async COPY remains disabled in NFSD until the Linux client has grown
> support for OFFLOAD_STATUS. Patches for that are available in the
> "fix-async-copy" branch of:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> 
> Changes since previous versions:
> - Loads of testing, improvements, and re-organization
> 
> Chuck Lever (8):
>   NFSD: Add a tracepoint to record canceled async COPY operations
>   NFSD: Fix nfsd4_shutdown_copy()
>   NFSD: Free async copy information in nfsd4_cb_offload_release()
>   NFSD: Handle an NFS4ERR_DELAY response to CB_OFFLOAD
>   NFSD: Block DESTROY_CLIENTID only when there are ongoing async COPY
>     operations
>   NFSD: Add a laundromat reaper for async copy state
>   NFSD: Add nfsd4_copy time-to-live
>   NFSD: Send CB_OFFLOAD on graceful shutdown
> 
>  fs/nfsd/nfs4proc.c  | 101 ++++++++++++++++++++++++++++++++++++++++----
>  fs/nfsd/nfs4state.c |   3 +-
>  fs/nfsd/state.h     |  17 ++++++++
>  fs/nfsd/trace.h     |  11 ++++-
>  fs/nfsd/xdr4.h      |   6 +++
>  5 files changed, 128 insertions(+), 10 deletions(-)
> 

This looks fine to me overall. Most of my comments were pretty minor
and can be addressed after the fact.

Reviewed-by: Jeff Layton <jlayton@kernel.org>