On 11/17/2010 01:52 AM, Jeff Layton wrote:
>>>> On Tue, Nov 16, 2010 at 1:03 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>>>>> On Tue, 16 Nov 2010 15:39:37 +0530
>>>>> Suresh Jayaraman <sjayaraman@xxxxxxx> wrote:
>>>>>
>>>>>> Currently, the attribute cache timeout for CIFS is 1 sec. This means that the
>>>>>> client might have to issue a QPATHINFO/QFILEINFO call every 1 sec to verify if
>>>>>> something has changed, which seems too expensive. On the otherhand, increasing
>>>>>> this value further may not work well for all the users.
>>>>>>
>>>>>> This patch introduces a tunable mount option 'actimeo' that can be used to tune
>>>>>> the attribute cache timeout. This patch takes a conservative approach and sets
>>>>>> the default timeout is set to 3 seconds while limiting maximum timeout to 60
>>>>>> seconds. Ideally, it is preferred to set attribute cache timeouts separately
>>>>>> for files and directories like how NFS does. However, I think it is better to
>>>>>> keep it simple without too many options, introduce the option to users, get
>>>>>> feedback from them and then decide what is working better for CIFS.
>>>>>>
>>>>>> This patch has been tested lightly and no adverse effects were seen.
>>>>>>
>>>>>> Signed-off-by: Suresh Jayaraman <sjayaraman@xxxxxxx>
>>>>>> ---
>>>>>> ïfs/cifs/cifs_fs_sb.h | ï ï1 +
>>>>>> ïfs/cifs/cifsglob.h ï | ï ï3 +++
>>>>>> ïfs/cifs/connect.c ï ï| ï 16 ++++++++++++++++
>>>>>> ïfs/cifs/inode.c ï ï ï| ï ï6 +++---
>>>>>> ï4 files changed, 23 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
>>>>>> index e9a393c..efd2d73 100644
>>>>>> --- a/fs/cifs/cifs_fs_sb.h
>>>>>> +++ b/fs/cifs/cifs_fs_sb.h
>>>>>> @@ -48,6 +48,7 @@ struct cifs_sb_info {
>>>>>> ï ï ï struct nls_table *local_nls;
>>>>>> ï ï ï unsigned int rsize;
>>>>>> ï ï ï unsigned int wsize;
>>>>>> + ï ï unsigned int actimeo;
>>>>>> ï ï ï atomic_t active;
>>>>>> ï ï ï uid_t ï mnt_uid;
>>>>>> ï ï ï gid_t ï mnt_gid;
>>>>>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>>>>>> index b577bf0..eb22130 100644
>>>>>> --- a/fs/cifs/cifsglob.h
>>>>>> +++ b/fs/cifs/cifsglob.h
>>>>>> @@ -44,6 +44,9 @@
>>>>>>
>>>>>> ï#define CIFS_MIN_RCV_POOL 4
>>>>>>
>>>>>> +#define CIFS_DEF_ACTIMEO (3) /* default attribute cache timeout (seconds) */
>>>>>> +#define CIFS_MAX_ACTIMEO (60) ï ï ï ï/* max allowed attribute cache timeout */
>>>>>> +
>>>>>
>>>>> I too think that the 1s actimeo is too aggressive in general, but I'm a
>>>>> little leery that changing the default here might mean subtle
>>>>> regressions. Cache consistency is really hard to get right, so we need
>>>>> to take great care when we change its behavior.
>>>>>
>>>>> What do you think about respinning this patch and leaving the default
>>>>> at 1s? We could consider increasing it later if we can prove to
>>>>> ourselves that it won't cause problems.
>>>>
I thought 3 sec was on the conservative side, but you're right - it
might cause regressions with applications which expect strict cache
coherency.
>>>>
>>>> Also probably should allow maximum that is longer than 60 seconds
>>>> timeout (perhaps an hour? a day?).
>>>>
>>>> IIRC there is no maximum specified for DirectoryCacheTimeout in
>>>> Windows - and there are cases where I can imagine longer than 60
>>>> seconds being useful.
>>>
>>> Agreed. I don't see any reason not to allow someone to shoot themselves
>>> in the foot. I see no need for an arbitrary limit. If you do that
>>> though, you probably do want to limit it to 2^31 jiffies or so to avoid
>>> wraparound issues on 32 bit arches.
I thought about this a bit and I'm leaning towards not setting any
arbitrary maximum limit primarily because different HZ values would lead
to different cache timeout values and it could be problematic in a setup
with multiple clients with different HZ values.
I'll respin the patch with 1 sec default.
--
Suresh Jayaraman
--
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
