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

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

[Suresh Jayaraman]

Currently, the attribute cache timeout for CIFS is hardcoded to 1 second. This
means that the client might have to issue a QPATHINFO/QFILEINFO call every 1
second to verify if something has changes, which seems too expensive. On the
other hand, if the timeout is hardcoded to a higher value, workloads that
expect strict cache coherency might see unexpected results.

Making attribute cache timeout as a tunable will allow us to make a tradeoff
between performance and cache metadata correctness depending on the
application/workload needs.

Add 'actimeo' tunable that can be used to tune the attribute cache timeout.
The default timeout is set to 1 second.

Also, display actimeo option value in /proc/mounts.

Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |    1 +
 fs/cifs/cifsfs.c     |    1 +
 fs/cifs/cifsglob.h   |    5 +++++
 fs/cifs/connect.c    |   10 ++++++++++
 fs/cifs/inode.c      |    6 +++---
 5 files changed, 20 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/cifsfs.c b/fs/cifs/cifsfs.c
index 9c37897..13be23d 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -461,6 +461,7 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
 
 	seq_printf(s, ",rsize=%d", cifs_sb->rsize);
 	seq_printf(s, ",wsize=%d", cifs_sb->wsize);
+	seq_printf(s, ",actimeo=%d", cifs_sb->actimeo);
 
 	return 0;
 }
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index b577bf0..037f793 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -45,6 +45,11 @@
 #define CIFS_MIN_RCV_POOL 4
 
 /*
+ * default attribute cache timeout (seconds)
+ */
+#define CIFS_DEF_ACTIMEO (1)
+
+/*
  * MAX_REQ is the maximum number of requests that WE will send
  * on one socket concurrently. It also matches the most common
  * value of max multiplex returned by servers.  We may
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 251a17c..7f92328 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,11 @@ 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)
+		cifs_sb->actimeo = pvolume_info->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: [PATCH] cifs: add attribute cache timeout (actimeo) tunable

[Jeff Layton]

On Wed, 17 Nov 2010 23:53:34 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> Currently, the attribute cache timeout for CIFS is hardcoded to 1 second. This
> means that the client might have to issue a QPATHINFO/QFILEINFO call every 1
> second to verify if something has changes, which seems too expensive. On the
> other hand, if the timeout is hardcoded to a higher value, workloads that
> expect strict cache coherency might see unexpected results.
> 
> Making attribute cache timeout as a tunable will allow us to make a tradeoff
> between performance and cache metadata correctness depending on the
> application/workload needs.
> 
> Add 'actimeo' tunable that can be used to tune the attribute cache timeout.
> The default timeout is set to 1 second.
> 
> Also, display actimeo option value in /proc/mounts.
> 
> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
> ---
>  fs/cifs/cifs_fs_sb.h |    1 +
>  fs/cifs/cifsfs.c     |    1 +
>  fs/cifs/cifsglob.h   |    5 +++++
>  fs/cifs/connect.c    |   10 ++++++++++
>  fs/cifs/inode.c      |    6 +++---
>  5 files changed, 20 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/cifsfs.c b/fs/cifs/cifsfs.c
> index 9c37897..13be23d 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -461,6 +461,7 @@ cifs_show_options(struct seq_file *s, struct vfsmount *m)
>  
>  	seq_printf(s, ",rsize=%d", cifs_sb->rsize);
>  	seq_printf(s, ",wsize=%d", cifs_sb->wsize);
> +	seq_printf(s, ",actimeo=%d", cifs_sb->actimeo);
>  
>  	return 0;
>  }
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index b577bf0..037f793 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -45,6 +45,11 @@
>  #define CIFS_MIN_RCV_POOL 4
>  
>  /*
> + * default attribute cache timeout (seconds)
> + */
> +#define CIFS_DEF_ACTIMEO (1)
> +
> +/*
>   * MAX_REQ is the maximum number of requests that WE will send
>   * on one socket concurrently. It also matches the most common
>   * value of max multiplex returned by servers.  We may
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 251a17c..7f92328 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,11 @@ 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)
> +		cifs_sb->actimeo = pvolume_info->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)))

	Problem:

	Suppose (cifs_i->time + (cifs_sb->actimeo * HZ)) carries you
	past two sign bit changes. At that point, it may look like
	the resulting time is in the past, not the future.

	You actually do need to impose a limit here. I suggest looking
	very closely at time_before/time_after macros. It'll also be
	easier to deal storing actimeo in terms of jiffies instead of
	seconds.

	Finally, this value needs to be displayed in /proc/mounts.

>  		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;
>  


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

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

[Suresh Jayaraman]

On 11/18/2010 12:09 AM, Jeff Layton wrote:
> On Wed, 17 Nov 2010 23:53:34 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> 
>> Currently, the attribute cache timeout for CIFS is hardcoded to 1 second. This
>> means that the client might have to issue a QPATHINFO/QFILEINFO call every 1
>> second to verify if something has changes, which seems too expensive. On the
>> other hand, if the timeout is hardcoded to a higher value, workloads that
>> expect strict cache coherency might see unexpected results.
>>
>> Making attribute cache timeout as a tunable will allow us to make a tradeoff
>> between performance and cache metadata correctness depending on the
>> application/workload needs.
>>
>> Add 'actimeo' tunable that can be used to tune the attribute cache timeout.
>> The default timeout is set to 1 second.
>>
>> Also, display actimeo option value in /proc/mounts.
>>
>> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
>> ---
>>  fs/cifs/cifs_fs_sb.h |    1 +
>>  fs/cifs/cifsfs.c     |    1 +
>>  fs/cifs/cifsglob.h   |    5 +++++
>>  fs/cifs/connect.c    |   10 ++++++++++
>>  fs/cifs/inode.c      |    6 +++---
>>  5 files changed, 20 insertions(+), 3 deletions(-)
>>
>> 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)))
> 
> 	Problem:
> 
> 	Suppose (cifs_i->time + (cifs_sb->actimeo * HZ)) carries you
> 	past two sign bit changes. At that point, it may look like
> 	the resulting time is in the past, not the future.

Good catch.

> 	You actually do need to impose a limit here. I suggest looking
> 	very closely at time_before/time_after macros. It'll also be
> 	easier to deal storing actimeo in terms of jiffies instead of
> 	seconds.

> 	Finally, this value needs to be displayed in /proc/mounts.
> 

Did you mean displaying values in jiffies in /proc/mounts? Wouldn't it
be confusing for users to see such a higher numbers (usually) when they
had used a simple 2 digit number for e.g. during mount?



-- 
Suresh Jayaraman

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

[Jeff Layton]

On Thu, 18 Nov 2010 00:46:08 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 11/18/2010 12:09 AM, Jeff Layton wrote:
> > On Wed, 17 Nov 2010 23:53:34 +0530
> > Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> > 
> >> Currently, the attribute cache timeout for CIFS is hardcoded to 1 second. This
> >> means that the client might have to issue a QPATHINFO/QFILEINFO call every 1
> >> second to verify if something has changes, which seems too expensive. On the
> >> other hand, if the timeout is hardcoded to a higher value, workloads that
> >> expect strict cache coherency might see unexpected results.
> >>
> >> Making attribute cache timeout as a tunable will allow us to make a tradeoff
> >> between performance and cache metadata correctness depending on the
> >> application/workload needs.
> >>
> >> Add 'actimeo' tunable that can be used to tune the attribute cache timeout.
> >> The default timeout is set to 1 second.
> >>
> >> Also, display actimeo option value in /proc/mounts.
> >>
> >> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
> >> ---
> >>  fs/cifs/cifs_fs_sb.h |    1 +
> >>  fs/cifs/cifsfs.c     |    1 +
> >>  fs/cifs/cifsglob.h   |    5 +++++
> >>  fs/cifs/connect.c    |   10 ++++++++++
> >>  fs/cifs/inode.c      |    6 +++---
> >>  5 files changed, 20 insertions(+), 3 deletions(-)
> >>
> >> 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)))
> > 
> > 	Problem:
> > 
> > 	Suppose (cifs_i->time + (cifs_sb->actimeo * HZ)) carries you
> > 	past two sign bit changes. At that point, it may look like
> > 	the resulting time is in the past, not the future.
> 
> Good catch.
> 
> > 	You actually do need to impose a limit here. I suggest looking
> > 	very closely at time_before/time_after macros. It'll also be
> > 	easier to deal storing actimeo in terms of jiffies instead of
> > 	seconds.
> 
> > 	Finally, this value needs to be displayed in /proc/mounts.
> > 
> 
> Did you mean displaying values in jiffies in /proc/mounts? Wouldn't it
> be confusing for users to see such a higher numbers (usually) when they
> had used a simple 2 digit number for e.g. during mount?
> 
> 
> 

No, you'll want to display it in seconds in /proc/mounts so that it
matches the option that the user specified. So there you'll need to
display (cifs_sb->actimeo / HZ).

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

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

[Steve French]

On Wed, Nov 17, 2010 at 1:16 PM, Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> On 11/18/2010 12:09 AM, Jeff Layton wrote:
>> On Wed, 17 Nov 2010 23:53:34 +0530
>> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>>
>>> Currently, the attribute cache timeout for CIFS is hardcoded to 1 second. This
>>> means that the client might have to issue a QPATHINFO/QFILEINFO call every 1
>>> second to verify if something has changes, which seems too expensive. On the
>>> other hand, if the timeout is hardcoded to a higher value, workloads that
>>> expect strict cache coherency might see unexpected results.
>>>
>>> Making attribute cache timeout as a tunable will allow us to make a tradeoff
>>> between performance and cache metadata correctness depending on the
>>> application/workload needs.
>>>
>>> Add 'actimeo' tunable that can be used to tune the attribute cache timeout.
>>> The default timeout is set to 1 second.
>>>
>>> Also, display actimeo option value in /proc/mounts.
>>>
>>> Cc: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
>>> ---
>>>  fs/cifs/cifs_fs_sb.h |    1 +
>>>  fs/cifs/cifsfs.c     |    1 +
>>>  fs/cifs/cifsglob.h   |    5 +++++
>>>  fs/cifs/connect.c    |   10 ++++++++++
>>>  fs/cifs/inode.c      |    6 +++---
>>>  5 files changed, 20 insertions(+), 3 deletions(-)
>>>
>>> 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)))
>>
>>       Problem:
>>
>>       Suppose (cifs_i->time + (cifs_sb->actimeo * HZ)) carries you
>>       past two sign bit changes. At that point, it may look like
>>       the resulting time is in the past, not the future.
>
> Good catch.
>
>>       You actually do need to impose a limit here. I suggest looking
>>       very closely at time_before/time_after macros. It'll also be
>>       easier to deal storing actimeo in terms of jiffies instead of
>>       seconds.
>
>>       Finally, this value needs to be displayed in /proc/mounts.
>>
>
> Did you mean displaying values in jiffies in /proc/mounts? Wouldn't it
> be confusing for users to see such a higher numbers (usually) when they
> had used a simple 2 digit number for e.g. during mount?

That is a good question - seems like we ought to allow setting more granular
timeouts (ie jiffies) but nfs actimeo assumes the value is in seconds
by default.
I am fine with allowing actimeo to be specified more than one way (seconds vs
milliseconds or seconds vs. jiffies) with suffix or prefix or keyword variant.

Probably should display it in seconds (or milliseconds) - jiffies
would be confusing
(I had trouble finding a way - short of looking at .config - what
jiffies equivalent
in seconds is)



-- 
Thanks,

Steve

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

[Steve French]

On Wed, Nov 17, 2010 at 1:40 PM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Nov 17, 2010 at 1:16 PM, Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>> On 11/18/2010 12:09 AM, Jeff Layton wrote:
>>> On Wed, 17 Nov 2010 23:53:34 +0530
>>> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>>>
>>>> Currently, the attribute cache timeout for CIFS is hardcoded to 1 second. This
>>>> means that the client might have to issue a QPATHINFO/QFILEINFO call every 1
>>>> second to verify if something has changes, which seems too expensive. On the
>>>> other hand, if the timeout is hardcoded to a higher value, workloads that
>>>> expect strict cache coherency might see unexpected results.
>>>>
>>>> Making attribute cache timeout as a tunable will allow us to make a tradeoff
>>>> between performance and cache metadata correctness depending on the
>>>> application/workload needs.
>>>>
>>>> Add 'actimeo' tunable that can be used to tune the attribute cache timeout.
>>>> The default timeout is set to 1 second.
>>>>
>>>> Also, display actimeo option value in /proc/mounts.
>>>       You actually do need to impose a limit here. I suggest looking
>>>       very closely at time_before/time_after macros. It'll also be
>>>       easier to deal storing actimeo in terms of jiffies instead of
>>>       seconds.
>>
>>>       Finally, this value needs to be displayed in /proc/mounts.
>>>
>>
>> Did you mean displaying values in jiffies in /proc/mounts? Wouldn't it
>> be confusing for users to see such a higher numbers (usually) when they
>> had used a simple 2 digit number for e.g. during mount?
>
> That is a good question - seems like we ought to allow setting more granular
> timeouts (ie jiffies) but nfs actimeo assumes the value is in seconds
> by default.
> I am fine with allowing actimeo to be specified more than one way (seconds vs
> milliseconds or seconds vs. jiffies) with suffix or prefix or keyword variant.

This assumes there are apps that might someday need stricter
guarantees (sub 1 second - e.g. 15 per second) but not disable metadata caching.
If we don't think that would ever happen - then I am fine with seconds
everywhere
(actimeo only in seconds, /proc display only in seconds).

> Probably should display it in seconds (or milliseconds) - jiffies
> would be confusing
> (I had trouble finding a way - short of looking at .config - what
> jiffies equivalent
> in seconds is)



-- 
Thanks,

Steve

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

[Jeff Layton]

On Wed, 17 Nov 2010 13:43:52 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Nov 17, 2010 at 1:40 PM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Wed, Nov 17, 2010 at 1:16 PM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
> >> On 11/18/2010 12:09 AM, Jeff Layton wrote:
> >>> On Wed, 17 Nov 2010 23:53:34 +0530
> >>> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> >>>
> >>>> Currently, the attribute cache timeout for CIFS is hardcoded to 1 second. This
> >>>> means that the client might have to issue a QPATHINFO/QFILEINFO call every 1
> >>>> second to verify if something has changes, which seems too expensive. On the
> >>>> other hand, if the timeout is hardcoded to a higher value, workloads that
> >>>> expect strict cache coherency might see unexpected results.
> >>>>
> >>>> Making attribute cache timeout as a tunable will allow us to make a tradeoff
> >>>> between performance and cache metadata correctness depending on the
> >>>> application/workload needs.
> >>>>
> >>>> Add 'actimeo' tunable that can be used to tune the attribute cache timeout.
> >>>> The default timeout is set to 1 second.
> >>>>
> >>>> Also, display actimeo option value in /proc/mounts.
> >>>       You actually do need to impose a limit here. I suggest looking
> >>>       very closely at time_before/time_after macros. It'll also be
> >>>       easier to deal storing actimeo in terms of jiffies instead of
> >>>       seconds.
> >>
> >>>       Finally, this value needs to be displayed in /proc/mounts.
> >>>
> >>
> >> Did you mean displaying values in jiffies in /proc/mounts? Wouldn't it
> >> be confusing for users to see such a higher numbers (usually) when they
> >> had used a simple 2 digit number for e.g. during mount?
> >
> > That is a good question - seems like we ought to allow setting more granular
> > timeouts (ie jiffies) but nfs actimeo assumes the value is in seconds
> > by default.
> > I am fine with allowing actimeo to be specified more than one way (seconds vs
> > milliseconds or seconds vs. jiffies) with suffix or prefix or keyword variant.
> 
> This assumes there are apps that might someday need stricter
> guarantees (sub 1 second - e.g. 15 per second) but not disable metadata caching.
> If we don't think that would ever happen - then I am fine with seconds
> everywhere
> (actimeo only in seconds, /proc display only in seconds).
> 

I don't see any real need for that level of granularity. 1s should be
enough for anyone. :)

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