[PATCH] NFS: Adjust fs_context error logging

[PATCH] NFS: Adjust fs_context error logging

[Scott Mayhew]

Several existing dprink()/dfprintk() calls were converted to use the new
mount API logging macros by commit ce8866f0913f ("NFS: Attach
supplementary error information to fs_context").  If the fs_context was
not created using fsopen() then it will not have had a log buffer
allocated for it, and the new mount API logging macros will wind up
calling printk().

This can result in syslog messages being logged where previously there
were none... most notably "NFS4: Couldn't follow remote path", which can
happen if the client is auto-negotiating a protocol version with an NFS
server that doesn't support the higher v4.x versions.

Convert the nfs_errorf(), nfs_invalf(), and nfs_warnf() macros to check
for the existence of the fs_context's log buffer and call dprintk() if
it doesn't exist.  Add nfs_ferrorf(), nfs_finvalf(), and nfs_warnf(),
which do the same thing but take an NFS debug flag as an argument and
call dfprintk().  Finally, modify the "NFS4: Couldn't follow remote
path" message to use nfs_ferrorf().

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207385
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 fs/nfs/internal.h  | 26 +++++++++++++++++++++++---
 fs/nfs/nfs4super.c |  4 ++--
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index b840d0a91c9d..6bdee7ab3a6c 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -136,9 +136,29 @@ struct nfs_fs_context {
 	} clone_data;
 };
 
-#define nfs_errorf(fc, fmt, ...) errorf(fc, fmt, ## __VA_ARGS__)
-#define nfs_invalf(fc, fmt, ...) invalf(fc, fmt, ## __VA_ARGS__)
-#define nfs_warnf(fc, fmt, ...) warnf(fc, fmt, ## __VA_ARGS__)
+#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_invalf(fc, fmt, ...) ((fc)->log.log ?		\
+	invalf(fc, fmt, ## __VA_ARGS__) :			\
+	({ dprintk(fmt "\n", ## __VA_ARGS__);  -EINVAL; }))
+
+#define nfs_finvalf(fc, fac, fmt, ...) ((fc)->log.log ?		\
+	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)
 {
diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
index 984cc42ee54d..d09bcfd7db89 100644
--- a/fs/nfs/nfs4super.c
+++ b/fs/nfs/nfs4super.c
@@ -227,7 +227,7 @@ int nfs4_try_get_tree(struct fs_context *fc)
 			   fc, ctx->nfs_server.hostname,
 			   ctx->nfs_server.export_path);
 	if (err) {
-		nfs_errorf(fc, "NFS4: Couldn't follow remote path");
+		nfs_ferrorf(fc, MOUNT, "NFS4: Couldn't follow remote path");
 		dfprintk(MOUNT, "<-- nfs4_try_get_tree() = %d [error]\n", err);
 	} else {
 		dfprintk(MOUNT, "<-- nfs4_try_get_tree() = 0\n");
@@ -250,7 +250,7 @@ int nfs4_get_referral_tree(struct fs_context *fc)
 			    fc, ctx->nfs_server.hostname,
 			    ctx->nfs_server.export_path);
 	if (err) {
-		nfs_errorf(fc, "NFS4: Couldn't follow remote path");
+		nfs_ferrorf(fc, MOUNT, "NFS4: Couldn't follow remote path");
 		dfprintk(MOUNT, "<-- nfs4_get_referral_tree() = %d [error]\n", err);
 	} else {
 		dfprintk(MOUNT, "<-- nfs4_get_referral_tree() = 0\n");
-- 
2.25.4

Re: [PATCH] NFS: Adjust fs_context error logging

[Benjamin Coddington]

On 5 Jan 2021, at 8:54, Scott Mayhew wrote:

> Several existing dprink()/dfprintk() calls were converted to use the new
> mount API logging macros by commit ce8866f0913f ("NFS: Attach
> supplementary error information to fs_context").  If the fs_context was
> not created using fsopen() then it will not have had a log buffer
> allocated for it, and the new mount API logging macros will wind up
> calling printk().
>
> This can result in syslog messages being logged where previously there
> were none... most notably "NFS4: Couldn't follow remote path", which can
> happen if the client is auto-negotiating a protocol version with an NFS
> server that doesn't support the higher v4.x versions.
>
> Convert the nfs_errorf(), nfs_invalf(), and nfs_warnf() macros to check
> for the existence of the fs_context's log buffer and call dprintk() if
> it doesn't exist.  Add nfs_ferrorf(), nfs_finvalf(), and nfs_warnf(),
> which do the same thing but take an NFS debug flag as an argument and
> call dfprintk().  Finally, modify the "NFS4: Couldn't follow remote
> path" message to use nfs_ferrorf().
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207385
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>

I hope someday we can convert all the old debugging to tracepoints.  I know
you considered just removing the debug lines or converting these to a
tracepoint and decided to fix what we have for now.  It does make for a
better stable fix.

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