On Wed, Nov 17, 2010 at 1:16 PM, Suresh Jayaraman <sjayaraman@xxxxxxx> wrote:
> On 11/18/2010 12:09 AM, Jeff Layton wrote:
>> On Wed, 17 Nov 2010 23:53:34 +0530
>> Suresh Jayaraman <sjayaraman@xxxxxxx> wrote:
>>
>>> Currently, the attribute cache timeout for CIFS is hardcoded to 1 second. This
>>> means that the client might have to issue a QPATHINFO/QFILEINFO call every 1
>>> second to verify if something has changes, which seems too expensive. On the
>>> other hand, if the timeout is hardcoded to a higher value, workloads that
>>> expect strict cache coherency might see unexpected results.
>>>
>>> Making attribute cache timeout as a tunable will allow us to make a tradeoff
>>> between performance and cache metadata correctness depending on the
>>> application/workload needs.
>>>
>>> Add 'actimeo' tunable that can be used to tune the attribute cache timeout.
>>> The default timeout is set to 1 second.
>>>
>>> Also, display actimeo option value in /proc/mounts.
>>>
>>> Cc: Jeff Layton <jlayton@xxxxxxxxxx>
>>> Signed-off-by: Suresh Jayaraman <sjayaraman@xxxxxxx>
>>> ---
>>> fs/cifs/cifs_fs_sb.h | 1 +
>>> fs/cifs/cifsfs.c | 1 +
>>> fs/cifs/cifsglob.h | 5 +++++
>>> fs/cifs/connect.c | 10 ++++++++++
>>> fs/cifs/inode.c | 6 +++---
>>> 5 files changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>>> index ff7d299..f30700d 100644
>>> --- a/fs/cifs/inode.c
>>> +++ b/fs/cifs/inode.c
>>> @@ -1648,6 +1648,7 @@ static bool
>>> cifs_inode_needs_reval(struct inode *inode)
>>> {
>>> struct cifsInodeInfo *cifs_i = CIFS_I(inode);
>>> + struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>>>
>>> if (cifs_i->clientCanCacheRead)
>>> return false;
>>> @@ -1658,12 +1659,11 @@ cifs_inode_needs_reval(struct inode *inode)
>>> if (cifs_i->time == 0)
>>> return true;
>>>
>>> - /* FIXME: the actimeo should be tunable */
>>> - if (time_after_eq(jiffies, cifs_i->time + HZ))
>>> + if (time_after_eq(jiffies, cifs_i->time + (cifs_sb->actimeo * HZ)))
>>
>> Problem:
>>
>> Suppose (cifs_i->time + (cifs_sb->actimeo * HZ)) carries you
>> past two sign bit changes. At that point, it may look like
>> the resulting time is in the past, not the future.
>
> Good catch.
>
>> You actually do need to impose a limit here. I suggest looking
>> very closely at time_before/time_after macros. It'll also be
>> easier to deal storing actimeo in terms of jiffies instead of
>> seconds.
>
>> Finally, this value needs to be displayed in /proc/mounts.
>>
>
> Did you mean displaying values in jiffies in /proc/mounts? Wouldn't it
> be confusing for users to see such a higher numbers (usually) when they
> had used a simple 2 digit number for e.g. during mount?
That is a good question - seems like we ought to allow setting more granular
timeouts (ie jiffies) but nfs actimeo assumes the value is in seconds
by default.
I am fine with allowing actimeo to be specified more than one way (seconds vs
milliseconds or seconds vs. jiffies) with suffix or prefix or keyword variant.
Probably should display it in seconds (or milliseconds) - jiffies
would be confusing
(I had trouble finding a way - short of looking at .config - what
jiffies equivalent
in seconds is)
--
Thanks,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
