Re: Hurd NFS translator

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.