Re: [PATCH 1/2] smb: client: disable directory caching when dir_cache_timeout is zero

[Paulo Alcantara <pc@manguebit.com>]

Henrique Carvalho <henrique.carvalho@suse.com> writes:

> On Fri, Nov 22, 2024 at 07:55:35PM GMT, Paulo Alcantara wrote:
>> Henrique Carvalho <henrique.carvalho@suse.com> writes:
>> 
>> > On Fri, Nov 22, 2024 at 07:07:06PM GMT, Paulo Alcantara wrote:
>> >> Henrique Carvalho <henrique.carvalho@suse.com> 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@suse.com>
>> >> > ---
>> >> >  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.
>
> --
> Henrique