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.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
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
