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;
Date:   Fri, 4 Aug 2023 10:02:57 +0200
From:   Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
To:     NeilBrown <neilb@suse.de>
Cc:     Chuck Lever <chuck.lever@oracle.com>,
        Lorenzo Bianconi <lorenzo@kernel.org>,
        linux-nfs@vger.kernel.org, jlayton@kernel.org
Subject: Re: [PATCH v4 2/2] NFSD: add rpc_status entry in nfsd debug
 filesystem
Message-ID: <ZMywsSb9Qz7K3V/f@lore-rh-laptop>
References: <cover.1690569488.git.lorenzo@kernel.org>
 <a23a0482a465299ac06d07d191e0c9377a11a4d1.1690569488.git.lorenzo@kernel.org>
 <ZMQVX5RlQaWt5wkn@tissot.1015granger.net>
 <169058229140.32308.9984914710358617790@noble.neil.brown.name>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="yZioOiVPm9KFfq5z"
Content-Disposition: inline
In-Reply-To: <169058229140.32308.9984914710358617790@noble.neil.brown.name>
Precedence: bulk


--yZioOiVPm9KFfq5z
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

> On Sat, 29 Jul 2023, Chuck Lever wrote:
> > On Fri, Jul 28, 2023 at 08:44:04PM +0200, Lorenzo Bianconi wrote:

[...]

> >=20
> > Neil said:
> >=20
> > > I suggest you add add a counter to the rqstp which is incremented from
> > > even to odd after parsing a request - including he v4 parsing needed =
to
> > > have a sable ->opcnt - and then incremented from odd to even when the
> > > request is complete.
> > > Then this code samples the counter, skips the rqst if the counter is
> > > even, and resamples the counter after collecting the data.  If it has
> > > changed, the drop the record.
> >=20
> > I don't see a check if the status counter is even.
>=20
> ...and there does need to be one.  If the counter is even, then the
> fields are meaningless and unstable.  The RQ_BUSY check is, I think,
> meant to check if the fields are meaningful, but they aren't meaningful
> until some time after RQ_BUSY is clear.
>=20
> I would replace the "RQ_BUSY not set" test with "counter is even"

ack, I will fix it.

>=20
> >=20
> > Also, as above, I'm not sure atomic_read() is necessary here. Maybe
> > just READ_ONCE() ? Neil, any thoughts?
>=20
> Agree - we don't need an atomic as there is a single writer.
> I think
>   smp_store_release(rqstp->counter, rqstp->counter|1)
> to increment it after parsing the request.  This makes it abundantly
> clear the value will be odd, and ensures that if another thread sees the
> 'odd' value, then it can also see the results of the parse.
>=20
> To increment after processing the request,
>    smp_store_release(rqstp->counter, rqstp->counter + 1)
>=20
> Then
>   counter =3D smp_load_acquire(rqstp->counter);
>   if ((counter & 1) =3D=3D 0)
> to test if it is even before reading the state.  This ensure that if it
> sees "odd' it will see the results of the parse.
>=20
> and
>   if ((smp_load_acquire(counter) =3D=3D counter)  continue;
>=20
> before trusting that the data we read was consistent.
>=20
> Note that we "release" *after* something and "acquire" *before"
> something.
> I think it helps to always think about what the access is "before" or
> "after" when reasoning about barriers.
> checkpatch will want a comment before these acquire and release
> operation.  I recommend using the corresponding word "before" or "after"
> in that comment.


ack, I will add it.

Regards,
Lorenzo

>=20
> NeilBrown
>=20

--yZioOiVPm9KFfq5z
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCZMywrgAKCRA6cBh0uS2t
rB8rAP9/cJT1egOk95+jOJMdkrjNwhCXPsCeSbPUhknQ6k2pXgEA8DQtcGppIOCV
WFTwlCEsvM8vaMIQct7WTsKLcA8yYw8=
=U5uw
-----END PGP SIGNATURE-----

--yZioOiVPm9KFfq5z--