[PATCH -next v3 0/2] sunrpc: Fix W=1 compiler warnings

[PATCH -next v3 0/2] sunrpc: Fix W=1 compiler warnings

[Geert Uytterhoeven]

	Hi all,

This patch series fixes W=1 compiler warnings in sunrpc, related to
variables that are used only when debugging is enabled.

Changes compared to v2:
  - New patch "sunrpc: Wrap read accesses to rpc_task.tk_pid",
  - Add Acked-by.

Changes compared to v1:
  - s/uncontionally/unconditionally/,
  - Drop CONFIG_SUNRPC_DEBUG check in fs/lockd/svclock.c to fix build
    failure.

Thanks for your comments!

Geert Uytterhoeven (2):
  sunrpc: Wrap read accesses to rpc_task.tk_pid
  sunrpc: Use no_printk() in dfprintk*() dummies

 fs/lockd/svclock.c                     |  2 --
 fs/nfs/filelayout/filelayout.c         | 12 ++++++------
 fs/nfs/flexfilelayout/flexfilelayout.c |  9 +++------
 include/linux/sunrpc/debug.h           |  6 +++---
 include/linux/sunrpc/sched.h           | 10 ++++++++++
 net/sunrpc/clnt.c                      |  2 +-
 net/sunrpc/debugfs.c                   |  2 +-
 7 files changed, 24 insertions(+), 19 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH -next v3 1/2] sunrpc: Wrap read accesses to rpc_task.tk_pid

[Geert Uytterhoeven]

The tk_pid member in the rpc_task structure exists conditionally on
debug or tracing being enabled.

Introduce and use a wapper to read the value of this member, so users
outside tracing no longer have to care about these conditions.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202310121759.0CF34DcN-lkp@intel.com/
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - New.
---
 fs/nfs/filelayout/filelayout.c         | 12 ++++++------
 fs/nfs/flexfilelayout/flexfilelayout.c |  9 +++------
 include/linux/sunrpc/sched.h           | 10 ++++++++++
 net/sunrpc/clnt.c                      |  2 +-
 net/sunrpc/debugfs.c                   |  2 +-
 5 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
