On Tue, Nov 16, 2010 at 1:42 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Tue, 16 Nov 2010 13:25:37 -0600
> Steve French <smfrench@xxxxxxxxx> 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 */
>> >> +
>> >> /*
>> >> * MAX_REQ is the maximum number of requests that WE will send
>> >> * on one socket concurrently. It also matches the most common
>> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> >> index 251a17c..39edc54 100644
>> >> --- a/fs/cifs/connect.c
>> >> +++ b/fs/cifs/connect.c
>> >> @@ -103,6 +103,7 @@ struct smb_vol {
>> >> bool multiuser:1;
>> >> unsigned int rsize;
>> >> unsigned int wsize;
>> >> + unsigned int actimeo; /* attribute cache timeout */
>> >> bool sockopt_tcp_nodelay:1;
>> >> unsigned short int port;
>> >> char *prepath;
>> >> @@ -1214,6 +1215,10 @@ cifs_parse_mount_options(char *options, const char *devname,
>> >> printk(KERN_WARNING "CIFS: server net"
>> >> "biosname longer than 15 truncated.\n");
>> >> }
>> >> + } else if (strnicmp(data, "actimeo", 7) == 0) {
>> >> + if (value && *value)
>> >> + vol->actimeo =
>> >> + simple_strtoul(value, &value, 0);
>> >> } else if (strnicmp(data, "credentials", 4) == 0) {
>> >> /* ignore */
>> >> } else if (strnicmp(data, "version", 3) == 0) {
>> >> @@ -2566,6 +2571,17 @@ static void setup_cifs_sb(struct smb_vol *pvolume_info,
>> >> cFYI(1, "file mode: 0x%x dir mode: 0x%x",
>> >> cifs_sb->mnt_file_mode, cifs_sb->mnt_dir_mode);
>> >>
>> >> + if ((pvolume_info->actimeo) &&
>> >> + (pvolume_info->actimeo < CIFS_MAX_ACTIMEO))
>> >> + cifs_sb->actimeo = pvolume_info->actimeo;
>> >> + else if ((pvolume_info->actimeo) &&
>> >> + (pvolume_info->actimeo >= CIFS_MAX_ACTIMEO)) {
>> >> + cifs_sb->actimeo = CIFS_MAX_ACTIMEO;
>> >> + cERROR(1, "actimeo %d too large, using %d instead",
>> >> + pvolume_info->actimeo, CIFS_MAX_ACTIMEO);
>> >> + } else /* default */
>> >> + cifs_sb->actimeo = CIFS_DEF_ACTIMEO;
>> >> +
>> >> if (pvolume_info->noperm)
>> >> cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_NO_PERM;
>> >> if (pvolume_info->setuids)
>> >> 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)))
>> >> return true;
>> >>
>> >> /* hardlinked files w/ noserverino get "special" treatment */
>> >> - if (!(CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) &&
>> >> + if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) &&
>> >> S_ISREG(inode->i_mode) && inode->i_nlink != 1)
>> >> return true;
>> >>
>> >
>> > 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.
>>
>> This patch could be helpful.
>>
>> Yes.
>>
>> 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.
>
> It also wouldn't hurt to store the value internally in terms of
> jiffies. You should also update the function that displays options
> in /proc/mounts to show this value too.
Yes - might as well. 1 second is a very long time in today's world ...
So if we default to equivalent of 1 second (in jiffies), max would be
2^31 jiffies ie about 100 days - which should be long enough for most
people :)
--
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
