| Message ID | 20250113153235.48706-15-cel@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Client-side OFFLOAD_STATUS implementation | expand |
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(©->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(©->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(©->completion); > - spin_lock(&dst_server->nfs_client->cl_lock); > - list_del_init(©->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(©->src_copies); > - spin_unlock(&src_server->nfs_client->cl_lock); > - } > +wait: > + status = wait_for_completion_interruptible_timeout(©->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, ©->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, ©->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
On 1/13/25 11:51 AM, Benjamin Coddington wrote: > 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(©->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(©->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(©->completion); >> - spin_lock(&dst_server->nfs_client->cl_lock); >> - list_del_init(©->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(©->src_copies); >> - spin_unlock(&src_server->nfs_client->cl_lock); >> - } >> +wait: >> + status = wait_for_completion_interruptible_timeout(©->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, ©->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, ©->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? I think you mean in the rare case when the /server/ returns REMOTEIO? I changed nfs42_proc_offload_status() so that it now always initializes @copied. Thanks for your review! > 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 >
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(©->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(©->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(©->completion); - spin_lock(&dst_server->nfs_client->cl_lock); - list_del_init(©->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(©->src_copies); - spin_unlock(&src_server->nfs_client->cl_lock); - } +wait: + status = wait_for_completion_interruptible_timeout(©->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, ©->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, ©->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; + 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);