| Message ID | 20241122203901.283703-1-henrique.carvalho@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] smb: client: disable directory caching when dir_cache_timeout is zero | expand |
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") And yes, is_smb1_server() check makes no sense as tcon->nohandlecache would already be set.
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). The other changes are mostly cosmetic and could be dropped. Sounds good? -- Henrique
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?
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. -- Henrique
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
On Fri, Nov 22, 2024 at 08:59:53PM GMT, Paulo Alcantara wrote: > 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. Resent. Thanks a lot Paulo!
diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c index 8b510c858f4ff..d8b1cf1043c35 100644 --- a/fs/smb/client/cached_dir.c +++ b/fs/smb/client/cached_dir.c @@ -162,15 +162,17 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, const char *npath; int retries = 0, cur_sleep = 1; - if (tcon == NULL || tcon->cfids == NULL || tcon->nohandlecache || - is_smb1_server(tcon->ses->server) || (dir_cache_timeout == 0)) + if (cifs_sb->root == NULL) + return -ENOENT; + + if (tcon == NULL) return -EOPNOTSUPP; ses = tcon->ses; cfids = tcon->cfids; - if (cifs_sb->root == NULL) - return -ENOENT; + if (cfids == NULL) + return -EOPNOTSUPP; replay_again: /* reinitialize for possible replay */ @@ -394,7 +396,7 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon, struct cached_fids *cfids = tcon->cfids; if (cfids == NULL) - return -ENOENT; + return -EOPNOTSUPP; spin_lock(&cfids->cfid_list_lock); list_for_each_entry(cfid, &cfids->entries, entry) { diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h index 075985bfb13a8..d89d31b6dd97a 100644 --- a/fs/smb/client/cifsproto.h +++ b/fs/smb/client/cifsproto.h @@ -530,7 +530,7 @@ extern int CIFSSMBLogoff(const unsigned int xid, struct cifs_ses *ses); extern struct cifs_ses *sesInfoAlloc(void); extern void sesInfoFree(struct cifs_ses *); -extern struct cifs_tcon *tcon_info_alloc(bool dir_leases_enabled, +extern struct cifs_tcon *tcon_info_alloc(bool enable_dir_cache, enum smb3_tcon_ref_trace trace); extern void tconInfoFree(struct cifs_tcon *tcon, enum smb3_tcon_ref_trace trace); diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c index b227d61a6f205..f74e0b94f848c 100644 --- a/fs/smb/client/connect.c +++ b/fs/smb/client/connect.c @@ -2593,7 +2593,7 @@ static struct cifs_tcon * cifs_get_tcon(struct cifs_ses *ses, struct smb3_fs_context *ctx) { struct cifs_tcon *tcon; - bool nohandlecache; + bool enable_dir_cache; int rc, xid; tcon = cifs_find_tcon(ses, ctx); @@ -2614,15 +2614,15 @@ 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; + enable_dir_cache = !ctx->nohandlecache && (dir_cache_timeout != 0); else - nohandlecache = true; - tcon = tcon_info_alloc(!nohandlecache, netfs_trace_tcon_ref_new); + enable_dir_cache = false; + tcon = tcon_info_alloc(enable_dir_cache, netfs_trace_tcon_ref_new); if (tcon == NULL) { rc = -ENOMEM; goto out_fail; } - tcon->nohandlecache = nohandlecache; + tcon->nohandlecache = !enable_dir_cache; if (ctx->snapshot_time) { if (ses->server->vals->protocol_id == 0) { diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c index 4373dd64b66d4..60f35d827382c 100644 --- a/fs/smb/client/misc.c +++ b/fs/smb/client/misc.c @@ -111,7 +111,7 @@ sesInfoFree(struct cifs_ses *buf_to_free) } struct cifs_tcon * -tcon_info_alloc(bool dir_leases_enabled, enum smb3_tcon_ref_trace trace) +tcon_info_alloc(bool enable_dir_cache, enum smb3_tcon_ref_trace trace) { struct cifs_tcon *ret_buf; static atomic_t tcon_debug_id; @@ -120,7 +120,7 @@ tcon_info_alloc(bool dir_leases_enabled, enum smb3_tcon_ref_trace trace) if (!ret_buf) return NULL; - if (dir_leases_enabled == true) { + if (enable_dir_cache) { ret_buf->cfids = init_cached_dirs(); if (!ret_buf->cfids) { kfree(ret_buf);
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(-)