[PATCH] convert revalidate of directories to using directory metadata cache timeout

[PATCH] convert revalidate of directories to using directory metadata cache timeout

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

nfs and cifs on Linux currently have a mount parameter "actimeo" to
control metadata (attribute) caching but cifs does not have additional
mount parameters to allow distinguishing between caching directory
metadata (e.g. needed to revalidate paths) and that for files.

Add new mount parameter "acdirmax" to allow caching metadata for
directories more loosely than file data.  NFS adjusts metadata
caching from acdirmin to acdirmax (and another two mount parms
for files) but to reduce complexity, it is safer to just introduce
the one mount parm to allow caching directories longer (30 seconds
vs. the 1 second default for file metadata) which is still more
conservative than other Linux filesystems (e.g. NFS sets acdirmax
to 60 seconds)

-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-Add-new-mount-parameter-acdirmax-to-allow-cachi.patch --]
[-- Type: text/x-patch, Size: 4916 bytes --]

From 505d2041479f568c4f4b9a170c484f866bc72c57 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 23 Feb 2021 15:50:57 -0600
Subject: [PATCH 1/2] cifs: Add new mount parameter "acdirmax" to allow caching
 directory metadata

nfs and cifs on Linux currently have a mount parameter "actimeo" to control
metadata (attribute) caching but cifs does not have additional mount
parameters to allow distinguishing between caching directory metadata
(e.g. needed to revalidate paths) and that for files.

Add new mount parameter "acdirmax" to allow caching metadata for
directories more loosely than file data.  NFS adjusts metadata
caching from acdirmin to acdirmax (and another two mount parms
for files) but to reduce complexity, it is safer to just introduce
the one mount parm to allow caching directories longer (30 seconds
vs. the 1 second default for file metadata) which is still more
conservative than other Linux filesystems (e.g. NFS sets acdirmax
to 60 seconds)

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c     | 3 ++-
 fs/cifs/cifsglob.h   | 8 +++++++-
 fs/cifs/connect.c    | 2 ++
 fs/cifs/fs_context.c | 9 +++++++++
 fs/cifs/fs_context.h | 4 +++-
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 6f33ff3f625f..4e0b0b26e844 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -637,8 +637,9 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
 		seq_printf(s, ",snapshot=%llu", tcon->snapshot_time);
 	if (tcon->handle_timeout)
 		seq_printf(s, ",handletimeout=%u", tcon->handle_timeout);
-	/* convert actimeo and display it in seconds */
+	/* convert actimeo and directory attribute timeout and display in seconds */
 	seq_printf(s, ",actimeo=%lu", cifs_sb->ctx->actimeo / HZ);
+	seq_printf(s, ",acdirmax=%lu", cifs_sb->ctx->acdirmax / HZ);
 
 	if (tcon->ses->chan_max > 1)
 		seq_printf(s, ",multichannel,max_channels=%zu",
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 3de3c5908a72..9a14f6f6ebd8 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -50,11 +50,17 @@
 #define CIFS_MIN_RCV_POOL 4
 
 #define MAX_REOPEN_ATT	5 /* these many maximum attempts to reopen a file */
+
 /*
- * default attribute cache timeout (jiffies)
+ * default attribute cache timeout for files (jiffies)
  */
 #define CIFS_DEF_ACTIMEO (1 * HZ)
 
+/*
+ * default attribute cache timeout for directories (jiffies)
+ */
+#define CIFS_DEF_ACDIRMAX (30 * HZ)
+
 /*
  * max attribute cache timeout (jiffies) - 2^30
  */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index cd6dbeaf2166..a9dc39aee9f4 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2278,6 +2278,8 @@ compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
 
 	if (old->ctx->actimeo != new->ctx->actimeo)
 		return 0;
+	if (old->ctx->acdirmax != new->ctx->acdirmax)
+		return 0;
 
 	return 1;
 }
diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index 7d04f2255624..555969c8d586 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -140,6 +140,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
 	fsparam_u32("rsize", Opt_rsize),
 	fsparam_u32("wsize", Opt_wsize),
 	fsparam_u32("actimeo", Opt_actimeo),
+	fsparam_u32("acdirmax", Opt_acdirmax),
 	fsparam_u32("echo_interval", Opt_echo_interval),
 	fsparam_u32("max_credits", Opt_max_credits),
 	fsparam_u32("handletimeout", Opt_handletimeout),
@@ -936,6 +937,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 			goto cifs_parse_mount_err;
 		}
 		break;
+	case Opt_acdirmax:
+		ctx->acdirmax = HZ * result.uint_32;
+		if (ctx->acdirmax > CIFS_MAX_ACTIMEO) {
+			cifs_dbg(VFS, "acdirmax too large\n");
+			goto cifs_parse_mount_err;
+		}
+		break;
 	case Opt_echo_interval:
 		ctx->echo_interval = result.uint_32;
 		break;
@@ -1362,6 +1370,7 @@ int smb3_init_fs_context(struct fs_context *fc)
 	ctx->strict_io = true;
 
 	ctx->actimeo = CIFS_DEF_ACTIMEO;
+	ctx->acdirmax = CIFS_DEF_ACDIRMAX;
 
 	/* Most clients set timeout to 0, allows server to use its default */
 	ctx->handle_timeout = 0; /* See MS-SMB2 spec section 2.2.14.2.12 */
