Re: [PATCH v1 00/36] Clarify module API boundaries

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-12-18 at 15:13 -0500, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> The first nine patches in this series refactor the lockd code base
> to clearly separate its public API from internal implementation
> details.
> 
> The lockd subsystem currently exposes internal implementation headers
> through include/linux/lockd/, creating implicit API contracts that
> complicate maintenance. External consumers such as NFSD and the NFS
> client have developed dependencies on internal structures like struct
> nlm_host, and wire protocol constants leak into high-level module
> interfaces.
> 
> These patches work to establish clean architectural boundaries. The
> public API in include/linux/lockd/ is reduced to bind.h and nlm.h,
> which define the contract between lockd and its consumers. Private
> implementation details including XDR definitions, share management,
> and host structures are relocated to fs/lockd/ where they belong.
> Layering violations are corrected: the NFS client now uses accessor
> helpers instead of dereferencing internal structures, and nlm_fopen()
> returns errno values instead of wire protocol codes.
> 
> These changes enable subsequent work to modernize the NLMv4 XDR
> layer using xdrgen without risk of breaking external consumers.
> This work appears in the remaining patches in this series, which
> are presented here only to provide context for the API adjustments.
> No need to review those closely just yet.
> 
> The series is based on commit 771cfff228fc in the public
> nfsd-testing branch.
> 
> 
> Chuck Lever (36):
>   lockd: Have nlm_fopen() return errno values
>   lockd: Relocate nlmsvc_unlock API declarations
>   NFS: Use nlmclnt_rpc_clnt() helper to retrieve nlm_host's rpc_clnt
>   lockd: Move xdr4.h from include/linux/lockd/ to fs/lockd/
>   lockd: Move share.h from include/linux/lockd/ to fs/lockd/
>   lockd: Relocate include/linux/lockd/lockd.h
>   lockd: Remove lockd/debug.h
>   lockd: Move xdr.h from include/linux/lockd/ to fs/lockd/
>   Documentation: Add the RPC language description of NLM version 4
>   lockd: Use xdrgen XDR functions for the NLMv4 NULL procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 TEST procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 LOCK procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 CANCEL procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 UNLOCK procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 GRANTED procedure
>   lockd: Refactor nlm4svc_callback()
>   lockd: Use xdrgen XDR functions for the NLMv4 TEST_MSG procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 LOCK_MSG procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 CANCEL_MSG procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 UNLOCK_MSG procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 GRANTED_MSG procedure
>   lockd: Convert server-side NLMPROC4_TEST_RES to use xdrgen
>   lockd: Convert server-side NLMPROC4_LOCK_RES to use xdrgen
>   lockd: Convert server-side NLMPROC4_CANCEL_RES to use xdrgen
>   lockd: Convert server-side NLMPROC4_UNLOCK_RES to use xdrgen
>   lockd: Convert server-side NLMPROC4_GRANTED_RES to use xdrgen
>   lockd: Use xdrgen XDR functions for the NLMv4 SM_NOTIFY procedure
>   lockd: Convert server-side undefined procedures to xdrgen
>   lockd: Hoist file_lock init out of nlm4svc_decode_shareargs()
>   lockd: Update share_file helpers
>   lockd: Use xdrgen XDR functions for the NLMv4 SHARE procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 UNSHARE procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 NM_LOCK procedure
>   lockd: Use xdrgen XDR functions for the NLMv4 FREE_ALL procedure
>   lockd: Remove utilities that are no longer used
>   lockd: Remove dead code from fs/lockd/xdr4.c
> 
>  Documentation/sunrpc/xdr/nlm4.x     |  211 ++++
>  fs/lockd/Makefile                   |   21 +-
>  fs/lockd/clnt4xdr.c                 |    5 +-
>  fs/lockd/clntlock.c                 |    2 +-
>  fs/lockd/clntproc.c                 |    2 +-
>  fs/lockd/clntxdr.c                  |    5 +-
>  fs/lockd/host.c                     |    2 +-
>  {include/linux => fs}/lockd/lockd.h |   49 +-
>  fs/lockd/mon.c                      |    2 +-
>  fs/lockd/nlm4xdr_gen.c              |  724 +++++++++++++
>  fs/lockd/nlm4xdr_gen.h              |   32 +
>  {include/linux => fs}/lockd/share.h |   16 +-
>  fs/lockd/svc.c                      |    2 +-
>  fs/lockd/svc4proc.c                 | 1562 +++++++++++++++++----------
>  fs/lockd/svclock.c                  |    5 +-
>  fs/lockd/svcproc.c                  |   13 +-
>  fs/lockd/svcshare.c                 |   40 +-
>  fs/lockd/svcsubs.c                  |   43 +-
>  fs/lockd/trace.h                    |    3 +-
>  fs/lockd/xdr.c                      |    3 +-
>  {include/linux => fs}/lockd/xdr.h   |   12 +-
>  fs/lockd/xdr4.c                     |  347 ------
>  fs/lockd/xdr4.h                     |  104 ++
>  fs/nfs/sysfs.c                      |   10 +-
>  fs/nfsd/lockd.c                     |   51 +-
>  fs/nfsd/nfsctl.c                    |    2 +-
>  include/linux/lockd/bind.h          |   17 +-
>  include/linux/lockd/debug.h         |   40 -
>  include/linux/lockd/xdr4.h          |   43 -
>  include/linux/sunrpc/xdrgen/nlm4.h  |  231 ++++
>  30 files changed, 2513 insertions(+), 1086 deletions(-)
>  create mode 100644 Documentation/sunrpc/xdr/nlm4.x
>  rename {include/linux => fs}/lockd/lockd.h (94%)
>  create mode 100644 fs/lockd/nlm4xdr_gen.c
>  create mode 100644 fs/lockd/nlm4xdr_gen.h
>  rename {include/linux => fs}/lockd/share.h (63%)
>  rename {include/linux => fs}/lockd/xdr.h (94%)
>  delete mode 100644 fs/lockd/xdr4.c
>  create mode 100644 fs/lockd/xdr4.h
>  delete mode 100644 include/linux/lockd/debug.h
>  delete mode 100644 include/linux/lockd/xdr4.h
>  create mode 100644 include/linux/sunrpc/xdrgen/nlm4.h

Big pile! I looked over the more interesting bits and didn't see
anything objectionable. This looks like a nice change that removes a
pile of stuff from include/ and gets lockd working with xdrgen'ed
definitions. You can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>