[PATCH][SMB3 client] allow deferred close timeout to be configurable

[PATCH][SMB3 client] allow deferred close timeout to be configurable

[Steve French]

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

Deferred close can be a very useful feature for very safely
allowing caching data for read, and for minimizing the number
of reopens needed for a file that is repeatedly opened and
close but there are workloads where its default (1 second,
similar to actimeo/acregmax) is too small.

Allow the user to configure the amount of time we can
defer sending the final smb3 close when we have a
handle lease on the file (rather than forcing it to depend
on 1 second or actimeo which is often unrelated).

Adds new mount parameter "closetime=" which is the maximum
number of seconds we can wait before sending an SMB3
close when we have a handle lease for it.  Default value
is unchanged at 1 second.

-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-allow-deferred-close-timeout-to-be-configurable.patch --]
[-- Type: text/x-patch, Size: 4833 bytes --]

From 464efd4fec1eed5d34a0953c23dea951b9f528b1 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 11 Aug 2022 00:53:00 -0500
Subject: [PATCH] smb3: allow deferred close timeout to be configurable

Deferred close can be a very useful feature for allowing
caching data for read, and for minimizing the number of
reopens needed for a file that is repeatedly opened and
close but there are workloads where its default (1 second,
similar to actimeo/acregmax) is too small.

Allow the user to configure the amount of time we can
defer sending the final smb3 close when we have a
handle lease on the file (rather than forcing it to depend
on 1 second or actimeo which is often unrelated).

Adds new mount parameter "closetime=" which is the maximum
number of seconds we can wait before sending an SMB3
close when we have a handle lease for it.  Default value
is unchanged at 1 second.

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

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 945fb083cea7..af6114e17fb5 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -693,6 +693,8 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
 		seq_printf(s, ",acdirmax=%lu", cifs_sb->ctx->acdirmax / HZ);
 		seq_printf(s, ",acregmax=%lu", cifs_sb->ctx->acregmax / HZ);
 	}
+	if (cifs_sb->ctx->closetimeo != cifs_sb->ctx->acregmax)
+		seq_printf(s, ",closetimeo=%lu", cifs_sb->ctx->closetimeo / 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 7f205a9a2de4..9111c025bcb8 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2681,6 +2681,8 @@ compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
 		return 0;
 	if (old->ctx->acdirmax != new->ctx->acdirmax)
 		return 0;
+	if (old->ctx->closetimeo != new->ctx->closetimeo)
+		return 0;
 
 	return 1;
 }
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 42f2639a1a66..2c5eae7d31f4 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -964,12 +964,12 @@ int cifs_close(struct inode *inode, struct file *file)
 				 * So, Increase the ref count to avoid use-after-free.
 				 */
 				if (!mod_delayed_work(deferredclose_wq,
-						&cfile->deferred, cifs_sb->ctx->acregmax))
+						&cfile->deferred, cifs_sb->ctx->closetimeo))
 					cifsFileInfo_get(cfile);
 			} else {
 				/* Deferred close for files */
 				queue_delayed_work(deferredclose_wq,
-						&cfile->deferred, cifs_sb->ctx->acregmax);
+						&cfile->deferred, cifs_sb->ctx->closetimeo);
 				cfile->deferred_close_scheduled = true;
 				spin_unlock(&cinode->deferred_lock);
 				return 0;
diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index 8dc0d923ef6a..28a339c57beb 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -147,6 +147,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
 	fsparam_u32("actimeo", Opt_actimeo),
 	fsparam_u32("acdirmax", Opt_acdirmax),
 	fsparam_u32("acregmax", Opt_acregmax),
+	fsparam_u32("closetimeo", Opt_closetimeo),
 	fsparam_u32("echo_interval", Opt_echo_interval),
 	fsparam_u32("max_credits", Opt_max_credits),
 	fsparam_u32("handletimeout", Opt_handletimeout),
@@ -1074,6 +1075,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 		}
 		ctx->acdirmax = ctx->acregmax = HZ * result.uint_32;
 		break;