diff --git a/fs/cifs/fs_context.h b/fs/cifs/fs_context.h
index 1c44a460e2c0..472372fec4e9 100644
--- a/fs/cifs/fs_context.h
+++ b/fs/cifs/fs_context.h
@@ -118,6 +118,7 @@ enum cifs_param {
 	Opt_rsize,
 	Opt_wsize,
 	Opt_actimeo,
+	Opt_acdirmax,
 	Opt_echo_interval,
 	Opt_max_credits,
 	Opt_snapshot,
@@ -232,7 +233,8 @@ struct smb3_fs_context {
 	unsigned int wsize;
 	unsigned int min_offload;
 	bool sockopt_tcp_nodelay:1;
-	unsigned long actimeo; /* attribute cache timeout (jiffies) */
+	unsigned long actimeo; /* attribute cache timeout for files (jiffies) */
+	unsigned long acdirmax; /* attribute cache timeout for directories (jiffies) */
 	struct smb_version_operations *ops;
 	struct smb_version_values *vals;
 	char *prepath;
-- 
2.27.0


[-- Attachment #3: 0002-cifs-convert-revalidate-of-directories-to-using-dire.patch --]
[-- Type: text/x-patch, Size: 1700 bytes --]

From 79f731e14559be57d48647d22283b93600caf192 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 23 Feb 2021 16:16:09 -0600
Subject: [PATCH 2/2] cifs: convert revalidate of directories to using
 directory metadata cache timeout

The new optional mount parm, "acdirmax" allows caching the metadata
for a directory longer than file metadata, which can be very helpful
for performance.  Convert cifs_inode_needs_reval to check acdirmax
for revalidating directory metadata (actimeo is checked for files)

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/inode.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index a83b3a8ffaac..cfd31cc4520f 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2198,12 +2198,23 @@ cifs_inode_needs_reval(struct inode *inode)
 	if (!lookupCacheEnabled)
 		return true;
 
-	if (!cifs_sb->ctx->actimeo)
-		return true;
-
-	if (!time_in_range(jiffies, cifs_i->time,
-				cifs_i->time + cifs_sb->ctx->actimeo))
-		return true;
+	/*
+	 * depending on inode type, check if attribute caching disabled for
+	 * files or directories
+	 */
+	if (S_ISDIR(inode->i_mode)) {
+		if (!cifs_sb->ctx->acdirmax)
+			return true;
+		if (!time_in_range(jiffies, cifs_i->time,
+				   cifs_i->time + cifs_sb->ctx->acdirmax))
+			return true;
+	} else { /* file */
+		if (!cifs_sb->ctx->actimeo)
+			return true;
+		if (!time_in_range(jiffies, cifs_i->time,
+				   cifs_i->time + cifs_sb->ctx->actimeo))
+			return true;
+	}
 
 	/* hardlinked files w/ noserverino get "special" treatment */
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) &&
-- 
2.27.0

Re: [PATCH] convert revalidate of directories to using directory metadata cache timeout

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 5517 bytes --]

Updated version incorporates Ronnie's suggestion of leaving the
default (for directory caching) the same as it is today, 1 second, to
avoid
unnecessary risk.   Most users can safely improve performance by
mounting with acdirmax to a higher value (e.g. 60 seconds as NFS
defaults to).

nfs and cifs on Linux currently have a mount parameter "actimeo" to control
metadata (attribute) caching but cifs does not have additional mount
parameters to allow distinguishing between caching directory metadata
(e.g. needed to revalidate paths) and that for files.

Add new mount parameter "acdirmax" to allow caching metadata for
directories more loosely than file data.  NFS adjusts metadata
caching from acdirmin to acdirmax (and another two mount parms
for files) but to reduce complexity, it is safer to just introduce
the one mount parm to allow caching directories longer. The
defaults for acdirmax and actimeo (for cifs.ko) are conservative,
1 second (NFS defaults acdirmax to 60 seconds). For many workloads,
setting acdirmax to a higher value is safe and will improve
performance.  This patch leaves unchanged the default values
for caching metadata for files and directories but gives the
user more flexibility in adjusting them safely for their workload
via the new mount parm.

Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cifsfs.c     | 3 ++-
 fs/cifs/cifsglob.h   | 8 +++++++-
 fs/cifs/connect.c    | 2 ++
 fs/cifs/fs_context.c | 9 +++++++++
 fs/cifs/fs_context.h | 4 +++-
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 6f33ff3f625f..4e0b0b26e844 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -637,8 +637,9 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
  seq_printf(s, ",snapshot=%llu", tcon->snapshot_time);
  if (tcon->handle_timeout)
  seq_printf(s, ",handletimeout=%u", tcon->handle_timeout);
- /* convert actimeo and display it in seconds */
+ /* convert actimeo and directory attribute timeout and display in seconds */
  seq_printf(s, ",actimeo=%lu", cifs_sb->ctx->actimeo / HZ);
+ seq_printf(s, ",acdirmax=%lu", cifs_sb->ctx->acdirmax / HZ);

  if (tcon->ses->chan_max > 1)
  seq_printf(s, ",multichannel,max_channels=%zu",
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index cd6dbeaf2166..a9dc39aee9f4 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2278,6 +2278,8 @@ compare_mount_options(struct super_block *sb,
struct cifs_mnt_data *mnt_data)

  if (old->ctx->actimeo != new->ctx->actimeo)
  return 0;
+ if (old->ctx->acdirmax != new->ctx->acdirmax)
+ return 0;

  return 1;
 }
diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index 7d04f2255624..555969c8d586 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -140,6 +140,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
  fsparam_u32("rsize", Opt_rsize),
  fsparam_u32("wsize", Opt_wsize),
  fsparam_u32("actimeo", Opt_actimeo),
+ fsparam_u32("acdirmax", Opt_acdirmax),
  fsparam_u32("echo_interval", Opt_echo_interval),
  fsparam_u32("max_credits", Opt_max_credits),
  fsparam_u32("handletimeout", Opt_handletimeout),
@@ -936,6 +937,13 @@ static int smb3_fs_context_parse_param(struct
fs_context *fc,
  goto cifs_parse_mount_err;
  }
  break;
+ case Opt_acdirmax:
+ ctx->acdirmax = HZ * result.uint_32;
+ if (ctx->acdirmax > CIFS_MAX_ACTIMEO) {
+ cifs_dbg(VFS, "acdirmax too large\n");
+ goto cifs_parse_mount_err;
+ }
+ break;
  case Opt_echo_interval:
  ctx->echo_interval = result.uint_32;
  break;
@@ -1362,6 +1370,7 @@ int smb3_init_fs_context(struct fs_context *fc)
  ctx->strict_io = true;

  ctx->actimeo = CIFS_DEF_ACTIMEO;
+ ctx->acdirmax = CIFS_DEF_ACTIMEO;

  /* Most clients set timeout to 0, allows server to use its default */
  ctx->handle_timeout = 0; /* See MS-SMB2 spec section 2.2.14.2.12 */
diff --git a/fs/cifs/fs_context.h b/fs/cifs/fs_context.h
index 1c44a460e2c0..472372fec4e9 100644
--- a/fs/cifs/fs_context.h
+++ b/fs/cifs/fs_context.h
@@ -118,6 +118,7 @@ enum cifs_param {
  Opt_rsize,
  Opt_wsize,
  Opt_actimeo,
+ Opt_acdirmax,
  Opt_echo_interval,
  Opt_max_credits,
  Opt_snapshot,
@@ -232,7 +233,8 @@ struct smb3_fs_context {
  unsigned int wsize;
  unsigned int min_offload;
  bool sockopt_tcp_nodelay:1;
- unsigned long actimeo; /* attribute cache timeout (jiffies) */
+ unsigned long actimeo; /* attribute cache timeout for files (jiffies) */
+ unsigned long acdirmax; /* attribute cache timeout for directories
(jiffies) */
  struct smb_version_operations *ops;
  struct smb_version_values *vals;
  char *prepath;

On Tue, Feb 23, 2021 at 4:22 PM Steve French <smfrench@gmail.com> wrote:
>
> nfs and cifs on Linux currently have a mount parameter "actimeo" to
> control metadata (attribute) caching but cifs does not have additional
> mount parameters to allow distinguishing between caching directory
> metadata (e.g. needed to revalidate paths) and that for files.
>
> Add new mount parameter "acdirmax" to allow caching metadata for
> directories more loosely than file data.  NFS adjusts metadata
> caching from acdirmin to acdirmax (and another two mount parms
> for files) but to reduce complexity, it is safer to just introduce
> the one mount parm to allow caching directories longer (30 seconds
> vs. the 1 second default for file metadata) which is still more
> conservative than other Linux filesystems (e.g. NFS sets acdirmax
> to 60 seconds)
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-Add-new-mount-parameter-acdirmax-to-allow-cachi.patch --]
[-- Type: text/x-patch, Size: 4631 bytes --]

From 505d2041479f568c4f4b9a170c484f866bc72c57 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 23 Feb 2021 15:50:57 -0600
Subject: [PATCH 1/2] cifs: Add new mount parameter "acdirmax" to allow caching
 directory metadata

nfs and cifs on Linux currently have a mount parameter "actimeo" to control
metadata (attribute) caching but cifs does not have additional mount
parameters to allow distinguishing between caching directory metadata
(e.g. needed to revalidate paths) and that for files.

Add new mount parameter "acdirmax" to allow caching metadata for
directories more loosely than file data.  NFS adjusts metadata
caching from acdirmin to acdirmax (and another two mount parms
for files) but to reduce complexity, it is safer to just introduce
the one mount parm to allow caching directories longer. The
defaults for acdirmax and actimeo (for cifs.ko) are conservative,
1 second (NFS defaults acdirmax to 60 seconds). For many workloads,
setting acdirmax to a higher value is safe and will improve
performance.  This patch leaves unchanged the default values
for caching metadata for files and directories but gives the
user more flexibility in adjusting them safely for their workload
via the new mount parm.

Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cifsfs.c     | 3 ++-
 fs/cifs/cifsglob.h   | 8 +++++++-
 fs/cifs/connect.c    | 2 ++
 fs/cifs/fs_context.c | 9 +++++++++
 fs/cifs/fs_context.h | 4 +++-
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 6f33ff3f625f..4e0b0b26e844 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -637,8 +637,9 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
 		seq_printf(s, ",snapshot=%llu", tcon->snapshot_time);
 	if (tcon->handle_timeout)
 		seq_printf(s, ",handletimeout=%u", tcon->handle_timeout);
-	/* convert actimeo and display it in seconds */
+	/* convert actimeo and directory attribute timeout and display in seconds */
 	seq_printf(s, ",actimeo=%lu", cifs_sb->ctx->actimeo / HZ);
+	seq_printf(s, ",acdirmax=%lu", cifs_sb->ctx->acdirmax / HZ);
 
 	if (tcon->ses->chan_max > 1)
 		seq_printf(s, ",multichannel,max_channels=%zu",
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index cd6dbeaf2166..a9dc39aee9f4 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2278,6 +2278,8 @@ compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
 
 	if (old->ctx->actimeo != new->ctx->actimeo)
 		return 0;
+	if (old->ctx->acdirmax != new->ctx->acdirmax)
+		return 0;
 
 	return 1;
 }
diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index 7d04f2255624..555969c8d586 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -140,6 +140,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
 	fsparam_u32("rsize", Opt_rsize),
 	fsparam_u32("wsize", Opt_wsize),
 	fsparam_u32("actimeo", Opt_actimeo),
+	fsparam_u32("acdirmax", Opt_acdirmax),
 	fsparam_u32("echo_interval", Opt_echo_interval),
 	fsparam_u32("max_credits", Opt_max_credits),
 	fsparam_u32("handletimeout", Opt_handletimeout),
@@ -936,6 +937,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 			goto cifs_parse_mount_err;
 		}
 		break;
+	case Opt_acdirmax:
+		ctx->acdirmax = HZ * result.uint_32;
+		if (ctx->acdirmax > CIFS_MAX_ACTIMEO) {
+			cifs_dbg(VFS, "acdirmax too large\n");
+			goto cifs_parse_mount_err;
+		}
+		break;
 	case Opt_echo_interval:
 		ctx->echo_interval = result.uint_32;
 		break;
@@ -1362,6 +1370,7 @@ int smb3_init_fs_context(struct fs_context *fc)
 	ctx->strict_io = true;
 
 	ctx->actimeo = CIFS_DEF_ACTIMEO;
+	ctx->acdirmax = CIFS_DEF_ACTIMEO;
 
 	/* Most clients set timeout to 0, allows server to use its default */
 	ctx->handle_timeout = 0; /* See MS-SMB2 spec section 2.2.14.2.12 */
diff --git a/fs/cifs/fs_context.h b/fs/cifs/fs_context.h
index 1c44a460e2c0..472372fec4e9 100644
--- a/fs/cifs/fs_context.h
+++ b/fs/cifs/fs_context.h
@@ -118,6 +118,7 @@ enum cifs_param {
 	Opt_rsize,
 	Opt_wsize,
 	Opt_actimeo,
+	Opt_acdirmax,
 	Opt_echo_interval,
 	Opt_max_credits,
 	Opt_snapshot,
@@ -232,7 +233,8 @@ struct smb3_fs_context {
 	unsigned int wsize;
 	unsigned int min_offload;
 	bool sockopt_tcp_nodelay:1;
-	unsigned long actimeo; /* attribute cache timeout (jiffies) */
+	unsigned long actimeo; /* attribute cache timeout for files (jiffies) */
+	unsigned long acdirmax; /* attribute cache timeout for directories (jiffies) */
 	struct smb_version_operations *ops;
 	struct smb_version_values *vals;
 	char *prepath;
-- 
2.27.0


[-- Attachment #3: 0002-cifs-convert-revalidate-of-directories-to-using-dire.patch --]
[-- Type: text/x-patch, Size: 1751 bytes --]

From 04dcd66227ff36784e6141782afcc5508bae1ba6 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 23 Feb 2021 16:16:09 -0600
Subject: [PATCH 2/2] cifs: convert revalidate of directories to using
 directory metadata cache timeout

The new optional mount parm, "acdirmax" allows caching the metadata
for a directory longer than file metadata, which can be very helpful
for performance.  Convert cifs_inode_needs_reval to check acdirmax
for revalidating directory metadata (actimeo is checked for files)

Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/inode.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index a83b3a8ffaac..cfd31cc4520f 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2198,12 +2198,23 @@ cifs_inode_needs_reval(struct inode *inode)
 	if (!lookupCacheEnabled)
 		return true;
 
-	if (!cifs_sb->ctx->actimeo)
-		return true;
-
-	if (!time_in_range(jiffies, cifs_i->time,
-				cifs_i->time + cifs_sb->ctx->actimeo))
-		return true;
+	/*
+	 * depending on inode type, check if attribute caching disabled for
+	 * files or directories
+	 */
+	if (S_ISDIR(inode->i_mode)) {
+		if (!cifs_sb->ctx->acdirmax)
+			return true;
+		if (!time_in_range(jiffies, cifs_i->time,
+				   cifs_i->time + cifs_sb->ctx->acdirmax))
+			return true;
+	} else { /* file */
+		if (!cifs_sb->ctx->actimeo)
+			return true;
+		if (!time_in_range(jiffies, cifs_i->time,
+				   cifs_i->time + cifs_sb->ctx->actimeo))
+			return true;
+	}
 
 	/* hardlinked files w/ noserverino get "special" treatment */
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) &&
-- 
2.27.0

Re: [PATCH] convert revalidate of directories to using directory metadata cache timeout

[Tom Talpey]

On 2/23/2021 8:03 PM, Steve French wrote:
> Updated version incorporates Ronnie's suggestion of leaving the
> default (for directory caching) the same as it is today, 1 second, to
> avoid
> unnecessary risk.   Most users can safely improve performance by
> mounting with acdirmax to a higher value (e.g. 60 seconds as NFS
> defaults to).
> 
> nfs and cifs on Linux currently have a mount parameter "actimeo" to control
> metadata (attribute) caching but cifs does not have additional mount
> parameters to allow distinguishing between caching directory metadata
> (e.g. needed to revalidate paths) and that for files.

The behaviors seem to be slightly different with this change.
With NFS, the actimeo option overrides the four min/max options,
and by default the directory ac timers range between 30 and 60.

The CIFS code I see below seems to completely separate actimeo
and acdirmax, and if not set, uses the historic 1 second value.
That's fine, but it's completely different from NFS. Shouldn't we
use a different mount option, to avoid confusing the admin?

> +	/*
> +	 * depending on inode type, check if attribute caching disabled for
> +	 * files or directories
> +	 */
> +	if (S_ISDIR(inode->i_mode)) {
> +		if (!cifs_sb->ctx->acdirmax)
> +			return true;
> +		if (!time_in_range(jiffies, cifs_i->time,
> +				   cifs_i->time + cifs_sb->ctx->acdirmax))
> +			return true;
> +	} else { /* file */
> +		if (!cifs_sb->ctx->actimeo)
> +			return true;
> +		if (!time_in_range(jiffies, cifs_i->time,
> +				   cifs_i->time + cifs_sb->ctx->actimeo))
> +			return true;
> +	}



> Add new mount parameter "acdirmax" to allow caching metadata for
> directories more loosely than file data.  NFS adjusts metadata
> caching from acdirmin to acdirmax (and another two mount parms
> for files) but to reduce complexity, it is safer to just introduce
> the one mount parm to allow caching directories longer. The
> defaults for acdirmax and actimeo (for cifs.ko) are conservative,
> 1 second (NFS defaults acdirmax to 60 seconds). For many workloads,
> setting acdirmax to a higher value is safe and will improve
> performance.  This patch leaves unchanged the default values
> for caching metadata for files and directories but gives the
> user more flexibility in adjusting them safely for their workload
> via the new mount parm.
> 
> Signed-off-by: Steve French <stfrench@microsoft.com>
> Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>   fs/cifs/cifsfs.c     | 3 ++-
>   fs/cifs/cifsglob.h   | 8 +++++++-
>   fs/cifs/connect.c    | 2 ++
>   fs/cifs/fs_context.c | 9 +++++++++
>   fs/cifs/fs_context.h | 4 +++-
>   5 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 6f33ff3f625f..4e0b0b26e844 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -637,8 +637,9 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
>    seq_printf(s, ",snapshot=%llu", tcon->snapshot_time);
>    if (tcon->handle_timeout)
>    seq_printf(s, ",handletimeout=%u", tcon->handle_timeout);
> - /* convert actimeo and display it in seconds */
> + /* convert actimeo and directory attribute timeout and display in seconds */
>    seq_printf(s, ",actimeo=%lu", cifs_sb->ctx->actimeo / HZ);
> + seq_printf(s, ",acdirmax=%lu", cifs_sb->ctx->acdirmax / HZ);
> 
>    if (tcon->ses->chan_max > 1)
>    seq_printf(s, ",multichannel,max_channels=%zu",
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index cd6dbeaf2166..a9dc39aee9f4 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2278,6 +2278,8 @@ compare_mount_options(struct super_block *sb,
> struct cifs_mnt_data *mnt_data)
> 
>    if (old->ctx->actimeo != new->ctx->actimeo)
>    return 0;
> + if (old->ctx->acdirmax != new->ctx->acdirmax)
> + return 0;
> 
>    return 1;
>   }
> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> index 7d04f2255624..555969c8d586 100644
> --- a/fs/cifs/fs_context.c
> +++ b/fs/cifs/fs_context.c
> @@ -140,6 +140,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
>    fsparam_u32("rsize", Opt_rsize),
>    fsparam_u32("wsize", Opt_wsize),
>    fsparam_u32("actimeo", Opt_actimeo),
> + fsparam_u32("acdirmax", Opt_acdirmax),
>    fsparam_u32("echo_interval", Opt_echo_interval),
>    fsparam_u32("max_credits", Opt_max_credits),
>    fsparam_u32("handletimeout", Opt_handletimeout),
> @@ -936,6 +937,13 @@ static int smb3_fs_context_parse_param(struct
> fs_context *fc,
>    goto cifs_parse_mount_err;
>    }
>    break;
> + case Opt_acdirmax:
> + ctx->acdirmax = HZ * result.uint_32;
> + if (ctx->acdirmax > CIFS_MAX_ACTIMEO) {
> + cifs_dbg(VFS, "acdirmax too large\n");
> + goto cifs_parse_mount_err;
> + }
> + break;
>    case Opt_echo_interval:
>    ctx->echo_interval = result.uint_32;
>    break;
> @@ -1362,6 +1370,7 @@ int smb3_init_fs_context(struct fs_context *fc)
>    ctx->strict_io = true;
> 
>    ctx->actimeo = CIFS_DEF_ACTIMEO;
> + ctx->acdirmax = CIFS_DEF_ACTIMEO;
> 
>    /* Most clients set timeout to 0, allows server to use its default */
>    ctx->handle_timeout = 0; /* See MS-SMB2 spec section 2.2.14.2.12 */
> diff --git a/fs/cifs/fs_context.h b/fs/cifs/fs_context.h
> index 1c44a460e2c0..472372fec4e9 100644
> --- a/fs/cifs/fs_context.h
> +++ b/fs/cifs/fs_context.h
> @@ -118,6 +118,7 @@ enum cifs_param {
>    Opt_rsize,
>    Opt_wsize,
>    Opt_actimeo,
> + Opt_acdirmax,
>    Opt_echo_interval,
>    Opt_max_credits,
>    Opt_snapshot,
> @@ -232,7 +233,8 @@ struct smb3_fs_context {
>    unsigned int wsize;
>    unsigned int min_offload;
>    bool sockopt_tcp_nodelay:1;
> - unsigned long actimeo; /* attribute cache timeout (jiffies) */
> + unsigned long actimeo; /* attribute cache timeout for files (jiffies) */
> + unsigned long acdirmax; /* attribute cache timeout for directories
> (jiffies) */
>    struct smb_version_operations *ops;
>    struct smb_version_values *vals;
>    char *prepath;
> 
> On Tue, Feb 23, 2021 at 4:22 PM Steve French <smfrench@gmail.com> wrote:
>>
>> nfs and cifs on Linux currently have a mount parameter "actimeo" to
>> control metadata (attribute) caching but cifs does not have additional
>> mount parameters to allow distinguishing between caching directory
>> metadata (e.g. needed to revalidate paths) and that for files.
>>
>> Add new mount parameter "acdirmax" to allow caching metadata for
>> directories more loosely than file data.  NFS adjusts metadata
>> caching from acdirmin to acdirmax (and another two mount parms
>> for files) but to reduce complexity, it is safer to just introduce
>> the one mount parm to allow caching directories longer (30 seconds
>> vs. the 1 second default for file metadata) which is still more
>> conservative than other Linux filesystems (e.g. NFS sets acdirmax
>> to 60 seconds)
>>
>> --
>> Thanks,
>>
>> Steve
> 
> 
> 

Re: [PATCH] convert revalidate of directories to using directory metadata cache timeout

[Steve French]

On Wed, Feb 24, 2021 at 10:11 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 2/23/2021 8:03 PM, Steve French wrote:
> > Updated version incorporates Ronnie's suggestion of leaving the
> > default (for directory caching) the same as it is today, 1 second, to
> > avoid
> > unnecessary risk.   Most users can safely improve performance by
> > mounting with acdirmax to a higher value (e.g. 60 seconds as NFS
> > defaults to).
> >
> > nfs and cifs on Linux currently have a mount parameter "actimeo" to control
> > metadata (attribute) caching but cifs does not have additional mount
> > parameters to allow distinguishing between caching directory metadata
> > (e.g. needed to revalidate paths) and that for files.
>
> The behaviors seem to be slightly different with this change.
> With NFS, the actimeo option overrides the four min/max options,
> and by default the directory ac timers range between 30 and 60.
>
> The CIFS code I see below seems to completely separate actimeo
> and acdirmax, and if not set, uses the historic 1 second value.
> That's fine, but it's completely different from NFS. Shouldn't we
> use a different mount option, to avoid confusing the admin?

Ugh ... You are probably right.  I was trying to avoid two problems:
1) (a minor one) adding a second mount option rather than just one (to
solve the same problem).  But reducing confusion is worth an extra
mount option

2) how to avoid the user specifying *both* actimeo and acregmax -
which one 'wins' (presumably the last one in the mount line)
We could check for this and warn the user in mount.cifs so maybe not
important to worry about in the kernel though.

I will add the acregmax mount option and change actimeo to mean
    if (actimeo is set)
            acregmax = acdirmax = actimeo


-- 
Thanks,

Steve

Re: [PATCH] convert revalidate of directories to using directory metadata cache timeout

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

Added additional patch to add "acregmax" so now behavior is more similar to nfs.

"acregmax" changes file metadata caching timeout
"acdirmax" changes directory metadata caching
"actimeo" does what it did before - and changes both.

On Wed, Feb 24, 2021 at 11:31 AM Steve French <smfrench@gmail.com> wrote:
>
> On Wed, Feb 24, 2021 at 10:11 AM Tom Talpey <tom@talpey.com> wrote:
> >
> > On 2/23/2021 8:03 PM, Steve French wrote:
> > > Updated version incorporates Ronnie's suggestion of leaving the
> > > default (for directory caching) the same as it is today, 1 second, to
> > > avoid
> > > unnecessary risk.   Most users can safely improve performance by
> > > mounting with acdirmax to a higher value (e.g. 60 seconds as NFS
> > > defaults to).
> > >
> > > nfs and cifs on Linux currently have a mount parameter "actimeo" to control
> > > metadata (attribute) caching but cifs does not have additional mount
> > > parameters to allow distinguishing between caching directory metadata
> > > (e.g. needed to revalidate paths) and that for files.
> >
> > The behaviors seem to be slightly different with this change.
> > With NFS, the actimeo option overrides the four min/max options,
> > and by default the directory ac timers range between 30 and 60.
> >
> > The CIFS code I see below seems to completely separate actimeo
> > and acdirmax, and if not set, uses the historic 1 second value.
> > That's fine, but it's completely different from NFS. Shouldn't we
> > use a different mount option, to avoid confusing the admin?
>
> Ugh ... You are probably right.  I was trying to avoid two problems:
> 1) (a minor one) adding a second mount option rather than just one (to
> solve the same problem).  But reducing confusion is worth an extra
> mount option
>
> 2) how to avoid the user specifying *both* actimeo and acregmax -
> which one 'wins' (presumably the last one in the mount line)
> We could check for this and warn the user in mount.cifs so maybe not
> important to worry about in the kernel though.
>
> I will add the acregmax mount option and change actimeo to mean
>     if (actimeo is set)
>             acregmax = acdirmax = actimeo
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-Add-new-parameter-acregmax-for-distinct-file-an.patch --]
[-- Type: text/x-patch, Size: 5989 bytes --]

