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
--- 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
