From: Enzo Matsumiya <ematsumiya@suse.de>
To: linux-cifs@vger.kernel.org
Cc: smfrench@gmail.com, pc@manguebit.com, ronniesahlberg@gmail.com,
sprasad@microsoft.com, tom@talpey.com, bharathsm@microsoft.com,
henrique.carvalho@suse.com
Subject: [PATCH v2 12/20] smb: client: prevent lease breaks of cached parents when opening children
Date: Wed, 1 Oct 2025 15:21:11 -0300 [thread overview]
Message-ID: <20251001182111.731114-1-ematsumiya@suse.de> (raw)
In SMB2_open_init() lookup for a cached parent of target path and set
ParentLeaseKey in lease context if found.
Introduce invalidate_cached_dirents() to allow revalidation of cached
dirents but still keep the cached directory.
Testing with e.g. xfstests generic/637 (small simple test) shows a
performance gain of ~17% less create/open ops, and ~83% less lease
breaks:
Before patch
Create/open:
# tshark -Y 'smb2.create.disposition == 1' -r unpatched.pcap
169
Lease breaks:
# tshark -Y 'smb2.cmd == 18' -r unpatched.pcap
12
-----------------
After patch:
Create/open:
# tshark -Y 'smb2.create.disposition == 1' -r patched.pcap
144
Lease breaks:
# tshark -Y 'smb2.cmd == 18' -r patched.pcap
2
Other:
- set oparms->cifs_sb in open_cached_dir() as we need it in
check_cached_parent(); use CIFS_OPARMS() too
- introduce CFID_LOOKUP_PARENT lookup mode (for string paths only)
- add cached_fids->dirsep to save dir separator, used by parent lookups
Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
v2:
- added invalidate_cached_dirents()
- fixed conditions to add ParentLeaseKey
- fixed wrong function name in commit message
- added performance comparison without and with patch to commit message
fs/smb/client/cached_dir.c | 106 ++++++++++++++++++++++++-------------
fs/smb/client/cached_dir.h | 5 ++
fs/smb/client/dir.c | 23 +-------
fs/smb/client/smb2inode.c | 10 +++-
fs/smb/client/smb2pdu.c | 63 +++++++++++++++++-----
5 files changed, 133 insertions(+), 74 deletions(-)
diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index ff71f2c06b72..b48c36500e77 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -71,11 +71,29 @@ static struct cached_fid *find_cfid(struct cached_fids *cfids, const void *key,
bool wait_open)
{
struct cached_fid *cfid, *found;
+ const char *parent_path = NULL;
bool match;
if (!cfids || !key)
return NULL;
+ if (mode == CFID_LOOKUP_PARENT) {
+ const char *path = key;
+
+ if (!*path)
+ return NULL;
+
+ parent_path = strrchr(path, cfids->dirsep);
+ if (!parent_path)
+ return NULL;
+
+ parent_path = kstrndup(path, parent_path - path, GFP_KERNEL);
+ if (WARN_ON_ONCE(!parent_path))
+ return NULL;
+
+ key = parent_path;
+ mode = CFID_LOOKUP_PATH;
+ }
retry_find:
found = NULL;
@@ -114,6 +132,8 @@ static struct cached_fid *find_cfid(struct cached_fids *cfids, const void *key,
kref_get(&found->refcount);
}
+ kfree(parent_path);
+
return found;
}
@@ -226,7 +246,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, const char *path,
struct cached_fids *cfids;
const char *npath;
int retries = 0, cur_sleep = 1;
- __le32 lease_flags = 0;
if (cifs_sb->root == NULL)
return -ENOENT;
@@ -236,9 +255,11 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, const char *path,
ses = tcon->ses;
cfids = tcon->cfids;
-
if (!cfids)
return -EOPNOTSUPP;
+
+ if (!cfids->dirsep)
+ cfids->dirsep = CIFS_DIR_SEP(cifs_sb);
replay_again:
/* reinitialize for possible replay */
flags = 0;
@@ -306,24 +327,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, const char *path,
rc = -ENOENT;
goto out;
}
- if (dentry->d_parent && server->dialect >= SMB30_PROT_ID) {
- struct cached_fid *parent_cfid;
-
- spin_lock(&cfids->cfid_list_lock);
- list_for_each_entry(parent_cfid, &cfids->entries, entry) {
- if (parent_cfid->dentry == dentry->d_parent) {
- if (!cfid_is_valid(parent_cfid))
- break;
-
- cifs_dbg(FYI, "found a parent cached file handle\n");
- lease_flags |= SMB2_LEASE_FLAG_PARENT_LEASE_KEY_SET_LE;
- memcpy(pfid->parent_lease_key, parent_cfid->fid.lease_key,
- SMB2_LEASE_KEY_SIZE);
- break;
- }
- }
- spin_unlock(&cfids->cfid_list_lock);
- }
}
cfid->dentry = dentry;
cfid->tcon = tcon;
@@ -350,20 +353,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, const char *path,
rqst[0].rq_iov = open_iov;
rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE;
- oparms = (struct cifs_open_parms) {
- .tcon = tcon,
- .path = path,
- .create_options = cifs_create_options(cifs_sb, CREATE_NOT_FILE),
- .desired_access = FILE_READ_DATA | FILE_READ_ATTRIBUTES |
- FILE_READ_EA,
- .disposition = FILE_OPEN,
- .fid = pfid,
- .lease_flags = lease_flags,
- .replay = !!(retries),
- };
-
- rc = SMB2_open_init(tcon, server,
- &rqst[0], &oplock, &oparms, utf16_path);
+ oparms = CIFS_OPARMS(cifs_sb, tcon, path,
+ FILE_READ_DATA | FILE_READ_ATTRIBUTES | FILE_READ_EA, FILE_OPEN,
+ cifs_create_options(cifs_sb, CREATE_NOT_FILE), 0);
+ oparms.fid = pfid;
+ oparms.replay = !!retries;
+
+ rc = SMB2_open_init(tcon, server, &rqst[0], &oplock, &oparms, utf16_path);
if (rc)
goto oshr_free;
smb2_set_next_command(tcon, &rqst[0]);
@@ -478,20 +474,34 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, const char *path,
return rc;
}
-static void smb2_close_cached_fid(struct kref *ref)
+static void __invalidate_cached_dirents(struct cached_fid *cfid)
{
- struct cached_fid *cfid = container_of(ref, struct cached_fid, refcount);
struct cached_dirent *de, *q;
- drop_cfid(cfid);
+ if (!cfid)
+ return;
- /* Delete all cached dirent names */
+ mutex_lock(&cfid->dirents.de_mutex);
list_for_each_entry_safe(de, q, &cfid->dirents.entries, entry) {
list_del(&de->entry);
kfree(de->name);
kfree(de);
}
+ cfid->dirents.is_valid = false;
+ cfid->dirents.is_failed = false;
+ cfid->dirents.file = NULL;
+ cfid->dirents.pos = 0;
+ mutex_unlock(&cfid->dirents.de_mutex);
+}
+
+static void smb2_close_cached_fid(struct kref *ref)
+{
+ struct cached_fid *cfid = container_of(ref, struct cached_fid, refcount);
+
+ __invalidate_cached_dirents(cfid);
+ drop_cfid(cfid);
+
kfree(cfid->file_all_info);
cfid->file_all_info = NULL;
kfree(cfid->path);
@@ -531,6 +541,26 @@ bool drop_cached_dir(struct cached_fids *cfids, const void *key, int mode)
return true;
}
+/*
+ * Invalidate cached dirents from @key's parent, regardless if @key itself is a cached dir.
+ *
+ * Lease breaks don't necessarily require this, and would require finding the child to begin with,
+ * so skip such cases.
+ */
+void invalidate_cached_dirents(struct cached_fids *cfids, const void *key, int mode)
+{
+ struct cached_fid *cfid = NULL;
+
+ if (mode == CFID_LOOKUP_LEASEKEY)
+ return;
+
+ cfid = find_cfid(cfids, key, mode, false);
+ if (cfid) {
+ __invalidate_cached_dirents(cfid);
+ kref_put(&cfid->refcount, smb2_close_cached_fid);
+ }
+}
+
void close_cached_dir(struct cached_fid *cfid)
{
kref_put(&cfid->refcount, smb2_close_cached_fid);
diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
index c45151446049..343963a589e6 100644
--- a/fs/smb/client/cached_dir.h
+++ b/fs/smb/client/cached_dir.h
@@ -54,11 +54,15 @@ struct cached_fids {
int num_entries;
struct list_head entries;
struct delayed_work laundromat_work;
+
+ /* convenience for parent lookups */
+ int dirsep;
};
/* Lookup modes for find_cached_dir() */
enum {
CFID_LOOKUP_PATH,
+ CFID_LOOKUP_PARENT,
CFID_LOOKUP_DENTRY,
CFID_LOOKUP_LEASEKEY,
};
@@ -79,6 +83,7 @@ extern int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, const char
struct cifs_sb_info *cifs_sb, struct cached_fid **cfid);
extern void close_cached_dir(struct cached_fid *cfid);
extern bool drop_cached_dir(struct cached_fids *cfids, const void *key, int mode);
+extern void invalidate_cached_dirents(struct cached_fids *cfids, const void *key, int mode);
extern void close_all_cached_dirs(struct cifs_sb_info *cifs_sb);
extern void invalidate_all_cached_dirs(struct cached_fids *cfids);
#endif /* _CACHED_DIR_H */
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index e5372c2c799d..03aa54edba3e 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -190,9 +190,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
int disposition;
struct TCP_Server_Info *server = tcon->ses->server;
struct cifs_open_parms oparms;
- struct cached_fid *parent_cfid = NULL;
int rdwr_for_fscache = 0;
- __le32 lease_flags = 0;
*oplock = 0;
if (tcon->ses->server->oplocks)
@@ -314,26 +312,8 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
if (!tcon->unix_ext && (mode & S_IWUGO) == 0)
create_options |= CREATE_OPTION_READONLY;
-
retry_open:
- if (tcon->cfids && direntry->d_parent && server->dialect >= SMB30_PROT_ID) {
- parent_cfid = NULL;
- spin_lock(&tcon->cfids->cfid_list_lock);
- list_for_each_entry(parent_cfid, &tcon->cfids->entries, entry) {
- if (parent_cfid->dentry == direntry->d_parent) {
- if (!cfid_is_valid(parent_cfid))
- break;
-
- cifs_dbg(FYI, "found a parent cached file handle\n");
- lease_flags |= SMB2_LEASE_FLAG_PARENT_LEASE_KEY_SET_LE;
- memcpy(fid->parent_lease_key, parent_cfid->fid.lease_key,
- SMB2_LEASE_KEY_SIZE);
- parent_cfid->dirents.is_valid = false;
- break;
- }
- }
- spin_unlock(&tcon->cfids->cfid_list_lock);
- }
+ invalidate_cached_dirents(tcon->cfids, direntry->d_parent, CFID_LOOKUP_DENTRY);
oparms = (struct cifs_open_parms) {
.tcon = tcon,
@@ -343,7 +323,6 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
.disposition = disposition,
.path = full_path,
.fid = fid,
- .lease_flags = lease_flags,
.mode = mode,
};
rc = server->ops->open(xid, &oparms, oplock, buf);
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 8ccdd1a3ba2c..53bc1250391f 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -1120,6 +1120,8 @@ smb2_mkdir(const unsigned int xid, struct inode *parent_inode, umode_t mode,
{
struct cifs_open_parms oparms;
+ invalidate_cached_dirents(tcon->cfids, name, CFID_LOOKUP_PARENT);
+
oparms = CIFS_OPARMS(cifs_sb, tcon, name, FILE_WRITE_ATTRIBUTES,
FILE_CREATE, CREATE_NOT_FILE, mode);
return smb2_compound_op(xid, tcon, cifs_sb,
@@ -1141,6 +1143,8 @@ smb2_mkdir_setinfo(struct inode *inode, const char *name,
u32 dosattrs;
int tmprc;
+ invalidate_cached_dirents(tcon->cfids, name, CFID_LOOKUP_PARENT);
+
in_iov.iov_base = &data;
in_iov.iov_len = sizeof(data);
cifs_i = CIFS_I(inode);
@@ -1180,8 +1184,12 @@ smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
struct inode *inode = NULL;
int rc;
- if (dentry)
+ if (dentry) {
inode = d_inode(dentry);
+ invalidate_cached_dirents(tcon->cfids, dentry->d_parent, CFID_LOOKUP_DENTRY);
+ } else {
+ invalidate_cached_dirents(tcon->cfids, name, CFID_LOOKUP_PARENT);
+ }
oparms = CIFS_OPARMS(cifs_sb, tcon, name, DELETE,
FILE_OPEN, OPEN_REPARSE_POINT, ACL_NO_MODE);
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 07ba61583114..f85a2e2555a2 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -2419,7 +2419,8 @@ add_lease_context(struct TCP_Server_Info *server,
if (iov[num].iov_base == NULL)
return -ENOMEM;
iov[num].iov_len = server->vals->create_lease_size;
- req->RequestedOplockLevel = SMB2_OPLOCK_LEVEL_LEASE;
+ /* keep the requested oplock level in case of just setting ParentLeaseKey */
+ req->RequestedOplockLevel = *oplock;
*num_iovec = num + 1;
return 0;
}
@@ -3001,6 +3002,34 @@ int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
return rc;
}
+/*
+ * When opening a path, set ParentLeaseKey in @oparms if its parent is cached.
+ * We only have RH caching for dirs, so skip this on mkdir, unlink, rmdir.
+ *
+ * Ref: MS-SMB2 3.3.5.9 and MS-FSA 2.1.5.1
+ *
+ * Return: 0 if ParentLeaseKey was set in @oparms, -errno otherwise.
+ */
+static int check_cached_parent(struct cached_fids *cfids, struct cifs_open_parms *oparms)
+{
+ struct cached_fid *cfid;
+
+ if (!cfids || !oparms || !oparms->cifs_sb || !*oparms->path)
+ return -EINVAL;
+
+ cfid = find_cached_dir(cfids, oparms->path, CFID_LOOKUP_PARENT);
+ if (!cfid)
+ return -ENOENT;
+
+ cifs_dbg(FYI, "%s: found cached parent for path: %s\n", __func__, oparms->path);
+
+ memcpy(oparms->fid->parent_lease_key, cfid->fid.lease_key, SMB2_LEASE_KEY_SIZE);
+ oparms->lease_flags |= SMB2_LEASE_FLAG_PARENT_LEASE_KEY_SET_LE;
+ close_cached_dir(cfid);
+
+ return 0;
+}
+
int
SMB2_open_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server,
struct smb_rqst *rqst, __u8 *oplock,
@@ -3077,20 +3106,28 @@ SMB2_open_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server,
iov[1].iov_len = uni_path_len;
iov[1].iov_base = path;
- if ((!server->oplocks) || (tcon->no_lease))
+ if (!server->oplocks || tcon->no_lease)
*oplock = SMB2_OPLOCK_LEVEL_NONE;
- if (!(server->capabilities & SMB2_GLOBAL_CAP_LEASING) ||
- *oplock == SMB2_OPLOCK_LEVEL_NONE)
- req->RequestedOplockLevel = *oplock;
- else if (!(server->capabilities & SMB2_GLOBAL_CAP_DIRECTORY_LEASING) &&
- (oparms->create_options & CREATE_NOT_FILE))
- req->RequestedOplockLevel = *oplock; /* no srv lease support */
- else {
- rc = add_lease_context(server, req, iov, &n_iov,
- oparms->fid->lease_key, oplock,
- oparms->fid->parent_lease_key,
- oparms->lease_flags);
+ req->RequestedOplockLevel = *oplock;
+
+ /*
+ * MS-SMB2 "Product Behavior" says Windows only checks/sets ParentLeaseKey when a lease is
+ * requested for the child/target.
+ * Practically speaking, adding the lease context with ParentLeaseKey set, even with oplock
+ * none, works fine.
+ * As a precaution, however, only set it for oplocks != none.
+ */
+ if ((server->capabilities & SMB2_GLOBAL_CAP_LEASING) &&
+ *oplock != SMB2_OPLOCK_LEVEL_NONE) {
+ rc = -EOPNOTSUPP;
+ if (server->capabilities & SMB2_GLOBAL_CAP_DIRECTORY_LEASING)
+ rc = check_cached_parent(tcon->cfids, oparms);
+
+ if (!rc || *oplock != SMB2_OPLOCK_LEVEL_NONE)
+ rc = add_lease_context(server, req, iov, &n_iov, oparms->fid->lease_key,
+ oplock, oparms->fid->parent_lease_key,
+ oparms->lease_flags);
if (rc)
return rc;
}
--
2.49.0
next reply other threads:[~2025-10-01 18:22 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-01 18:21 Enzo Matsumiya [this message]
-- strict thread matches above, loose matches on Subject: below --
2025-10-07 17:42 [PATCH v2 00/20] cached dir fixes and improvements Enzo Matsumiya
2025-10-07 17:42 ` [PATCH v2 12/20] smb: client: prevent lease breaks of cached parents when opening children Enzo Matsumiya
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251001182111.731114-1-ematsumiya@suse.de \
--to=ematsumiya@suse.de \
--cc=bharathsm@microsoft.com \
--cc=henrique.carvalho@suse.com \
--cc=linux-cifs@vger.kernel.org \
--cc=pc@manguebit.com \
--cc=ronniesahlberg@gmail.com \
--cc=smfrench@gmail.com \
--cc=sprasad@microsoft.com \
--cc=tom@talpey.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).