| Message ID | 20241008134719.116825-14-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | async COPY fixes | expand |
On Wed, 09 Oct 2024, 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 a3c564a9596c..02e73ebbfe5c 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -1613,6 +1613,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; Is 5 tries at 1 second interval really sufficient? It is common to double the delay on each retry failure, so delays of 1,2,4,8,16 would give at total of 30 seconds for the client to get over whatever congestion is affecting it. That seems safer. NeilBrown > + } > + } > return 1; > } > > @@ -1742,6 +1749,7 @@ static void nfsd4_send_cb_offload(struct nfsd4_copy *copy) > memcpy(&cbo->co_res, ©->cp_res, sizeof(copy->cp_res)); > memcpy(&cbo->co_fh, ©->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.46.2 > > >
On Tue, Oct 08, 2024 at 05:54:27PM -0400, NeilBrown wrote: > On Wed, 09 Oct 2024, 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 a3c564a9596c..02e73ebbfe5c 100644 > > --- a/fs/nfsd/nfs4proc.c > > +++ b/fs/nfsd/nfs4proc.c > > @@ -1613,6 +1613,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; > > Is 5 tries at 1 second interval really sufficient? It doesn't matter, as long as the client can send an OFFLOAD_STATUS when it hasn't seen the expected CB_OFFLOAD. In fact IMO an even shorter delay would be better. This is not a situation where the service endpoint is waiting for a slow I/O device. The important part of this logic is the retry, not the delay. > It is common to double the delay on each retry failure, so delays of > 1,2,4,8,16 would give at total of 30 seconds for the client to get over > whatever congestion is affecting it. That seems safer. I didn't find other callback operations in NFSD that implemented exponential backoff. I could compromise and do .1 sec, .2 sec, .4 sec, .8 sec, 1.6 sec. > NeilBrown > > > + } > > + } > > return 1; > > } > > > > @@ -1742,6 +1749,7 @@ static void nfsd4_send_cb_offload(struct nfsd4_copy *copy) > > memcpy(&cbo->co_res, ©->cp_res, sizeof(copy->cp_res)); > > memcpy(&cbo->co_fh, ©->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.46.2 > > > > > > >
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index a3c564a9596c..02e73ebbfe5c 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -1613,6 +1613,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; } @@ -1742,6 +1749,7 @@ static void nfsd4_send_cb_offload(struct nfsd4_copy *copy) memcpy(&cbo->co_res, ©->cp_res, sizeof(copy->cp_res)); memcpy(&cbo->co_fh, ©->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; };