Maybe there is a nested call possible via the nfs4_attr_cache() -> nfs4_purge_caches() -> nfs4_flush_pages() -> VOP_PUTPAGE() sequence.  In any case, if was_serial is always FALSE it is harmless and not related to my fix.

But you would agree that your new code use this value/variable and we need to know all cases for calling of nfs4_attr_cache() before modify its behaviour.
Nevertheless, nfs4_putpage/nfs4_putpages don't call r4_do_attrcache() or something similar that can call nfs4_attr_cache().

But you would agree that your new code use this value/variable and we need to know all cases for calling of nfs4_attr_cache() before modify its behaviour.

I'm not sure I understand your arguing.  No, my code is not using was_serial.



Nevertheless, nfs4_putpage/nfs4_putpages don't call r4_do_attrcache() or something similar that can call nfs4_attr_cache().

If that's the case then was_serial is probably always FALSE.  But I do not understand how is that related to my fix and how it affects my fix.  Please elaborate.

Also, I do not understand what is the issue you are suggesting to fix.  Please be explicit what is the problem you see here.  What is the scenario that could cause a problem?  How do you suggest to fix the problem you see?  Thanks.

| I'm not sure I understand your arguing.  No, my code is not using was_serial.
| If that's the case then was_serial is probably always FALSE.  But I do not understand how is that related to my fix and how it affects my fix.  Please elaborate.
There are two places:

1.

 -  if (rp->r_serial && !was_serial) {

 -      klwp_t *lwp = ttolwp(curthread);

 -

 +  if (rp->r_serial != NULL && !was_serial) {


Code Under this condition "if (rp->r_serial != NULL && !was_serial)". Line 477,478.


All above mean that was_serial has meaning and use that value. Therefore if was_serial has no meaning, does it mean that patch that correcting original code and adding a new code should be improved ? I suppose yes.

| Also, I do not understand what is the issue you are suggesting to fix.  Please be explicit what is the problem you see here.  What is the scenario that could cause a problem?  How do you suggest to fix the problem you see?  Thanks.

My intention is: new code and fixes should not introduce a new uncertainty or leave old uncertainties. Maybe there is not any issue, but to answer for sure, we need to clear understand what each variable means and what each line does. Without it the new code will be as old one. But we want to make the code better. Don't we?
About scenario. If was_serial is always FALSE , rp->r_serial != NULL means that simultaneously another "nfs4_attr_cache" is being executed. For this case, when you remove check on "recov", does it mean that PURGE_ATTRCACHE4_LOCKED(rp) will almost always clean cached attributes and this may increase number of GETATTR requests ?

All above mean that was_serial has meaning and use that value.

Sure.



Therefore if was_serial has no meaning, does it mean that patch that correcting original code and adding a new code should be improved ? I suppose yes.

I never said was_serial has no meaning.



My intention is: new code and fixes should not introduce a new uncertainty or leave old uncertainties.

Sorry, I disagree.  Trying to achieve that I would need to fix all possible uncertanities you could find anywhere in the code.



Maybe there is not any issue, but to answer for sure, we need to clear understand what each variable means and what each line does.

You are free to do so and propose an improvement if you find something needs to be changed.



Without it the new code will be as old one.

No.



But we want to make the code better. Don't we?

Yes.  And because of that I propose this fix.



About scenario. If was_serial is always FALSE , rp->r_serial != NULL means that simultaneously another "nfs4_attr_cache" is being executed. For this case, when you remove check on "recov", does it mean that PURGE_ATTRCACHE4_LOCKED(rp) will almost always clean cached attributes and this may increase number of GETATTR requests ?

Yes, but only for this dangerous case (i.e. when there is a risk of possible deadlock).  It is far better to issue one more GETATTR than to risk a deadlock.
BTW, this fix is in production on all our NFSv4 clients for 2+ years now without any issue.