From bfe9d6d26de6f30d38b206d2676109660931ad4e Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Wed, 24 Feb 2021 12:12:53 -0600
Subject: [PATCH] cifs: Add new parameter "acregmax" for distinct file and
 directory metadata timeout

The new optional mount parameter "acregmax" allows a different
timeout for file metadata ("acdirmax" now allows controlling timeout
for directory metadata).  Setting "actimeo" still works as before,
and changes timeout for both files and directories, but
specifying "acregmax" or "acdirmax" allows overriding the
default more granularly which can be a big performance benefit
on some workloads. "acregmax" is already used by NFS as a mount
parameter (albeit with a larger default and thus looser caching).

Suggested-by: Tom Talpey <tom@talpey.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c     | 15 ++++++++++++---
 fs/cifs/connect.c    |  2 +-
 fs/cifs/fs_context.c | 23 ++++++++++++++++++-----
 fs/cifs/fs_context.h |  6 ++++--
 fs/cifs/inode.c      |  4 ++--
 5 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 4e0b0b26e844..3b61f09f3e1b 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -637,9 +637,18 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
 		seq_printf(s, ",snapshot=%llu", tcon->snapshot_time);
 	if (tcon->handle_timeout)
 		seq_printf(s, ",handletimeout=%u", tcon->handle_timeout);