+	case Opt_closetimeo:
+		ctx->closetimeo = HZ * result.uint_32;
+		if (ctx->closetimeo > CIFS_MAX_ACTIMEO) {
+			cifs_errorf(fc, "closetimeo too large\n");
+			goto cifs_parse_mount_err;
+		}
+		break;
 	case Opt_echo_interval:
 		ctx->echo_interval = result.uint_32;
 		break;
@@ -1521,6 +1529,7 @@ int smb3_init_fs_context(struct fs_context *fc)
 
 	ctx->acregmax = CIFS_DEF_ACTIMEO;
 	ctx->acdirmax = CIFS_DEF_ACTIMEO;
+	ctx->closetimeo = 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 5f093cb7e9b9..927a5f2f9919 100644
--- a/fs/cifs/fs_context.h
+++ b/fs/cifs/fs_context.h
@@ -125,6 +125,7 @@ enum cifs_param {
 	Opt_actimeo,
 	Opt_acdirmax,
 	Opt_acregmax,
+	Opt_closetimeo,
 	Opt_echo_interval,
 	Opt_max_credits,
 	Opt_snapshot,
@@ -247,6 +248,8 @@ struct smb3_fs_context {
 	/* attribute cache timemout for files and directories in jiffies */
 	unsigned long acregmax;
 	unsigned long acdirmax;
+	/* timeout for deferred close of files in jiffies */
+	unsigned long closetimeo;
 	struct smb_version_operations *ops;
 	struct smb_version_values *vals;
 	char *prepath;
-- 
2.34.1

Re: [PATCH][SMB3 client] allow deferred close timeout to be configurable

[Steve French]

Will fix the typos thanks.

There are a couple of minor differences from Bharath's earlier patch e.g.

"closetimeo" rather than "dclosetimeo" (I am ok if you prefer the longer name),
and also this mount option is printed in list of mount options if set.


On Thu, Aug 11, 2022 at 11:03 AM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Steve French <smfrench@gmail.com> writes:
>
> > Deferred close can be a very useful feature for very safely
> > allowing caching data for read, and for minimizing the number
> > of reopens needed for a file that is repeatedly opened and
> > close but there are workloads where its default (1 second,
> > similar to actimeo/acregmax) is too small.
> >
> > Allow the user to configure the amount of time we can
> > defer sending the final smb3 close when we have a
> > handle lease on the file (rather than forcing it to depend
> > on 1 second or actimeo which is often unrelated).
> >
> > Adds new mount parameter "closetime=" which is the maximum
>                             ^^^^^^^^^ should be closetimeo
>
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 945fb083cea7..af6114e17fb5 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -693,6 +693,8 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
> >               seq_printf(s, ",acdirmax=%lu", cifs_sb->ctx->acdirmax / HZ);
> >               seq_printf(s, ",acregmax=%lu", cifs_sb->ctx->acregmax / HZ);
> >       }
> > +     if (cifs_sb->ctx->closetimeo != cifs_sb->ctx->acregmax)
> > +             seq_printf(s, ",closetimeo=%lu", cifs_sb->ctx->closetimeo / HZ);
>
> Hrm - I think you can rid of this check.  I'd rather print it out
> unconditionally.
>
> > diff --git a/fs/cifs/fs_context.h b/fs/cifs/fs_context.h
> > index 5f093cb7e9b9..927a5f2f9919 100644
> > --- a/fs/cifs/fs_context.h
> > +++ b/fs/cifs/fs_context.h
> > @@ -125,6 +125,7 @@ enum cifs_param {
> >       Opt_actimeo,
> >       Opt_acdirmax,
> >       Opt_acregmax,
> > +     Opt_closetimeo,
> >       Opt_echo_interval,
> >       Opt_max_credits,
> >       Opt_snapshot,
> > @@ -247,6 +248,8 @@ struct smb3_fs_context {
> >       /* attribute cache timemout for files and directories in jiffies */
> >       unsigned long acregmax;
> >       unsigned long acdirmax;
> > +     /* timeout for deferred close of files in jiffies */
>                                                   ^^^^^^^ seconds
>
> Otherwise, looks good to me.
>
> Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>



--
Thanks,

Steve

Re: [PATCH][SMB3 client] allow deferred close timeout to be configurable

[Paulo Alcantara]

Steve French <smfrench@gmail.com> writes:

> Will fix the typos thanks.

Thanks.

> There are a couple of minor differences from Bharath's earlier patch e.g.
>
> "closetimeo" rather than "dclosetimeo" (I am ok if you prefer the longer name),
> and also this mount option is printed in list of mount options if set.

Both look good to me.  I personally don't care much about naming,
though.

Re: [PATCH][SMB3 client] allow deferred close timeout to be configurable

[Steve French]

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

The "jiffies vs. seconds" in comment was the only suggestion I didn't include.
See updated patch v2 (attached), I made minor updates.  Added the
Suggested-by from Bharath. Moved the defines for default/max to
different name with SMB3 (and in fs_context.h) since it is an smb3
feature (so not confused with cifs).  I increased the default to 5
seconds (although that is still lower than some other clients - it
should help perf.  As you suggested, unconditionally print the value
used on the mount.
for some workloads).

On Thu, Aug 11, 2022 at 11:16 AM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Steve French <smfrench@gmail.com> writes:
>
> > Will fix the typos thanks.
>
> Thanks.
>
> > There are a couple of minor differences from Bharath's earlier patch e.g.
> >
> > "closetimeo" rather than "dclosetimeo" (I am ok if you prefer the longer name),
> > and also this mount option is printed in list of mount options if set.
>
> Both look good to me.  I personally don't care much about naming,
> though.



-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-allow-deferred-close-timeout-to-be-configurable.patch --]
[-- Type: text/x-patch, Size: 5505 bytes --]

From a6f4b057f04724f88431c624b35b0aa1e9a76125 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 11 Aug 2022 00:53:00 -0500
Subject: [PATCH] smb3: allow deferred close timeout to be configurable

Deferred close can be a very useful feature for allowing
caching data for read, and for minimizing the number of
reopens needed for a file that is repeatedly opened and
close but there are workloads where its default (1 second,
similar to actimeo/acregmax) is much too small.

Allow the user to configure the amount of time we can
defer sending the final smb3 close when we have a
handle lease on the file (rather than forcing it to depend
on value of actimeo which is often unrelated, and less safe).

Adds new mount parameter "closetimeo=" which is the maximum
number of seconds we can wait before sending an SMB3
close when we have a handle lease for it.  Default value
also is set to slightly larger at 5 seconds (although some
other clients use larger default this should still help).

Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
Suggested-by: Bharath SM <bharathsm@microsoft.com>
Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c     | 1 +
 fs/cifs/connect.c    | 2 ++
 fs/cifs/file.c       | 4 ++--
 fs/cifs/fs_context.c | 9 +++++++++
 fs/cifs/fs_context.h | 8 ++++++++
 5 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 945fb083cea7..f54d8bf2732a 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -693,6 +693,7 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
 		seq_printf(s, ",acdirmax=%lu", cifs_sb->ctx->acdirmax / HZ);
 		seq_printf(s, ",acregmax=%lu", cifs_sb->ctx->acregmax / HZ);
 	}
+	seq_printf(s, ",closetimeo=%lu", cifs_sb->ctx->closetimeo / 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 7f205a9a2de4..9111c025bcb8 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2681,6 +2681,8 @@ compare_mount_options(struct super_block *sb, struct cifs_mnt_data *mnt_data)
 		return 0;
 	if (old->ctx->acdirmax != new->ctx->acdirmax)
 		return 0;
+	if (old->ctx->closetimeo != new->ctx->closetimeo)
+		return 0;
 
 	return 1;
 }
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 42f2639a1a66..2c5eae7d31f4 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -964,12 +964,12 @@ int cifs_close(struct inode *inode, struct file *file)
 				 * So, Increase the ref count to avoid use-after-free.
 				 */
 				if (!mod_delayed_work(deferredclose_wq,
-						&cfile->deferred, cifs_sb->ctx->acregmax))
+						&cfile->deferred, cifs_sb->ctx->closetimeo))
 					cifsFileInfo_get(cfile);
 			} else {
 				/* Deferred close for files */
 				queue_delayed_work(deferredclose_wq,
-						&cfile->deferred, cifs_sb->ctx->acregmax);
+						&cfile->deferred, cifs_sb->ctx->closetimeo);
 				cfile->deferred_close_scheduled = true;
 				spin_unlock(&cinode->deferred_lock);
 				return 0;
diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index 8dc0d923ef6a..0e13dec86b25 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -147,6 +147,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
 	fsparam_u32("actimeo", Opt_actimeo),
 	fsparam_u32("acdirmax", Opt_acdirmax),
 	fsparam_u32("acregmax", Opt_acregmax),
+	fsparam_u32("closetimeo", Opt_closetimeo),
 	fsparam_u32("echo_interval", Opt_echo_interval),
 	fsparam_u32("max_credits", Opt_max_credits),
 	fsparam_u32("handletimeout", Opt_handletimeout),
@@ -1074,6 +1075,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 		}
 		ctx->acdirmax = ctx->acregmax = HZ * result.uint_32;
 		break;
+	case Opt_closetimeo:
+		ctx->closetimeo = HZ * result.uint_32;
+		if (ctx->closetimeo > SMB3_MAX_DCLOSETIMEO) {
+			cifs_errorf(fc, "closetimeo too large\n");
+			goto cifs_parse_mount_err;
+		}
+		break;
 	case Opt_echo_interval:
 		ctx->echo_interval = result.uint_32;
 		break;
@@ -1521,6 +1529,7 @@ int smb3_init_fs_context(struct fs_context *fc)
 
 	ctx->acregmax = CIFS_DEF_ACTIMEO;
 	ctx->acdirmax = CIFS_DEF_ACTIMEO;
