| Message ID | 20250123-nfsd-6-14-v1-3-c1137a4fa2ae@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | nfsd: CB_SEQUENCE error handling fixes and cleanups | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 1/23/25 3:25 PM, Jeff Layton wrote: > RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY: > > "For any of a number of reasons, the replier could not process this > operation in what was deemed a reasonable time. The client should wait > and then try the request with a new slot and sequence value." A little farther down, Section 15.1.1.3 says this: "If NFS4ERR_DELAY is returned on a SEQUENCE operation, the request is retried in full with the SEQUENCE operation containing the same slot and sequence values." And: "If NFS4ERR_DELAY is returned on an operation other than the first in the request, the request when retried MUST contain a SEQUENCE operation that is different than the original one, with either the slot ID or the sequence value different from that in the original request." My impression is that the slot needs to be held and used again only if the server responded with NFS4ERR_DELAY on the SEQUENCE operation. If the NFS4ERR_DELAY was the status of the 2nd or later operation in the COMPOUND, then yes, a different slot, or the same slot with a bumped sequence number, must be used. The current code in nfsd4_cb_sequence_done() appears to be correct in this regard. > This is CB_SEQUENCE, but I believe the same rule applies. Release the > slot before submitting the delayed RPC. > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4callback.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > goto need_restart; > case -NFS4ERR_DELAY: > cb->cb_seq_status = 1; > + nfsd41_cb_release_slot(cb); > if (!rpc_restart_call(task)) > goto out; > >
On Thu, 2025-01-23 at 17:18 -0500, Chuck Lever wrote: > On 1/23/25 3:25 PM, Jeff Layton wrote: > > RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY: > > > > "For any of a number of reasons, the replier could not process this > > operation in what was deemed a reasonable time. The client should wait > > and then try the request with a new slot and sequence value." > > A little farther down, Section 15.1.1.3 says this: > > "If NFS4ERR_DELAY is returned on a SEQUENCE operation, the request is > retried in full with the SEQUENCE operation containing the same slot > and sequence values." > > And: > > "If NFS4ERR_DELAY is returned on an operation other than the first in > the request, the request when retried MUST contain a SEQUENCE operation > that is different than the original one, with either the slot ID or the > sequence value different from that in the original request." > > My impression is that the slot needs to be held and used again only if > the server responded with NFS4ERR_DELAY on the SEQUENCE operation. If > the NFS4ERR_DELAY was the status of the 2nd or later operation in the > COMPOUND, then yes, a different slot, or the same slot with a bumped > sequence number, must be used. > > The current code in nfsd4_cb_sequence_done() appears to be correct in > this regard. > Ok! I stand corrected. We should be able to just drop this patch, but some of the later patches may need some trivial merge conflicts fixed up. Any idea why SEQUENCE is different in this regard? This rule seems a bit arbitrary. If the response is NFS4ERR_DELAY, then why would it matter which slot you use when retransmitting? The responder is just saying "go away and come back later". What if the responder repeatedly returns NFS4ERR_DELAY (maybe because it's under resource pressure), and also shrinks the slot table in the meantime? It seems like that might put the requestor in an untenable position. Maybe we should lobby to get this changed in the spec? > > > This is CB_SEQUENCE, but I believe the same rule applies. Release the > > slot before submitting the delayed RPC. > > > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs4callback.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > index bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644 > > --- a/fs/nfsd/nfs4callback.c > > +++ b/fs/nfsd/nfs4callback.c > > @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > > goto need_restart; > > case -NFS4ERR_DELAY: > > cb->cb_seq_status = 1; > > + nfsd41_cb_release_slot(cb); > > if (!rpc_restart_call(task)) > > goto out; > > > > > >
On 1/23/2025 6:20 PM, Jeff Layton wrote: > On Thu, 2025-01-23 at 17:18 -0500, Chuck Lever wrote: >> On 1/23/25 3:25 PM, Jeff Layton wrote: >>> RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY: >>> >>> "For any of a number of reasons, the replier could not process this >>> operation in what was deemed a reasonable time. The client should wait >>> and then try the request with a new slot and sequence value." >> >> A little farther down, Section 15.1.1.3 says this: >> >> "If NFS4ERR_DELAY is returned on a SEQUENCE operation, the request is >> retried in full with the SEQUENCE operation containing the same slot >> and sequence values." >> >> And: >> >> "If NFS4ERR_DELAY is returned on an operation other than the first in >> the request, the request when retried MUST contain a SEQUENCE operation >> that is different than the original one, with either the slot ID or the >> sequence value different from that in the original request." >> >> My impression is that the slot needs to be held and used again only if >> the server responded with NFS4ERR_DELAY on the SEQUENCE operation. If >> the NFS4ERR_DELAY was the status of the 2nd or later operation in the >> COMPOUND, then yes, a different slot, or the same slot with a bumped >> sequence number, must be used. >> >> The current code in nfsd4_cb_sequence_done() appears to be correct in >> this regard. >> > > Ok! I stand corrected. We should be able to just drop this patch, but > some of the later patches may need some trivial merge conflicts fixed > up. > > Any idea why SEQUENCE is different in this regard? This rule seems a > bit arbitrary. If the response is NFS4ERR_DELAY, then why would it > matter which slot you use when retransmitting? The responder is just > saying "go away and come back later". SEQUENCE is special because of the minor versioning rules. It is prepended to COMPOUNDs in a way that allows it to be optional in order to comply with the "no new required operations" rule. As such, it's not handled quite the same as other compounded ops, and has some exceptional handling. It's not beautiful, honestly. But it allowed the session to be introduced in v4.1. > What if the responder repeatedly returns NFS4ERR_DELAY (maybe because > it's under resource pressure), and also shrinks the slot table in the > meantime? It seems like that might put the requestor in an untenable > position. > > Maybe we should lobby to get this changed in the spec? If there's a defect, absolutely. There's an IETF errata process. Tom. >> >>> This is CB_SEQUENCE, but I believe the same rule applies. Release the >>> slot before submitting the delayed RPC. >>> >>> Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> fs/nfsd/nfs4callback.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >>> index bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644 >>> --- a/fs/nfsd/nfs4callback.c >>> +++ b/fs/nfsd/nfs4callback.c >>> @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback >>> goto need_restart; >>> case -NFS4ERR_DELAY: >>> cb->cb_seq_status = 1; >>> + nfsd41_cb_release_slot(cb); >>> if (!rpc_restart_call(task)) >>> goto out; >>> >>> >> >> >
On Thu, Jan 23, 2025 at 06:20:08PM -0500, Jeff Layton wrote: > On Thu, 2025-01-23 at 17:18 -0500, Chuck Lever wrote: > > On 1/23/25 3:25 PM, Jeff Layton wrote: > > > RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY: > > > > > > "For any of a number of reasons, the replier could not process this > > > operation in what was deemed a reasonable time. The client should wait > > > and then try the request with a new slot and sequence value." > > > > A little farther down, Section 15.1.1.3 says this: > > > > "If NFS4ERR_DELAY is returned on a SEQUENCE operation, the request is > > retried in full with the SEQUENCE operation containing the same slot > > and sequence values." > > > > And: > > > > "If NFS4ERR_DELAY is returned on an operation other than the first in > > the request, the request when retried MUST contain a SEQUENCE operation > > that is different than the original one, with either the slot ID or the > > sequence value different from that in the original request." > > > > My impression is that the slot needs to be held and used again only if > > the server responded with NFS4ERR_DELAY on the SEQUENCE operation. If > > the NFS4ERR_DELAY was the status of the 2nd or later operation in the > > COMPOUND, then yes, a different slot, or the same slot with a bumped > > sequence number, must be used. > > > > The current code in nfsd4_cb_sequence_done() appears to be correct in > > this regard. > > > > Ok! I stand corrected. We should be able to just drop this patch, but > some of the later patches may need some trivial merge conflicts fixed > up. > > Any idea why SEQUENCE is different in this regard? Isn't DELAY on SEQUENCE an indication that the operation is still in progress? The difference between retrying the same slot or not is whether you're asking the server again for the result of the previous operation, or whether you're asking it to perform a new one. If you get DELAY on a later op and then keep retrying with the same seqid/slot then I'd expect you to get stuck in an infinite loop getting a DELAY response out of the reply cache. --b. > This rule seems a > bit arbitrary. If the response is NFS4ERR_DELAY, then why would it > matter which slot you use when retransmitting? The responder is just > saying "go away and come back later". > > What if the responder repeatedly returns NFS4ERR_DELAY (maybe because > it's under resource pressure), and also shrinks the slot table in the > meantime? It seems like that might put the requestor in an untenable > position. > > Maybe we should lobby to get this changed in the spec? > > > > > > This is CB_SEQUENCE, but I believe the same rule applies. Release the > > > slot before submitting the delayed RPC. > > > > > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > fs/nfsd/nfs4callback.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > > index bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644 > > > --- a/fs/nfsd/nfs4callback.c > > > +++ b/fs/nfsd/nfs4callback.c > > > @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > > > goto need_restart; > > > case -NFS4ERR_DELAY: > > > cb->cb_seq_status = 1; > > > + nfsd41_cb_release_slot(cb); > > > if (!rpc_restart_call(task)) > > > goto out; > > > > > > > > > > > > -- > Jeff Layton <jlayton@kernel.org>
On Fri, 2025-01-24 at 09:00 -0500, J. Bruce Fields wrote: > On Thu, Jan 23, 2025 at 06:20:08PM -0500, Jeff Layton wrote: > > On Thu, 2025-01-23 at 17:18 -0500, Chuck Lever wrote: > > > On 1/23/25 3:25 PM, Jeff Layton wrote: > > > > RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY: > > > > > > > > "For any of a number of reasons, the replier could not process this > > > > operation in what was deemed a reasonable time. The client should wait > > > > and then try the request with a new slot and sequence value." > > > > > > A little farther down, Section 15.1.1.3 says this: > > > > > > "If NFS4ERR_DELAY is returned on a SEQUENCE operation, the request is > > > retried in full with the SEQUENCE operation containing the same slot > > > and sequence values." > > > > > > And: > > > > > > "If NFS4ERR_DELAY is returned on an operation other than the first in > > > the request, the request when retried MUST contain a SEQUENCE operation > > > that is different than the original one, with either the slot ID or the > > > sequence value different from that in the original request." > > > > > > My impression is that the slot needs to be held and used again only if > > > the server responded with NFS4ERR_DELAY on the SEQUENCE operation. If > > > the NFS4ERR_DELAY was the status of the 2nd or later operation in the > > > COMPOUND, then yes, a different slot, or the same slot with a bumped > > > sequence number, must be used. > > > > > > The current code in nfsd4_cb_sequence_done() appears to be correct in > > > this regard. > > > > > > > Ok! I stand corrected. We should be able to just drop this patch, but > > some of the later patches may need some trivial merge conflicts fixed > > up. > > > > Any idea why SEQUENCE is different in this regard? > > Isn't DELAY on SEQUENCE an indication that the operation is still in > progress? The difference between retrying the same slot or not is > whether you're asking the server again for the result of the previous > operation, or whether you're asking it to perform a new one. > > If you get DELAY on a later op and then keep retrying with the same > seqid/slot then I'd expect you to get stuck in an infinite loop getting > a DELAY response out of the reply cache. > Hi Bruce! That may be the case. From RFC8881, section 2.10.6.2: "A retry might be sent while the original request is still in progress on the replier. The replier SHOULD deal with the issue by returning NFS4ERR_DELAY as the reply to SEQUENCE or CB_SEQUENCE operation, but implementations MAY return NFS4ERR_MISORDERED. Since errors from SEQUENCE and CB_SEQUENCE are never recorded in the reply cache, this approach allows the results of the execution of the original request to be properly recorded in the reply cache (assuming that the requester specified the reply to be cached)." > > > > This rule seems a > > bit arbitrary. If the response is NFS4ERR_DELAY, then why would it > > matter which slot you use when retransmitting? The responder is just > > saying "go away and come back later". > > > > What if the responder repeatedly returns NFS4ERR_DELAY (maybe because > > it's under resource pressure), and also shrinks the slot table in the > > meantime? It seems like that might put the requestor in an untenable > > position. > > > > Maybe we should lobby to get this changed in the spec? > > > > > > > > > This is CB_SEQUENCE, but I believe the same rule applies. Release the > > > > slot before submitting the delayed RPC. > > > > > > > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > fs/nfsd/nfs4callback.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > > > index bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644 > > > > --- a/fs/nfsd/nfs4callback.c > > > > +++ b/fs/nfsd/nfs4callback.c > > > > @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > > > > goto need_restart; > > > > case -NFS4ERR_DELAY: > > > > cb->cb_seq_status = 1; > > > > + nfsd41_cb_release_slot(cb); > > > > if (!rpc_restart_call(task)) > > > > goto out; > > > > > > > > > > > > > > > > > > -- > > Jeff Layton <jlayton@kernel.org>
On Fri, Jan 24, 2025 at 9:08 AM J. Bruce Fields <bfields@fieldses.org> wrote: > > On Thu, Jan 23, 2025 at 06:20:08PM -0500, Jeff Layton wrote: > > On Thu, 2025-01-23 at 17:18 -0500, Chuck Lever wrote: > > > On 1/23/25 3:25 PM, Jeff Layton wrote: > > > > RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY: > > > > > > > > "For any of a number of reasons, the replier could not process this > > > > operation in what was deemed a reasonable time. The client should wait > > > > and then try the request with a new slot and sequence value." > > > > > > A little farther down, Section 15.1.1.3 says this: > > > > > > "If NFS4ERR_DELAY is returned on a SEQUENCE operation, the request is > > > retried in full with the SEQUENCE operation containing the same slot > > > and sequence values." > > > > > > And: > > > > > > "If NFS4ERR_DELAY is returned on an operation other than the first in > > > the request, the request when retried MUST contain a SEQUENCE operation > > > that is different than the original one, with either the slot ID or the > > > sequence value different from that in the original request." > > > > > > My impression is that the slot needs to be held and used again only if > > > the server responded with NFS4ERR_DELAY on the SEQUENCE operation. If > > > the NFS4ERR_DELAY was the status of the 2nd or later operation in the > > > COMPOUND, then yes, a different slot, or the same slot with a bumped > > > sequence number, must be used. > > > > > > The current code in nfsd4_cb_sequence_done() appears to be correct in > > > this regard. > > > > > > > Ok! I stand corrected. We should be able to just drop this patch, but > > some of the later patches may need some trivial merge conflicts fixed > > up. > > > > Any idea why SEQUENCE is different in this regard? > > Isn't DELAY on SEQUENCE an indication that the operation is still in > progress? The difference between retrying the same slot or not is > whether you're asking the server again for the result of the previous > operation, or whether you're asking it to perform a new one. > > If you get DELAY on a later op and then keep retrying with the same > seqid/slot then I'd expect you to get stuck in an infinite loop getting > a DELAY response out of the reply cache. If the client would keep re-using the same seqid for the operation it got a DELAY on then it would be a broken client. When the linux client gets ERR_DELAY, it retries the operation but it increments the seqid. > > --b. > > > > This rule seems a > > bit arbitrary. If the response is NFS4ERR_DELAY, then why would it > > matter which slot you use when retransmitting? The responder is just > > saying "go away and come back later". > > > > What if the responder repeatedly returns NFS4ERR_DELAY (maybe because > > it's under resource pressure), and also shrinks the slot table in the > > meantime? It seems like that might put the requestor in an untenable > > position. > > > > Maybe we should lobby to get this changed in the spec? > > > > > > > > > This is CB_SEQUENCE, but I believe the same rule applies. Release the > > > > slot before submitting the delayed RPC. > > > > > > > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > fs/nfsd/nfs4callback.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > > > index bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644 > > > > --- a/fs/nfsd/nfs4callback.c > > > > +++ b/fs/nfsd/nfs4callback.c > > > > @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > > > > goto need_restart; > > > > case -NFS4ERR_DELAY: > > > > cb->cb_seq_status = 1; > > > > + nfsd41_cb_release_slot(cb); > > > > if (!rpc_restart_call(task)) > > > > goto out; > > > > > > > > > > > > > > > > > > -- > > Jeff Layton <jlayton@kernel.org> >
On Fri, Jan 24, 2025 at 12:45 PM Olga Kornievskaia <okorniev@redhat.com> wrote: > > On Fri, Jan 24, 2025 at 9:08 AM J. Bruce Fields <bfields@fieldses.org> wrote: > > > > On Thu, Jan 23, 2025 at 06:20:08PM -0500, Jeff Layton wrote: > > > On Thu, 2025-01-23 at 17:18 -0500, Chuck Lever wrote: > > > > On 1/23/25 3:25 PM, Jeff Layton wrote: > > > > > RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY: > > > > > > > > > > "For any of a number of reasons, the replier could not process this > > > > > operation in what was deemed a reasonable time. The client should wait > > > > > and then try the request with a new slot and sequence value." > > > > > > > > A little farther down, Section 15.1.1.3 says this: > > > > > > > > "If NFS4ERR_DELAY is returned on a SEQUENCE operation, the request is > > > > retried in full with the SEQUENCE operation containing the same slot > > > > and sequence values." > > > > > > > > And: > > > > > > > > "If NFS4ERR_DELAY is returned on an operation other than the first in > > > > the request, the request when retried MUST contain a SEQUENCE operation > > > > that is different than the original one, with either the slot ID or the > > > > sequence value different from that in the original request." > > > > > > > > My impression is that the slot needs to be held and used again only if > > > > the server responded with NFS4ERR_DELAY on the SEQUENCE operation. If > > > > the NFS4ERR_DELAY was the status of the 2nd or later operation in the > > > > COMPOUND, then yes, a different slot, or the same slot with a bumped > > > > sequence number, must be used. > > > > > > > > The current code in nfsd4_cb_sequence_done() appears to be correct in > > > > this regard. > > > > > > > > > > Ok! I stand corrected. We should be able to just drop this patch, but > > > some of the later patches may need some trivial merge conflicts fixed > > > up. > > > > > > Any idea why SEQUENCE is different in this regard? > > > > Isn't DELAY on SEQUENCE an indication that the operation is still in > > progress? The difference between retrying the same slot or not is > > whether you're asking the server again for the result of the previous > > operation, or whether you're asking it to perform a new one. > > > > If you get DELAY on a later op and then keep retrying with the same > > seqid/slot then I'd expect you to get stuck in an infinite loop getting > > a DELAY response out of the reply cache. > > If the client would keep re-using the same seqid for the operation it > got a DELAY on then it would be a broken client. When the linux client > gets ERR_DELAY, it retries the operation but it increments the seqid. Urgh I stand corrected this is on the SEQUENCE not an operation. > > > > > --b. > > > > > > > This rule seems a > > > bit arbitrary. If the response is NFS4ERR_DELAY, then why would it > > > matter which slot you use when retransmitting? The responder is just > > > saying "go away and come back later". > > > > > > What if the responder repeatedly returns NFS4ERR_DELAY (maybe because > > > it's under resource pressure), and also shrinks the slot table in the > > > meantime? It seems like that might put the requestor in an untenable > > > position. > > > > > > Maybe we should lobby to get this changed in the spec? > > > > > > > > > > > > This is CB_SEQUENCE, but I believe the same rule applies. Release the > > > > > slot before submitting the delayed RPC. > > > > > > > > > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > --- > > > > > fs/nfsd/nfs4callback.c | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > > > > index bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644 > > > > > --- a/fs/nfsd/nfs4callback.c > > > > > +++ b/fs/nfsd/nfs4callback.c > > > > > @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > > > > > goto need_restart; > > > > > case -NFS4ERR_DELAY: > > > > > cb->cb_seq_status = 1; > > > > > + nfsd41_cb_release_slot(cb); > > > > > if (!rpc_restart_call(task)) > > > > > goto out; > > > > > > > > > > > > > > > > > > > > > > > > -- > > > Jeff Layton <jlayton@kernel.org> > >
On 1/24/2025 9:11 AM, Jeff Layton wrote: > On Fri, 2025-01-24 at 09:00 -0500, J. Bruce Fields wrote: >> On Thu, Jan 23, 2025 at 06:20:08PM -0500, Jeff Layton wrote: >>> On Thu, 2025-01-23 at 17:18 -0500, Chuck Lever wrote: >>>> On 1/23/25 3:25 PM, Jeff Layton wrote: >>>>> RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY: >>>>> >>>>> "For any of a number of reasons, the replier could not process this >>>>> operation in what was deemed a reasonable time. The client should wait >>>>> and then try the request with a new slot and sequence value." >>>> >>>> A little farther down, Section 15.1.1.3 says this: >>>> >>>> "If NFS4ERR_DELAY is returned on a SEQUENCE operation, the request is >>>> retried in full with the SEQUENCE operation containing the same slot >>>> and sequence values." >>>> >>>> And: >>>> >>>> "If NFS4ERR_DELAY is returned on an operation other than the first in >>>> the request, the request when retried MUST contain a SEQUENCE operation >>>> that is different than the original one, with either the slot ID or the >>>> sequence value different from that in the original request." >>>> >>>> My impression is that the slot needs to be held and used again only if >>>> the server responded with NFS4ERR_DELAY on the SEQUENCE operation. If >>>> the NFS4ERR_DELAY was the status of the 2nd or later operation in the >>>> COMPOUND, then yes, a different slot, or the same slot with a bumped >>>> sequence number, must be used. >>>> >>>> The current code in nfsd4_cb_sequence_done() appears to be correct in >>>> this regard. >>>> >>> >>> Ok! I stand corrected. We should be able to just drop this patch, but >>> some of the later patches may need some trivial merge conflicts fixed >>> up. >>> >>> Any idea why SEQUENCE is different in this regard? >> >> Isn't DELAY on SEQUENCE an indication that the operation is still in >> progress? The difference between retrying the same slot or not is >> whether you're asking the server again for the result of the previous >> operation, or whether you're asking it to perform a new one. >> >> If you get DELAY on a later op and then keep retrying with the same >> seqid/slot then I'd expect you to get stuck in an infinite loop getting >> a DELAY response out of the reply cache. >> > > Hi Bruce! > > That may be the case. From RFC8881, section 2.10.6.2: > > "A retry might be sent while the original request is still in progress > on the replier. The replier SHOULD deal with the issue by returning > NFS4ERR_DELAY as the reply to SEQUENCE or CB_SEQUENCE operation, but > implementations MAY return NFS4ERR_MISORDERED. Since errors from > SEQUENCE and CB_SEQUENCE are never recorded in the reply cache, this > approach allows the results of the execution of the original request to > be properly recorded in the reply cache (assuming that the requester > specified the reply to be cached)." It's true, but note that the NFS4ERR_DELAY response is a SHOULD, so it's not the complete picture. It seems rude to return MISORDERED as suggested, but it's just as possible that the server may not respond at all, and simply wait for the in-progress operation to complete. We certainly can't count on all servers to do the same thing. Tom. >>> This rule seems a >>> bit arbitrary. If the response is NFS4ERR_DELAY, then why would it >>> matter which slot you use when retransmitting? The responder is just >>> saying "go away and come back later". >>> >>> What if the responder repeatedly returns NFS4ERR_DELAY (maybe because >>> it's under resource pressure), and also shrinks the slot table in the >>> meantime? It seems like that might put the requestor in an untenable >>> position. >>> >>> Maybe we should lobby to get this changed in the spec? >>> >>>> >>>>> This is CB_SEQUENCE, but I believe the same rule applies. Release the >>>>> slot before submitting the delayed RPC. >>>>> >>>>> Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") >>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>>>> --- >>>>> fs/nfsd/nfs4callback.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >>>>> index bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644 >>>>> --- a/fs/nfsd/nfs4callback.c >>>>> +++ b/fs/nfsd/nfs4callback.c >>>>> @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback >>>>> goto need_restart; >>>>> case -NFS4ERR_DELAY: >>>>> cb->cb_seq_status = 1; >>>>> + nfsd41_cb_release_slot(cb); >>>>> if (!rpc_restart_call(task)) >>>>> goto out; >>>>> >>>>> >>>> >>>> >>> >>> -- >>> Jeff Layton <jlayton@kernel.org> >
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback goto need_restart; case -NFS4ERR_DELAY: cb->cb_seq_status = 1; + nfsd41_cb_release_slot(cb); if (!rpc_restart_call(task)) goto out;
RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY: "For any of a number of reasons, the replier could not process this operation in what was deemed a reasonable time. The client should wait and then try the request with a new slot and sequence value." This is CB_SEQUENCE, but I believe the same rule applies. Release the slot before submitting the delayed RPC. Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4callback.c | 1 + 1 file changed, 1 insertion(+)