-	/* convert actimeo and directory attribute timeout and display in seconds */
-	seq_printf(s, ",actimeo=%lu", cifs_sb->ctx->actimeo / HZ);
-	seq_printf(s, ",acdirmax=%lu", cifs_sb->ctx->acdirmax / HZ);
+
+	/*
+	 * Display file and directory attribute timeout in seconds.
+	 * If file and directory attribute timeout the same then actimeo
+	 * was likely specified on mount
+	 */
+	if (cifs_sb->ctx->acdirmax == cifs_sb->ctx->acregmax)
+		seq_printf(s, ",actimeo=%lu", cifs_sb->ctx->acregmax / HZ);
+	else {
+		seq_printf(s, ",acdirmax=%lu", cifs_sb->ctx->acdirmax / HZ);
+		seq_printf(s, ",acregmax=%lu", cifs_sb->ctx->acregmax / HZ);
+	}
 
 	if (tcon->ses->chan_max > 1)
 		seq_printf(s, ",multichannel,max_channels=%zu",
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index a9dc39aee9f4..9ecd8098c2b6 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2276,7 +2276,7 @@ compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
 	if (strcmp(old->local_nls->charset, new->local_nls->charset))
 		return 0;
 
-	if (old->ctx->actimeo != new->ctx->actimeo)
+	if (old->ctx->acregmax != new->ctx->acregmax)
 		return 0;
 	if (old->ctx->acdirmax != new->ctx->acdirmax)
 		return 0;
diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index f3be07f4671d..14c955a30006 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -141,6 +141,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
 	fsparam_u32("wsize", Opt_wsize),
 	fsparam_u32("actimeo", Opt_actimeo),
 	fsparam_u32("acdirmax", Opt_acdirmax),
+	fsparam_u32("acregmax", Opt_acregmax),
 	fsparam_u32("echo_interval", Opt_echo_interval),
 	fsparam_u32("max_credits", Opt_max_credits),
 	fsparam_u32("handletimeout", Opt_handletimeout),
@@ -930,10 +931,10 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 		ctx->wsize = result.uint_32;
 		ctx->got_wsize = true;
 		break;
-	case Opt_actimeo:
-		ctx->actimeo = HZ * result.uint_32;
-		if (ctx->actimeo > CIFS_MAX_ACTIMEO) {
-			cifs_dbg(VFS, "attribute cache timeout too large\n");
+	case Opt_acregmax:
+		ctx->acregmax = HZ * result.uint_32;
+		if (ctx->acregmax > CIFS_MAX_ACTIMEO) {
+			cifs_dbg(VFS, "acregmax too large\n");
 			goto cifs_parse_mount_err;
 		}
 		break;
@@ -944,6 +945,18 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 			goto cifs_parse_mount_err;
 		}
 		break;
+	case Opt_actimeo:
+		if (HZ * result.uint_32 > CIFS_MAX_ACTIMEO) {
+			cifs_dbg(VFS, "timeout too large\n");
+			goto cifs_parse_mount_err;
+		}
+		if ((ctx->acdirmax != CIFS_DEF_ACTIMEO) ||
+		    (ctx->acregmax != CIFS_DEF_ACTIMEO)) {
+			cifs_dbg(VFS, "actimeo ignored since acregmax or acdirmax specified\n");
+			break;
+		}
+		ctx->acdirmax = ctx->acregmax = HZ * result.uint_32;
+		break;
 	case Opt_echo_interval:
 		ctx->echo_interval = result.uint_32;
 		break;
@@ -1369,7 +1382,7 @@ int smb3_init_fs_context(struct fs_context *fc)
 	/* default is to use strict cifs caching semantics */
 	ctx->strict_io = true;
 
-	ctx->actimeo = CIFS_DEF_ACTIMEO;
+	ctx->acregmax = CIFS_DEF_ACTIMEO;
 	ctx->acdirmax = CIFS_DEF_ACTIMEO;
 
 	/* Most clients set timeout to 0, allows server to use its default */
diff --git a/fs/cifs/fs_context.h b/fs/cifs/fs_context.h
index 472372fec4e9..87dd1f7168f2 100644
--- a/fs/cifs/fs_context.h
+++ b/fs/cifs/fs_context.h
@@ -119,6 +119,7 @@ enum cifs_param {
 	Opt_wsize,
 	Opt_actimeo,
 	Opt_acdirmax,
+	Opt_acregmax,
 	Opt_echo_interval,
 	Opt_max_credits,
 	Opt_snapshot,
@@ -233,8 +234,9 @@ struct smb3_fs_context {
 	unsigned int wsize;
 	unsigned int min_offload;
 	bool sockopt_tcp_nodelay:1;
-	unsigned long actimeo; /* attribute cache timeout for files (jiffies) */
-	unsigned long acdirmax; /* attribute cache timeout for directories (jiffies) */
+	/* attribute cache timemout for files and directories in jiffies */
+	unsigned long acregmax;
+	unsigned long acdirmax;
 	struct smb_version_operations *ops;
 	struct smb_version_values *vals;
 	char *prepath;
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index cfd31cc4520f..0b0b01ef3ecb 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2209,10 +2209,10 @@ cifs_inode_needs_reval(struct inode *inode)
 				   cifs_i->time + cifs_sb->ctx->acdirmax))
 			return true;
 	} else { /* file */
-		if (!cifs_sb->ctx->actimeo)
+		if (!cifs_sb->ctx->acregmax)
 			return true;
 		if (!time_in_range(jiffies, cifs_i->time,
-				   cifs_i->time + cifs_sb->ctx->actimeo))
+				   cifs_i->time + cifs_sb->ctx->acregmax))
 			return true;
 	}
 
