Re: [PATCH V2] make "noac" and "actimeo=0" work correctly

[Peter Staubach <staubach@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2493 bytes --]

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

[-- Attachment #2: attrtimeo.devel.2 --]
[-- Type: text/plain, Size: 1538 bytes --]

--- linux-2.6.25.i686/fs/nfs/inode.c.org
+++ linux-2.6.25.i686/fs/nfs/inode.c
@@ -706,13 +706,6 @@ int nfs_attribute_timeout(struct inode *
 
 	if (nfs_have_delegation(inode, FMODE_READ))
 		return 0;
-	/*
-	 * Special case: if the attribute timeout is set to 0, then always
-	 * 		 treat the cache as having expired (unless holding
-	 * 		 a delegation).
-	 */
-	if (nfsi->attrtimeo == 0)
-		return 1;
 	return !time_in_range(jiffies, nfsi->read_cache_jiffies, nfsi->read_cache_jiffies + nfsi->attrtimeo);
 }
 
--- linux-2.6.25.i686/include/linux/nfs_fs.h.org
+++ linux-2.6.25.i686/include/linux/nfs_fs.h
@@ -121,7 +121,10 @@ struct nfs_inode {
 	 *
 	 * We need to revalidate the cached attrs for this inode if
 	 *
-	 *	jiffies - read_cache_jiffies > attrtimeo
+	 *	jiffies - read_cache_jiffies >= attrtimeo
+	 *
+	 * Please note the comparison is greater than or equal
+	 * so that zero timeout values can be specified.
 	 */
 	unsigned long		read_cache_jiffies;
 	unsigned long		attrtimeo;
--- linux-2.6.25.i686/include/linux/jiffies.h.org
+++ linux-2.6.25.i686/include/linux/jiffies.h
@@ -115,9 +115,12 @@ static inline u64 get_jiffies_64(void)
 	 ((long)(a) - (long)(b) >= 0))
 #define time_before_eq(a,b)	time_after_eq(b,a)
 
+/*
+ * Calculate whether a in the range of [b, c).
+ */
 #define time_in_range(a,b,c) \
 	(time_after_eq(a,b) && \
-	 time_before_eq(a,c))
+	 time_before(a,c))
 
 /* Same as above, but does so with platform independent 64bit types.
  * These must be used when utilizing jiffies_64 (i.e. return value of