* [PATCH 2/6] smb: client: short-circuit in open_cached_dir_by_dentry() if !dentry
2025-09-19 15:24 [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid Henrique Carvalho
@ 2025-09-19 15:24 ` Henrique Carvalho
2025-09-19 16:15 ` Enzo Matsumiya
2025-09-19 15:24 ` [PATCH 3/6] smb: client: short-circuit negative lookups when parent dir is fully cached Henrique Carvalho
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Henrique Carvalho @ 2025-09-19 15:24 UTC (permalink / raw)
To: smfrench; +Cc: ematsumiya, linux-cifs, Henrique Carvalho
When dentry is NULL, the current code acquires the spinlock and traverses
the entire list, but the condition (dentry && cfid->dentry == dentry)
ensures no match will ever be found.
Return -ENOENT early in this case, avoiding unnecessary lock acquisition
and list traversal.
Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
fs/smb/client/cached_dir.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index 63dc9add4f13..df6cabd0966d 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -416,9 +416,12 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
if (cfids == NULL)
return -EOPNOTSUPP;
+ if (!dentry)
+ return -ENOENT;
+
spin_lock(&cfids->cfid_list_lock);
list_for_each_entry(cfid, &cfids->entries, entry) {
- if (dentry && cfid->dentry == dentry) {
+ if (cfid->dentry == dentry) {
if (!is_valid_cached_dir(cfid))
break;
cifs_dbg(FYI, "found a cached file handle by dentry\n");
--
2.50.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/6] smb: client: short-circuit in open_cached_dir_by_dentry() if !dentry
2025-09-19 15:24 ` [PATCH 2/6] smb: client: short-circuit in open_cached_dir_by_dentry() if !dentry Henrique Carvalho
@ 2025-09-19 16:15 ` Enzo Matsumiya
0 siblings, 0 replies; 14+ messages in thread
From: Enzo Matsumiya @ 2025-09-19 16:15 UTC (permalink / raw)
To: Henrique Carvalho; +Cc: smfrench, linux-cifs
On 09/19, Henrique Carvalho wrote:
>When dentry is NULL,
I know this is good practice, but... Is a NULL dentry ever passed
down to open_cached_dir_by_dentry()?
All callers I checked (even with your series applied), seems to
guarantee a non-NULL dentry at this point.
Either way,
Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
> the current code acquires the spinlock and traverses
>the entire list, but the condition (dentry && cfid->dentry == dentry)
>ensures no match will ever be found.
>
>Return -ENOENT early in this case, avoiding unnecessary lock acquisition
>and list traversal.
>
>Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
>---
> fs/smb/client/cached_dir.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
>index 63dc9add4f13..df6cabd0966d 100644
>--- a/fs/smb/client/cached_dir.c
>+++ b/fs/smb/client/cached_dir.c
>@@ -416,9 +416,12 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
> if (cfids == NULL)
> return -EOPNOTSUPP;
>
>+ if (!dentry)
>+ return -ENOENT;
>+
> spin_lock(&cfids->cfid_list_lock);
> list_for_each_entry(cfid, &cfids->entries, entry) {
>- if (dentry && cfid->dentry == dentry) {
>+ if (cfid->dentry == dentry) {
> if (!is_valid_cached_dir(cfid))
> break;
> cifs_dbg(FYI, "found a cached file handle by dentry\n");
>--
>2.50.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/6] smb: client: short-circuit negative lookups when parent dir is fully cached
2025-09-19 15:24 [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid Henrique Carvalho
2025-09-19 15:24 ` [PATCH 2/6] smb: client: short-circuit in open_cached_dir_by_dentry() if !dentry Henrique Carvalho
@ 2025-09-19 15:24 ` Henrique Carvalho
2025-09-19 18:54 ` Enzo Matsumiya
2025-09-19 15:24 ` [PATCH 4/6] smb: client: update cfid->last_access_time in open_cached_dir_by_dentry() Henrique Carvalho
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Henrique Carvalho @ 2025-09-19 15:24 UTC (permalink / raw)
To: smfrench; +Cc: ematsumiya, linux-cifs, Henrique Carvalho
When the parent directory has a valid and complete cached enumeration we
can assume that negative dentries are not present in the directory, thus
we can return without issuing a request.
This reduces traffic for common ENOENT when the directory entries are
cached.
Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
fs/smb/client/dir.c | 48 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 47 insertions(+), 1 deletion(-)
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index 56c59b67ecc2..d382fc76974f 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -683,6 +683,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
const char *full_path;
void *page;
int retry_count = 0;
+ struct cached_fid *cfid = NULL;
xid = get_xid();
@@ -722,6 +723,29 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
cifs_dbg(FYI, "non-NULL inode in lookup\n");
} else {
cifs_dbg(FYI, "NULL inode in lookup\n");
+
+ /*
+ * We can only rely on negative dentries having the same
+ * spelling as the cached dirent if case insensitivity is
+ * forced on mount.
+ *
+ * XXX: if servers correctly announce Case Sensitivity Search
+ * on GetInfo of FileFSAttributeInformation, then we can take
+ * correct action even if case insensitive is not forced on
+ * mount.
+ *
+ */
+ if (pTcon->nocase && !open_cached_dir_by_dentry(pTcon, direntry->d_parent, &cfid)) {
+ /*
+ * dentry is negative and parent is fully cached:
+ * we can assume file does not exist
+ */
+ if (cfid->dirents.is_valid) {
+ close_cached_dir(cfid);
+ goto out;
+ }
+ close_cached_dir(cfid);
+ }
}
cifs_dbg(FYI, "Full path: %s inode = 0x%p\n",
full_path, d_inode(direntry));
@@ -755,6 +779,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
}
newInode = ERR_PTR(rc);
}
+
+out:
free_dentry_path(page);
cifs_put_tlink(tlink);
free_xid(xid);
@@ -765,7 +791,11 @@ static int
cifs_d_revalidate(struct inode *dir, const struct qstr *name,
struct dentry *direntry, unsigned int flags)
{
- struct inode *inode;
+ struct inode *inode = NULL;
+ struct cifs_sb_info *cifs_sb;
+ struct cifs_tcon *tcon;
+ struct cached_fid *cfid;
+
int rc;
if (flags & LOOKUP_RCU)
@@ -812,6 +842,22 @@ cifs_d_revalidate(struct inode *dir, const struct qstr *name,
return 1;
}
+ } else {
+ inode = d_inode(direntry->d_parent);
+ cifs_sb = CIFS_SB(inode->i_sb);
+ tcon = cifs_sb_master_tcon(cifs_sb);
+
+ if (!open_cached_dir_by_dentry(tcon, direntry->d_parent, &cfid)) {
+ /*
+ * dentry is negative and parent is fully cached:
+ * we can assume file does not exist
+ */
+ if (cfid->dirents.is_valid) {
+ close_cached_dir(cfid);
+ return 1;
+ }
+ close_cached_dir(cfid);
+ }
}
/*
--
2.50.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/6] smb: client: short-circuit negative lookups when parent dir is fully cached
2025-09-19 15:24 ` [PATCH 3/6] smb: client: short-circuit negative lookups when parent dir is fully cached Henrique Carvalho
@ 2025-09-19 18:54 ` Enzo Matsumiya
2025-09-19 20:49 ` Henrique Carvalho
0 siblings, 1 reply; 14+ messages in thread
From: Enzo Matsumiya @ 2025-09-19 18:54 UTC (permalink / raw)
To: Henrique Carvalho; +Cc: smfrench, linux-cifs
On 09/19, Henrique Carvalho wrote:
>When the parent directory has a valid and complete cached enumeration we
>can assume that negative dentries are not present in the directory, thus
>we can return without issuing a request.
>
>This reduces traffic for common ENOENT when the directory entries are
>cached.
>
>Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
>---
> fs/smb/client/dir.c | 48 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 47 insertions(+), 1 deletion(-)
>
>diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
>index 56c59b67ecc2..d382fc76974f 100644
>--- a/fs/smb/client/dir.c
>+++ b/fs/smb/client/dir.c
>@@ -683,6 +683,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> const char *full_path;
> void *page;
> int retry_count = 0;
>+ struct cached_fid *cfid = NULL;
>
> xid = get_xid();
>
>@@ -722,6 +723,29 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> cifs_dbg(FYI, "non-NULL inode in lookup\n");
> } else {
> cifs_dbg(FYI, "NULL inode in lookup\n");
(unrelated to the patch)
Can a dentry ever get here being positive? AFAICS it can't, but I might
have missed some obscure code path.
>+
>+ /*
>+ * We can only rely on negative dentries having the same
>+ * spelling as the cached dirent if case insensitivity is
>+ * forced on mount.
>+ *
>+ * XXX: if servers correctly announce Case Sensitivity Search
>+ * on GetInfo of FileFSAttributeInformation, then we can take
>+ * correct action even if case insensitive is not forced on
>+ * mount.
>+ *
>+ */
If you're going into the case-sensitiveness hole, several other things
can be fixed re: cached dirs (and other areas, as we know lol):
# mount ... /mnt # not using nocase
# cd /mnt
# mkdir abc
# ls
# ls abc
# ls ABC
# ls ABc
# cat /proc/fs/cifs/open_dirs
# Version:1
# Format:
# <tree id> <sess id> <persistent fid> <path>
Num entries: 4
0x5 0x1d800a8000021 0x760051e015 \ABc valid file info, valid dirents
0x5 0x1d800a8000021 0x760051e012 \ABC valid file info, valid dirents
0x5 0x1d800a8000021 0x760051e00f \abc valid file info, valid dirents
0x5 0x1d800a8000021 0x760051e00c valid file info, valid dirents
So, as I understand, for the 'nocase' cases, it might be worth building
the path string and comparing with strcasecmp().
Other than that, this seems to work fine though.
(also, extra line at the end of the comment)
>+ if (pTcon->nocase && !open_cached_dir_by_dentry(pTcon, direntry->d_parent, &cfid)) {
>+ /*
>+ * dentry is negative and parent is fully cached:
>+ * we can assume file does not exist
>+ */
>+ if (cfid->dirents.is_valid) {
>+ close_cached_dir(cfid);
>+ goto out;
>+ }
>+ close_cached_dir(cfid);
>+ }
> }
> cifs_dbg(FYI, "Full path: %s inode = 0x%p\n",
> full_path, d_inode(direntry));
>@@ -755,6 +779,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> }
> newInode = ERR_PTR(rc);
> }
>+
>+out:
> free_dentry_path(page);
> cifs_put_tlink(tlink);
> free_xid(xid);
>@@ -765,7 +791,11 @@ static int
> cifs_d_revalidate(struct inode *dir, const struct qstr *name,
> struct dentry *direntry, unsigned int flags)
> {
>- struct inode *inode;
>+ struct inode *inode = NULL;
>+ struct cifs_sb_info *cifs_sb;
>+ struct cifs_tcon *tcon;
>+ struct cached_fid *cfid;
>+
extra line
> int rc;
>
> if (flags & LOOKUP_RCU)
>@@ -812,6 +842,22 @@ cifs_d_revalidate(struct inode *dir, const struct qstr *name,
>
> return 1;
> }
>+ } else {
>+ inode = d_inode(direntry->d_parent);
arg @dir == d_inode(direntry->d_parent))
>+ cifs_sb = CIFS_SB(inode->i_sb);
>+ tcon = cifs_sb_master_tcon(cifs_sb);
Positive dentries are the common case, so maybe move tcon and cifs_sb
declarations to this scope?
>+
>+ if (!open_cached_dir_by_dentry(tcon, direntry->d_parent, &cfid)) {
>+ /*
>+ * dentry is negative and parent is fully cached:
>+ * we can assume file does not exist
>+ */
>+ if (cfid->dirents.is_valid) {
>+ close_cached_dir(cfid);
>+ return 1;
>+ }
>+ close_cached_dir(cfid);
>+ }
> }
>
> /*
>--
>2.50.1
Cheers,
Enzo
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/6] smb: client: short-circuit negative lookups when parent dir is fully cached
2025-09-19 18:54 ` Enzo Matsumiya
@ 2025-09-19 20:49 ` Henrique Carvalho
0 siblings, 0 replies; 14+ messages in thread
From: Henrique Carvalho @ 2025-09-19 20:49 UTC (permalink / raw)
To: Enzo Matsumiya; +Cc: smfrench, linux-cifs
Thanks, Enzo.
I'll address the comments in an updated patch.
On 9/19/25 3:54 PM, Enzo Matsumiya wrote:
> On 09/19, Henrique Carvalho wrote:
>> When the parent directory has a valid and complete cached enumeration we
>> can assume that negative dentries are not present in the directory, thus
>> we can return without issuing a request.
>>
>> This reduces traffic for common ENOENT when the directory entries are
>> cached.
>>
>> Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
>
> (unrelated to the patch)
> Can a dentry ever get here being positive? AFAICS it can't, but I might
> have missed some obscure code path.
AFAICS no, but I would also like someone else to confirm.
>
>> +
>> + /*
>> + * We can only rely on negative dentries having the same
>> + * spelling as the cached dirent if case insensitivity is
>> + * forced on mount.
>> + *
>> + * XXX: if servers correctly announce Case Sensitivity Search
>> + * on GetInfo of FileFSAttributeInformation, then we can take
>> + * correct action even if case insensitive is not forced on
>> + * mount.
>> + *
>> + */
>
> If you're going into the case-sensitiveness hole, several other things
> can be fixed re: cached dirs (and other areas, as we know lol):
>
> # mount ... /mnt # not using nocase
> # cd /mnt
> # mkdir abc
> # ls
> # ls abc
> # ls ABC
> # ls ABc
> # cat /proc/fs/cifs/open_dirs
> # Version:1
> # Format:
> # <tree id> <sess id> <persistent fid> <path>
> Num entries: 4
> 0x5 0x1d800a8000021 0x760051e015 \ABc valid file info, valid
> dirents
> 0x5 0x1d800a8000021 0x760051e012 \ABC valid file info, valid
> dirents
> 0x5 0x1d800a8000021 0x760051e00f \abc valid file info, valid
> dirents
> 0x5 0x1d800a8000021 0x760051e00c valid file info, valid dirents
Agree. The code can likely be improved for users who don't care about
case-exact matching.
> So, as I understand, for the 'nocase' cases, it might be worth building
> the path string and comparing with strcasecmp().
>
> Other than that, this seems to work fine though.
>
> (also, extra line at the end of the comment)
>
--
Henrique
SUSE Labs
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/6] smb: client: update cfid->last_access_time in open_cached_dir_by_dentry()
2025-09-19 15:24 [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid Henrique Carvalho
2025-09-19 15:24 ` [PATCH 2/6] smb: client: short-circuit in open_cached_dir_by_dentry() if !dentry Henrique Carvalho
2025-09-19 15:24 ` [PATCH 3/6] smb: client: short-circuit negative lookups when parent dir is fully cached Henrique Carvalho
@ 2025-09-19 15:24 ` Henrique Carvalho
2025-09-19 15:24 ` [PATCH 5/6] smb: client: remove pointless cfid->has_lease check Henrique Carvalho
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Henrique Carvalho @ 2025-09-19 15:24 UTC (permalink / raw)
To: smfrench; +Cc: ematsumiya, linux-cifs, Henrique Carvalho
open_cached_dir_by_dentry() was missing an update of
cfid->last_access_time to jiffies, similar to what open_cached_dir()
has.
Add it to the function.
Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
fs/smb/client/cached_dir.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index df6cabd0966d..22b1c5dd4913 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -427,6 +427,7 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
cifs_dbg(FYI, "found a cached file handle by dentry\n");
kref_get(&cfid->refcount);
*ret_cfid = cfid;
+ cfid->last_access_time = jiffies;
spin_unlock(&cfids->cfid_list_lock);
return 0;
}
--
2.50.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 5/6] smb: client: remove pointless cfid->has_lease check
2025-09-19 15:24 [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid Henrique Carvalho
` (2 preceding siblings ...)
2025-09-19 15:24 ` [PATCH 4/6] smb: client: update cfid->last_access_time in open_cached_dir_by_dentry() Henrique Carvalho
@ 2025-09-19 15:24 ` Henrique Carvalho
2025-09-19 18:55 ` Enzo Matsumiya
2025-09-19 15:24 ` [PATCH 6/6] smb: client: remove unused fid_lock Henrique Carvalho
2025-09-19 15:55 ` [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid Enzo Matsumiya
5 siblings, 1 reply; 14+ messages in thread
From: Henrique Carvalho @ 2025-09-19 15:24 UTC (permalink / raw)
To: smfrench; +Cc: ematsumiya, linux-cifs, Henrique Carvalho
open_cached_dir() will only return a valid cfid, which has both
has_lease = true and time != 0.
Remove the pointless check of cfid->has_lease right after
open_cached_dir() returns no error.
Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
fs/smb/client/smb2ops.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 94b1d7a395d5..e6077e76047a 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -954,11 +954,8 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid);
if (!rc) {
- if (cfid->has_lease) {
- close_cached_dir(cfid);
- return 0;
- }
close_cached_dir(cfid);
+ return 0;
}
utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
--
2.50.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 5/6] smb: client: remove pointless cfid->has_lease check
2025-09-19 15:24 ` [PATCH 5/6] smb: client: remove pointless cfid->has_lease check Henrique Carvalho
@ 2025-09-19 18:55 ` Enzo Matsumiya
2025-10-02 3:39 ` Steve French
0 siblings, 1 reply; 14+ messages in thread
From: Enzo Matsumiya @ 2025-09-19 18:55 UTC (permalink / raw)
To: Henrique Carvalho; +Cc: smfrench, linux-cifs
On 09/19, Henrique Carvalho wrote:
>open_cached_dir() will only return a valid cfid, which has both
>has_lease = true and time != 0.
>
>Remove the pointless check of cfid->has_lease right after
>open_cached_dir() returns no error.
>
>Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
>---
> fs/smb/client/smb2ops.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
>diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>index 94b1d7a395d5..e6077e76047a 100644
>--- a/fs/smb/client/smb2ops.c
>+++ b/fs/smb/client/smb2ops.c
>@@ -954,11 +954,8 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
>
> rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid);
> if (!rc) {
>- if (cfid->has_lease) {
>- close_cached_dir(cfid);
>- return 0;
>- }
> close_cached_dir(cfid);
>+ return 0;
> }
>
> utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
>--
>2.50.1
Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 5/6] smb: client: remove pointless cfid->has_lease check
2025-09-19 18:55 ` Enzo Matsumiya
@ 2025-10-02 3:39 ` Steve French
0 siblings, 0 replies; 14+ messages in thread
From: Steve French @ 2025-10-02 3:39 UTC (permalink / raw)
To: Enzo Matsumiya, Henrique Carvalho, Bharath S M; +Cc: CIFS
Had missed this when for-next updated. Added the RB from Enzo and
readded to cifs-2.6.git for-next
On Fri, Sep 19, 2025 at 1:55 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> On 09/19, Henrique Carvalho wrote:
> >open_cached_dir() will only return a valid cfid, which has both
> >has_lease = true and time != 0.
> >
> >Remove the pointless check of cfid->has_lease right after
> >open_cached_dir() returns no error.
> >
> >Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> >---
> > fs/smb/client/smb2ops.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> >diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> >index 94b1d7a395d5..e6077e76047a 100644
> >--- a/fs/smb/client/smb2ops.c
> >+++ b/fs/smb/client/smb2ops.c
> >@@ -954,11 +954,8 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon,
> >
> > rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid);
> > if (!rc) {
> >- if (cfid->has_lease) {
> >- close_cached_dir(cfid);
> >- return 0;
> >- }
> > close_cached_dir(cfid);
> >+ return 0;
> > }
> >
> > utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
> >--
> >2.50.1
>
> Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 6/6] smb: client: remove unused fid_lock
2025-09-19 15:24 [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid Henrique Carvalho
` (3 preceding siblings ...)
2025-09-19 15:24 ` [PATCH 5/6] smb: client: remove pointless cfid->has_lease check Henrique Carvalho
@ 2025-09-19 15:24 ` Henrique Carvalho
2025-09-19 18:56 ` Enzo Matsumiya
2025-09-19 15:55 ` [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid Enzo Matsumiya
5 siblings, 1 reply; 14+ messages in thread
From: Henrique Carvalho @ 2025-09-19 15:24 UTC (permalink / raw)
To: smfrench; +Cc: ematsumiya, linux-cifs, Henrique Carvalho
The fid_lock in struct cached_fid does not currently provide any real
synchronization. Previously, it had the intention to prevent a double
release of the dentry, but every change to cfid->dentry is already
protected either by cfid_list_lock (while the entry is in the list) or
happens after the cfid has been removed (so no other thread should find
it).
Since there is no scenario in which fid_lock prevents any race, it is
vestigial and can be removed along with its associated
spin_lock()/spin_unlock() calls.
Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
fs/smb/client/cached_dir.c | 17 +++--------------
fs/smb/client/cached_dir.h | 1 -
2 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index 22b1c5dd4913..eba7ce047b63 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -527,10 +527,9 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb)
spin_unlock(&cifs_sb->tlink_tree_lock);
goto done;
}
- spin_lock(&cfid->fid_lock);
+
tmp_list->dentry = cfid->dentry;
cfid->dentry = NULL;
- spin_unlock(&cfid->fid_lock);
list_add_tail(&tmp_list->entry, &entry);
}
@@ -613,14 +612,9 @@ static void cached_dir_put_work(struct work_struct *work)
{
struct cached_fid *cfid = container_of(work, struct cached_fid,
put_work);
- struct dentry *dentry;
-
- spin_lock(&cfid->fid_lock);
- dentry = cfid->dentry;
+ dput(cfid->dentry);
cfid->dentry = NULL;
- spin_unlock(&cfid->fid_lock);
- dput(dentry);
queue_work(serverclose_wq, &cfid->close_work);
}
@@ -678,7 +672,6 @@ static struct cached_fid *init_cached_dir(const char *path)
INIT_LIST_HEAD(&cfid->entry);
INIT_LIST_HEAD(&cfid->dirents.entries);
mutex_init(&cfid->dirents.de_mutex);
- spin_lock_init(&cfid->fid_lock);
kref_init(&cfid->refcount);
return cfid;
}
@@ -730,7 +723,6 @@ static void cfids_laundromat_worker(struct work_struct *work)
{
struct cached_fids *cfids;
struct cached_fid *cfid, *q;
- struct dentry *dentry;
LIST_HEAD(entry);
cfids = container_of(work, struct cached_fids, laundromat_work.work);
@@ -757,12 +749,9 @@ static void cfids_laundromat_worker(struct work_struct *work)
list_for_each_entry_safe(cfid, q, &entry, entry) {
list_del(&cfid->entry);
- spin_lock(&cfid->fid_lock);
- dentry = cfid->dentry;
+ dput(cfid->dentry);
cfid->dentry = NULL;
- spin_unlock(&cfid->fid_lock);
- dput(dentry);
if (cfid->is_open) {
spin_lock(&cifs_tcp_ses_lock);
++cfid->tcon->tc_count;
diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
index aa12382b4249..f4bdb8b28ffa 100644
--- a/fs/smb/client/cached_dir.h
+++ b/fs/smb/client/cached_dir.h
@@ -41,7 +41,6 @@ struct cached_fid {
unsigned long last_access_time; /* jiffies of when last accessed */
struct kref refcount;
struct cifs_fid fid;
- spinlock_t fid_lock;
struct cifs_tcon *tcon;
struct dentry *dentry;
struct work_struct put_work;
--
2.50.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 6/6] smb: client: remove unused fid_lock
2025-09-19 15:24 ` [PATCH 6/6] smb: client: remove unused fid_lock Henrique Carvalho
@ 2025-09-19 18:56 ` Enzo Matsumiya
0 siblings, 0 replies; 14+ messages in thread
From: Enzo Matsumiya @ 2025-09-19 18:56 UTC (permalink / raw)
To: Henrique Carvalho; +Cc: smfrench, linux-cifs
On 09/19, Henrique Carvalho wrote:
>The fid_lock in struct cached_fid does not currently provide any real
>synchronization. Previously, it had the intention to prevent a double
>release of the dentry, but every change to cfid->dentry is already
>protected either by cfid_list_lock (while the entry is in the list) or
>happens after the cfid has been removed (so no other thread should find
>it).
>
>Since there is no scenario in which fid_lock prevents any race, it is
>vestigial and can be removed along with its associated
>spin_lock()/spin_unlock() calls.
>
>Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
>---
> fs/smb/client/cached_dir.c | 17 +++--------------
> fs/smb/client/cached_dir.h | 1 -
> 2 files changed, 3 insertions(+), 15 deletions(-)
>
>diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
>index 22b1c5dd4913..eba7ce047b63 100644
>--- a/fs/smb/client/cached_dir.c
>+++ b/fs/smb/client/cached_dir.c
>@@ -527,10 +527,9 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb)
> spin_unlock(&cifs_sb->tlink_tree_lock);
> goto done;
> }
>- spin_lock(&cfid->fid_lock);
>+
> tmp_list->dentry = cfid->dentry;
> cfid->dentry = NULL;
>- spin_unlock(&cfid->fid_lock);
>
> list_add_tail(&tmp_list->entry, &entry);
> }
>@@ -613,14 +612,9 @@ static void cached_dir_put_work(struct work_struct *work)
> {
> struct cached_fid *cfid = container_of(work, struct cached_fid,
> put_work);
>- struct dentry *dentry;
>-
>- spin_lock(&cfid->fid_lock);
>- dentry = cfid->dentry;
>+ dput(cfid->dentry);
> cfid->dentry = NULL;
>- spin_unlock(&cfid->fid_lock);
>
>- dput(dentry);
> queue_work(serverclose_wq, &cfid->close_work);
> }
>
>@@ -678,7 +672,6 @@ static struct cached_fid *init_cached_dir(const char *path)
> INIT_LIST_HEAD(&cfid->entry);
> INIT_LIST_HEAD(&cfid->dirents.entries);
> mutex_init(&cfid->dirents.de_mutex);
>- spin_lock_init(&cfid->fid_lock);
> kref_init(&cfid->refcount);
> return cfid;
> }
>@@ -730,7 +723,6 @@ static void cfids_laundromat_worker(struct work_struct *work)
> {
> struct cached_fids *cfids;
> struct cached_fid *cfid, *q;
>- struct dentry *dentry;
> LIST_HEAD(entry);
>
> cfids = container_of(work, struct cached_fids, laundromat_work.work);
>@@ -757,12 +749,9 @@ static void cfids_laundromat_worker(struct work_struct *work)
> list_for_each_entry_safe(cfid, q, &entry, entry) {
> list_del(&cfid->entry);
>
>- spin_lock(&cfid->fid_lock);
>- dentry = cfid->dentry;
>+ dput(cfid->dentry);
> cfid->dentry = NULL;
>- spin_unlock(&cfid->fid_lock);
>
>- dput(dentry);
> if (cfid->is_open) {
> spin_lock(&cifs_tcp_ses_lock);
> ++cfid->tcon->tc_count;
>diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
>index aa12382b4249..f4bdb8b28ffa 100644
>--- a/fs/smb/client/cached_dir.h
>+++ b/fs/smb/client/cached_dir.h
>@@ -41,7 +41,6 @@ struct cached_fid {
> unsigned long last_access_time; /* jiffies of when last accessed */
> struct kref refcount;
> struct cifs_fid fid;
>- spinlock_t fid_lock;
> struct cifs_tcon *tcon;
> struct dentry *dentry;
> struct work_struct put_work;
>--
>2.50.1
Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid
2025-09-19 15:24 [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid Henrique Carvalho
` (4 preceding siblings ...)
2025-09-19 15:24 ` [PATCH 6/6] smb: client: remove unused fid_lock Henrique Carvalho
@ 2025-09-19 15:55 ` Enzo Matsumiya
2025-10-02 3:10 ` Steve French
5 siblings, 1 reply; 14+ messages in thread
From: Enzo Matsumiya @ 2025-09-19 15:55 UTC (permalink / raw)
To: Henrique Carvalho; +Cc: smfrench, linux-cifs
On 09/19, Henrique Carvalho wrote:
>open_cached_dir_by_dentry() was exposing an invalid cached directory to
>callers. The validity check outside the function was exclusively based
>on cfid->time.
>
>Add validity check before returning success and introduce
>is_valid_cached_dir() helper for consistent checks across the code.
>
>Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
>---
> fs/smb/client/cached_dir.c | 9 +++++----
> fs/smb/client/cached_dir.h | 6 ++++++
> fs/smb/client/dir.c | 2 +-
> fs/smb/client/inode.c | 2 +-
> 4 files changed, 13 insertions(+), 6 deletions(-)
>
>diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
>index b69daeb1301b..63dc9add4f13 100644
>--- a/fs/smb/client/cached_dir.c
>+++ b/fs/smb/client/cached_dir.c
>@@ -36,9 +36,8 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
> * fully cached or it may be in the process of
> * being deleted due to a lease break.
> */
>- if (!cfid->time || !cfid->has_lease) {
>+ if (!is_valid_cached_dir(cfid))
> return NULL;
>- }
> kref_get(&cfid->refcount);
> return cfid;
> }
>@@ -194,7 +193,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> * Otherwise, it is either a new entry or laundromat worker removed it
> * from @cfids->entries. Caller will put last reference if the latter.
> */
>- if (cfid->has_lease && cfid->time) {
>+ if (is_valid_cached_dir(cfid)) {
> cfid->last_access_time = jiffies;
> spin_unlock(&cfids->cfid_list_lock);
> *ret_cfid = cfid;
>@@ -233,7 +232,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> list_for_each_entry(parent_cfid, &cfids->entries, entry) {
> if (parent_cfid->dentry == dentry->d_parent) {
> cifs_dbg(FYI, "found a parent cached file handle\n");
>- if (parent_cfid->has_lease && parent_cfid->time) {
>+ if (is_valid_cached_dir(parent_cfid)) {
> lease_flags
> |= SMB2_LEASE_FLAG_PARENT_LEASE_KEY_SET_LE;
> memcpy(pfid->parent_lease_key,
>@@ -420,6 +419,8 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
> spin_lock(&cfids->cfid_list_lock);
> list_for_each_entry(cfid, &cfids->entries, entry) {
> if (dentry && cfid->dentry == dentry) {
>+ if (!is_valid_cached_dir(cfid))
>+ break;
> cifs_dbg(FYI, "found a cached file handle by dentry\n");
> kref_get(&cfid->refcount);
> *ret_cfid = cfid;
>diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
>index 46b5a2fdf15b..aa12382b4249 100644
>--- a/fs/smb/client/cached_dir.h
>+++ b/fs/smb/client/cached_dir.h
>@@ -64,6 +64,12 @@ struct cached_fids {
> struct delayed_work laundromat_work;
> };
>
>+static inline bool
>+is_valid_cached_dir(struct cached_fid *cfid)
>+{
>+ return cfid->time && cfid->has_lease;
>+}
>+
> extern struct cached_fids *init_cached_dirs(void);
> extern void free_cached_dirs(struct cached_fids *cfids);
> extern int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
>index 5223edf6d11a..56c59b67ecc2 100644
>--- a/fs/smb/client/dir.c
>+++ b/fs/smb/client/dir.c
>@@ -322,7 +322,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
> list_for_each_entry(parent_cfid, &tcon->cfids->entries, entry) {
> if (parent_cfid->dentry == direntry->d_parent) {
> cifs_dbg(FYI, "found a parent cached file handle\n");
>- if (parent_cfid->has_lease && parent_cfid->time) {
>+ if (is_valid_cached_dir(parent_cfid)) {
> lease_flags
> |= SMB2_LEASE_FLAG_PARENT_LEASE_KEY_SET_LE;
> memcpy(fid->parent_lease_key,
>diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
>index fe453a4b3dc8..9c8b8bd20edd 100644
>--- a/fs/smb/client/inode.c
>+++ b/fs/smb/client/inode.c
>@@ -2639,7 +2639,7 @@ cifs_dentry_needs_reval(struct dentry *dentry)
> return true;
>
> if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) {
>- if (cfid->time && cifs_i->time > cfid->time) {
>+ if (cifs_i->time > cfid->time) {
> close_cached_dir(cfid);
> return false;
> }
>--
>2.50.1
Reviwed-by: Enzo Matsumiya <ematsumiya@suse.de>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid
2025-09-19 15:55 ` [PATCH 1/6] smb: client: ensure open_cached_dir_by_dentry() only returns valid cfid Enzo Matsumiya
@ 2025-10-02 3:10 ` Steve French
0 siblings, 0 replies; 14+ messages in thread
From: Steve French @ 2025-10-02 3:10 UTC (permalink / raw)
To: Enzo Matsumiya; +Cc: Henrique Carvalho, linux-cifs
Enzo,
Added your RB to the 3 patches in Henrique's series that you had indicated.
Also have reordered patches in cifs-2.6.git for-next to put higher
priority/reviewed ones lower down,
and have temporarily pulled out Bharath's patch
("0004-smb-client-cap-smb-directory-cache-memory-via-module.patch")
pending more updates/review etc.
On Fri, Sep 19, 2025 at 10:55 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> On 09/19, Henrique Carvalho wrote:
> >open_cached_dir_by_dentry() was exposing an invalid cached directory to
> >callers. The validity check outside the function was exclusively based
> >on cfid->time.
> >
> >Add validity check before returning success and introduce
> >is_valid_cached_dir() helper for consistent checks across the code.
> >
> >Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
> >---
> > fs/smb/client/cached_dir.c | 9 +++++----
> > fs/smb/client/cached_dir.h | 6 ++++++
> > fs/smb/client/dir.c | 2 +-
> > fs/smb/client/inode.c | 2 +-
> > 4 files changed, 13 insertions(+), 6 deletions(-)
> >
> >diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> >index b69daeb1301b..63dc9add4f13 100644
> >--- a/fs/smb/client/cached_dir.c
> >+++ b/fs/smb/client/cached_dir.c
> >@@ -36,9 +36,8 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
> > * fully cached or it may be in the process of
> > * being deleted due to a lease break.
> > */
> >- if (!cfid->time || !cfid->has_lease) {
> >+ if (!is_valid_cached_dir(cfid))
> > return NULL;
> >- }
> > kref_get(&cfid->refcount);
> > return cfid;
> > }
> >@@ -194,7 +193,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> > * Otherwise, it is either a new entry or laundromat worker removed it
> > * from @cfids->entries. Caller will put last reference if the latter.
> > */
> >- if (cfid->has_lease && cfid->time) {
> >+ if (is_valid_cached_dir(cfid)) {
> > cfid->last_access_time = jiffies;
> > spin_unlock(&cfids->cfid_list_lock);
> > *ret_cfid = cfid;
> >@@ -233,7 +232,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> > list_for_each_entry(parent_cfid, &cfids->entries, entry) {
> > if (parent_cfid->dentry == dentry->d_parent) {
> > cifs_dbg(FYI, "found a parent cached file handle\n");
> >- if (parent_cfid->has_lease && parent_cfid->time) {
> >+ if (is_valid_cached_dir(parent_cfid)) {
> > lease_flags
> > |= SMB2_LEASE_FLAG_PARENT_LEASE_KEY_SET_LE;
> > memcpy(pfid->parent_lease_key,
> >@@ -420,6 +419,8 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
> > spin_lock(&cfids->cfid_list_lock);
> > list_for_each_entry(cfid, &cfids->entries, entry) {
> > if (dentry && cfid->dentry == dentry) {
> >+ if (!is_valid_cached_dir(cfid))
> >+ break;
> > cifs_dbg(FYI, "found a cached file handle by dentry\n");
> > kref_get(&cfid->refcount);
> > *ret_cfid = cfid;
> >diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
> >index 46b5a2fdf15b..aa12382b4249 100644
> >--- a/fs/smb/client/cached_dir.h
> >+++ b/fs/smb/client/cached_dir.h
> >@@ -64,6 +64,12 @@ struct cached_fids {
> > struct delayed_work laundromat_work;
> > };
> >
> >+static inline bool
> >+is_valid_cached_dir(struct cached_fid *cfid)
> >+{
> >+ return cfid->time && cfid->has_lease;
> >+}
> >+
> > extern struct cached_fids *init_cached_dirs(void);
> > extern void free_cached_dirs(struct cached_fids *cfids);
> > extern int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
> >index 5223edf6d11a..56c59b67ecc2 100644
> >--- a/fs/smb/client/dir.c
> >+++ b/fs/smb/client/dir.c
> >@@ -322,7 +322,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
> > list_for_each_entry(parent_cfid, &tcon->cfids->entries, entry) {
> > if (parent_cfid->dentry == direntry->d_parent) {
> > cifs_dbg(FYI, "found a parent cached file handle\n");
> >- if (parent_cfid->has_lease && parent_cfid->time) {
> >+ if (is_valid_cached_dir(parent_cfid)) {
> > lease_flags
> > |= SMB2_LEASE_FLAG_PARENT_LEASE_KEY_SET_LE;
> > memcpy(fid->parent_lease_key,
> >diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> >index fe453a4b3dc8..9c8b8bd20edd 100644
> >--- a/fs/smb/client/inode.c
> >+++ b/fs/smb/client/inode.c
> >@@ -2639,7 +2639,7 @@ cifs_dentry_needs_reval(struct dentry *dentry)
> > return true;
> >
> > if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) {
> >- if (cfid->time && cifs_i->time > cfid->time) {
> >+ if (cifs_i->time > cfid->time) {
> > close_cached_dir(cfid);
> > return false;
> > }
> >--
> >2.50.1
>
> Reviwed-by: Enzo Matsumiya <ematsumiya@suse.de>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 14+ messages in thread