-- 
2.27.0

Re: [PATCH] convert revalidate of directories to using directory metadata cache timeout

[Tom Talpey]

On 2/24/2021 1:20 PM, Steve French wrote:
> Added additional patch to add "acregmax" so now behavior is more similar to nfs.
> 
> "acregmax" changes file metadata caching timeout
> "acdirmax" changes directory metadata caching
> "actimeo" does what it did before - and changes both.

Clever. It's a little weird that specifying either max with
actimeo will kick a warning, and maybe surprising to someone
who sets both maxes to the same value will see actimeo instead.
But they'll get over that. :)

You can add to all three, my
Reviewed-By: Tom Talpey <tom@talpey.com>

> On Wed, Feb 24, 2021 at 11:31 AM Steve French <smfrench@gmail.com> wrote:
>>
>> On Wed, Feb 24, 2021 at 10:11 AM Tom Talpey <tom@talpey.com> wrote:
>>>
>>> On 2/23/2021 8:03 PM, Steve French wrote:
>>>> Updated version incorporates Ronnie's suggestion of leaving the
>>>> default (for directory caching) the same as it is today, 1 second, to
>>>> avoid
>>>> unnecessary risk.   Most users can safely improve performance by
>>>> mounting with acdirmax to a higher value (e.g. 60 seconds as NFS
>>>> defaults to).
>>>>
>>>> nfs and cifs on Linux currently have a mount parameter "actimeo" to control
>>>> metadata (attribute) caching but cifs does not have additional mount
>>>> parameters to allow distinguishing between caching directory metadata
>>>> (e.g. needed to revalidate paths) and that for files.
>>>
>>> The behaviors seem to be slightly different with this change.
>>> With NFS, the actimeo option overrides the four min/max options,
>>> and by default the directory ac timers range between 30 and 60.
>>>
>>> The CIFS code I see below seems to completely separate actimeo
>>> and acdirmax, and if not set, uses the historic 1 second value.
>>> That's fine, but it's completely different from NFS. Shouldn't we
>>> use a different mount option, to avoid confusing the admin?
>>
>> Ugh ... You are probably right.  I was trying to avoid two problems:
>> 1) (a minor one) adding a second mount option rather than just one (to
>> solve the same problem).  But reducing confusion is worth an extra
>> mount option
>>
>> 2) how to avoid the user specifying *both* actimeo and acregmax -
>> which one 'wins' (presumably the last one in the mount line)
>> We could check for this and warn the user in mount.cifs so maybe not
>> important to worry about in the kernel though.
>>
>> I will add the acregmax mount option and change actimeo to mean
>>      if (actimeo is set)
>>              acregmax = acdirmax = actimeo
>>
>>
>> --
>> Thanks,
>>
>> Steve
> 
> 
> 

