Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Chuck Lever <chuck.lever@oracle.com>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH 2/8] nfsd: Add tracing to nfsd_set_fh_dentry()
Date:   Tue, 3 Mar 2020 08:27:51 -0500
Message-Id: <26084E44-0677-42A6-8DC8-71169070FE8D@oracle.com>
References: <3fa96b3a1ec599624d464085854a39a6b2b86447.camel@hammerspace.com>
Cc:     "chucklever@gmail.com" <chucklever@gmail.com>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "bfields@redhat.com" <bfields@redhat.com>
In-Reply-To: <3fa96b3a1ec599624d464085854a39a6b2b86447.camel@hammerspace.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Mar 3, 2020, at 12:59 AM, Trond Myklebust <trondmy@hammerspace.com> wro=
te:
>=20
> =EF=BB=BFOn 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:
>>>=20
>>> Add tracing to allow us to figure out where any stale filehandle
>>> issues
>>> may be originating from.
>>>=20
>>> 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(-)
>>>=20
>>> 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"
>>>=20
>>> #define NFSDDBG_FACILITY        NFSDDBG_FH
>>>=20
>>> @@ -209,11 +210,14 @@ static __be32 nfsd_set_fh_dentry(struct
>>> svc_rqst *rqstp, struct svc_fh *fhp)
>>>    }
>>>=20
>>>    error =3D nfserr_stale;
>>> -    if (PTR_ERR(exp) =3D=3D -ENOENT)
>>> -        return error;
>>> +    if (IS_ERR(exp)) {
>>> +        trace_nfsd_set_fh_dentry_badexport(rqstp, fhp,
>>> PTR_ERR(exp));
>>> +
>>> +        if (PTR_ERR(exp) =3D=3D -ENOENT)
>>> +            return error;
>>>=20
>>> -    if (IS_ERR(exp))
>>>        return nfserrno(PTR_ERR(exp));
>>> +    }
>>>=20
>>>    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 =3D 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);
>>=20
>> If you'll be respinning this series, a handful of nits:
>>=20
>=20
> 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
>>=20
>>>    }
>>>    if (dentry =3D=3D 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)
>>> )
>>>=20
>>> +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 =3D be32_to_cpu(rqstp->rq_xid);
>>> +        __entry->fh_hash =3D knfsd_fh_hash(&fhp->fh_handle);
>>> +        __entry->status =3D status;
>>> +    ),
>>> +    TP_printk("xid=3D0x%08x fh_hash=3D0x%08x status=3D%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,
>>> --=20
>>> 2.24.1
>>>=20
>>=20
>> --
>> Chuck Lever
>> chucklever@gmail.com
>>=20
>>=20
>>=20
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>=20
>=20