[PATCH v3 0/3] sunrpc/nfs: cleanup redundant debug checks and refactor macros

[PATCH v3 0/3] sunrpc/nfs: cleanup redundant debug checks and refactor macros

[Sean Chang]

This series cleans up redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards 
across sunrpc, nfsd, and lockd, as these checks are already handled 
within the dprintk macros.

Additionally, it refactors the nfs_errorf() macros into a safer 
do-while(0) pattern and removes unused nfs_warnf() macros to improve 
code maintainability.

v3:
- Added nfs_errorf refactoring and removed unused nfs_warnf macros.
- Split sunrpc and nfsd changes for better clarity.

v2:
 - Follow reversed xmas tree order for variables in svc_rdma_transport.c
   as requested by Andy Shevchenko.
 - Polish commit message: use dprintk() and remove redundant file list.
 - Correct the technical claim about dprintk() type checking.

Sean Chang (3):
  nfsd/lockd: Remove redundant debug checks
  svcrdma: remove redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards
  nfs: refactor nfs_errorf macros and remove unused ones

 fs/lockd/svclock.c                       |  7 ------
 fs/nfs/internal.h                        | 28 +++++++++++-------------
 fs/nfsd/nfsfh.c                          |  8 +++----
 net/sunrpc/xprtrdma/svc_rdma_transport.c | 25 ++++++++++-----------
 4 files changed, 27 insertions(+), 41 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] nfsd/lockd: Remove redundant debug checks

[Sean Chang]

Remove unnecessary IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards and #ifdefs
in nfsfh.c and svclock.c.

Verification with .lst files under -O2 confirms that the compiler
successfully performs "dead code elimination". Even when variables
(like char buf[] in nfsfh.c) or static helper functions (like
nlmdbg_cookie2a() in svclock.c) are declared without #ifdef, they are
completely optimized out (no stack allocation, no symbol references in
the final executable) as they are only referenced within no_printk().

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 fs/lockd/svclock.c | 7 -------
 fs/nfsd/nfsfh.c    | 8 +++-----
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index ee23f5802af1..9b978a087b3c 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -47,7 +47,6 @@ static const struct rpc_call_ops nlmsvc_grant_ops;
 static LIST_HEAD(nlm_blocked);
 static DEFINE_SPINLOCK(nlm_blocked_lock);
 
-#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 static const char *nlmdbg_cookie2a(const struct nlm_cookie *cookie)
 {
 	/*
@@ -74,12 +73,6 @@ static const char *nlmdbg_cookie2a(const struct nlm_cookie *cookie)
 
 	return buf;
 }
-#else
-static inline const char *nlmdbg_cookie2a(const struct nlm_cookie *cookie)
-{
-	return "???";
-}
-#endif
 
 /*
  * Insert a blocked lock into the global list
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 68b629fbaaeb..91514326d1b4 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -105,12 +105,10 @@ static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
 {
 	/* Check if the request originated from a secure port. */
 	if (rqstp && !nfsd_originating_port_ok(rqstp, cred, exp)) {
-		if (IS_ENABLED(CONFIG_SUNRPC_DEBUG)) {
-			char buf[RPC_MAX_ADDRBUFLEN];
+		char buf[RPC_MAX_ADDRBUFLEN];
 
-			dprintk("nfsd: request from insecure port %s!\n",
-			        svc_print_addr(rqstp, buf, sizeof(buf)));
-		}
+		dprintk("nfsd: request from insecure port %s!\n",
+			svc_print_addr(rqstp, buf, sizeof(buf)));
 		return nfserr_perm;
 	}
 
-- 
2.34.1

[PATCH v3 2/3] svcrdma: remove redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards

[Sean Chang]

Remove redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards in
svc_rdma_accept(). Since dprintk() already evaluates to a no-op
(via no_printk) when debugging is disabled, these explicit guards
are unnecessary.

Verification with .lst files under -O2 confirms that the compiler
successfully performs "dead code elimination". Even when variables
(like 'sap' in this case) are declared outside of #ifdef, they are
completely optimized out (no stack allocation, no symbol references
in the final executable) as they are only referenced within dprintk().

Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 net/sunrpc/xprtrdma/svc_rdma_transport.c | 25 +++++++++++-------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index f2d72181a6fe..0759444bda50 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -413,6 +413,7 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
 	struct rpcrdma_connect_private pmsg;
 	struct ib_qp_init_attr qp_attr;
 	struct ib_device *dev;
+	struct sockaddr *sap;
 	int ret = 0;
 
 	listen_rdma = container_of(xprt, struct svcxprt_rdma, sc_xprt);
@@ -559,20 +560,16 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
 		goto errout;
 	}
 