index ce8f8934bca517c0..5af545f49c54db4f 100644
--- a/fs/nfs/filelayout/filelayout.c
+++ b/fs/nfs/filelayout/filelayout.c
@@ -93,8 +93,7 @@ static void filelayout_reset_write(struct nfs_pgio_header *hdr)
 	if (!test_and_set_bit(NFS_IOHDR_REDO, &hdr->flags)) {
 		dprintk("%s Reset task %5u for i/o through MDS "
 			"(req %s/%llu, %u bytes @ offset %llu)\n", __func__,
-			hdr->task.tk_pid,
-			hdr->inode->i_sb->s_id,
+			rpc_tk_pid(task), hdr->inode->i_sb->s_id,
 			(unsigned long long)NFS_FILEID(hdr->inode),
 			hdr->args.count,
 			(unsigned long long)hdr->args.offset);
@@ -110,8 +109,7 @@ static void filelayout_reset_read(struct nfs_pgio_header *hdr)
 	if (!test_and_set_bit(NFS_IOHDR_REDO, &hdr->flags)) {
 		dprintk("%s Reset task %5u for i/o through MDS "
 			"(req %s/%llu, %u bytes @ offset %llu)\n", __func__,
-			hdr->task.tk_pid,
-			hdr->inode->i_sb->s_id,
+			rpc_tk_pid(task), hdr->inode->i_sb->s_id,
 			(unsigned long long)NFS_FILEID(hdr->inode),
 			hdr->args.count,
 			(unsigned long long)hdr->args.offset);
@@ -274,7 +272,8 @@ static void filelayout_read_prepare(struct rpc_task *task, void *data)
 		return;
 	}
 	if (filelayout_reset_to_mds(hdr->lseg)) {
-		dprintk("%s task %u reset io to MDS\n", __func__, task->tk_pid);
+		dprintk("%s task %u reset io to MDS\n", __func__,
+			rpc_tk_pid(task));
 		filelayout_reset_read(hdr);
 		rpc_exit(task, 0);
 		return;
@@ -372,7 +371,8 @@ static void filelayout_write_prepare(struct rpc_task *task, void *data)
 		return;
 	}
 	if (filelayout_reset_to_mds(hdr->lseg)) {
-		dprintk("%s task %u reset io to MDS\n", __func__, task->tk_pid);
+		dprintk("%s task %u reset io to MDS\n", __func__,
+			rpc_tk_pid(task));
 		filelayout_reset_write(hdr);
 		rpc_exit(task, 0);
 		return;
diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index a1dc338649062de3..3dd17f675d433f4d 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -1017,8 +1017,7 @@ static void ff_layout_reset_write(struct nfs_pgio_header *hdr, bool retry_pnfs)
 	if (retry_pnfs) {
 		dprintk("%s Reset task %5u for i/o through pNFS "
 			"(req %s/%llu, %u bytes @ offset %llu)\n", __func__,
-			hdr->task.tk_pid,
-			hdr->inode->i_sb->s_id,
+			rpc_tk_pid(task), hdr->inode->i_sb->s_id,
 			(unsigned long long)NFS_FILEID(hdr->inode),
 			hdr->args.count,
 			(unsigned long long)hdr->args.offset);
@@ -1030,8 +1029,7 @@ static void ff_layout_reset_write(struct nfs_pgio_header *hdr, bool retry_pnfs)
 	if (!test_and_set_bit(NFS_IOHDR_REDO, &hdr->flags)) {
 		dprintk("%s Reset task %5u for i/o through MDS "
 			"(req %s/%llu, %u bytes @ offset %llu)\n", __func__,
-			hdr->task.tk_pid,
-			hdr->inode->i_sb->s_id,
+			rpc_tk_pid(task), hdr->inode->i_sb->s_id,
 			(unsigned long long)NFS_FILEID(hdr->inode),
 			hdr->args.count,
 			(unsigned long long)hdr->args.offset);
@@ -1066,8 +1064,7 @@ static void ff_layout_reset_read(struct nfs_pgio_header *hdr)
 	if (!test_and_set_bit(NFS_IOHDR_REDO, &hdr->flags)) {
 		dprintk("%s Reset task %5u for i/o through MDS "
 			"(req %s/%llu, %u bytes @ offset %llu)\n", __func__,
-			hdr->task.tk_pid,
-			hdr->inode->i_sb->s_id,
+			rpc_tk_pid(task), hdr->inode->i_sb->s_id,
 			(unsigned long long)NFS_FILEID(hdr->inode),
 			hdr->args.count,
 			(unsigned long long)hdr->args.offset);
diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 8ada7dc802d30507..0f39e60d28ed0132 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -270,6 +270,11 @@ void		rpc_prepare_task(struct rpc_task *task);
 gfp_t		rpc_task_gfp_mask(void);
 
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG) || IS_ENABLED(CONFIG_TRACEPOINTS)
+static inline unsigned short rpc_tk_pid(const struct rpc_task *task)
+{
+	return task->tk_pid;
+}
+
 static inline const char * rpc_qname(const struct rpc_wait_queue *q)
 {
 	return ((q && q->name) ? q->name : "unknown");
@@ -281,6 +286,11 @@ static inline void rpc_assign_waitqueue_name(struct rpc_wait_queue *q,
 	q->name = name;
 }
 #else
+static inline unsigned short rpc_tk_pid(const struct rpc_task *task)
+{
+	return 0;
+}
+
 static inline void rpc_assign_waitqueue_name(struct rpc_wait_queue *q,
 		const char *name)
 {
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 9c210273d06b7f51..2f37e4143789b0cc 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -3320,7 +3320,7 @@ static void rpc_show_task(const struct rpc_clnt *clnt,
 		rpc_waitq = rpc_qname(task->tk_waitqueue);
 
 	printk(KERN_INFO "%5u %04x %6d %8p %8p %8ld %8p %sv%u %s a:%ps q:%s\n",
-		task->tk_pid, task->tk_flags, task->tk_status,
+		rpc_tk_pid(task), task->tk_flags, task->tk_status,
 		clnt, task->tk_rqstp, rpc_task_timeout(task), task->tk_ops,
 		clnt->cl_program->name, clnt->cl_vers, rpc_proc_name(task),
 		task->tk_action, rpc_waitq);
diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index a176d5a0b0ee9a2c..8896518dd6f3ce0e 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -31,7 +31,7 @@ tasks_show(struct seq_file *f, void *v)
 		xid = be32_to_cpu(task->tk_rqstp->rq_xid);
 
 	seq_printf(f, "%5u %04x %6d 0x%x 0x%x %8ld %ps %sv%u %s a:%ps q:%s\n",
-		task->tk_pid, task->tk_flags, task->tk_status,
+		rpc_tk_pid(task), task->tk_flags, task->tk_status,
 		clnt->cl_clid, xid, rpc_task_timeout(task), task->tk_ops,
 		clnt->cl_program->name, clnt->cl_vers, rpc_proc_name(task),
 		task->tk_action, rpc_waitq);
-- 
2.34.1

Re: [PATCH -next v3 1/2] sunrpc: Wrap read accesses to rpc_task.tk_pid

[Benjamin Coddington]

On 16 Oct 2023, at 9:09, Geert Uytterhoeven wrote:

> The tk_pid member in the rpc_task structure exists conditionally on
> debug or tracing being enabled.
>
> Introduce and use a wapper to read the value of this member, so users
> outside tracing no longer have to care about these conditions.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202310121759.0CF34DcN-lkp@intel.com/
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

I never work on kernels that don't have tk_pid, but I can say its so useful
that for 2 out of the 224 bytes that rpc_task uses (on aarch64), I'd be
inclined to just include it all the time.  That way its around for folks to
reference with realtime tools (like bpftrace, stap).

Does anyone know if there is a good reason not to include it for all
configurations?

Ben

..also:
Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Re: [PATCH -next v3 1/2] sunrpc: Wrap read accesses to rpc_task.tk_pid

[Geert Uytterhoeven]

Hi Ben.

On Mon, Oct 16, 2023 at 11:15 PM Benjamin Coddington
<bcodding@redhat.com> wrote:
> On 16 Oct 2023, at 9:09, Geert Uytterhoeven wrote:
> > The tk_pid member in the rpc_task structure exists conditionally on
> > debug or tracing being enabled.
> >
> > Introduce and use a wapper to read the value of this member, so users
> > outside tracing no longer have to care about these conditions.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202310121759.0CF34DcN-lkp@intel.com/
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I never work on kernels that don't have tk_pid, but I can say its so useful
> that for 2 out of the 224 bytes that rpc_task uses (on aarch64), I'd be
> inclined to just include it all the time.  That way its around for folks to
> reference with realtime tools (like bpftrace, stap).

That would work, too.
In fact always including it should not increase the size of struct rpc_task,
as there's still unused spaced in the gap at the end.

> Does anyone know if there is a good reason not to include it for all
> configurations?
>
> Ben
>
> ..also:
> Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH -next v3 2/2] sunrpc: Use no_printk() in dfprintk*() dummies

[Geert Uytterhoeven]

When building NFS with W=1 and CONFIG_WERROR=y, but
CONFIG_SUNRPC_DEBUG=n:

    fs/nfs/nfs4proc.c: In function ‘nfs4_proc_create_session’:
    fs/nfs/nfs4proc.c:9276:19: error: variable ‘ptr’ set but not used [-Werror=unused-but-set-variable]
     9276 |         unsigned *ptr;
	  |                   ^~~
      CC      fs/nfs/callback.o
    fs/nfs/callback.c: In function ‘nfs41_callback_svc’:
    fs/nfs/callback.c:98:13: error: variable ‘error’ set but not used [-Werror=unused-but-set-variable]
       98 |         int error;
	  |             ^~~~~
      CC      fs/nfs/flexfilelayout/flexfilelayout.o
    fs/nfs/flexfilelayout/flexfilelayout.c: In function ‘ff_layout_io_track_ds_error’:
    fs/nfs/flexfilelayout/flexfilelayout.c:1230:13: error: variable ‘err’ set but not used [-Werror=unused-but-set-variable]
     1230 |         int err;
	  |             ^~~
      CC      fs/nfs/flexfilelayout/flexfilelayoutdev.o
    fs/nfs/flexfilelayout/flexfilelayoutdev.c: In function ‘nfs4_ff_alloc_deviceid_node’:
    fs/nfs/flexfilelayout/flexfilelayoutdev.c:55:16: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]
       55 |         int i, ret = -ENOMEM;
	  |                ^~~

All these are due to variables that are set unconditionally, but are
used only when debugging is enabled.

Fix this by changing the dfprintk*() dummy macros from empty loops to
calls to the no_printk() helper.  This informs the compiler that the
passed debug parameters are actually used, and enables format specifier
checking as a bonus.

This requires removing the protection by CONFIG_SUNRPC_DEBUG of the
declaration of nlmdbg_cookie2a() in fs/lockd/svclock.c, as its reference
is now visible to the compiler, but optimized away.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Jeff Layton <jlayton@kernel.org>
---
v3:
  - Add Acked-by,

v2:
  - s/uncontionally/unconditionally/,
  - Drop CONFIG_SUNRPC_DEBUG check in fs/lockd/svclock.c to fix build
    failure.
---
 fs/lockd/svclock.c           | 2 --
 include/linux/sunrpc/debug.h | 6 +++---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 43aeba9de55cbbc5..b80e0e143c5db16a 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -55,7 +55,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)
 {
 	/*
@@ -82,7 +81,6 @@ static const char *nlmdbg_cookie2a(const struct nlm_cookie *cookie)
 
 	return buf;
 }
-#endif
 
 /*
  * Insert a blocked lock into the global list
diff --git a/include/linux/sunrpc/debug.h b/include/linux/sunrpc/debug.h
index f6aeed07fe04e3d5..76539c6673f2fb15 100644
--- a/include/linux/sunrpc/debug.h
+++ b/include/linux/sunrpc/debug.h
@@ -67,9 +67,9 @@ do {									\
 # define RPC_IFDEBUG(x)		x
 #else
 # define ifdebug(fac)		if (0)
-# define dfprintk(fac, fmt, ...)	do {} while (0)
-# define dfprintk_cont(fac, fmt, ...)	do {} while (0)
-# define dfprintk_rcu(fac, fmt, ...)	do {} while (0)
+# define dfprintk(fac, fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
+# define dfprintk_cont(fac, fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
+# define dfprintk_rcu(fac, fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
 # define RPC_IFDEBUG(x)
 #endif
 
-- 
2.34.1

Re: [PATCH -next v3 2/2] sunrpc: Use no_printk() in dfprintk*() dummies

[Benjamin Coddington]

On 16 Oct 2023, at 9:09, Geert Uytterhoeven wrote:

> When building NFS with W=1 and CONFIG_WERROR=y, but
> CONFIG_SUNRPC_DEBUG=n:
>
>     fs/nfs/nfs4proc.c: In function ‘nfs4_proc_create_session’:
>     fs/nfs/nfs4proc.c:9276:19: error: variable ‘ptr’ set but not used [-Werror=unused-but-set-variable]
>      9276 |         unsigned *ptr;
> 	  |                   ^~~
>       CC      fs/nfs/callback.o
>     fs/nfs/callback.c: In function ‘nfs41_callback_svc’:
>     fs/nfs/callback.c:98:13: error: variable ‘error’ set but not used [-Werror=unused-but-set-variable]
>        98 |         int error;
> 	  |             ^~~~~
>       CC      fs/nfs/flexfilelayout/flexfilelayout.o
>     fs/nfs/flexfilelayout/flexfilelayout.c: In function ‘ff_layout_io_track_ds_error’:
>     fs/nfs/flexfilelayout/flexfilelayout.c:1230:13: error: variable ‘err’ set but not used [-Werror=unused-but-set-variable]
>      1230 |         int err;
> 	  |             ^~~
>       CC      fs/nfs/flexfilelayout/flexfilelayoutdev.o
>     fs/nfs/flexfilelayout/flexfilelayoutdev.c: In function ‘nfs4_ff_alloc_deviceid_node’:
>     fs/nfs/flexfilelayout/flexfilelayoutdev.c:55:16: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]
>        55 |         int i, ret = -ENOMEM;
> 	  |                ^~~
>
> All these are due to variables that are set unconditionally, but are
> used only when debugging is enabled.
>
> Fix this by changing the dfprintk*() dummy macros from empty loops to
> calls to the no_printk() helper.  This informs the compiler that the
> passed debug parameters are actually used, and enables format specifier
> checking as a bonus.
>
> This requires removing the protection by CONFIG_SUNRPC_DEBUG of the
> declaration of nlmdbg_cookie2a() in fs/lockd/svclock.c, as its reference
> is now visible to the compiler, but optimized away.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Jeff Layton <jlayton@kernel.org>

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben

Re: [PATCH -next v3 2/2] sunrpc: Use no_printk() in dfprintk*() dummies

[Geert Uytterhoeven]

On Mon, Oct 16, 2023 at 3:09 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> When building NFS with W=1 and CONFIG_WERROR=y, but
> CONFIG_SUNRPC_DEBUG=n:
>
>     fs/nfs/nfs4proc.c: In function ‘nfs4_proc_create_session’:
>     fs/nfs/nfs4proc.c:9276:19: error: variable ‘ptr’ set but not used [-Werror=unused-but-set-variable]
>      9276 |         unsigned *ptr;
>           |                   ^~~
>       CC      fs/nfs/callback.o
>     fs/nfs/callback.c: In function ‘nfs41_callback_svc’:
>     fs/nfs/callback.c:98:13: error: variable ‘error’ set but not used [-Werror=unused-but-set-variable]
>        98 |         int error;
>           |             ^~~~~
>       CC      fs/nfs/flexfilelayout/flexfilelayout.o
>     fs/nfs/flexfilelayout/flexfilelayout.c: In function ‘ff_layout_io_track_ds_error’:
>     fs/nfs/flexfilelayout/flexfilelayout.c:1230:13: error: variable ‘err’ set but not used [-Werror=unused-but-set-variable]
>      1230 |         int err;
>           |             ^~~
>       CC      fs/nfs/flexfilelayout/flexfilelayoutdev.o
>     fs/nfs/flexfilelayout/flexfilelayoutdev.c: In function ‘nfs4_ff_alloc_deviceid_node’:
>     fs/nfs/flexfilelayout/flexfilelayoutdev.c:55:16: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable]
>        55 |         int i, ret = -ENOMEM;
>           |                ^~~
>
> All these are due to variables that are set unconditionally, but are
> used only when debugging is enabled.
>
> Fix this by changing the dfprintk*() dummy macros from empty loops to
> calls to the no_printk() helper.  This informs the compiler that the
> passed debug parameters are actually used, and enables format specifier
> checking as a bonus.
>
> This requires removing the protection by CONFIG_SUNRPC_DEBUG of the
> declaration of nlmdbg_cookie2a() in fs/lockd/svclock.c, as its reference
> is now visible to the compiler, but optimized away.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Jeff Layton <jlayton@kernel.org>

> --- a/include/linux/sunrpc/debug.h
> +++ b/include/linux/sunrpc/debug.h
> @@ -67,9 +67,9 @@ do {                                                                  \
>  # define RPC_IFDEBUG(x)                x
>  #else
>  # define ifdebug(fac)          if (0)
> -# define dfprintk(fac, fmt, ...)       do {} while (0)
> -# define dfprintk_cont(fac, fmt, ...)  do {} while (0)
> -# define dfprintk_rcu(fac, fmt, ...)   do {} while (0)
> +# define dfprintk(fac, fmt, ...)       no_printk(fmt, ##__VA_ARGS__)
> +# define dfprintk_cont(fac, fmt, ...)  no_printk(fmt, ##__VA_ARGS__)
> +# define dfprintk_rcu(fac, fmt, ...)   no_printk(fmt, ##__VA_ARGS__)
>  # define RPC_IFDEBUG(x)
>  #endif

I discovered a new build issue related to the use of RPC_IFDEBUG()
in fs/nfsd/nfsfh.c. So there will be a v4...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds