On 11/07/2025 00:24, Samuel Thibault wrote:
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.
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.
+
+ 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.
