Re: [PATCH v3 6/7] NFS: Use NFSv4.2's OFFLOAD_STATUS operation

Benjamin Coddington
2025-01-13T16:51:34Z

On 13 Jan 2025, at 10:32, cel@kernel.org wrote:

> From: Chuck Lever <chuck.lever@oracle.com>
>
> We've found that there are cases where a transport disconnection
> results in the loss of callback RPCs. NFS servers typically do not
> retransmit callback operations after a disconnect.
>
> This can be a problem for the Linux NFS client's current
> implementation of asynchronous COPY, which waits indefinitely for a
> CB_OFFLOAD callback. If a transport disconnect occurs while an async
> COPY is running, there's a good chance the client will never get the
> completing CB_OFFLOAD.
>
> Fix this by implementing the OFFLOAD_STATUS operation so that the
> Linux NFS client can probe the NFS server if it doesn't see a
> CB_OFFLOAD in a reasonable amount of time.
>
> This patch implements a simplistic check. As future work, the client
> might also be able to detect whether there is no forward progress on
> the request asynchronous COPY operation, and CANCEL it.
>
> Suggested-by: Olga Kornievskaia <kolga@netapp.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218735
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> fs/nfs/nfs42proc.c | 70 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 59 insertions(+), 11 deletions(-)
>
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index 23508669d051..4baa66cd966a 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -175,6 +175,20 @@ int nfs42_proc_deallocate(struct file *filep, loff_t offset, loff_t len)
> return err;
> }
>
> +static void nfs4_copy_dequeue_callback(struct nfs_server *dst_server,
> + struct nfs_server *src_server,
> + struct nfs4_copy_state *copy)
> +{
> + spin_lock(&dst_server->nfs_client->cl_lock);
> + list_del_init(&copy->copies);
> + spin_unlock(&dst_server->nfs_client->cl_lock);
> + if (dst_server != src_server) {
> + spin_lock(&src_server->nfs_client->cl_lock);
> + list_del_init(&copy->src_copies);
> + spin_unlock(&src_server->nfs_client->cl_lock);
> + }
> +}
> +
> static int handle_async_copy(struct nfs42_copy_res *res,
> struct nfs_server *dst_server,
> struct nfs_server *src_server,
> @@ -184,9 +198,12 @@ static int handle_async_copy(struct nfs42_copy_res *res,
> bool *restart)
> {
> struct nfs4_copy_state *copy, *tmp_copy = NULL, *iter;
> - int status = NFS4_OK;
> struct nfs_open_context *dst_ctx = nfs_file_open_context(dst);
> struct nfs_open_context *src_ctx = nfs_file_open_context(src);
> + struct nfs_client *clp = dst_server->nfs_client;
> + unsigned long timeout = 3 * HZ;
> + int status = NFS4_OK;
> + u64 copied;
>
> copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_KERNEL);
> if (!copy)
> @@ -224,15 +241,12 @@ static int handle_async_copy(struct nfs42_copy_res *res,
> spin_unlock(&src_server->nfs_client->cl_lock);
> }
>
> - status = wait_for_completion_interruptible(&copy->completion);
> - spin_lock(&dst_server->nfs_client->cl_lock);
> - list_del_init(&copy->copies);
> - spin_unlock(&dst_server->nfs_client->cl_lock);
> - if (dst_server != src_server) {
> - spin_lock(&src_server->nfs_client->cl_lock);
> - list_del_init(&copy->src_copies);
> - spin_unlock(&src_server->nfs_client->cl_lock);
> - }
> +wait:
> + status = wait_for_completion_interruptible_timeout(&copy->completion,
> + timeout);
> + if (!status)
> + goto timeout;
> + nfs4_copy_dequeue_callback(dst_server, src_server, copy);
> if (status == -ERESTARTSYS) {
> goto out_cancel;
> } else if (copy->flags || copy->error == NFS4ERR_PARTNER_NO_AUTH) {
> @@ -242,6 +256,7 @@ static int handle_async_copy(struct nfs42_copy_res *res,
> }
> out:
> res->write_res.count = copy->count;
> + /* Copy out the updated write verifier provided by CB_OFFLOAD. */
> memcpy(&res->write_res.verifier, &copy->verf, sizeof(copy->verf));
> status = -copy->error;
>
> @@ -253,6 +268,39 @@ static int handle_async_copy(struct nfs42_copy_res *res,
> if (!nfs42_files_from_same_server(src, dst))
> nfs42_do_offload_cancel_async(src, src_stateid);
> goto out_free;
> +timeout:
> + timeout <<= 1;
> + if (timeout > (clp->cl_lease_time >> 1))
> + timeout = clp->cl_lease_time >> 1;
> + status = nfs42_proc_offload_status(dst, &copy->stateid, &copied);
> + if (status == -EINPROGRESS)
> + goto wait;
> + nfs4_copy_dequeue_callback(dst_server, src_server, copy);
> + switch (status) {
> + case 0:
> + /* The server recognized the copy stateid, so it hasn't
> + * rebooted. Don't overwrite the verifier returned in the
> + * COPY result. */
> + res->write_res.count = copied;
> + goto out_free;
> + case -EREMOTEIO:
> + /* COPY operation failed on the server. */
> + status = -EOPNOTSUPP;
> + res->write_res.count = copied;

I think "copied" can be the original uninitialized stack var in this path,
is that a problem?

Ben

> + goto out_free;
> + case -EBADF:
> + /* Server did not recognize the copy stateid. It has
> + * probably restarted and lost the plot. */
> + res->write_res.count = 0;
> + status = -EOPNOTSUPP;
> + break;
> + case -EOPNOTSUPP:
> + /* RFC 7862 REQUIREs server to support OFFLOAD_STATUS when
> + * it has signed up for an async COPY, so server is not
> + * spec-compliant. */
> + res->write_res.count = 0;
> + }
> + goto out_free;
> }
>
> static int process_copy_commit(struct file *dst, loff_t pos_dst,
> @@ -643,7 +691,7 @@ _nfs42_proc_offload_status(struct nfs_server *server, struct file *file,
> * Other negative errnos indicate the client could not complete the
> * request.
> */
> -static int __maybe_unused
> +static int
> nfs42_proc_offload_status(struct file *dst, nfs4_stateid *stateid, u64 *copied)
> {
> struct inode *inode = file_inode(dst);
> --
> 2.47.0