Trond Myklebust wrote:
> On Thu, 2008-07-10 at 13:41 -0400, Peter Staubach wrote:
>   
>>> include/linux/jiffies.h claims it handles jiffy wrapping correctly.
>>> Why isn't time_in_range() sufficient if 'c' has wrapped?  If it isn't,
>>> should you fix time_in_range() too?
>>>
>>>   
>>>       
>> Clearly, time_in_range() is not sufficient if the 'c' has
>> wrapped.  It only tests to see if a >=b and a <= c.  If 'c'
>> is less than 'b', then time_in_range() will return false.
>>     
>
> Hmm... The actual test in the current time_in_range() should be
>
>     ((long)b - (long)a) <= 0) && ((long)a - (long)c) <= 0
>
> Which is _not_ the same as testing for a>=b && a<=c in the case of a
> sign wrap. Can you show me a case where we might have a problem?
>
> The only case I can think of is if
>
>     ((long) c - (long) b) < 0
>
> (IOW: if the range itself is too large to fit into a signed long). I
> can't imagine that we will ever find ourselves in that situation.
>
>
>   
>> The change, which makes attrtimeo=0 work for free, is to figure out
>> that if the attrtimeo is N, then the attribute cache is valid from
>> time, T, to T + N - 1, not T + N.  Thus, the current attribute
>> cache implementation is off by one because the attribute cache
>> should expire at time, T + N.  The time_in_range() macro was handy
>> and looked right, but wasn't quite right for the desired semantics.
>>
>> Adding tests to check to see if b and c are equal is tuning for
>> the wrong case, I think.  I believe that the majority of file
>> systems are not mounted with "noac" or "actimeo=0", so the extra
>> test would just be overhead for the common case.
>>     
>
> I agree with this.
>
>   

I think that the case that I was looking at is the case that you
described as the difference between b and c being too large to
fit into a signed long as a positive value.

I would agree, that it is probably not worth addressing.  I
suppose that the real solution would be to convert the time
basis to be something which is not subject to wrapping, but the
obvious candidate, the current time, seems a little expensive
to be constantly retrieving.

Given that we seem to "own" time_in_range(), how about the
attached patch which just modifies time_in_range() to calculate
[b,c) instead of [b,c], removes the special case for attrtimeo==0
in nfs_attribute_timeout() and adds a comment that Chuck requested
concerning the need to ensure that zero timeout values continue
to work?

    Thanx...

       ps