| Message ID | 20250122-nfsd-6-14-v1-1-6e1fb49ac545@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Chuck Lever |
| Headers | show |
| Series | nfsd: always handle RPC_SIGNALLED earlier in nfsd4_cb_sequence_done() | expand |
On 1/22/25 10:10 AM, Jeff Layton wrote: > The v4.0 client always restarts the callback when the connection is shut > down (which is indicated by RPC_SIGNALLED()). The RPC is then requeued > and the result eventually should complete (or be aborted). > > The v4.1 code instead processes the result and only later decides to > restart the call. Even more problematic is the fact that it releases the > slot beforehand. The restarted call may get a new slot, which would > could break DRC handling. "break DRC handling" -- I'd like to understand this. NFSD always sets cachethis to false in CB_SEQUENCE, so there is no DRC for these operations. The only thing the client saves is the slot sequence number IIUC. Is retrying an uncached operation via a different slot a problem? > Change nfsd4_cb_sequence_done() such that RPC_SIGNALLED() is handled the > same way irrespective of the minorversion. Check it early, before > processing the CB_SEQUENCE result, and immediately restart the call. It would be lovely to have some test cases that exercise the server's backchannel retry logic. > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4callback.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index 50e468bdb8d4838b5217346dcc2bd0fec1765c1a..05afdf94c6478c7d698b059fa33dd9e7af49b91e 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -1334,21 +1334,24 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > struct nfsd4_session *session = clp->cl_cb_session; > bool ret = true; > > - if (!clp->cl_minorversion) { > - /* > - * If the backchannel connection was shut down while this > - * task was queued, we need to resubmit it after setting up > - * a new backchannel connection. > - * > - * Note that if we lost our callback connection permanently > - * the submission code will error out, so we don't need to > - * handle that case here. > - */ > - if (RPC_SIGNALLED(task)) > - goto need_restart; > + /* > + * If the backchannel connection was shut down while this > + * task was queued, resubmit it after setting up a new backchannel > + * connection. > + * > + * If the backchannel connection is permanently lost, the submission > + * code will error out, so there is no need to handle that case here. > + * > + * For the v4.1+ case, do this without releasing the slot or doing > + * any further processing. It's possible that the restarted call will > + * be a retransmission of an earlier call to the server and that will > + * help ensure that the DRC works correctly. > + */ > + if (RPC_SIGNALLED(task)) > + goto need_restart; > > + if (!clp->cl_minorversion) > return true; > - } > > if (cb->cb_held_slot < 0) > goto need_restart; > @@ -1403,9 +1406,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > } > trace_nfsd_cb_free_slot(task, cb); > nfsd41_cb_release_slot(cb); > - > - if (RPC_SIGNALLED(task)) > - goto need_restart; > out: > return ret; > retry_nowait: > > --- > base-commit: a8acd0de47f22619d62d548b86bcfc9a4de2b2c6 > change-id: 20250122-nfsd-6-14-77c1ad5d9f01 > > Best regards,
On 1/22/2025 10:20 AM, Chuck Lever wrote: > On 1/22/25 10:10 AM, Jeff Layton wrote: >> The v4.0 client always restarts the callback when the connection is shut >> down (which is indicated by RPC_SIGNALLED()). The RPC is then requeued >> and the result eventually should complete (or be aborted). >> >> The v4.1 code instead processes the result and only later decides to >> restart the call. Even more problematic is the fact that it releases the >> slot beforehand. The restarted call may get a new slot, which would >> could break DRC handling. > > "break DRC handling" -- I'd like to understand this. > > NFSD always sets cachethis to false in CB_SEQUENCE, so there is no DRC > for these operations. The only thing the client saves is the slot > sequence number IIUC. > > Is retrying an uncached operation via a different slot a problem? The server would ignore any in-progress status and therefore might cause two requests to be processed in parallel. It might also rearrange the order of replies. Other than those, maybe not! I do think it's safest to not reallocate the slot in the 4.1 case. The slot sequence number is only meaningful in the context of the original slot. Tom. >> Change nfsd4_cb_sequence_done() such that RPC_SIGNALLED() is handled the >> same way irrespective of the minorversion. Check it early, before >> processing the CB_SEQUENCE result, and immediately restart the call. > > It would be lovely to have some test cases that exercise the server's > backchannel retry logic. > > >> Signed-off-by: Jeff Layton <jlayton@kernel.org> >> --- >> fs/nfsd/nfs4callback.c | 32 ++++++++++++++++---------------- >> 1 file changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >> index >> 50e468bdb8d4838b5217346dcc2bd0fec1765c1a..05afdf94c6478c7d698b059fa33dd9e7af49b91e 100644 >> --- a/fs/nfsd/nfs4callback.c >> +++ b/fs/nfsd/nfs4callback.c >> @@ -1334,21 +1334,24 @@ static bool nfsd4_cb_sequence_done(struct >> rpc_task *task, struct nfsd4_callback >> struct nfsd4_session *session = clp->cl_cb_session; >> bool ret = true; >> - if (!clp->cl_minorversion) { >> - /* >> - * If the backchannel connection was shut down while this >> - * task was queued, we need to resubmit it after setting up >> - * a new backchannel connection. >> - * >> - * Note that if we lost our callback connection permanently >> - * the submission code will error out, so we don't need to >> - * handle that case here. >> - */ >> - if (RPC_SIGNALLED(task)) >> - goto need_restart; >> + /* >> + * If the backchannel connection was shut down while this >> + * task was queued, resubmit it after setting up a new backchannel >> + * connection. >> + * >> + * If the backchannel connection is permanently lost, the submission >> + * code will error out, so there is no need to handle that case >> here. >> + * >> + * For the v4.1+ case, do this without releasing the slot or doing >> + * any further processing. It's possible that the restarted call >> will >> + * be a retransmission of an earlier call to the server and that >> will >> + * help ensure that the DRC works correctly. >> + */ >> + if (RPC_SIGNALLED(task)) >> + goto need_restart; >> + if (!clp->cl_minorversion) >> return true; >> - } >> if (cb->cb_held_slot < 0) >> goto need_restart; >> @@ -1403,9 +1406,6 @@ static bool nfsd4_cb_sequence_done(struct >> rpc_task *task, struct nfsd4_callback >> } >> trace_nfsd_cb_free_slot(task, cb); >> nfsd41_cb_release_slot(cb); >> - >> - if (RPC_SIGNALLED(task)) >> - goto need_restart; >> out: >> return ret; >> retry_nowait: >> >> --- >> base-commit: a8acd0de47f22619d62d548b86bcfc9a4de2b2c6 >> change-id: 20250122-nfsd-6-14-77c1ad5d9f01 >> >> Best regards, > >
On Wed, 2025-01-22 at 10:20 -0500, Chuck Lever wrote: > On 1/22/25 10:10 AM, Jeff Layton wrote: > > The v4.0 client always restarts the callback when the connection is shut > > down (which is indicated by RPC_SIGNALLED()). The RPC is then requeued > > and the result eventually should complete (or be aborted). > > > > The v4.1 code instead processes the result and only later decides to > > restart the call. Even more problematic is the fact that it releases the > > slot beforehand. The restarted call may get a new slot, which would > > could break DRC handling. > > "break DRC handling" -- I'd like to understand this. > > NFSD always sets cachethis to false in CB_SEQUENCE, so there is no DRC > for these operations. The only thing the client saves is the slot > sequence number IIUC. > > Is retrying an uncached operation via a different slot a problem? > Ahh, I missed that we always set cachethis to false. So, there is probably now a problem with the DRC after all. Still, I don't see a good argument for processing the CB_SEQUENCE result, when we intend to retransmit the call anyway. > > > Change nfsd4_cb_sequence_done() such that RPC_SIGNALLED() is handled the > > same way irrespective of the minorversion. Check it early, before > > processing the CB_SEQUENCE result, and immediately restart the call. > > It would be lovely to have some test cases that exercise the server's > backchannel retry logic. > It sure would. I think that sort of thing would require a synthetic client (like pynfs). Since we're on the subject too, pynfs is broken on Fedora 41. That moved to python 3.13, which dropped support for the the xdrlib module. There is a forked xdrlib3 module which may be a suitable replacement, but I need to see if I can get it to work. > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs4callback.c | 32 ++++++++++++++++---------------- > > 1 file changed, 16 insertions(+), 16 deletions(-) > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > index 50e468bdb8d4838b5217346dcc2bd0fec1765c1a..05afdf94c6478c7d698b059fa33dd9e7af49b91e 100644 > > --- a/fs/nfsd/nfs4callback.c > > +++ b/fs/nfsd/nfs4callback.c > > @@ -1334,21 +1334,24 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > > struct nfsd4_session *session = clp->cl_cb_session; > > bool ret = true; > > > > - if (!clp->cl_minorversion) { > > - /* > > - * If the backchannel connection was shut down while this > > - * task was queued, we need to resubmit it after setting up > > - * a new backchannel connection. > > - * > > - * Note that if we lost our callback connection permanently > > - * the submission code will error out, so we don't need to > > - * handle that case here. > > - */ > > - if (RPC_SIGNALLED(task)) > > - goto need_restart; > > + /* > > + * If the backchannel connection was shut down while this > > + * task was queued, resubmit it after setting up a new backchannel > > + * connection. > > + * > > + * If the backchannel connection is permanently lost, the submission > > + * code will error out, so there is no need to handle that case here. > > + * > > + * For the v4.1+ case, do this without releasing the slot or doing > > + * any further processing. It's possible that the restarted call will > > + * be a retransmission of an earlier call to the server and that will > > + * help ensure that the DRC works correctly. > > + */ > > + if (RPC_SIGNALLED(task)) > > + goto need_restart; > > > > + if (!clp->cl_minorversion) > > return true; > > - } > > > > if (cb->cb_held_slot < 0) > > goto need_restart; > > @@ -1403,9 +1406,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > > } > > trace_nfsd_cb_free_slot(task, cb); > > nfsd41_cb_release_slot(cb); > > - > > - if (RPC_SIGNALLED(task)) > > - goto need_restart; > > out: > > return ret; > > retry_nowait: > > > > --- > > base-commit: a8acd0de47f22619d62d548b86bcfc9a4de2b2c6 > > change-id: 20250122-nfsd-6-14-77c1ad5d9f01 > > > > Best regards, > >
On 1/22/25 10:44 AM, Jeff Layton wrote: > On Wed, 2025-01-22 at 10:20 -0500, Chuck Lever wrote: >> On 1/22/25 10:10 AM, Jeff Layton wrote: >>> The v4.0 client always restarts the callback when the connection is shut >>> down (which is indicated by RPC_SIGNALLED()). The RPC is then requeued >>> and the result eventually should complete (or be aborted). >>> >>> The v4.1 code instead processes the result and only later decides to >>> restart the call. Even more problematic is the fact that it releases the >>> slot beforehand. The restarted call may get a new slot, which would >>> could break DRC handling. >> >> "break DRC handling" -- I'd like to understand this. >> >> NFSD always sets cachethis to false in CB_SEQUENCE, so there is no DRC >> for these operations. The only thing the client saves is the slot >> sequence number IIUC. >> >> Is retrying an uncached operation via a different slot a problem? >> > > Ahh, I missed that we always set cachethis to false. So, there is > probably now a problem with the DRC after all. Still, I don't see a > good argument for processing the CB_SEQUENCE result, when we intend to > retransmit the call anyway. I expect that the rationale is that the slot sequence number needs to be advanced appropriately before the slot can be used again.
On Wed, 2025-01-22 at 11:06 -0500, Chuck Lever wrote: > On 1/22/25 10:44 AM, Jeff Layton wrote: > > On Wed, 2025-01-22 at 10:20 -0500, Chuck Lever wrote: > > > On 1/22/25 10:10 AM, Jeff Layton wrote: > > > > The v4.0 client always restarts the callback when the connection is shut > > > > down (which is indicated by RPC_SIGNALLED()). The RPC is then requeued > > > > and the result eventually should complete (or be aborted). > > > > > > > > The v4.1 code instead processes the result and only later decides to > > > > restart the call. Even more problematic is the fact that it releases the > > > > slot beforehand. The restarted call may get a new slot, which would > > > > could break DRC handling. > > > > > > "break DRC handling" -- I'd like to understand this. > > > > > > NFSD always sets cachethis to false in CB_SEQUENCE, so there is no DRC > > > for these operations. The only thing the client saves is the slot > > > sequence number IIUC. > > > > > > Is retrying an uncached operation via a different slot a problem? > > > > > > > Ahh, I missed that we always set cachethis to false. So, there is > > probably now a problem with the DRC after all. Still, I don't see a > > good argument for processing the CB_SEQUENCE result, when we intend to > > retransmit the call anyway. > > I expect that the rationale is that the slot sequence number needs to be > advanced appropriately before the slot can be used again. > > Once RPC_SIGNALLED returns true, the callback code can either trust the result of the rpc_task or not. If it's going to trust that result, then there is no need to restart the call. If it's not going to trust it, then the RPC call might as well have not happened, and there is no need to increment the slot sequence number or do anything else. Is my understanding wrong here?
On 1/22/25 11:50 AM, Jeff Layton wrote: > On Wed, 2025-01-22 at 11:06 -0500, Chuck Lever wrote: >> On 1/22/25 10:44 AM, Jeff Layton wrote: >>> On Wed, 2025-01-22 at 10:20 -0500, Chuck Lever wrote: >>>> On 1/22/25 10:10 AM, Jeff Layton wrote: >>>>> The v4.0 client always restarts the callback when the connection is shut >>>>> down (which is indicated by RPC_SIGNALLED()). The RPC is then requeued >>>>> and the result eventually should complete (or be aborted). >>>>> >>>>> The v4.1 code instead processes the result and only later decides to >>>>> restart the call. Even more problematic is the fact that it releases the >>>>> slot beforehand. The restarted call may get a new slot, which would >>>>> could break DRC handling. >>>> >>>> "break DRC handling" -- I'd like to understand this. >>>> >>>> NFSD always sets cachethis to false in CB_SEQUENCE, so there is no DRC >>>> for these operations. The only thing the client saves is the slot >>>> sequence number IIUC. >>>> >>>> Is retrying an uncached operation via a different slot a problem? >>>> >>> >>> Ahh, I missed that we always set cachethis to false. So, there is >>> probably now a problem with the DRC after all. Still, I don't see a >>> good argument for processing the CB_SEQUENCE result, when we intend to >>> retransmit the call anyway. >> >> I expect that the rationale is that the slot sequence number needs to be >> advanced appropriately before the slot can be used again. > > Once RPC_SIGNALLED returns true, the callback code can either trust the > result of the rpc_task or not. If it's going to trust that result, then > there is no need to restart the call. > > If it's not going to trust it, then the RPC call might as well have not > happened, and there is no need to increment the slot sequence number or > do anything else. > > Is my understanding wrong here? It might be. The callback client is careful to initialize cb_seq_status to 1 before it sends the RPC call. Thus if cb_seq_status is any value other than 1 in nfsd4_cb_sequence_done(), that means an RPC reply was received successfully and the XDR decoder was successful. RPC_SIGNALLED doesn't have anything to do with whether a reply arrived or can be trusted. nfsd4_cb_sequence_done() needs to process the reply unconditionally, otherwise the server and client will disagree on the slot sequence number for that slot.
On Thu, 2025-01-23 at 09:37 -0500, Chuck Lever wrote: > On 1/22/25 11:50 AM, Jeff Layton wrote: > > On Wed, 2025-01-22 at 11:06 -0500, Chuck Lever wrote: > > > On 1/22/25 10:44 AM, Jeff Layton wrote: > > > > On Wed, 2025-01-22 at 10:20 -0500, Chuck Lever wrote: > > > > > On 1/22/25 10:10 AM, Jeff Layton wrote: > > > > > > The v4.0 client always restarts the callback when the connection is shut > > > > > > down (which is indicated by RPC_SIGNALLED()). The RPC is then requeued > > > > > > and the result eventually should complete (or be aborted). > > > > > > > > > > > > The v4.1 code instead processes the result and only later decides to > > > > > > restart the call. Even more problematic is the fact that it releases the > > > > > > slot beforehand. The restarted call may get a new slot, which would > > > > > > could break DRC handling. > > > > > > > > > > "break DRC handling" -- I'd like to understand this. > > > > > > > > > > NFSD always sets cachethis to false in CB_SEQUENCE, so there is no DRC > > > > > for these operations. The only thing the client saves is the slot > > > > > sequence number IIUC. > > > > > > > > > > Is retrying an uncached operation via a different slot a problem? > > > > > > > > > > > > > Ahh, I missed that we always set cachethis to false. So, there is > > > > probably now a problem with the DRC after all. Still, I don't see a > > > > good argument for processing the CB_SEQUENCE result, when we intend to > > > > retransmit the call anyway. > > > > > > I expect that the rationale is that the slot sequence number needs to be > > > advanced appropriately before the slot can be used again. > > > > Once RPC_SIGNALLED returns true, the callback code can either trust the > > result of the rpc_task or not. If it's going to trust that result, then > > there is no need to restart the call. > > > > If it's not going to trust it, then the RPC call might as well have not > > happened, and there is no need to increment the slot sequence number or > > do anything else. > > > > Is my understanding wrong here? > > It might be. > > The callback client is careful to initialize cb_seq_status to 1 before > it sends the RPC call. Thus if cb_seq_status is any value other than 1 > in nfsd4_cb_sequence_done(), that means an RPC reply was received > successfully and the XDR decoder was successful. > > RPC_SIGNALLED doesn't have anything to do with whether a reply arrived > or can be trusted. > It may indicate that the reply from the client is no longer applicable. CB_SEQUENCE is for synchronizing the communications on the callback channel. If RPC_SIGNALLED() is true, then that means that the callback client has been torn down and will be recreated, so we probably don't want to be doing anything that might cause that to happen again on the new one. Today, this code both processes the CB_SEQUENCE reply _and_ restarts the call. That seems wrong. It sounds like we really only want to restart the call when the client didn't respond, or the response was malformed (-ESERVERFAULT case). > nfsd4_cb_sequence_done() needs to process the reply unconditionally, > otherwise the server and client will disagree on the slot sequence > number for that slot. > In that case, there is no need to restart the call just because it was RPC_SIGNALLED.
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index 50e468bdb8d4838b5217346dcc2bd0fec1765c1a..05afdf94c6478c7d698b059fa33dd9e7af49b91e 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -1334,21 +1334,24 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback struct nfsd4_session *session = clp->cl_cb_session; bool ret = true; - if (!clp->cl_minorversion) { - /* - * If the backchannel connection was shut down while this - * task was queued, we need to resubmit it after setting up - * a new backchannel connection. - * - * Note that if we lost our callback connection permanently - * the submission code will error out, so we don't need to - * handle that case here. - */ - if (RPC_SIGNALLED(task)) - goto need_restart; + /* + * If the backchannel connection was shut down while this + * task was queued, resubmit it after setting up a new backchannel + * connection. + * + * If the backchannel connection is permanently lost, the submission + * code will error out, so there is no need to handle that case here. + * + * For the v4.1+ case, do this without releasing the slot or doing + * any further processing. It's possible that the restarted call will + * be a retransmission of an earlier call to the server and that will + * help ensure that the DRC works correctly. + */ + if (RPC_SIGNALLED(task)) + goto need_restart; + if (!clp->cl_minorversion) return true; - } if (cb->cb_held_slot < 0) goto need_restart; @@ -1403,9 +1406,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback } trace_nfsd_cb_free_slot(task, cb); nfsd41_cb_release_slot(cb); - - if (RPC_SIGNALLED(task)) - goto need_restart; out: return ret; retry_nowait:
The v4.0 client always restarts the callback when the connection is shut down (which is indicated by RPC_SIGNALLED()). The RPC is then requeued and the result eventually should complete (or be aborted). The v4.1 code instead processes the result and only later decides to restart the call. Even more problematic is the fact that it releases the slot beforehand. The restarted call may get a new slot, which would could break DRC handling. Change nfsd4_cb_sequence_done() such that RPC_SIGNALLED() is handled the same way irrespective of the minorversion. Check it early, before processing the CB_SEQUENCE result, and immediately restart the call. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4callback.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) --- base-commit: a8acd0de47f22619d62d548b86bcfc9a4de2b2c6 change-id: 20250122-nfsd-6-14-77c1ad5d9f01 Best regards,