-	if (IS_ENABLED(CONFIG_SUNRPC_DEBUG)) {
-		struct sockaddr *sap;
-
-		dprintk("svcrdma: new connection accepted on device %s:\n", dev->name);
-		sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
-		dprintk("    local address   : %pIS:%u\n", sap, rpc_get_port(sap));
-		sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
-		dprintk("    remote address  : %pIS:%u\n", sap, rpc_get_port(sap));
-		dprintk("    max_sge         : %d\n", newxprt->sc_max_send_sges);
-		dprintk("    sq_depth        : %d\n", newxprt->sc_sq_depth);
-		dprintk("    rdma_rw_ctxs    : %d\n", ctxts);
-		dprintk("    max_requests    : %d\n", newxprt->sc_max_requests);
-		dprintk("    ord             : %d\n", conn_param.initiator_depth);
-	}
+	dprintk("svcrdma: new connection accepted on device %s:\n", dev->name);
+	sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr;
+	dprintk("    local address   : %pIS:%u\n", sap, rpc_get_port(sap));
+	sap = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;
+	dprintk("    remote address  : %pIS:%u\n", sap, rpc_get_port(sap));
+	dprintk("    max_sge         : %d\n", newxprt->sc_max_send_sges);
+	dprintk("    sq_depth        : %d\n", newxprt->sc_sq_depth);
+	dprintk("    rdma_rw_ctxs    : %d\n", ctxts);
+	dprintk("    max_requests    : %d\n", newxprt->sc_max_requests);
+	dprintk("    ord             : %d\n", conn_param.initiator_depth);
 
 	return &newxprt->sc_xprt;
 
-- 
2.34.1

[PATCH v3 3/3] nfs: refactor nfs_errorf macros and remove unused ones

[Sean Chang]

refactor nfs_errorf() and nfs_ferrorf() to the standard do-while(0)
pattern for safer macro expansion and kernel style compliance.

additionally, remove nfs_warnf() and nfs_fwarnf() as git grep
confirms they have no callers in the current tree.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202603110038.P6d14oxa-lkp@intel.com/
Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 fs/nfs/internal.h | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 63e09dfc27a8..59ab43542390 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -161,13 +161,19 @@ enum nfs_lock_status {
 	NFS_LOCK_NOLOCK		= 2,
 };
 
