Re: [PATCH v4 2/2] NFSD: add rpc_status entry in nfsd debug filesystem

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2394 bytes --]

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

[...]

> > 
> > Neil said:
> > 
> > > 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.
> > 
> > I don't see a check if the status counter is even.
> 
> ...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.
> 
> I would replace the "RQ_BUSY not set" test with "counter is even"

ack, I will fix it.

> 
> > 
> > Also, as above, I'm not sure atomic_read() is necessary here. Maybe
> > just READ_ONCE() ? Neil, any thoughts?
> 
> 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.
> 
> To increment after processing the request,
>    smp_store_release(rqstp->counter, rqstp->counter + 1)
> 
> Then
>   counter = smp_load_acquire(rqstp->counter);
>   if ((counter & 1) == 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.
> 
> and
>   if ((smp_load_acquire(counter) == counter)  continue;
> 
> before trusting that the data we read was consistent.
> 
> 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

> 
> NeilBrown
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]