Commit 64672d55d93c26fb4035fd1a84a803cbc09cb058

Authored by Peter Staubach
Committed by Trond Myklebust
1 parent dc0b027dfa

optimize attribute timeouts for "noac" and "actimeo=0"

Hi.

I've been looking at a bugzilla which describes a problem where
a customer was advised to use either the "noac" or "actimeo=0"
mount options to solve a consistency problem that they were
seeing in the file attributes.  It turned out that this solution
did not work reliably for them because sometimes, the local
attribute cache was believed to be valid and not timed out.
(With an attribute cache timeout of 0, the cache should always
appear to be timed out.)

In looking at this situation, it appears to me that the problem
is that the attribute cache timeout code has an off-by-one
error in it.  It is assuming that the cache is valid in the
region, [read_cache_jiffies, read_cache_jiffies + attrtimeo].  The
cache should be considered valid only in the region,
[read_cache_jiffies, read_cache_jiffies + attrtimeo).  With this
change, the options, "noac" and "actimeo=0", work as originally
expected.

This problem was previously addressed by special casing the
attrtimeo == 0 case.  However, since the problem is only an off-
by-one error, the cleaner solution is address the off-by-one
error and thus, not require the special case.

    Thanx...

        ps

Signed-off-by: Peter Staubach <staubach@redhat.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>

Showing 5 changed files with 18 additions and 12 deletions Side-by-side Diff

... ... @@ -1798,7 +1798,7 @@
1798 1798 if (cache == NULL)
1799 1799 goto out;
1800 1800 if (!nfs_have_delegation(inode, FMODE_READ) &&
1801   - !time_in_range(jiffies, cache->jiffies, cache->jiffies + nfsi->attrtimeo))
  1801 + !time_in_range_open(jiffies, cache->jiffies, cache->jiffies + nfsi->attrtimeo))
1802 1802 goto out_stale;
1803 1803 res->jiffies = cache->jiffies;
1804 1804 res->cred = cache->cred;
... ... @@ -712,14 +712,7 @@
712 712  
713 713 if (nfs_have_delegation(inode, FMODE_READ))
714 714 return 0;
715   - /*
716   - * Special case: if the attribute timeout is set to 0, then always
717   - * treat the cache as having expired (unless holding
718   - * a delegation).
719   - */
720   - if (nfsi->attrtimeo == 0)
721   - return 1;
722   - return !time_in_range(jiffies, nfsi->read_cache_jiffies, nfsi->read_cache_jiffies + nfsi->attrtimeo);
  715 + return !time_in_range_open(jiffies, nfsi->read_cache_jiffies, nfsi->read_cache_jiffies + nfsi->attrtimeo);
723 716 }
724 717  
725 718 /**
... ... @@ -1182,7 +1175,7 @@
1182 1175 nfsi->attrtimeo_timestamp = now;
1183 1176 nfsi->attr_gencount = nfs_inc_attr_generation_counter();
1184 1177 } else {
1185   - if (!time_in_range(now, nfsi->attrtimeo_timestamp, nfsi->attrtimeo_timestamp + nfsi->attrtimeo)) {
  1178 + if (!time_in_range_open(now, nfsi->attrtimeo_timestamp, nfsi->attrtimeo_timestamp + nfsi->attrtimeo)) {
1186 1179 if ((nfsi->attrtimeo <<= 1) > NFS_MAXATTRTIMEO(inode))
1187 1180 nfsi->attrtimeo = NFS_MAXATTRTIMEO(inode);
1188 1181 nfsi->attrtimeo_timestamp = now;
include/linux/jiffies.h
... ... @@ -115,9 +115,19 @@
115 115 ((long)(a) - (long)(b) >= 0))
116 116 #define time_before_eq(a,b) time_after_eq(b,a)
117 117  
  118 +/*
  119 + * Calculate whether a is in the range of [b, c].
  120 + */
118 121 #define time_in_range(a,b,c) \
119 122 (time_after_eq(a,b) && \
120 123 time_before_eq(a,c))
  124 +
  125 +/*
  126 + * Calculate whether a is in the range of [b, c).
  127 + */
  128 +#define time_in_range_open(a,b,c) \
  129 + (time_after_eq(a,b) && \
  130 + time_before(a,c))
121 131  
122 132 /* Same as above, but does so with platform independent 64bit types.
123 133 * These must be used when utilizing jiffies_64 (i.e. return value of
include/linux/nfs_fs.h
... ... @@ -130,7 +130,10 @@
130 130 *
131 131 * We need to revalidate the cached attrs for this inode if
132 132 *
133   - * jiffies - read_cache_jiffies > attrtimeo
  133 + * jiffies - read_cache_jiffies >= attrtimeo
  134 + *
  135 + * Please note the comparison is greater than or equal
  136 + * so that zero timeout values can be specified.
134 137 */
135 138 unsigned long read_cache_jiffies;
136 139 unsigned long attrtimeo;
... ... @@ -234,7 +234,7 @@
234 234 list_for_each_entry_safe(cred, next, &cred_unused, cr_lru) {
235 235  
236 236 /* Enforce a 60 second garbage collection moratorium */
237   - if (time_in_range(cred->cr_expire, expired, jiffies) &&
  237 + if (time_in_range_open(cred->cr_expire, expired, jiffies) &&
238 238 test_bit(RPCAUTH_CRED_HASHED, &cred->cr_flags) != 0)
239 239 continue;
240 240