diff mbox series

[v14,07/25] NFSD: Short-circuit fh_verify tracepoints for LOCALIO

Message ID 20240829010424.83693-8-snitzer@kernel.org (mailing list archive)
State New
Headers show
Series nfs/nfsd: add support for LOCALIO | expand

Commit Message

Mike Snitzer Aug. 29, 2024, 1:04 a.m. UTC 
From: Chuck Lever <chuck.lever@oracle.com>

LOCALIO will be able to call fh_verify() with a NULL rqstp. In this
case, the existing trace points need to be skipped because they
want to dereference the address fields in the passed-in rqstp.

Temporarily make these trace points conditional to avoid a seg
fault in this case. Putting the "rqstp != NULL" check in the trace
points themselves makes the check more efficient.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfsd/trace.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Jeff Layton Aug. 29, 2024, 2:33 p.m. UTC | #1 
On Wed, 2024-08-28 at 21:04 -0400, Mike Snitzer wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> LOCALIO will be able to call fh_verify() with a NULL rqstp. In this
> case, the existing trace points need to be skipped because they
> want to dereference the address fields in the passed-in rqstp.
> 
> Temporarily make these trace points conditional to avoid a seg
> fault in this case. Putting the "rqstp != NULL" check in the trace
> points themselves makes the check more efficient.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> ---
>  fs/nfsd/trace.h | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 77bbd23aa150..d22027e23761 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -193,7 +193,7 @@ TRACE_EVENT(nfsd_compound_encode_err,
>  		{ S_IFIFO,		"FIFO" }, \
>  		{ S_IFSOCK,		"SOCK" })
>  
> -TRACE_EVENT(nfsd_fh_verify,
> +TRACE_EVENT_CONDITION(nfsd_fh_verify,
>  	TP_PROTO(
>  		const struct svc_rqst *rqstp,
>  		const struct svc_fh *fhp,
> @@ -201,6 +201,7 @@ TRACE_EVENT(nfsd_fh_verify,
>  		int access
>  	),
>  	TP_ARGS(rqstp, fhp, type, access),
> +	TP_CONDITION(rqstp != NULL),
>  	TP_STRUCT__entry(
>  		__field(unsigned int, netns_ino)
>  		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
> @@ -239,7 +240,7 @@ TRACE_EVENT_CONDITION(nfsd_fh_verify_err,
>  		__be32 error
>  	),
>  	TP_ARGS(rqstp, fhp, type, access, error),
> -	TP_CONDITION(error),
> +	TP_CONDITION(rqstp != NULL && error),
>  	TP_STRUCT__entry(
>  		__field(unsigned int, netns_ino)
>  		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
> @@ -295,12 +296,13 @@ DECLARE_EVENT_CLASS(nfsd_fh_err_class,
>  		  __entry->status)
>  )
>  
> -#define DEFINE_NFSD_FH_ERR_EVENT(name)		\
> -DEFINE_EVENT(nfsd_fh_err_class, nfsd_##name,	\
> -	TP_PROTO(struct svc_rqst *rqstp,	\
> -		 struct svc_fh	*fhp,		\
> -		 int		status),	\
> -	TP_ARGS(rqstp, fhp, status))
> +#define DEFINE_NFSD_FH_ERR_EVENT(name)			\
> +DEFINE_EVENT_CONDITION(nfsd_fh_err_class, nfsd_##name,	\
> +	TP_PROTO(struct svc_rqst *rqstp,		\
> +		 struct svc_fh	*fhp,			\
> +		 int		status),		\
> +	TP_ARGS(rqstp, fhp, status),			\
> +	TP_CONDITION(rqstp != NULL))
>  
>  DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
>  DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);

A bit ugly. We really only want the rqstp here to get at the socket
structures. I'm still looking at the rest of the set, so I'll assume
that this gets cleaned up later.

Acked-by: Jeff Layton <jlayton@kernel.org>
Chuck Lever III Aug. 29, 2024, 2:35 p.m. UTC | #2 
On Thu, Aug 29, 2024 at 10:33:18AM -0400, Jeff Layton wrote:
> On Wed, 2024-08-28 at 21:04 -0400, Mike Snitzer wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > LOCALIO will be able to call fh_verify() with a NULL rqstp. In this
> > case, the existing trace points need to be skipped because they
> > want to dereference the address fields in the passed-in rqstp.
> > 
> > Temporarily make these trace points conditional to avoid a seg
> > fault in this case. Putting the "rqstp != NULL" check in the trace
> > points themselves makes the check more efficient.
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  fs/nfsd/trace.h | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > index 77bbd23aa150..d22027e23761 100644
> > --- a/fs/nfsd/trace.h
> > +++ b/fs/nfsd/trace.h
> > @@ -193,7 +193,7 @@ TRACE_EVENT(nfsd_compound_encode_err,
> >  		{ S_IFIFO,		"FIFO" }, \
> >  		{ S_IFSOCK,		"SOCK" })
> >  
> > -TRACE_EVENT(nfsd_fh_verify,
> > +TRACE_EVENT_CONDITION(nfsd_fh_verify,
> >  	TP_PROTO(
> >  		const struct svc_rqst *rqstp,
> >  		const struct svc_fh *fhp,
> > @@ -201,6 +201,7 @@ TRACE_EVENT(nfsd_fh_verify,
> >  		int access
> >  	),
> >  	TP_ARGS(rqstp, fhp, type, access),
> > +	TP_CONDITION(rqstp != NULL),
> >  	TP_STRUCT__entry(
> >  		__field(unsigned int, netns_ino)
> >  		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
> > @@ -239,7 +240,7 @@ TRACE_EVENT_CONDITION(nfsd_fh_verify_err,
> >  		__be32 error
> >  	),
> >  	TP_ARGS(rqstp, fhp, type, access, error),
> > -	TP_CONDITION(error),
> > +	TP_CONDITION(rqstp != NULL && error),
> >  	TP_STRUCT__entry(
> >  		__field(unsigned int, netns_ino)
> >  		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
> > @@ -295,12 +296,13 @@ DECLARE_EVENT_CLASS(nfsd_fh_err_class,
> >  		  __entry->status)
> >  )
> >  
> > -#define DEFINE_NFSD_FH_ERR_EVENT(name)		\
> > -DEFINE_EVENT(nfsd_fh_err_class, nfsd_##name,	\
> > -	TP_PROTO(struct svc_rqst *rqstp,	\
> > -		 struct svc_fh	*fhp,		\
> > -		 int		status),	\
> > -	TP_ARGS(rqstp, fhp, status))
> > +#define DEFINE_NFSD_FH_ERR_EVENT(name)			\
> > +DEFINE_EVENT_CONDITION(nfsd_fh_err_class, nfsd_##name,	\
> > +	TP_PROTO(struct svc_rqst *rqstp,		\
> > +		 struct svc_fh	*fhp,			\
> > +		 int		status),		\
> > +	TP_ARGS(rqstp, fhp, status),			\
> > +	TP_CONDITION(rqstp != NULL))
> >  
> >  DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
> >  DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);
> 
> A bit ugly. We really only want the rqstp here to get at the socket
> structures. I'm still looking at the rest of the set, so I'll assume
> that this gets cleaned up later.

No, it doesn't. We don't have a solution for how to trace
LOCALIO activity here yet.
diff mbox series

Patch

diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index 77bbd23aa150..d22027e23761 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -193,7 +193,7 @@  TRACE_EVENT(nfsd_compound_encode_err,
 		{ S_IFIFO,		"FIFO" }, \
 		{ S_IFSOCK,		"SOCK" })
 
-TRACE_EVENT(nfsd_fh_verify,
+TRACE_EVENT_CONDITION(nfsd_fh_verify,
 	TP_PROTO(
 		const struct svc_rqst *rqstp,
 		const struct svc_fh *fhp,
@@ -201,6 +201,7 @@  TRACE_EVENT(nfsd_fh_verify,
 		int access
 	),
 	TP_ARGS(rqstp, fhp, type, access),
+	TP_CONDITION(rqstp != NULL),
 	TP_STRUCT__entry(
 		__field(unsigned int, netns_ino)
 		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
@@ -239,7 +240,7 @@  TRACE_EVENT_CONDITION(nfsd_fh_verify_err,
 		__be32 error
 	),
 	TP_ARGS(rqstp, fhp, type, access, error),
-	TP_CONDITION(error),
+	TP_CONDITION(rqstp != NULL && error),
 	TP_STRUCT__entry(
 		__field(unsigned int, netns_ino)
 		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
@@ -295,12 +296,13 @@  DECLARE_EVENT_CLASS(nfsd_fh_err_class,
 		  __entry->status)
 )
 
-#define DEFINE_NFSD_FH_ERR_EVENT(name)		\
-DEFINE_EVENT(nfsd_fh_err_class, nfsd_##name,	\
-	TP_PROTO(struct svc_rqst *rqstp,	\
-		 struct svc_fh	*fhp,		\
-		 int		status),	\
-	TP_ARGS(rqstp, fhp, status))
+#define DEFINE_NFSD_FH_ERR_EVENT(name)			\
+DEFINE_EVENT_CONDITION(nfsd_fh_err_class, nfsd_##name,	\
+	TP_PROTO(struct svc_rqst *rqstp,		\
+		 struct svc_fh	*fhp,			\
+		 int		status),		\
+	TP_ARGS(rqstp, fhp, status),			\
+	TP_CONDITION(rqstp != NULL))
 
 DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
 DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);