+	ctx->closetimeo = SMB3_DEF_DCLOSETIMEO;
 
 	/* 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 5f093cb7e9b9..bbaee4c2281f 100644
--- a/fs/cifs/fs_context.h
+++ b/fs/cifs/fs_context.h
@@ -125,6 +125,7 @@ enum cifs_param {
 	Opt_actimeo,
 	Opt_acdirmax,
 	Opt_acregmax,
+	Opt_closetimeo,
 	Opt_echo_interval,
 	Opt_max_credits,
 	Opt_snapshot,
@@ -247,6 +248,8 @@ struct smb3_fs_context {
 	/* attribute cache timemout for files and directories in jiffies */
 	unsigned long acregmax;
 	unsigned long acdirmax;
+	/* timeout for deferred close of files in jiffies */
+	unsigned long closetimeo;
 	struct smb_version_operations *ops;
 	struct smb_version_values *vals;
 	char *prepath;
@@ -279,4 +282,9 @@ static inline struct smb3_fs_context *smb3_fc2context(const struct fs_context *f
 extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct smb3_fs_context *ctx);
 extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
 
+/*
+ * max deferred close timeout (jiffies) - 2^30
+ */
+#define SMB3_MAX_DCLOSETIMEO (1 << 30)
+#define SMB3_DEF_DCLOSETIMEO (5 * HZ) /* Can increase later, other clients use larger */
 #endif
-- 
2.34.1

Re: [PATCH][SMB3 client] allow deferred close timeout to be configurable

[ronnie sahlberg]

On Fri, 12 Aug 2022 at 03:17, Steve French <smfrench@gmail.com> wrote:
>
> The "jiffies vs. seconds" in comment was the only suggestion I didn't include.
> See updated patch v2 (attached), I made minor updates.  Added the
> Suggested-by from Bharath. Moved the defines for default/max to
> different name with SMB3 (and in fs_context.h) since it is an smb3
> feature (so not confused with cifs).  I increased the default to 5
> seconds (although that is still lower than some other clients - it
> should help perf.  As you suggested, unconditionally print the value
> used on the mount.
> for some workloads).

nack.
The problem with this is that it is a mount option that is impossible
for a sys admin to set correctly.

If we need this as a mount option we need documentation on it too.

1, How does a sys admin determine that there is an issue and that
changing this value will  fix it?
2, How does a sys admin determine what to set it to?

