[v4,0/2] nfsd: CB_SEQUENCE error handling fixes and cleanups

Message

Jeff Layton Feb. 7, 2025, 1:45 p.m. UTC

Bruce convinced me that the single-threadedness of the workqueue and the
fact that the RPCs are killed synchronously was enough to ensure that
the session can't disappear out from under a running callback. Given
that, I'm breaking these patches out into a separate series that can
potentially be backported.

I think this rework makes sense, and I've run these against pynfs,
fstests, and nfstest, but I'm not sure how well that stresses the
backchannel error handling. I'd like to put this into linux-next for now
and see if any problems arise.

Signed-off-by: Jeff Layton <[e-mail]>
---
Changes in v4:
- Hold back on session refcounting changes for now and just send CB_SEQUENCE
  error handling rework.
- Link to v3: https://lore.kernel.org/r/[e-mail]

Changes in v3:
- rename cb_session_changed to nfsd4_cb_session_changed
- rename restart_callback to requeue_callback, and rename need_restart:
  label to requeue:
- don't increment seq_nr on -ESERVERFAULT
- comment cleanups
- drop client-side rpc patch (will send separately)
- Link to v2: https://lore.kernel.org/r/[e-mail]

Changes in v2:
- make nfsd4_session be RCU-freed
- change code to keep reference to session over callback RPCs
- rework error handling in nfsd4_cb_sequence_done()
- move NFSv4.0 handling out of nfsd4_cb_sequence_done()
- Link to v1: https://lore.kernel.org/r/[e-mail]

---
Jeff Layton (2):
      nfsd: overhaul CB_SEQUENCE error handling
      nfsd: lift NFSv4.0 handling out of nfsd4_cb_sequence_done()

 fs/nfsd/nfs4callback.c | 107 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 41 deletions(-)
---
base-commit: 50934b1a613cabba2b917879c3e722882b72f628
change-id: 20250123-nfsd-6-14-b0797e385dc0

Best regards,

Comments

J. Bruce Fields Feb. 7, 2025, 1:58 p.m. UTC | #1

On Fri, Feb 07, 2025 at 08:45:02AM -0500, Jeff Layton wrote:
> I think this rework makes sense, and I've run these against pynfs,
> fstests, and nfstest, but I'm not sure how well that stresses the
> backchannel error handling.

Very little to none at all, unfortunately, I'd guess.

pynfs/nfs4.1/server41tests/st_trunking.py had some ideas for some simple
tests, maybe that'd be one starting point.  Or maybe
server41tests/st_delegation.py

--b.

> I'd like to put this into linux-next for now
> and see if any problems arise.
> 
> Signed-off-by: Jeff Layton <[e-mail]>
> ---
> Changes in v4:
> - Hold back on session refcounting changes for now and just send CB_SEQUENCE
>   error handling rework.
> - Link to v3: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v3:
> - rename cb_session_changed to nfsd4_cb_session_changed
> - rename restart_callback to requeue_callback, and rename need_restart:
>   label to requeue:
> - don't increment seq_nr on -ESERVERFAULT
> - comment cleanups
> - drop client-side rpc patch (will send separately)
> - Link to v2: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v2:
> - make nfsd4_session be RCU-freed
> - change code to keep reference to session over callback RPCs
> - rework error handling in nfsd4_cb_sequence_done()
> - move NFSv4.0 handling out of nfsd4_cb_sequence_done()
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> 
> ---
> Jeff Layton (2):
>       nfsd: overhaul CB_SEQUENCE error handling
>       nfsd: lift NFSv4.0 handling out of nfsd4_cb_sequence_done()
> 
>  fs/nfsd/nfs4callback.c | 107 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 66 insertions(+), 41 deletions(-)
> ---
> base-commit: 50934b1a613cabba2b917879c3e722882b72f628
> change-id: 20250123-nfsd-6-14-b0797e385dc0
> 
> Best regards,
> -- 
> Jeff Layton <[e-mail]>

Chuck Lever Feb. 7, 2025, 3:08 p.m. UTC | #2

On 2/7/25 8:45 AM, Jeff Layton wrote:
> Bruce convinced me that the single-threadedness of the workqueue and the
> fact that the RPCs are killed synchronously was enough to ensure that
> the session can't disappear out from under a running callback. Given
> that, I'm breaking these patches out into a separate series that can
> potentially be backported.
> 
> I think this rework makes sense, and I've run these against pynfs,
> fstests, and nfstest, but I'm not sure how well that stresses the
> backchannel error handling. I'd like to put this into linux-next for now
> and see if any problems arise.

I'm good with the proposed code changes. A couple of general comments:

- I plan to put these changes in nfsd-testing first to get some local
  NFS testing experience under our belts before exposing them to the
  broader community of testers.

- nfsd-testing is not automatically merged with linux-next or fs-next.
  These changes would not appear in linux-next until I move them to
  nfsd-next.

- I prefer having the first patch broken into smaller changes to give
  bisection a little more leverage. Eg, the "restart:" and
  "bool = false" changes are fine in a single patch, but change the
  recovery actions for each status code in separate patches, maybe?


> Signed-off-by: Jeff Layton <[e-mail]>
> ---
> Changes in v4:
> - Hold back on session refcounting changes for now and just send CB_SEQUENCE
>   error handling rework.
> - Link to v3: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v3:
> - rename cb_session_changed to nfsd4_cb_session_changed
> - rename restart_callback to requeue_callback, and rename need_restart:
>   label to requeue:
> - don't increment seq_nr on -ESERVERFAULT
> - comment cleanups
> - drop client-side rpc patch (will send separately)
> - Link to v2: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v2:
> - make nfsd4_session be RCU-freed
> - change code to keep reference to session over callback RPCs
> - rework error handling in nfsd4_cb_sequence_done()
> - move NFSv4.0 handling out of nfsd4_cb_sequence_done()
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> 
> ---
> Jeff Layton (2):
>       nfsd: overhaul CB_SEQUENCE error handling
>       nfsd: lift NFSv4.0 handling out of nfsd4_cb_sequence_done()
> 
>  fs/nfsd/nfs4callback.c | 107 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 66 insertions(+), 41 deletions(-)
> ---
> base-commit: 50934b1a613cabba2b917879c3e722882b72f628
> change-id: 20250123-nfsd-6-14-b0797e385dc0
> 
> Best regards,