Re: [PATCH] convert revalidate of directories to using directory metadata cache timeout

[Shyam Prasad N]

Looks good to me.

I'd prefer it if the default value of acdirmax is much higher, and not
the same as acregmax value.
Otherwise, this becomes something that we ask the users to do only
after they start complaining about performance.
Maybe we can change it once we document these new options in the man page?

Regards,
Shyam

On Thu, Feb 25, 2021 at 9:20 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 2/24/2021 1:20 PM, Steve French wrote:
> > Added additional patch to add "acregmax" so now behavior is more similar to nfs.
> >
> > "acregmax" changes file metadata caching timeout
> > "acdirmax" changes directory metadata caching
> > "actimeo" does what it did before - and changes both.
>
> Clever. It's a little weird that specifying either max with
> actimeo will kick a warning, and maybe surprising to someone
> who sets both maxes to the same value will see actimeo instead.
> But they'll get over that. :)
>
> You can add to all three, my
> Reviewed-By: Tom Talpey <tom@talpey.com>
>
> > On Wed, Feb 24, 2021 at 11:31 AM Steve French <smfrench@gmail.com> wrote:
> >>
> >> On Wed, Feb 24, 2021 at 10:11 AM Tom Talpey <tom@talpey.com> wrote:
> >>>
> >>> On 2/23/2021 8:03 PM, Steve French wrote:
> >>>> Updated version incorporates Ronnie's suggestion of leaving the
> >>>> default (for directory caching) the same as it is today, 1 second, to
> >>>> avoid
> >>>> unnecessary risk.   Most users can safely improve performance by
> >>>> mounting with acdirmax to a higher value (e.g. 60 seconds as NFS
> >>>> defaults to).
> >>>>
> >>>> nfs and cifs on Linux currently have a mount parameter "actimeo" to control
> >>>> metadata (attribute) caching but cifs does not have additional mount
> >>>> parameters to allow distinguishing between caching directory metadata
> >>>> (e.g. needed to revalidate paths) and that for files.
> >>>
> >>> The behaviors seem to be slightly different with this change.
> >>> With NFS, the actimeo option overrides the four min/max options,
> >>> and by default the directory ac timers range between 30 and 60.
> >>>
> >>> The CIFS code I see below seems to completely separate actimeo
> >>> and acdirmax, and if not set, uses the historic 1 second value.
> >>> That's fine, but it's completely different from NFS. Shouldn't we
> >>> use a different mount option, to avoid confusing the admin?
> >>
> >> Ugh ... You are probably right.  I was trying to avoid two problems:
> >> 1) (a minor one) adding a second mount option rather than just one (to
> >> solve the same problem).  But reducing confusion is worth an extra
> >> mount option
> >>
> >> 2) how to avoid the user specifying *both* actimeo and acregmax -
> >> which one 'wins' (presumably the last one in the mount line)
> >> We could check for this and warn the user in mount.cifs so maybe not
> >> important to worry about in the kernel though.
> >>
> >> I will add the acregmax mount option and change actimeo to mean
> >>      if (actimeo is set)
> >>              acregmax = acdirmax = actimeo
> >>
> >>
> >> --
> >> Thanks,
> >>
> >> Steve
> >
> >
> >



-- 
Regards,
Shyam