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

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

[Suresh Jayaraman]

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 */
+
 /*
  * 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;
 

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

[Jeff Layton]

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 */
> +
>  /*
>   * 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.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Steve French]

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 */
>> +
>>  /*
>>   * 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.



-- 
Thanks,

Steve

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

[Jeff Layton]

On Tue, 16 Nov 2010 13:25:37 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 */
> >> +
> >>  /*
> >>   * 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Steve French]

On Tue, Nov 16, 2010 at 1:42 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 16 Nov 2010 13:25:37 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 */
>> >> +
>> >>  /*
>> >>   * 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

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

[Jeff Layton]

On Tue, 16 Nov 2010 14:13:52 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Nov 16, 2010 at 1:42 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, 16 Nov 2010 13:25:37 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 */
> >> >> +
> >> >>  /*
> >> >>   * 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Suresh Jayaraman]

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