diff mbox series

[2/8] nfsd: Add tracing to nfsd_set_fh_dentry()

Message ID 20200301232145.1465430-3-trond.myklebust@hammerspace.com (mailing list archive)
State New, archived
Headers show
Series Bugfixes and tracing enhancements for knfsd | expand

Commit Message

Trond Myklebust March 1, 2020, 11:21 p.m. UTC 
Add tracing to allow us to figure out where any stale filehandle issues
may be originating from.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfsd/nfsfh.c | 13 ++++++++++---
 fs/nfsd/trace.h | 30 ++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 3 deletions(-)

Comments

Chuck Lever March 3, 2020, 12:22 a.m. UTC | #1 
> On Mar 1, 2020, at 6:21 PM, Trond Myklebust <trondmy@gmail.com> wrote:
> 
> Add tracing to allow us to figure out where any stale filehandle issues
> may be originating from.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
> fs/nfsd/nfsfh.c | 13 ++++++++++---
> fs/nfsd/trace.h | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index b319080288c3..37bc8f5f4514 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -14,6 +14,7 @@
> #include "nfsd.h"
> #include "vfs.h"
> #include "auth.h"
> +#include "trace.h"
> 
> #define NFSDDBG_FACILITY		NFSDDBG_FH
> 
> @@ -209,11 +210,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
> 	}
> 
> 	error = nfserr_stale;
> -	if (PTR_ERR(exp) == -ENOENT)
> -		return error;
> +	if (IS_ERR(exp)) {
> +		trace_nfsd_set_fh_dentry_badexport(rqstp, fhp, PTR_ERR(exp));
> +
> +		if (PTR_ERR(exp) == -ENOENT)
> +			return error;
> 
> -	if (IS_ERR(exp))
> 		return nfserrno(PTR_ERR(exp));
> +	}
> 
> 	if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) {
> 		/* Elevate privileges so that the lack of 'r' or 'x'
> @@ -267,6 +271,9 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
> 		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
> 				data_left, fileid_type,
> 				nfsd_acceptable, exp);
> +		if (IS_ERR_OR_NULL(dentry))
> +			trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp,
> +					dentry ?  PTR_ERR(dentry) : -ESTALE);

If you'll be respinning this series, a handful of nits:

- the line above has a double space
- the trace point names here are a little long, will result in
  hard-to-read formatting in the trace log
- checkpatch.pl complains about a couple of the later patches,
  where one arm of an "if" statement has braces but the other
  does not


> 	}
> 	if (dentry == NULL)
> 		goto out;
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 06dd0d337049..9abd1591a841 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -50,6 +50,36 @@ TRACE_EVENT(nfsd_compound_status,
> 		__get_str(name), __entry->status)
> )
> 
> +DECLARE_EVENT_CLASS(nfsd_fh_err_class,
> +	TP_PROTO(struct svc_rqst *rqstp,
> +		 struct svc_fh	*fhp,
> +		 int		status),
> +	TP_ARGS(rqstp, fhp, status),
> +	TP_STRUCT__entry(
> +		__field(u32, xid)
> +		__field(u32, fh_hash)
> +		__field(int, status)
> +	),
> +	TP_fast_assign(
> +		__entry->xid = be32_to_cpu(rqstp->rq_xid);
> +		__entry->fh_hash = knfsd_fh_hash(&fhp->fh_handle);
> +		__entry->status = status;
> +	),
> +	TP_printk("xid=0x%08x fh_hash=0x%08x status=%d",
> +		  __entry->xid, __entry->fh_hash,
> +		  __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_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
> +DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);
> +
> DECLARE_EVENT_CLASS(nfsd_io_class,
> 	TP_PROTO(struct svc_rqst *rqstp,
> 		 struct svc_fh	*fhp,
> -- 
> 2.24.1
> 

--
Chuck Lever
chucklever@gmail.com
Trond Myklebust March 3, 2020, 5:59 a.m. UTC | #2 
On Mon, 2020-03-02 at 19:22 -0500, Chuck Lever wrote:
> > On Mar 1, 2020, at 6:21 PM, Trond Myklebust <trondmy@gmail.com>
> > wrote:
> > 
> > Add tracing to allow us to figure out where any stale filehandle
> > issues
> > may be originating from.
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> > fs/nfsd/nfsfh.c | 13 ++++++++++---
> > fs/nfsd/trace.h | 30 ++++++++++++++++++++++++++++++
> > 2 files changed, 40 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index b319080288c3..37bc8f5f4514 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -14,6 +14,7 @@
> > #include "nfsd.h"
> > #include "vfs.h"
> > #include "auth.h"
> > +#include "trace.h"
> > 
> > #define NFSDDBG_FACILITY		NFSDDBG_FH
> > 
> > @@ -209,11 +210,14 @@ static __be32 nfsd_set_fh_dentry(struct
> > svc_rqst *rqstp, struct svc_fh *fhp)
> > 	}
> > 
> > 	error = nfserr_stale;
> > -	if (PTR_ERR(exp) == -ENOENT)
> > -		return error;
> > +	if (IS_ERR(exp)) {
> > +		trace_nfsd_set_fh_dentry_badexport(rqstp, fhp,
> > PTR_ERR(exp));
> > +
> > +		if (PTR_ERR(exp) == -ENOENT)
> > +			return error;
> > 
> > -	if (IS_ERR(exp))
> > 		return nfserrno(PTR_ERR(exp));
> > +	}
> > 
> > 	if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) {
> > 		/* Elevate privileges so that the lack of 'r' or 'x'
> > @@ -267,6 +271,9 @@ static __be32 nfsd_set_fh_dentry(struct
> > svc_rqst *rqstp, struct svc_fh *fhp)
> > 		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
> > 				data_left, fileid_type,
> > 				nfsd_acceptable, exp);
> > +		if (IS_ERR_OR_NULL(dentry))
> > +			trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp,
> > +					dentry ?  PTR_ERR(dentry) :
> > -ESTALE);
> 
> If you'll be respinning this series, a handful of nits:
> 

I see no need to respin the entire series. Just the last patch.

> - the line above has a double space
> - the trace point names here are a little long, will result in
>   hard-to-read formatting in the trace log
> - checkpatch.pl complains about a couple of the later patches,
>   where one arm of an "if" statement has braces but the other
>   does not
> 
> > 	}
> > 	if (dentry == NULL)
> > 		goto out;
> > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > index 06dd0d337049..9abd1591a841 100644
> > --- a/fs/nfsd/trace.h
> > +++ b/fs/nfsd/trace.h
> > @@ -50,6 +50,36 @@ TRACE_EVENT(nfsd_compound_status,
> > 		__get_str(name), __entry->status)
> > )
> > 
> > +DECLARE_EVENT_CLASS(nfsd_fh_err_class,
> > +	TP_PROTO(struct svc_rqst *rqstp,
> > +		 struct svc_fh	*fhp,
> > +		 int		status),
> > +	TP_ARGS(rqstp, fhp, status),
> > +	TP_STRUCT__entry(
> > +		__field(u32, xid)
> > +		__field(u32, fh_hash)
> > +		__field(int, status)
> > +	),
> > +	TP_fast_assign(
> > +		__entry->xid = be32_to_cpu(rqstp->rq_xid);
> > +		__entry->fh_hash = knfsd_fh_hash(&fhp->fh_handle);
> > +		__entry->status = status;
> > +	),
> > +	TP_printk("xid=0x%08x fh_hash=0x%08x status=%d",
> > +		  __entry->xid, __entry->fh_hash,
> > +		  __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_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
> > +DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);
> > +
> > DECLARE_EVENT_CLASS(nfsd_io_class,
> > 	TP_PROTO(struct svc_rqst *rqstp,
> > 		 struct svc_fh	*fhp,
> > -- 
> > 2.24.1
> > 
> 
> --
> Chuck Lever
> chucklever@gmail.com
> 
> 
>
Chuck Lever March 3, 2020, 1:27 p.m. UTC | #3 
> On Mar 3, 2020, at 12:59 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Mon, 2020-03-02 at 19:22 -0500, Chuck Lever wrote:
>>> On Mar 1, 2020, at 6:21 PM, Trond Myklebust <trondmy@gmail.com>
>>> wrote:
>>> 
>>> Add tracing to allow us to figure out where any stale filehandle
>>> issues
>>> may be originating from.
>>> 
>>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> ---
>>> fs/nfsd/nfsfh.c | 13 ++++++++++---
>>> fs/nfsd/trace.h | 30 ++++++++++++++++++++++++++++++
>>> 2 files changed, 40 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
>>> index b319080288c3..37bc8f5f4514 100644
>>> --- a/fs/nfsd/nfsfh.c
>>> +++ b/fs/nfsd/nfsfh.c
>>> @@ -14,6 +14,7 @@
>>> #include "nfsd.h"
>>> #include "vfs.h"
>>> #include "auth.h"
>>> +#include "trace.h"
>>> 
>>> #define NFSDDBG_FACILITY        NFSDDBG_FH
>>> 
>>> @@ -209,11 +210,14 @@ static __be32 nfsd_set_fh_dentry(struct
>>> svc_rqst *rqstp, struct svc_fh *fhp)
>>>    }
>>> 
>>>    error = nfserr_stale;
>>> -    if (PTR_ERR(exp) == -ENOENT)
>>> -        return error;
>>> +    if (IS_ERR(exp)) {
>>> +        trace_nfsd_set_fh_dentry_badexport(rqstp, fhp,
>>> PTR_ERR(exp));
>>> +
>>> +        if (PTR_ERR(exp) == -ENOENT)
>>> +            return error;
>>> 
>>> -    if (IS_ERR(exp))
>>>        return nfserrno(PTR_ERR(exp));
>>> +    }
>>> 
>>>    if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) {
>>>        /* Elevate privileges so that the lack of 'r' or 'x'
>>> @@ -267,6 +271,9 @@ static __be32 nfsd_set_fh_dentry(struct
>>> svc_rqst *rqstp, struct svc_fh *fhp)
>>>        dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
>>>                data_left, fileid_type,
>>>                nfsd_acceptable, exp);
>>> +        if (IS_ERR_OR_NULL(dentry))
>>> +            trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp,
>>> +                    dentry ?  PTR_ERR(dentry) :
>>> -ESTALE);
>> 
>> If you'll be respinning this series, a handful of nits:
>> 
> 
> I see no need to respin the entire series. Just the last patch.

Fair enough.


>> - the line above has a double space
>> - the trace point names here are a little long, will result in
>>  hard-to-read formatting in the trace log
>> - checkpatch.pl complains about a couple of the later patches,
>>  where one arm of an "if" statement has braces but the other
>>  does not
>> 
>>>    }
>>>    if (dentry == NULL)
>>>        goto out;
>>> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
>>> index 06dd0d337049..9abd1591a841 100644
>>> --- a/fs/nfsd/trace.h
>>> +++ b/fs/nfsd/trace.h
>>> @@ -50,6 +50,36 @@ TRACE_EVENT(nfsd_compound_status,
>>>        __get_str(name), __entry->status)
>>> )
>>> 
>>> +DECLARE_EVENT_CLASS(nfsd_fh_err_class,
>>> +    TP_PROTO(struct svc_rqst *rqstp,
>>> +         struct svc_fh    *fhp,
>>> +         int        status),
>>> +    TP_ARGS(rqstp, fhp, status),
>>> +    TP_STRUCT__entry(
>>> +        __field(u32, xid)
>>> +        __field(u32, fh_hash)
>>> +        __field(int, status)
>>> +    ),
>>> +    TP_fast_assign(
>>> +        __entry->xid = be32_to_cpu(rqstp->rq_xid);
>>> +        __entry->fh_hash = knfsd_fh_hash(&fhp->fh_handle);
>>> +        __entry->status = status;
>>> +    ),
>>> +    TP_printk("xid=0x%08x fh_hash=0x%08x status=%d",
>>> +          __entry->xid, __entry->fh_hash,
>>> +          __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_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
>>> +DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);
>>> +
>>> DECLARE_EVENT_CLASS(nfsd_io_class,
>>>    TP_PROTO(struct svc_rqst *rqstp,
>>>         struct svc_fh    *fhp,
>>> -- 
>>> 2.24.1
>>> 
>> 
>> --
>> Chuck Lever
>> chucklever@gmail.com
>> 
>> 
>> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
>
diff mbox series

Patch

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index b319080288c3..37bc8f5f4514 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -14,6 +14,7 @@ 
 #include "nfsd.h"
 #include "vfs.h"
 #include "auth.h"
+#include "trace.h"
 
 #define NFSDDBG_FACILITY		NFSDDBG_FH
 
@@ -209,11 +210,14 @@  static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 	}
 
 	error = nfserr_stale;
-	if (PTR_ERR(exp) == -ENOENT)
-		return error;
+	if (IS_ERR(exp)) {
+		trace_nfsd_set_fh_dentry_badexport(rqstp, fhp, PTR_ERR(exp));
+
+		if (PTR_ERR(exp) == -ENOENT)
+			return error;
 
-	if (IS_ERR(exp))
 		return nfserrno(PTR_ERR(exp));
+	}
 
 	if (exp->ex_flags & NFSEXP_NOSUBTREECHECK) {
 		/* Elevate privileges so that the lack of 'r' or 'x'
@@ -267,6 +271,9 @@  static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 		dentry = exportfs_decode_fh(exp->ex_path.mnt, fid,
 				data_left, fileid_type,
 				nfsd_acceptable, exp);
+		if (IS_ERR_OR_NULL(dentry))
+			trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp,
+					dentry ?  PTR_ERR(dentry) : -ESTALE);
 	}
 	if (dentry == NULL)
 		goto out;
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index 06dd0d337049..9abd1591a841 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -50,6 +50,36 @@  TRACE_EVENT(nfsd_compound_status,
 		__get_str(name), __entry->status)
 )
 
+DECLARE_EVENT_CLASS(nfsd_fh_err_class,
+	TP_PROTO(struct svc_rqst *rqstp,
+		 struct svc_fh	*fhp,
+		 int		status),
+	TP_ARGS(rqstp, fhp, status),
+	TP_STRUCT__entry(
+		__field(u32, xid)
+		__field(u32, fh_hash)
+		__field(int, status)
+	),
+	TP_fast_assign(
+		__entry->xid = be32_to_cpu(rqstp->rq_xid);
+		__entry->fh_hash = knfsd_fh_hash(&fhp->fh_handle);
+		__entry->status = status;
+	),
+	TP_printk("xid=0x%08x fh_hash=0x%08x status=%d",
+		  __entry->xid, __entry->fh_hash,
+		  __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_NFSD_FH_ERR_EVENT(set_fh_dentry_badexport);
+DEFINE_NFSD_FH_ERR_EVENT(set_fh_dentry_badhandle);
+
 DECLARE_EVENT_CLASS(nfsd_io_class,
 	TP_PROTO(struct svc_rqst *rqstp,
 		 struct svc_fh	*fhp,