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

From: Steve French <smfrench@gmail.com>
To: ronnie sahlberg <ronniesahlberg@gmail.com>
Cc: Paulo Alcantara <pc@cjr.nz>,
	Bharath S M <bharathsm@microsoft.com>,
	Shyam Prasad N <nspmangalore@gmail.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: Thu, 11 Aug 2022 20:20:34 -0500	[thread overview]
Message-ID: <CAH2r5mvppBm7Of8M5YjLkxQQXnxEizGttLne-n2TQdvuhr-ULw@mail.gmail.com> (raw)
In-Reply-To: <CAN05THSv+7C2J9yv2Ph0_KApS5wucE-GwPLzihQ+zU_68ceq2g@mail.gmail.com>

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