Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\))
Subject: Re: [PATCH] nfsd: don't call trace_nfsd_deleg_none() if read
 delegation is given
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20200827070237.19942-1-houtao1@huawei.com>
Date:   Thu, 27 Aug 2020 10:43:20 -0400
Cc:     Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <6F61F417-95DA-4CD7-A81A-FA8C6299CF40@oracle.com>
References: <20200827070237.19942-1-houtao1@huawei.com>
To:     Hou Tao <houtao1@huawei.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Hello!

> On Aug 27, 2020, at 3:02 AM, Hou Tao <houtao1@huawei.com> wrote:
>=20
> Don't call trace_nfsd_deleg_none() if read delegation is given,
> else two exclusive traces will be printed:
>=20
>    nfsd_deleg_open: client 5f45b854:e6058001 stateid 00000030:00000001
>    nfsd_deleg_none: client 5f45b854:e6058001 stateid 0000002f:00000001

These are reporting two different state IDs: the first is a delegation
state ID, and the second is an open state ID.

So in the "no delegation" case, we want to see just the open state ID.
In the "delegation" case, we do want to see both.

You could argue (successfully) that the names of the tracepoints are
pretty lousy. Maybe better to rename:

  nfsd_deleg_open -> nfsd_deleg_read
  nfsd_deleg_none -> nfsd_open

What do you think?


> Fix it by calling trace_nfsd_deleg_none() directly in appropriate
> places instead of calling it by checking the value of =
op_delegate_type.
>=20
> Also remove the unnecessary assignment "status =3D nfs_ok", because
> we can ensure status will be nfs_ok after the call of
> nfs4_inc_and_copy_stateid().
>=20
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> fs/nfsd/nfs4state.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>=20
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c09a2a4281ec9..2e6376af701ff 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5131,6 +5131,8 @@ nfs4_open_delegation(struct svc_fh *fh, struct =
nfsd4_open *open,
> 	nfs4_put_stid(&dp->dl_stid);
> 	return;
> out_no_deleg:
> +	trace_nfsd_deleg_none(&stp->st_stid.sc_stateid);
> +
> 	open->op_delegate_type =3D NFS4_OPEN_DELEGATE_NONE;
> 	if (open->op_claim_type =3D=3D NFS4_OPEN_CLAIM_PREVIOUS &&
> 	    open->op_delegate_type !=3D NFS4_OPEN_DELEGATE_NONE) {
> @@ -5232,7 +5234,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, =
struct svc_fh *current_fh, struct nf
> 		if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) {
> 			open->op_delegate_type =3D =
NFS4_OPEN_DELEGATE_NONE_EXT;
> 			open->op_why_no_deleg =3D WND4_NOT_WANTED;
> -			goto nodeleg;
> +			trace_nfsd_deleg_none(&stp->st_stid.sc_stateid);
> +			goto out;
> 		}
> 	}
>=20
> @@ -5241,9 +5244,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, =
struct svc_fh *current_fh, struct nf
> 	* OPEN succeeds even if we fail.
> 	*/
> 	nfs4_open_delegation(current_fh, open, stp);
> -nodeleg:
> -	status =3D nfs_ok;
> -	trace_nfsd_deleg_none(&stp->st_stid.sc_stateid);
> out:
> 	/* 4.1 client trying to upgrade/downgrade delegation? */
> 	if (open->op_delegate_type =3D=3D NFS4_OPEN_DELEGATE_NONE && dp =
&&
> --=20
> 2.25.0.4.g0ad7144999
>=20

--
Chuck Lever