| Message ID | CAH2r5msJ6=LfoyGWyi94o+Z1FcJFdxpcLyPRz9K9gK5SpvPCUQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [SMB3,client] allow deferred close timeout to be configurable | expand |
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
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.
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.
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
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
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?
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