To me it seems this is an option that can only be used by developers
and thus it should not be
a mount option. We have too many ad-hoc mount options that end users
can not use correctly as it is.


>
> On Thu, Aug 11, 2022 at 11:16 AM Paulo Alcantara <pc@cjr.nz> wrote:
> >
> > Steve French <smfrench@gmail.com> writes:
> >
> > > Will fix the typos thanks.
> >
> > Thanks.
> >
> > > There are a couple of minor differences from Bharath's earlier patch e.g.
> > >
> > > "closetimeo" rather than "dclosetimeo" (I am ok if you prefer the longer name),
> > > and also this mount option is printed in list of mount options if set.
> >
> > Both look good to me.  I personally don't care much about naming,
> > though.
>
>
>
> --
> Thanks,
>
> Steve

Re: [PATCH][SMB3 client] allow deferred close timeout to be configurable

[Steve French]

I am planning to document this (one reason I made the mount option
name a bit shorter, and tried to make the defaults a bit more
intuitive)

We have workloads where we need this (e.g. cases where they want to
read cache files more aggressively, but safely - but the app closes
the file)

My main short term issue is how to separate it from actimeo which is
unrelated (and somewhat unsafe).

I do plan to update the man page for mount.cifs, but we definitely
need to be able to configure this.  Remember we recently had a bug
where it would have helped investigate it.

The main example I can think of is apps that do:

open/read/close, open/read/close, repeatedly with other clients
occasionally reading or writing the file.   Currently we only cache
the file for 1 second after close. Windows for longer.  Our goal is to
allow this as a performance tuning recommendation for advanced users
until we can pick an optimum value (probably pretty long).

I am fine with documenting this.   My intent was to document it
similar to the following:
- indicate it is an advanced tuning parameter, and its default is fine for many
- indicate that if your apps open many, many files, you may want to
consider setting it smaller if your server is resource constrained, to
put less load on your server (especially if your apps do not
repeatedly reopen the same files over and over)
- indicate that if you have a workload where you want to cache files
for long periods safely, and these files are only occasionally
accessed by other clients, then consider setting it longer.  We have
some example workloads that we were asked about this e.g.


On Thu, Aug 11, 2022 at 8:10 PM ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
>
> On Fri, 12 Aug 2022 at 03:17, Steve French <smfrench@gmail.com> wrote:
> >
> > The "jiffies vs. seconds" in comment was the only suggestion I didn't include.
> > See updated patch v2 (attached), I made minor updates.  Added the
> > Suggested-by from Bharath. Moved the defines for default/max to
> > different name with SMB3 (and in fs_context.h) since it is an smb3
> > feature (so not confused with cifs).  I increased the default to 5
> > seconds (although that is still lower than some other clients - it
> > should help perf.  As you suggested, unconditionally print the value
> > used on the mount.
> > for some workloads).
>
> nack.
> The problem with this is that it is a mount option that is impossible
> for a sys admin to set correctly.
>
> If we need this as a mount option we need documentation on it too.
>
> 1, How does a sys admin determine that there is an issue and that
> changing this value will  fix it?
> 2, How does a sys admin determine what to set it to?
>
> To me it seems this is an option that can only be used by developers
> and thus it should not be
> a mount option. We have too many ad-hoc mount options that end users
> can not use correctly as it is.
>
>
> >
> > On Thu, Aug 11, 2022 at 11:16 AM Paulo Alcantara <pc@cjr.nz> wrote:
> > >
> > > Steve French <smfrench@gmail.com> writes:
> > >
> > > > Will fix the typos thanks.
> > >
> > > Thanks.
> > >
> > > > There are a couple of minor differences from Bharath's earlier patch e.g.
> > > >
> > > > "closetimeo" rather than "dclosetimeo" (I am ok if you prefer the longer name),
> > > > and also this mount option is printed in list of mount options if set.
> > >
> > > Both look good to me.  I personally don't care much about naming,
> > > though.
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve



-- 
Thanks,

Steve

Re: [PATCH][SMB3 client] allow deferred close timeout to be configurable

[Shyam Prasad N]

