On 11/07/2025 00:24, Samuel Thibault wrote:
I added the additional block to free and return after a failed 'conduct_rpc' call to avoid having to indent 60+ lines of code and then forming a much larger diff.Hello, Michael Kelly, le jeu. 10 juil. 2025 22:29:35 +0100, a ecrit:*(p++) = ntohl (read_size); err = conduct_rpc (&rpcbuf, &p); - if (!err) + if (err)Better keep it factorized.
+ + if (protocol_version == 3) + /* 'post_op_attr' is present for any value of 'err' */ + p = process_returned_stat (dir, p, 1);But then it is no use doing it in case of error since we just return in that case.
There are several instances throughout the existing code where the returned 'stat' is updated regardless of error return. I believe the general approach is to take advantage of any data returned to keep the cached state up to date as suggested by RFC1813. See, for example, 'process_returned_stat' within 'netfs_attempt_read'. I chose to supply 1 for 'mod' argument to process_returned_stat rather than in this example using '!err'. It seemed prudent to me to be cautious and invalidate the 'stat_updated' in the case where no stat is returned.
I think that there are improvements to be made with the cache management which I can look at separately. For instance, if the various stat, cache and name-cache timeouts are raised to very high values of 180s, repeated attempts to 'ls' a remotely deleted directory all return ESTALE from the RPC and the 'ls' shows 'Permission denied' until the 180s timeout occurs. The code does not seem to take advantage of the ESTALE error to invalidate the appropriate cache(s).
Please advise if I have misunderstood your review comments and if you still require me to modify the patch.
Regards, Mike.