On Tue, 16 Nov 2010 14:13:52 -0600
Steve French <smfrench@xxxxxxxxx> wrote:
> 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 :)
>
>
2^31 jiffies / 1000 jiffies/s / 86400 s/day = 24.85513481481481481481
So I calculate 25 days or so...but it depends on what HZ is set to on
your machine really...
--
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
