On 11/17/2010 01:52 AM, Jeff Layton wrote: >>>> On Tue, Nov 16, 2010 at 1:03 PM, Jeff Layton <[email protected]> wrote: >>>>> On Tue, 16 Nov 2010 15:39:37 +0530 >>>>> Suresh Jayaraman <[email protected]> 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 <[email protected]> >>>>>> --- >>>>>> �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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
