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

Lorenzo Bianconi
2023-08-04T08:03:55Z

[-- 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 --]