On Fri, Nov 22, 2024 at 08:59:53PM GMT, Paulo Alcantara wrote:
> Henrique Carvalho <henrique.carvalho@xxxxxxxx> writes:
>
> > On Fri, Nov 22, 2024 at 07:55:35PM GMT, Paulo Alcantara wrote:
> >> Henrique Carvalho <henrique.carvalho@xxxxxxxx> writes:
> >>
> >> > On Fri, Nov 22, 2024 at 07:07:06PM GMT, Paulo Alcantara wrote:
> >> >> Henrique Carvalho <henrique.carvalho@xxxxxxxx> writes:
> >> >>
> >> >> > According to the dir_cache_timeout description, setting it to zero
> >> >> > should disable the caching of directory contents. However, even when
> >> >> > dir_cache_timeout is zero, some caching related functions are still
> >> >> > invoked, and the worker thread is initiated, which is unintended
> >> >> > behavior.
> >> >> >
> >> >> > Fix the issue by setting tcon->nohandlecache to true when
> >> >> > dir_cache_timeout is zero, ensuring that directory handle caching
> >> >> > is properly disabled.
> >> >> >
> >> >> > Clean up the code to reflect this change, to improve consistency,
> >> >> > and to remove other unnecessary checks.
> >> >> >
> >> >> > is_smb1_server() check inside open_cached_dir() can be removed because
> >> >> > dir caching is only enabled for SMB versions >= 2.0.
> >> >> >
> >> >> > Signed-off-by: Henrique Carvalho <henrique.carvalho@xxxxxxxx>
> >> >> > ---
> >> >> > fs/smb/client/cached_dir.c | 12 +++++++-----
> >> >> > fs/smb/client/cifsproto.h | 2 +-
> >> >> > fs/smb/client/connect.c | 10 +++++-----
> >> >> > fs/smb/client/misc.c | 4 ++--
> >> >> > 4 files changed, 15 insertions(+), 13 deletions(-)
> >> >>
> >> >> The fix could be simply this:
> >> >>
> >> >> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> >> >> index b227d61a6f20..62a29183c655 100644
> >> >> --- a/fs/smb/client/connect.c
> >> >> +++ b/fs/smb/client/connect.c
> >> >> @@ -2614,7 +2614,7 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb3_fs_context *ctx)
> >> >>
> >> >> if (ses->server->dialect >= SMB20_PROT_ID &&
> >> >> (ses->server->capabilities & SMB2_GLOBAL_CAP_DIRECTORY_LEASING))
> >> >> - nohandlecache = ctx->nohandlecache;
> >> >> + nohandlecache = ctx->nohandlecache || !dir_cache_timeout;
> >> >> else
> >> >> nohandlecache = true;
> >> >> tcon = tcon_info_alloc(!nohandlecache, netfs_trace_tcon_ref_new);
> >> >>
> >> >> and easily backported to -stable kernels that have
> >> >>
> >> >> 238b351d0935 ("smb3: allow controlling length of time directory entries are cached with dir leases")
> >> >>
> >> >
> >> > Then I could split this into two separate patches, one with the fix for
> >> > the mentioned commit, and another patch for the changes in cached_dir.c
> >> > (which I still make sense to have).
> >>
> >> Removing is_smb1_server() check looks good, but the other changes don't
> >> make much sense as we could potentially return -EOPNOTSUPP in
> >> cifs_readdir(), for example, and -ENOENT is probably what you want.
> >>
> >> Am I missing something?
> >
> > I might be missing something, but only place I'm changing the return
> > value is in open_cached_dir_by_dentry() so it is consistent with
> > open_cached_dir().
> >
> > open_cached_dir() already returns -EOPNOTSUPP for these checks:
> >
> > if (tcon == NULL || tcon->cfids == NULL || tcon->nohandlecache ||
> > is_smb1_server(tcon->ses->server) || (dir_cache_timeout == 0))
> > return -EOPNOTSUPP;
> >
> >
> > The changes in this function relate to removing unnecessary checks.
>
> Sounds good.
Resent. Thanks a lot Paulo!
