Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0240d09511336c5a5490607ec29ec49c12eff473.camel@kernel.org>
Subject: Re: [PATCH v2 04/15] NFS: Replace fs_context-related dprintk() call
 sites with tracepoints
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever <chuck.lever@oracle.com>, linux-nfs@vger.kernel.org
Cc:     trondmy@hammerspace.com
Date:   Wed, 06 Jul 2022 14:44:56 -0400
In-Reply-To: <165452705445.1496.2682789415111295436.stgit@oracle-102.nfsv4.dev>
References: <165452664596.1496.16204212908726904739.stgit@oracle-102.nfsv4.dev>
         <165452705445.1496.2682789415111295436.stgit@oracle-102.nfsv4.dev>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-06-06 at 10:50 -0400, Chuck Lever wrote:
> Contributed as part of the long patch series that converts NFS from
> using dprintk to tracepoints for observability.
>=20
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/fs_context.c |   25 ++++++++++-------
>  fs/nfs/nfstrace.h   |   77 +++++++++++++++++++++++++++++++++++++++++++++=
++++++
>  2 files changed, 92 insertions(+), 10 deletions(-)
>=20
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 9a16897e8dc6..35e400a557b9 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -21,6 +21,8 @@
>  #include "nfs.h"
>  #include "internal.h"
> =20
> +#include "nfstrace.h"
> +
>  #define NFSDBG_FACILITY		NFSDBG_MOUNT
> =20
>  #if IS_ENABLED(CONFIG_NFS_V3)
> @@ -284,7 +286,7 @@ static int nfs_verify_server_address(struct sockaddr =
*addr)
>  	}
>  	}
> =20
> -	dfprintk(MOUNT, "NFS: Invalid IP address specified\n");
> +	trace_nfs_mount_addr_err(addr);
>  	return 0;
>  }
> =20
> @@ -378,7 +380,7 @@ static int nfs_parse_security_flavors(struct fs_conte=
xt *fc,
>  	char *string =3D param->string, *p;
>  	int ret;
> =20
> -	dfprintk(MOUNT, "NFS: parsing %s=3D%s option\n", param->key, param->str=
ing);
> +	trace_nfs_mount_assign(param->key, string);
> =20
>  	while ((p =3D strsep(&string, ":")) !=3D NULL) {
>  		if (!*p)
> @@ -480,7 +482,7 @@ static int nfs_fs_context_parse_param(struct fs_conte=
xt *fc,
>  	unsigned int len;
>  	int ret, opt;
> =20
> -	dfprintk(MOUNT, "NFS:   parsing nfs mount option '%s'\n", param->key);
> +	trace_nfs_mount_option(param);
> =20
>  	opt =3D fs_parse(fc, nfs_fs_parameters, param, &result);
>  	if (opt < 0)
> @@ -683,6 +685,7 @@ static int nfs_fs_context_parse_param(struct fs_conte=
xt *fc,
>  			return ret;
>  		break;
>  	case Opt_vers:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		ret =3D nfs_parse_version_string(fc, param->string);
>  		if (ret < 0)
>  			return ret;
> @@ -694,6 +697,7 @@ static int nfs_fs_context_parse_param(struct fs_conte=
xt *fc,
>  		break;
> =20
>  	case Opt_proto:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		protofamily =3D AF_INET;
>  		switch (lookup_constant(nfs_xprt_protocol_tokens, param->string, -1)) =
{
>  		case Opt_xprt_udp6:
> @@ -729,6 +733,7 @@ static int nfs_fs_context_parse_param(struct fs_conte=
xt *fc,
>  		break;
> =20
>  	case Opt_mountproto:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		mountfamily =3D AF_INET;
>  		switch (lookup_constant(nfs_xprt_protocol_tokens, param->string, -1)) =
{
>  		case Opt_xprt_udp6:
> @@ -751,6 +756,7 @@ static int nfs_fs_context_parse_param(struct fs_conte=
xt *fc,
>  		break;
> =20
>  	case Opt_addr:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		len =3D rpc_pton(fc->net_ns, param->string, param->size,
>  			       &ctx->nfs_server.address,
>  			       sizeof(ctx->nfs_server._address));
> @@ -759,16 +765,19 @@ static int nfs_fs_context_parse_param(struct fs_con=
text *fc,
>  		ctx->nfs_server.addrlen =3D len;
>  		break;
>  	case Opt_clientaddr:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		kfree(ctx->client_address);
>  		ctx->client_address =3D param->string;
>  		param->string =3D NULL;
>  		break;
>  	case Opt_mounthost:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		kfree(ctx->mount_server.hostname);
>  		ctx->mount_server.hostname =3D param->string;
>  		param->string =3D NULL;
>  		break;
>  	case Opt_mountaddr:
> +		trace_nfs_mount_assign(param->key, param->string);
>  		len =3D rpc_pton(fc->net_ns, param->string, param->size,
>  			       &ctx->mount_server.address,
>  			       sizeof(ctx->mount_server._address));
> @@ -846,7 +855,6 @@ static int nfs_fs_context_parse_param(struct fs_conte=
xt *fc,
>  		 */
>  	case Opt_sloppy:
>  		ctx->sloppy =3D true;
> -		dfprintk(MOUNT, "NFS:   relaxing parsing rules\n");
>  		break;
>  	}
> =20
> @@ -879,10 +887,8 @@ static int nfs_parse_source(struct fs_context *fc,
>  	size_t len;
>  	const char *end;
> =20
> -	if (unlikely(!dev_name || !*dev_name)) {
> -		dfprintk(MOUNT, "NFS: device name not specified\n");
> +	if (unlikely(!dev_name || !*dev_name))
>  		return -EINVAL;
> -	}
> =20
>  	/* Is the host name protected with square brakcets? */
>  	if (*dev_name =3D=3D '[') {
> @@ -922,7 +928,7 @@ static int nfs_parse_source(struct fs_context *fc,
>  	if (!ctx->nfs_server.export_path)
>  		goto out_nomem;
> =20
> -	dfprintk(MOUNT, "NFS: MNTPATH: '%s'\n", ctx->nfs_server.export_path);
> +	trace_nfs_mount_path(ctx->nfs_server.export_path);
>  	return 0;
> =20
>  out_bad_devname:
> @@ -1116,7 +1122,6 @@ static int nfs23_parse_monolithic(struct fs_context=
 *fc,
>  	return nfs_invalf(fc, "NFS: nfs_mount_data version supports only AUTH_S=
YS");
> =20
>  out_nomem:
> -	dfprintk(MOUNT, "NFS: not enough memory to handle mount options");
>  	return -ENOMEM;
> =20
>  out_no_address:
> @@ -1248,7 +1253,7 @@ static int nfs4_parse_monolithic(struct fs_context =
*fc,
>  	if (IS_ERR(c))
>  		return PTR_ERR(c);
>  	ctx->nfs_server.export_path =3D c;
> -	dfprintk(MOUNT, "NFS: MNTPATH: '%s'\n", c);
> +	trace_nfs_mount_path(c);
> =20
>  	c =3D strndup_user(data->client_addr.data, 16);
>  	if (IS_ERR(c))
> diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
> index 012bd7339862..ccaeae42ee77 100644
> --- a/fs/nfs/nfstrace.h
> +++ b/fs/nfs/nfstrace.h
> @@ -1609,6 +1609,83 @@ TRACE_EVENT(nfs_fh_to_dentry,
>  		)
>  );
> =20
> +TRACE_EVENT(nfs_mount_addr_err,
> +	TP_PROTO(
> +		const struct sockaddr *sap
> +	),
> +
> +	TP_ARGS(sap),
> +
> +	TP_STRUCT__entry(
> +		__array(unsigned char, addr, sizeof(struct sockaddr_in6))
> +	),
> +
> +	TP_fast_assign(
> +		memcpy(__entry->addr, sap, sizeof(__entry->addr));
> +	),
> +
> +	TP_printk("addr=3D%pISpc", __entry->addr)
> +);
> +
> +TRACE_EVENT(nfs_mount_assign,
> +	TP_PROTO(
> +		const char *option,
> +		const char *value
> +	),
> +
> +	TP_ARGS(option, value),
> +
> +	TP_STRUCT__entry(
> +		__string(option, option)
> +		__string(value, value)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(option, option);
> +		__assign_str(value, value);
> +	),
> +
> +	TP_printk("option %s=3D%s",
> +		__get_str(option), __get_str(value)
> +	)
> +);
> +
> +TRACE_EVENT(nfs_mount_option,
> +	TP_PROTO(
> +		const struct fs_parameter *param
> +	),
> +
> +	TP_ARGS(param),
> +
> +	TP_STRUCT__entry(
> +		__string(option, param->key)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(option, param->key);
> +	),
> +
> +	TP_printk("option %s", __get_str(option))
> +);
> +
> +TRACE_EVENT(nfs_mount_path,
> +	TP_PROTO(
> +		const char *path
> +	),
> +
> +	TP_ARGS(path),
> +
> +	TP_STRUCT__entry(
> +		__string(path, path)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(path, path);
> +	),
> +
> +	TP_printk("path=3D'%s'", __get_str(path))
> +);
> +
>  DECLARE_EVENT_CLASS(nfs_xdr_event,
>  		TP_PROTO(
>  			const struct xdr_stream *xdr,
>=20
>=20

Seems like an improvement overall.=20

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