Re: [RFC][PATCH] cifs: add attribute cache timeout (actimeo) tunable

[Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>]

On 11/17/2010 01:52 AM, Jeff Layton wrote:
>>>> On Tue, Nov 16, 2010 at 1:03 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> On Tue, 16 Nov 2010 15:39:37 +0530
>>>>> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> 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-l3A5Bk7waGM@public.gmane.org>
>>>>>> ---
>>>>>> �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