-#define nfs_errorf(fc, fmt, ...) ((fc)->log.log ?		\
-	errorf(fc, fmt, ## __VA_ARGS__) :			\
-	({ dprintk(fmt "\n", ## __VA_ARGS__); }))
-
-#define nfs_ferrorf(fc, fac, fmt, ...) ((fc)->log.log ?		\
-	errorf(fc, fmt, ## __VA_ARGS__) :			\
-	({ dfprintk(fac, fmt "\n", ## __VA_ARGS__); }))
+#define nfs_errorf(fc, fmt, ...) do { \
+	if ((fc)->log.log) \
+		errorf(fc, fmt, ## __VA_ARGS__); \
+	else \
+		dprintk(fmt "\n", ## __VA_ARGS__); \
+} while (0)
+
+#define nfs_ferrorf(fc, fac, fmt, ...) do { \
+	if ((fc)->log.log) \
+		errorf(fc, fmt, ## __VA_ARGS__); \
+	else \
+		dfprintk(fac, fmt "\n", ## __VA_ARGS__); \
+} while (0)
 
 #define nfs_invalf(fc, fmt, ...) ((fc)->log.log ?		\
 	invalf(fc, fmt, ## __VA_ARGS__) :			\
@@ -177,14 +183,6 @@ enum nfs_lock_status {
 	invalf(fc, fmt, ## __VA_ARGS__) :			\
 	({ dfprintk(fac, fmt "\n", ## __VA_ARGS__);  -EINVAL; }))
 
-#define nfs_warnf(fc, fmt, ...) ((fc)->log.log ?		\
-	warnf(fc, fmt, ## __VA_ARGS__) :			\
-	({ dprintk(fmt "\n", ## __VA_ARGS__); }))
-
-#define nfs_fwarnf(fc, fac, fmt, ...) ((fc)->log.log ?		\
-	warnf(fc, fmt, ## __VA_ARGS__) :			\
-	({ dfprintk(fac, fmt "\n", ## __VA_ARGS__); }))
-
 static inline struct nfs_fs_context *nfs_fc2context(const struct fs_context *fc)
 {
 	return fc->fs_private;
-- 
2.34.1

Re: [PATCH v3 3/3] nfs: refactor nfs_errorf macros and remove unused ones

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 10:18:46PM +0800, Sean Chang wrote:
> refactor nfs_errorf() and nfs_ferrorf() to the standard do-while(0)
> pattern for safer macro expansion and kernel style compliance.
> 
> additionally, remove nfs_warnf() and nfs_fwarnf() as git grep
> confirms they have no callers in the current tree.

...

>  #define nfs_invalf(fc, fmt, ...) ((fc)->log.log ?		\
>  	invalf(fc, fmt, ## __VA_ARGS__) :			\

>  	invalf(fc, fmt, ## __VA_ARGS__) :			\
>  	({ dfprintk(fac, fmt "\n", ## __VA_ARGS__);  -EINVAL; }))

Why not all of them?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] nfs: refactor nfs_errorf macros and remove unused ones

[Sean Chang]

On Thu, Mar 19, 2026 at 10:41 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Thu, Mar 19, 2026 at 10:18:46PM +0800, Sean Chang wrote:
> > refactor nfs_errorf() and nfs_ferrorf() to the standard do-while(0)
> > pattern for safer macro expansion and kernel style compliance.
> >
> > additionally, remove nfs_warnf() and nfs_fwarnf() as git grep
> > confirms they have no callers in the current tree.
>
> ...
>
> >  #define nfs_invalf(fc, fmt, ...) ((fc)->log.log ?            \
> >       invalf(fc, fmt, ## __VA_ARGS__) :                       \
>
> >       invalf(fc, fmt, ## __VA_ARGS__) :                       \
> >       ({ dfprintk(fac, fmt "\n", ## __VA_ARGS__);  -EINVAL; }))
>
> Why not all of them?
>

I initially only refactored nfs_errorf because it doesn't return a value.
For nfs_invalf, it will always return -EINVAL. Would you prefer me to
refactor it using the ({ ... }) statement expression pattern to keep the
return value, or is it better to leave it as is ?

#define nfs_invalf(fc, fmt, ...) ({            \
    if ((fc)->log.log)                \
        invalf(fc, fmt, ## __VA_ARGS__);    \
    else                        \
        dfprintk(fac, fmt "\n", ## __VA_ARGS__);\
    -EINVAL;                    \
})

Re: [PATCH v3 3/3] nfs: refactor nfs_errorf macros and remove unused ones

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 10:59:02PM +0800, Sean Chang wrote:
> On Thu, Mar 19, 2026 at 10:41 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Thu, Mar 19, 2026 at 10:18:46PM +0800, Sean Chang wrote:

...

> > >  #define nfs_invalf(fc, fmt, ...) ((fc)->log.log ?            \
> > >       invalf(fc, fmt, ## __VA_ARGS__) :                       \
> >
> > >       invalf(fc, fmt, ## __VA_ARGS__) :                       \
> > >       ({ dfprintk(fac, fmt "\n", ## __VA_ARGS__);  -EINVAL; }))
> >
> > Why not all of them?
> 
> I initially only refactored nfs_errorf because it doesn't return a value.
> For nfs_invalf, it will always return -EINVAL. Would you prefer me to
> refactor it using the ({ ... }) statement expression pattern to keep the
> return value, or is it better to leave it as is ?

I don't think in this case it improves the situation. Yeah, it's unfortunate.

> #define nfs_invalf(fc, fmt, ...) ({            \
>     if ((fc)->log.log)                \
>         invalf(fc, fmt, ## __VA_ARGS__);    \

I believe this already has an error code inside, that's why it's only added to
the 'else' branch.

>     else                        \
>         dfprintk(fac, fmt "\n", ## __VA_ARGS__);\
>     -EINVAL;                    \
> })

Okay, let's go with your original approach (ideally these all probably should
be replaced by static inline:s).

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Tested-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Unrelated to the series, but if you want to address these:

nfs/super.c:1170:49: warning: incorrect type in initializer (different address spaces)
nfs/super.c:1170:49:    expected struct rpc_xprt *xprt1
nfs/super.c:1170:49:    got struct rpc_xprt [noderef] __rcu *cl_xprt
nfs/super.c:1171:49: warning: incorrect type in initializer (different address spaces)
nfs/super.c:1171:49:    expected struct rpc_xprt *xprt2
nfs/super.c:1171:49:    got struct rpc_xprt [noderef] __rcu *cl_xprt

nfs/./nfstrace.h:1488:1: warning: dereference of noderef expression
nfs/./nfs4trace.h:2168:1: error: too long token expansion
nfs/./nfs4trace.h:2234:1: error: too long token expansion

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] nfs: refactor nfs_errorf macros and remove unused ones

[Sean Chang]

On Thu, Mar 19, 2026 at 11:49 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> > > >  #define nfs_invalf(fc, fmt, ...) ((fc)->log.log ?            \
> > > >       invalf(fc, fmt, ## __VA_ARGS__) :                       \
> > >
> > > >       invalf(fc, fmt, ## __VA_ARGS__) :                       \
> > > >       ({ dfprintk(fac, fmt "\n", ## __VA_ARGS__);  -EINVAL; }))
> > >
> > > Why not all of them?
> >
> > I initially only refactored nfs_errorf because it doesn't return a value.
> > For nfs_invalf, it will always return -EINVAL. Would you prefer me to
> > refactor it using the ({ ... }) statement expression pattern to keep the
> > return value, or is it better to leave it as is ?
>
> I don't think in this case it improves the situation. Yeah, it's unfortunate.
>
> > #define nfs_invalf(fc, fmt, ...) ({            \
> >     if ((fc)->log.log)                \
> >         invalf(fc, fmt, ## __VA_ARGS__);    \
>
> I believe this already has an error code inside, that's why it's only added to
> the 'else' branch.
>
> >     else                        \
> >         dfprintk(fac, fmt "\n", ## __VA_ARGS__);\
> >     -EINVAL;                    \
> > })
>
> Okay, let's go with your original approach (ideally these all probably should
> be replaced by static inline:s).
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Tested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>

Thanks for your review and for providing the tags!
I'll include your Reviewed-by and Tested-by tags in the v4 submission.

> Unrelated to the series, but if you want to address these:
>
> nfs/super.c:1170:49: warning: incorrect type in initializer (different address spaces)
> nfs/super.c:1170:49:    expected struct rpc_xprt *xprt1
> nfs/super.c:1170:49:    got struct rpc_xprt [noderef] __rcu *cl_xprt
> nfs/super.c:1171:49: warning: incorrect type in initializer (different address spaces)
> nfs/super.c:1171:49:    expected struct rpc_xprt *xprt2
> nfs/super.c:1171:49:    got struct rpc_xprt [noderef] __rcu *cl_xprt
>
> nfs/./nfstrace.h:1488:1: warning: dereference of noderef expression
> nfs/./nfs4trace.h:2168:1: error: too long token expansion
> nfs/./nfs4trace.h:2234:1: error: too long token expansion
>

Regarding the Sparse warnings in super.c and the trace header errors,
I've noted them down. I'll look into them separately and may submit a
follow-up patch to address them later.

Best Regards,
Sean

Re: [PATCH v3 0/3] sunrpc/nfs: cleanup redundant debug checks and refactor macros

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 10:18:43PM +0800, Sean Chang wrote:
> This series cleans up redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards 
> across sunrpc, nfsd, and lockd, as these checks are already handled 
> within the dprintk macros.
> 
> Additionally, it refactors the nfs_errorf() macros into a safer 
> do-while(0) pattern and removes unused nfs_warnf() macros to improve 
> code maintainability.

Shall we also revert the commit ebae102897e7 ("nfsd: Mark variable
__maybe_unused to avoid W=1 build break") as it seems related to
dprintk() issues?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/3] sunrpc/nfs: cleanup redundant debug checks and refactor macros

[Sean Chang]

On Fri, Mar 20, 2026 at 8:08 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Thu, Mar 19, 2026 at 10:18:43PM +0800, Sean Chang wrote:
> > This series cleans up redundant IS_ENABLED(CONFIG_SUNRPC_DEBUG) guards
> > across sunrpc, nfsd, and lockd, as these checks are already handled
> > within the dprintk macros.
> >
> > Additionally, it refactors the nfs_errorf() macros into a safer
> > do-while(0) pattern and removes unused nfs_warnf() macros to improve
> > code maintainability.
>
> Shall we also revert the commit ebae102897e7 ("nfsd: Mark variable
> __maybe_unused to avoid W=1 build break") as it seems related to
> dprintk() issues?
>

That's a great catch. I've verified that with the new dprintk() refactoring,
those __maybe_unused attributes are indeed redundant. I will include a
new patch in v4 to remove them and properly credit the cleanup. Thanks!

Best Regards,
Sean