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

From: Shyam Prasad N <nspmangalore@gmail.com>
To: Steve French <smfrench@gmail.com>
Cc: ronnie sahlberg <ronniesahlberg@gmail.com>,
	Paulo Alcantara <pc@cjr.nz>,
	Bharath S M <bharathsm@microsoft.com>,
	CIFS <linux-cifs@vger.kernel.org>,
	rohiths msft <rohiths.msft@gmail.com>
Subject: Re: [PATCH][SMB3 client] allow deferred close timeout to be configurable
Date: Fri, 12 Aug 2022 09:36:49 +0530	[thread overview]
Message-ID: <CANT5p=oCKutTp=mfLEiVpPX=DbeWK09CWTGTT2ShTZf-1Ukqqw@mail.gmail.com> (raw)
In-Reply-To: <CAH2r5mvppBm7Of8M5YjLkxQQXnxEizGttLne-n2TQdvuhr-ULw@mail.gmail.com>

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