On Fri, Aug 12, 2022 at 6:50 AM Steve French <smfrench@gmail.com> wrote:
>
> I am planning to document this (one reason I made the mount option
> name a bit shorter, and tried to make the defaults a bit more
> intuitive)
>
> We have workloads where we need this (e.g. cases where they want to
> read cache files more aggressively, but safely - but the app closes
> the file)
>
> My main short term issue is how to separate it from actimeo which is
> unrelated (and somewhat unsafe).
>
> I do plan to update the man page for mount.cifs, but we definitely
> need to be able to configure this.  Remember we recently had a bug
> where it would have helped investigate it.
>
> The main example I can think of is apps that do:
>
> open/read/close, open/read/close, repeatedly with other clients
> occasionally reading or writing the file.   Currently we only cache
> the file for 1 second after close. Windows for longer.  Our goal is to
> allow this as a performance tuning recommendation for advanced users
> until we can pick an optimum value (probably pretty long).
>
> I am fine with documenting this.   My intent was to document it
> similar to the following:
> - indicate it is an advanced tuning parameter, and its default is fine for many
> - indicate that if your apps open many, many files, you may want to
> consider setting it smaller if your server is resource constrained, to
> put less load on your server (especially if your apps do not
> repeatedly reopen the same files over and over)
> - indicate that if you have a workload where you want to cache files
> for long periods safely, and these files are only occasionally
> accessed by other clients, then consider setting it longer.  We have
> some example workloads that we were asked about this e.g.
>
>
> On Thu, Aug 11, 2022 at 8:10 PM ronnie sahlberg
> <ronniesahlberg@gmail.com> wrote:
> >
> > On Fri, 12 Aug 2022 at 03:17, Steve French <smfrench@gmail.com> wrote:
> > >
> > > The "jiffies vs. seconds" in comment was the only suggestion I didn't include.
> > > See updated patch v2 (attached), I made minor updates.  Added the
> > > Suggested-by from Bharath. Moved the defines for default/max to
> > > different name with SMB3 (and in fs_context.h) since it is an smb3
> > > feature (so not confused with cifs).  I increased the default to 5
> > > seconds (although that is still lower than some other clients - it
> > > should help perf.  As you suggested, unconditionally print the value
> > > used on the mount.
> > > for some workloads).
> >
> > nack.
> > The problem with this is that it is a mount option that is impossible
> > for a sys admin to set correctly.
> >
> > If we need this as a mount option we need documentation on it too.
> >
> > 1, How does a sys admin determine that there is an issue and that
> > changing this value will  fix it?
> > 2, How does a sys admin determine what to set it to?
> >
> > To me it seems this is an option that can only be used by developers
> > and thus it should not be
> > a mount option. We have too many ad-hoc mount options that end users
> > can not use correctly as it is.
> >
> >
> > >
> > > On Thu, Aug 11, 2022 at 11:16 AM Paulo Alcantara <pc@cjr.nz> wrote:
> > > >
> > > > Steve French <smfrench@gmail.com> writes:
> > > >
> > > > > Will fix the typos thanks.
> > > >
> > > > Thanks.
> > > >
> > > > > There are a couple of minor differences from Bharath's earlier patch e.g.
> > > > >
> > > > > "closetimeo" rather than "dclosetimeo" (I am ok if you prefer the longer name),
> > > > > and also this mount option is printed in list of mount options if set.
> > > >
> > > > Both look good to me.  I personally don't care much about naming,
> > > > though.
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
>
>
>
> --
> Thanks,
>
> Steve

While I understand the concern that having too many options can be
confusing for users, these are the most fine grained tuning knobs
available to us.
Anything else today (procfs entry, module parameter etc) are not at
the level of each mounted filesystem.
As Steve said, this could be configured by advanced users, based on
the workload. The default should be good enough for regular users.
The patch looks good to me.

For the number of mount options the user has at his/her disposal, I
feel that we should work on a profile=X mount option.
Based on the value of X, which describes the user workload, we could
internally decide on the values of multiple fine tunable mount
options.
We should have a smaller set of values for X, based on different type
of workloads, and the default values of other mount options based on
this value.
Thoughts?

-- 
Regards,
Shyam