Cifs: caching of arbitrary directories and attributes

Cifs: caching of arbitrary directories and attributes

[Ronnie Sahlberg]

Steve, List

Here is an updated list of the additional patches needed to expand directory caching based on leases.

The first three patches should be small and trivial. They do not change functionality, just preparing
for the fourth, big patch.
The fourth patch is the meat of the series. This is where we change from a static allocatoin to
cache only the root-directory to instead dynamically cache any (within limits) directory.
The fourth patch contains a large comment describing the strategies for locking and reference counts
for these structures to make review easier.

The fifth patch expands how we cache attributes for the entries in these directories, which is based
on holding a reference to the dentry for the directory. (so that inode.c can find it)
This patch allows SAFE caching of all attributes in these directories for a long time and will
reduce the need for the ad-hoc and unsafe "cache the attributes for some time to prioritize performance"
which we have done for a long time. This caching is based and triggered from opendir(), so it will
only apply to directories we have already scanned (and read the entries and attributes for)

The sixth patch is a patch to recind the lease after an arbitrarily long timeout.

[PATCH 1/6] cifs: Make tcon contain a wrapper structure cached_fids instead of cached_fid

[Ronnie Sahlberg]

This wrapper structure will later be expanded to contain a list of
fids that are cached and not just the root fid.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cached_dir.c | 50 ++++++++++++++++++++++++--------------------
 fs/cifs/cached_dir.h |  8 +++++--
 fs/cifs/cifsglob.h   |  2 +-
 fs/cifs/misc.c       |  6 +++---
 4 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
index b401339f6e73..c2f5b71a3c9f 100644
--- a/fs/cifs/cached_dir.c
+++ b/fs/cifs/cached_dir.c
@@ -52,7 +52,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 
 	dentry = cifs_sb->root;
 
-	cfid = tcon->cfid;
+	cfid = &tcon->cfids->cfid;
 	mutex_lock(&cfid->fid_mutex);
 	if (cfid->is_valid) {
 		cifs_dbg(FYI, "found a cached root file handle\n");
@@ -226,7 +226,7 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
 {
 	struct cached_fid *cfid;
 
-	cfid = tcon->cfid;
+	cfid = &tcon->cfids->cfid;
 
 	mutex_lock(&cfid->fid_mutex);
 	if (cfid->dentry == dentry) {
@@ -320,7 +320,7 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb)
 		tcon = tlink_tcon(tlink);
 		if (IS_ERR(tcon))
 			continue;
-		cfid = tcon->cfid;
+		cfid = &tcon->cfids->cfid;
 		mutex_lock(&cfid->fid_mutex);
 		if (cfid->dentry) {
 			dput(cfid->dentry);
@@ -336,12 +336,14 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb)
  */
 void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
 {
-	mutex_lock(&tcon->cfid->fid_mutex);
-	tcon->cfid->is_valid = false;
+	struct cached_fid *cfid = &tcon->cfids->cfid;
+
+	mutex_lock(&cfid->fid_mutex);
+	cfid->is_valid = false;
 	/* cached handle is not valid, so SMB2_CLOSE won't be sent below */
-	close_cached_dir_lease_locked(tcon->cfid);
-	memset(&tcon->cfid->fid, 0, sizeof(struct cifs_fid));
-	mutex_unlock(&tcon->cfid->fid_mutex);
+	close_cached_dir_lease_locked(cfid);
+	memset(&cfid->fid, 0, sizeof(struct cifs_fid));
+	mutex_unlock(&cfid->fid_mutex);
 }
 
 static void
@@ -355,34 +357,36 @@ smb2_cached_lease_break(struct work_struct *work)
 
 int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
 {
-	if (tcon->cfid->is_valid &&
+	struct cached_fid *cfid = &tcon->cfids->cfid;
+
+	if (cfid->is_valid &&
 	    !memcmp(lease_key,
-		    tcon->cfid->fid.lease_key,
+		    cfid->fid.lease_key,
 		    SMB2_LEASE_KEY_SIZE)) {
-		tcon->cfid->time = 0;
-		INIT_WORK(&tcon->cfid->lease_break,
+		cfid->time = 0;
+		INIT_WORK(&cfid->lease_break,
 			  smb2_cached_lease_break);
 		queue_work(cifsiod_wq,
-			   &tcon->cfid->lease_break);
+			   &cfid->lease_break);
 		return true;
 	}
 	return false;
 }
 
-struct cached_fid *init_cached_dir(void)
+struct cached_fids *init_cached_dirs(void)
 {
-	struct cached_fid *cfid;
+	struct cached_fids *cfids;
 
-	cfid = kzalloc(sizeof(*cfid), GFP_KERNEL);
-	if (!cfid)
+	cfids = kzalloc(sizeof(*cfids), GFP_KERNEL);
+	if (!cfids)
 		return NULL;
-	INIT_LIST_HEAD(&cfid->dirents.entries);
-	mutex_init(&cfid->dirents.de_mutex);
-	mutex_init(&cfid->fid_mutex);
-	return cfid;
+	INIT_LIST_HEAD(&cfids->cfid.dirents.entries);
+	mutex_init(&cfids->cfid.dirents.de_mutex);
+	mutex_init(&cfids->cfid.fid_mutex);
+	return cfids;
 }
 
-void free_cached_dir(struct cifs_tcon *tcon)
+void free_cached_dirs(struct cached_fids *cfids)
 {
-	kfree(tcon->cfid);
+	kfree(cfids);
 }
diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
index bd262dc8b179..e430e1102296 100644
--- a/fs/cifs/cached_dir.h
+++ b/fs/cifs/cached_dir.h
@@ -45,8 +45,12 @@ struct cached_fid {
 	struct cached_dirents dirents;
 };
 
-extern struct cached_fid *init_cached_dir(void);
-extern void free_cached_dir(struct cifs_tcon *tcon);
+struct cached_fids {
+	struct cached_fid cfid;
+};
+
+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,
 			   const char *path,
 			   struct cifs_sb_info *cifs_sb,
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f15d7b0c123d..768003e52cc9 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1221,7 +1221,7 @@ struct cifs_tcon {
 	struct fscache_volume *fscache;	/* cookie for share */
 #endif
 	struct list_head pending_opens;	/* list of incomplete opens */
-	struct cached_fid *cfid; /* Cached root fid */
+	struct cached_fids *cfids;
 	/* BB add field for back pointer to sb struct(s)? */
 #ifdef CONFIG_CIFS_DFS_UPCALL
 	struct list_head ulist; /* cache update list */
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 87f60f736731..5cb7e1ae00a4 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -117,8 +117,8 @@ tconInfoAlloc(void)
 	ret_buf = kzalloc(sizeof(*ret_buf), GFP_KERNEL);
 	if (!ret_buf)
 		return NULL;
-	ret_buf->cfid = init_cached_dir();
-	if (!ret_buf->cfid) {
+	ret_buf->cfids = init_cached_dirs();
+	if (!ret_buf->cfids) {
 		kfree(ret_buf);
 		return NULL;
 	}
@@ -144,7 +144,7 @@ tconInfoFree(struct cifs_tcon *tcon)
 		cifs_dbg(FYI, "Null buffer passed to tconInfoFree\n");
 		return;
 	}
-	free_cached_dir(tcon);
+	free_cached_dirs(tcon->cfids);
 	atomic_dec(&tconInfoAllocCount);
 	kfree(tcon->nativeFileSystem);
 	kfree_sensitive(tcon->password);
-- 
2.35.3

[PATCH 2/6] cifs: cifs: handlecache, only track the dentry for the root handle

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cached_dir.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
index c2f5b71a3c9f..77880470c7ea 100644
--- a/fs/cifs/cached_dir.c
+++ b/fs/cifs/cached_dir.c
@@ -47,11 +47,12 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	if (cifs_sb->root == NULL)
 		return -ENOENT;
 
+	if (!strlen(path))
+		dentry = cifs_sb->root;
+
 	if (strlen(path))
 		return -ENOENT;
 
-	dentry = cifs_sb->root;
-
 	cfid = &tcon->cfids->cfid;
 	mutex_lock(&cfid->fid_mutex);
 	if (cfid->is_valid) {
@@ -177,7 +178,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	cfid->tcon = tcon;
 	cfid->is_valid = true;
 	cfid->dentry = dentry;
-	dget(dentry);
+	if (dentry)
+		dget(dentry);
 	kref_init(&cfid->refcount);
 
 	/* BB TBD check to see if oplock level check can be removed below */
-- 
2.35.3

[PATCH 3/6] cifs: store a pointer to a fid in the cfid structure instead of the struct

[Ronnie Sahlberg]

also create a constructor that takes a path name and stores it in the fid.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cached_dir.c | 63 ++++++++++++++++++++++++++++++++++++++------
 fs/cifs/cached_dir.h |  4 ++-
 2 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
index 77880470c7ea..594ec4385077 100644
--- a/fs/cifs/cached_dir.c
+++ b/fs/cifs/cached_dir.c
@@ -11,6 +11,8 @@
 #include "smb2proto.h"
 #include "cached_dir.h"
 
+struct cached_fid *init_cached_dir(const char *path);
+
 /*
  * Open the and cache a directory handle.
  * If error then *cfid is not initialized.
@@ -53,7 +55,14 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	if (strlen(path))
 		return -ENOENT;
 
-	cfid = &tcon->cfids->cfid;
+	cfid = tcon->cfids->cfid;
+	if (cfid == NULL) {
+		cfid = init_cached_dir(path);
+		tcon->cfids->cfid = cfid;
+	}
+	if (cfid == NULL)
+		return -ENOMEM;
+
 	mutex_lock(&cfid->fid_mutex);
 	if (cfid->is_valid) {
 		cifs_dbg(FYI, "found a cached root file handle\n");
@@ -228,7 +237,9 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
 {
 	struct cached_fid *cfid;
 
-	cfid = &tcon->cfids->cfid;
+	cfid = tcon->cfids->cfid;
+	if (cfid == NULL)
+		return -ENOENT;
 
 	mutex_lock(&cfid->fid_mutex);
 	if (cfid->dentry == dentry) {
@@ -322,7 +333,9 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb)
 		tcon = tlink_tcon(tlink);
 		if (IS_ERR(tcon))
 			continue;
-		cfid = &tcon->cfids->cfid;
+		cfid = tcon->cfids->cfid;
+		if (cfid == NULL)
+			continue;
 		mutex_lock(&cfid->fid_mutex);
 		if (cfid->dentry) {
 			dput(cfid->dentry);
@@ -338,7 +351,10 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb)
  */
 void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
 {
-	struct cached_fid *cfid = &tcon->cfids->cfid;
+	struct cached_fid *cfid = tcon->cfids->cfid;
+
+	if (cfid == NULL)
+		return;
 
 	mutex_lock(&cfid->fid_mutex);
 	cfid->is_valid = false;
@@ -359,7 +375,10 @@ smb2_cached_lease_break(struct work_struct *work)
 
 int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
 {
-	struct cached_fid *cfid = &tcon->cfids->cfid;
+	struct cached_fid *cfid = tcon->cfids->cfid;
+
+	if (cfid == NULL)
+		return false;
 
 	if (cfid->is_valid &&
 	    !memcmp(lease_key,
@@ -375,6 +394,32 @@ int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
 	return false;
 }
 
+struct cached_fid *init_cached_dir(const char *path)
+{
+	struct cached_fid *cfid;
+
+	cfid = kzalloc(sizeof(*cfid), GFP_KERNEL);
+	if (!cfid)
+		return NULL;
+	cfid->path = kstrdup(path, GFP_KERNEL);
+	if (!cfid->path) {
+		kfree(cfid);
+		return NULL;
+	}
+
+	INIT_LIST_HEAD(&cfid->dirents.entries);
+	mutex_init(&cfid->dirents.de_mutex);
+	mutex_init(&cfid->fid_mutex);
+	return cfid;
+}
+
+void free_cached_dir(struct cached_fid *cfid)
+{
+	kfree(cfid->path);
+	cfid->path = NULL;
+	kfree(cfid);
+}
+
 struct cached_fids *init_cached_dirs(void)
 {
 	struct cached_fids *cfids;
@@ -382,13 +427,15 @@ struct cached_fids *init_cached_dirs(void)
 	cfids = kzalloc(sizeof(*cfids), GFP_KERNEL);
 	if (!cfids)
 		return NULL;
-	INIT_LIST_HEAD(&cfids->cfid.dirents.entries);
-	mutex_init(&cfids->cfid.dirents.de_mutex);
-	mutex_init(&cfids->cfid.fid_mutex);
+	mutex_init(&cfids->cfid_list_mutex);
 	return cfids;
 }
 
 void free_cached_dirs(struct cached_fids *cfids)
 {
+	if (cfids->cfid) {
+		free_cached_dir(cfids->cfid);
+		cfids->cfid = NULL;
+	}
 	kfree(cfids);
 }
diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
index e430e1102296..bdf6c3866653 100644
--- a/fs/cifs/cached_dir.h
+++ b/fs/cifs/cached_dir.h
@@ -31,6 +31,7 @@ struct cached_dirents {
 };
 
 struct cached_fid {
+	const char *path;
 	bool is_valid:1;	/* Do we have a useable root fid */
 	bool file_all_info_is_valid:1;
 	bool has_lease:1;
@@ -46,7 +47,8 @@ struct cached_fid {
 };
 
 struct cached_fids {
-	struct cached_fid cfid;
+	struct mutex cfid_list_mutex;
+	struct cached_fid *cfid;
 };
 
 extern struct cached_fids *init_cached_dirs(void);
-- 
2.35.3

[PATCH 4/6] cifs: start caching all directories we open and get a lease for

[Ronnie Sahlberg]

This expands the directory caching to now cache an open handle for all
directories (up to a maximum) and not just the root directory.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cached_dir.c | 490 ++++++++++++++++++++++++++-----------------
 fs/cifs/cached_dir.h |  20 +-
 fs/cifs/inode.c      |   6 +-
 fs/cifs/smb2ops.c    |   2 +-
 4 files changed, 320 insertions(+), 198 deletions(-)

diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
index 594ec4385077..8732903aea03 100644
--- a/fs/cifs/cached_dir.c
+++ b/fs/cifs/cached_dir.c
@@ -11,7 +11,107 @@
 #include "smb2proto.h"
 #include "cached_dir.h"
 
-struct cached_fid *init_cached_dir(const char *path);
+static struct cached_fid *init_cached_dir(const char *path);
+static void free_cached_dir(struct cached_fid *cfid);
+
+/*
+ * Locking and reference count for cached directory handles:
+ *
+ * The main function to get a reference to a cached handle is
+ * find_or_create_cached_dir() called from open_cached_dir()
+ * These functions are protected under the cfid_list_lock spin-lock
+ * to make sure we do not race creating new references for cached dirs
+ * with deletion of expired ones.
+ *
+ * An successful open_cached_dir() will take out 2 references to the cfid if
+ * this was the very first and successful call to open the directory and
+ * it acquired a lease from the server.
+ * One reference is for the lease  and the other is for the cfid that we
+ * return. The is lease reference is tracked by cfid->has_lease.
+ * If the directory already has a handle with an active lease, then we just
+ * take out one new reference for the cfid and return it.
+ * It can happen that we have a thread that tries to open a cached directory
+ * where we have a cfid already but we do not, yet, have a working lease. In
+ * this case we will just return NULL, and this the caller will fall back to
+ * the case when no handle was available.
+ *
+ * In this model the total number of references we have on a cfid is
+ * 1 for while the handle is open and we have a lease, and one additional
+ * reference for each open instance of a cfid.
+ *
+ *
+ * Once we get a lease break (cached_dir_lease_break()) we remove the
+ * cfid from the list under the spinlock. This prevents any new threads to
+ * use it, and we also call smb2_cached_lease_break() via the work_queue
+ * in order to drop the reference we got for the lease (we drop it outside
+ * of the spin-lock.)
+ * Anytime a thread calls close_cached_dir() we also drop a reference to the
+ * cfid.
+ * When the last reference to the cfid is released smb2_close_cached_fid()
+ * will be invoked which will drop the reference ot the dentry we held for
+ * this cfid and it will also, if we the handle is open/has a lease
+ * also call SMB2_close() to close the handle on the server.
+ *
+ *
+ * Two events require special handling:
+ * invalidate_all_cached_dirs() this function is called from SMB2_tdis()
+ * and cifs_mark_open_files_invalid().
+ * In both cases the tcon is either gone already or will be shortly so
+ * we do not need to actually close the handles. They will be dropped
+ * server side as part of the tcon dropping.
+ * But we have to be careful about a potential race with a concurrent
+ * lease break so we need to take out additional refences to avoid the
+ * cfid from being freed while we are still referencing it.
+ *
+ * free_cached_dirs() which is called from tconInfoFree().
+ * This is called quite late in the umount process so there should no longer
+ * be any open handles or files and we can just free all the remaining data.
+ */
+
+static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
+						    const char *path,
+						    bool lookup_only)
+{
+	struct cached_fid *cfid;
+
+	spin_lock(&cfids->cfid_list_lock);
+	list_for_each_entry(cfid, &cfids->entries, entry) {
+		if (!strcmp(cfid->path, path)) {
+			/*
+			 * If it doesn't have a lease it is either not yet
+			 * fully cached or it may be in the process of
+			 * being deleted due to a lease break.
+			 */
+			if (!cfid->has_lease) {
+				spin_unlock(&cfids->cfid_list_lock);
+				return NULL;
+			}
+			kref_get(&cfid->refcount);
+			spin_unlock(&cfids->cfid_list_lock);
+			return cfid;
+		}
+	}
+	if (lookup_only) {
+		spin_unlock(&cfids->cfid_list_lock);
+		return NULL;
+	}
+	if (cfids->num_entries >= MAX_CACHED_FIDS) {
+		spin_unlock(&cfids->cfid_list_lock);
+		return NULL;
+	}
+	cfid = init_cached_dir(path);
+	if (cfid == NULL) {
+		spin_unlock(&cfids->cfid_list_lock);
+		return NULL;
+	}
+	cfid->cfids = cfids;
+	cfids->num_entries++;
+	list_add(&cfid->entry, &cfids->entries);
+	cfid->on_list = true;
+	kref_get(&cfid->refcount);
+	spin_unlock(&cfids->cfid_list_lock);
+	return cfid;
+}
 
 /*
  * Open the and cache a directory handle.
@@ -33,62 +133,65 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
 	struct kvec qi_iov[1];
 	int rc, flags = 0;
-	__le16 utf16_path = 0; /* Null - since an open of top of share */
+	__le16 *utf16_path = NULL;
 	u8 oplock = SMB2_OPLOCK_LEVEL_II;
 	struct cifs_fid *pfid;
-	struct dentry *dentry;
+	struct dentry *dentry = NULL;
 	struct cached_fid *cfid;
+	struct cached_fids *cfids;
+	  
 
-	if (tcon == NULL || tcon->nohandlecache ||
+	if (tcon == NULL || tcon->cfids == NULL || tcon->nohandlecache ||
 	    is_smb1_server(tcon->ses->server))
-		return -EOPNOTSUPP;
+		return -ENOTSUPP;
 
 	ses = tcon->ses;
 	server = ses->server;
+	cfids = tcon->cfids;
 
+	if (!server->ops->new_lease_key)
+		return -EIO;
+	
 	if (cifs_sb->root == NULL)
 		return -ENOENT;
 
+	/*
+	 * TODO: for better caching we need to find and use the dentry also
+	 * for non-root directories.
+	 */
 	if (!strlen(path))
 		dentry = cifs_sb->root;
 
-	if (strlen(path))
-		return -ENOENT;
+	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
+	if (!utf16_path)
+		return -ENOMEM;
 
-	cfid = tcon->cfids->cfid;
+	cfid = find_or_create_cached_dir(cfids, path, lookup_only);
 	if (cfid == NULL) {
-		cfid = init_cached_dir(path);
-		tcon->cfids->cfid = cfid;
+		kfree(utf16_path);
+		return -ENOENT;
 	}
-	if (cfid == NULL)
-		return -ENOMEM;
-
-	mutex_lock(&cfid->fid_mutex);
-	if (cfid->is_valid) {
-		cifs_dbg(FYI, "found a cached root file handle\n");
+	/*
+	 * At this point we either have a lease already and we can just
+	 * return it. If not we are guaranteed to be the only thread accessing
+	 * this cfid.
+	 */
+	if (cfid->has_lease) {
 		*ret_cfid = cfid;
-		kref_get(&cfid->refcount);
-		mutex_unlock(&cfid->fid_mutex);
+		kfree(utf16_path);
 		return 0;
 	}
 
 	/*
 	 * We do not hold the lock for the open because in case
-	 * SMB2_open needs to reconnect, it will end up calling
-	 * cifs_mark_open_files_invalid() which takes the lock again
-	 * thus causing a deadlock
+	 * SMB2_open needs to reconnect.
+	 * This is safe because no other thread will be able to get a ref
+	 * to the cfid until we have finished opening the file and (possibly)
+	 * aquired a lease.
 	 */
-	mutex_unlock(&cfid->fid_mutex);
-
-	if (lookup_only)
-		return -ENOENT;
-
 	if (smb3_encryption_required(tcon))
 		flags |= CIFS_TRANSFORM_REQ;
 
-	if (!server->ops->new_lease_key)
-		return -EIO;
-
 	pfid = &cfid->fid;
 	server->ops->new_lease_key(pfid);
 
@@ -109,7 +212,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	oparms.reconnect = false;
 
 	rc = SMB2_open_init(tcon, server,
-			    &rqst[0], &oplock, &oparms, &utf16_path);
+			    &rqst[0], &oplock, &oparms, utf16_path);
 	if (rc)
 		goto oshr_free;
 	smb2_set_next_command(tcon, &rqst[0]);
@@ -132,47 +235,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	rc = compound_send_recv(xid, ses, server,
 				flags, 2, rqst,
 				resp_buftype, rsp_iov);
-	mutex_lock(&cfid->fid_mutex);
-
-	/*
-	 * Now we need to check again as the cached root might have
-	 * been successfully re-opened from a concurrent process
-	 */
-
-	if (cfid->is_valid) {
-		/* work was already done */
-
-		/* stash fids for close() later */
-		struct cifs_fid fid = {
-			.persistent_fid = pfid->persistent_fid,
-			.volatile_fid = pfid->volatile_fid,
-		};
-
-		/*
-		 * caller expects this func to set the fid in cfid to valid
-		 * cached root, so increment the refcount.
-		 */
-		kref_get(&cfid->refcount);
-
-		mutex_unlock(&cfid->fid_mutex);
-
-		if (rc == 0) {
-			/* close extra handle outside of crit sec */
-			SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
-		}
-		rc = 0;
-		goto oshr_free;
-	}
-
-	/* Cached root is still invalid, continue normaly */
-
 	if (rc) {
 		if (rc == -EREMCHG) {
 			tcon->need_reconnect = true;
 			pr_warn_once("server share %s deleted\n",
 				     tcon->treeName);
 		}
-		goto oshr_exit;
+		goto oshr_free;
 	}
 
 	atomic_inc(&tcon->num_remote_opens);
@@ -185,49 +254,56 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 #endif /* CIFS_DEBUG2 */
 
 	cfid->tcon = tcon;
-	cfid->is_valid = true;
-	cfid->dentry = dentry;
-	if (dentry)
+	if (dentry) {
+		cfid->dentry = dentry;
 		dget(dentry);
-	kref_init(&cfid->refcount);
-
+	}
 	/* BB TBD check to see if oplock level check can be removed below */
-	if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) {
-		/*
-		 * See commit 2f94a3125b87. Increment the refcount when we
-		 * get a lease for root, release it if lease break occurs
-		 */
-		kref_get(&cfid->refcount);
-		cfid->has_lease = true;
-		smb2_parse_contexts(server, o_rsp,
-				&oparms.fid->epoch,
-				    oparms.fid->lease_key, &oplock,
-				    NULL, NULL);
-	} else
-		goto oshr_exit;
+	if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
+		goto oshr_free;
+	}
 
+	smb2_parse_contexts(server, o_rsp,
+			    &oparms.fid->epoch,
+			    oparms.fid->lease_key, &oplock,
+			    NULL, NULL);
+	
 	qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
 	if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
-		goto oshr_exit;
+		goto oshr_free;
 	if (!smb2_validate_and_copy_iov(
 				le16_to_cpu(qi_rsp->OutputBufferOffset),
 				sizeof(struct smb2_file_all_info),
 				&rsp_iov[1], sizeof(struct smb2_file_all_info),
 				(char *)&cfid->file_all_info))
 		cfid->file_all_info_is_valid = true;
-
 	cfid->time = jiffies;
+	cfid->is_open = true;
+	cfid->has_lease = true;
 
-oshr_exit:
-	mutex_unlock(&cfid->fid_mutex);
 oshr_free:
+	kfree(utf16_path);
 	SMB2_open_free(&rqst[0]);
 	SMB2_query_info_free(&rqst[1]);
 	free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
 	free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
-	if (rc == 0)
-		*ret_cfid = cfid;
-
+	spin_lock(&cfids->cfid_list_lock);
+	if (!cfid->has_lease) {
+		if (cfid->on_list) {
+			list_del(&cfid->entry);
+			cfid->on_list = false;
+			cfids->num_entries--;
+		}
+		rc = -ENOENT;
+	}
+	spin_unlock(&cfids->cfid_list_lock);
+	if (rc) {
+		free_cached_dir(cfid);
+		cfid = NULL;
+	}
+	if (rc == 0) {
+		*ret_cfid = cfid;	
+	}
 	return rc;
 }
 
@@ -236,20 +312,22 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
 			      struct cached_fid **ret_cfid)
 {
 	struct cached_fid *cfid;
+	struct cached_fids *cfids = tcon->cfids;
 
-	cfid = tcon->cfids->cfid;
-	if (cfid == NULL)
+	if (cfids == NULL)
 		return -ENOENT;
 
-	mutex_lock(&cfid->fid_mutex);
-	if (cfid->dentry == dentry) {
-		cifs_dbg(FYI, "found a cached root file handle by dentry\n");
-		*ret_cfid = cfid;
-		kref_get(&cfid->refcount);
-		mutex_unlock(&cfid->fid_mutex);
-		return 0;
+	spin_lock(&cfids->cfid_list_lock);
+	list_for_each_entry(cfid, &cfids->entries, entry) {
+		if (dentry && cfid->dentry == dentry) {
+			cifs_dbg(FYI, "found a cached root file handle by dentry\n");
+			kref_get(&cfid->refcount);
+			*ret_cfid = cfid;	
+			spin_unlock(&cfids->cfid_list_lock);
+			return 0;
+		}
 	}
-	mutex_unlock(&cfid->fid_mutex);
+	spin_unlock(&cfids->cfid_list_lock);
 	return -ENOENT;
 }
 
@@ -258,63 +336,29 @@ smb2_close_cached_fid(struct kref *ref)
 {
 	struct cached_fid *cfid = container_of(ref, struct cached_fid,
 					       refcount);
-	struct cached_dirent *dirent, *q;
 
-	if (cfid->is_valid) {
-		cifs_dbg(FYI, "clear cached root file handle\n");
-		SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
-			   cfid->fid.volatile_fid);
+	spin_lock(&cfid->cfids->cfid_list_lock);
+	if (cfid->on_list) {
+		list_del(&cfid->entry);
+		cfid->on_list = false;
+		cfid->cfids->num_entries--;
 	}
+	spin_unlock(&cfid->cfids->cfid_list_lock);
 
-	/*
-	 * We only check validity above to send SMB2_close,
-	 * but we still need to invalidate these entries
-	 * when this function is called
-	 */
-	cfid->is_valid = false;
-	cfid->file_all_info_is_valid = false;
-	cfid->has_lease = false;
-	if (cfid->dentry) {
-		dput(cfid->dentry);
-		cfid->dentry = NULL;
-	}
-	/*
-	 * Delete all cached dirent names
-	 */
-	mutex_lock(&cfid->dirents.de_mutex);
-	list_for_each_entry_safe(dirent, q, &cfid->dirents.entries, entry) {
-		list_del(&dirent->entry);
-		kfree(dirent->name);
-		kfree(dirent);
+	dput(cfid->dentry);
+	cfid->dentry = NULL;
+
+	if (cfid->is_open) {
+		SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
+			   cfid->fid.volatile_fid);
 	}
-	cfid->dirents.is_valid = 0;
-	cfid->dirents.is_failed = 0;
-	cfid->dirents.ctx = NULL;
-	cfid->dirents.pos = 0;
-	mutex_unlock(&cfid->dirents.de_mutex);
 
+	free_cached_dir(cfid);
 }
 
 void close_cached_dir(struct cached_fid *cfid)
 {
-	mutex_lock(&cfid->fid_mutex);
 	kref_put(&cfid->refcount, smb2_close_cached_fid);
-	mutex_unlock(&cfid->fid_mutex);
-}
-
-void close_cached_dir_lease_locked(struct cached_fid *cfid)
-{
-	if (cfid->has_lease) {
-		cfid->has_lease = false;
-		kref_put(&cfid->refcount, smb2_close_cached_fid);
-	}
-}
-
-void close_cached_dir_lease(struct cached_fid *cfid)
-{
-	mutex_lock(&cfid->fid_mutex);
-	close_cached_dir_lease_locked(cfid);
-	mutex_unlock(&cfid->fid_mutex);
 }
 
 /*
@@ -327,41 +371,62 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb)
 	struct cached_fid *cfid;
 	struct cifs_tcon *tcon;
 	struct tcon_link *tlink;
+	struct cached_fids *cfids;
 
 	for (node = rb_first(root); node; node = rb_next(node)) {
 		tlink = rb_entry(node, struct tcon_link, tl_rbnode);
 		tcon = tlink_tcon(tlink);
 		if (IS_ERR(tcon))
 			continue;
-		cfid = tcon->cfids->cfid;
-		if (cfid == NULL)
+		cfids = tcon->cfids;
+		if (cfids == NULL)
 			continue;
-		mutex_lock(&cfid->fid_mutex);
-		if (cfid->dentry) {
+		list_for_each_entry(cfid, &cfids->entries, entry) {
 			dput(cfid->dentry);
 			cfid->dentry = NULL;
 		}
-		mutex_unlock(&cfid->fid_mutex);
 	}
 }
 
 /*
- * Invalidate and close all cached dirs when a TCON has been reset
+ * Invalidate all cached dirs when a TCON has been reset
  * due to a session loss.
  */
 void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
 {
-	struct cached_fid *cfid = tcon->cfids->cfid;
-
-	if (cfid == NULL)
-		return;
-
-	mutex_lock(&cfid->fid_mutex);
-	cfid->is_valid = false;
-	/* cached handle is not valid, so SMB2_CLOSE won't be sent below */
-	close_cached_dir_lease_locked(cfid);
-	memset(&cfid->fid, 0, sizeof(struct cifs_fid));
-	mutex_unlock(&cfid->fid_mutex);
+	struct cached_fids *cfids = tcon->cfids;
+	struct cached_fid *cfid, *q;
+	struct list_head entry;
+
+	INIT_LIST_HEAD(&entry);
+	spin_lock(&cfids->cfid_list_lock);
+	list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
+		list_del(&cfid->entry);
+		list_add(&cfid->entry, &entry);
+		cfids->num_entries--;
+		cfid->is_open = false;
+		/* To prevent race with smb2_cached_lease_break() */
+		kref_get(&cfid->refcount);
+	}
+	spin_unlock(&cfids->cfid_list_lock);
+
+	list_for_each_entry_safe(cfid, q, &entry, entry) {
+		cfid->on_list = false;
+		list_del(&cfid->entry);
+		cancel_work_sync(&cfid->lease_break);
+		if (cfid->has_lease) {
+			/*
+			 * We lease was never cancelled from the server so we
+			 * we need to drop the reference.
+			 */
+			spin_lock(&cfids->cfid_list_lock);
+			cfid->has_lease = false;
+			spin_unlock(&cfids->cfid_list_lock);
+			kref_put(&cfid->refcount, smb2_close_cached_fid);
+		}
+		/* Drop the extra reference opened above*/
+		kref_put(&cfid->refcount, smb2_close_cached_fid);
+	}
 }
 
 static void
@@ -370,51 +435,83 @@ smb2_cached_lease_break(struct work_struct *work)
 	struct cached_fid *cfid = container_of(work,
 				struct cached_fid, lease_break);
 
-	close_cached_dir_lease(cfid);
+	spin_lock(&cfid->cfids->cfid_list_lock);
+	cfid->has_lease = false;
+	spin_unlock(&cfid->cfids->cfid_list_lock);
+	kref_put(&cfid->refcount, smb2_close_cached_fid);
 }
 
 int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
 {
-	struct cached_fid *cfid = tcon->cfids->cfid;
+	struct cached_fids *cfids = tcon->cfids;
+	struct cached_fid *cfid;
 
-	if (cfid == NULL)
+	if (cfids == NULL)
 		return false;
 
-	if (cfid->is_valid &&
-	    !memcmp(lease_key,
-		    cfid->fid.lease_key,
-		    SMB2_LEASE_KEY_SIZE)) {
-		cfid->time = 0;
-		INIT_WORK(&cfid->lease_break,
-			  smb2_cached_lease_break);
-		queue_work(cifsiod_wq,
-			   &cfid->lease_break);
-		return true;
+	spin_lock(&cfids->cfid_list_lock);
+	list_for_each_entry(cfid, &cfids->entries, entry) {
+		if (cfid->has_lease &&
+		    !memcmp(lease_key,
+			    cfid->fid.lease_key,
+			    SMB2_LEASE_KEY_SIZE)) {
+			cfid->time = 0;
+			/*
+			 * We found a lease remove it from the list
+			 * so no threads threads can access it.
+			 */
+			list_del(&cfid->entry);
+			cfid->on_list = false;
+			cfids->num_entries--;
+
+			queue_work(cifsiod_wq,
+				   &cfid->lease_break);
+			spin_unlock(&cfids->cfid_list_lock);
+			return true;
+		}
 	}
+	spin_unlock(&cfids->cfid_list_lock);
 	return false;
 }
 
-struct cached_fid *init_cached_dir(const char *path)
+static struct cached_fid *init_cached_dir(const char *path)
 {
 	struct cached_fid *cfid;
 
-	cfid = kzalloc(sizeof(*cfid), GFP_KERNEL);
+	cfid = kzalloc(sizeof(*cfid), GFP_ATOMIC);
 	if (!cfid)
 		return NULL;
-	cfid->path = kstrdup(path, GFP_KERNEL);
+	cfid->path = kstrdup(path, GFP_ATOMIC);
 	if (!cfid->path) {
 		kfree(cfid);
 		return NULL;
 	}
 
+	INIT_WORK(&cfid->lease_break, smb2_cached_lease_break);
+	INIT_LIST_HEAD(&cfid->entry);
 	INIT_LIST_HEAD(&cfid->dirents.entries);
 	mutex_init(&cfid->dirents.de_mutex);
-	mutex_init(&cfid->fid_mutex);
+	spin_lock_init(&cfid->fid_lock);
+	kref_init(&cfid->refcount);
 	return cfid;
 }
 
-void free_cached_dir(struct cached_fid *cfid)
+static void free_cached_dir(struct cached_fid *cfid)
 {
+	struct cached_dirent *dirent, *q;
+
+	dput(cfid->dentry);
+	cfid->dentry = NULL;
+
+	/*
+	 * Delete all cached dirent names
+	 */
+	list_for_each_entry_safe(dirent, q, &cfid->dirents.entries, entry) {
+		list_del(&dirent->entry);
+		kfree(dirent->name);
+		kfree(dirent);
+	}
+
 	kfree(cfid->path);
 	cfid->path = NULL;
 	kfree(cfid);
@@ -427,15 +524,34 @@ struct cached_fids *init_cached_dirs(void)
 	cfids = kzalloc(sizeof(*cfids), GFP_KERNEL);
 	if (!cfids)
 		return NULL;
-	mutex_init(&cfids->cfid_list_mutex);
+	spin_lock_init(&cfids->cfid_list_lock);
+	INIT_LIST_HEAD(&cfids->entries);
 	return cfids;
 }
 
+/*
+ * Called from tconInfoFree when we are tearing down the tcon.
+ * There are no active users or open files/directories at this point.
+ */
 void free_cached_dirs(struct cached_fids *cfids)
 {
-	if (cfids->cfid) {
-		free_cached_dir(cfids->cfid);
-		cfids->cfid = NULL;
+	struct cached_fid *cfid, *q;
+	struct list_head entry;
+
+       	INIT_LIST_HEAD(&entry);
+	spin_lock(&cfids->cfid_list_lock);
+	list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
+		cfid->on_list = false;
+		cfid->is_open = false;
+		list_del(&cfid->entry);
+		list_add(&cfid->entry, &entry);
+	}
+	spin_unlock(&cfids->cfid_list_lock);
+
+	list_for_each_entry_safe(cfid, q, &entry, entry) {
+		list_del(&cfid->entry);
+		free_cached_dir(cfid);
 	}
-	kfree(cfids);
+
+ 	kfree(cfids);
 }
diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
index bdf6c3866653..e536304ca2ce 100644
--- a/fs/cifs/cached_dir.h
+++ b/fs/cifs/cached_dir.h
@@ -31,14 +31,17 @@ struct cached_dirents {
 };
 
 struct cached_fid {
+	struct list_head entry;
+	struct cached_fids *cfids;
 	const char *path;
-	bool is_valid:1;	/* Do we have a useable root fid */
-	bool file_all_info_is_valid:1;
 	bool has_lease:1;
+	bool is_open:1;
+	bool on_list:1;
+	bool file_all_info_is_valid:1;
 	unsigned long time; /* jiffies of when lease was taken */
 	struct kref refcount;
 	struct cifs_fid fid;
-	struct mutex fid_mutex;
+	spinlock_t fid_lock;
 	struct cifs_tcon *tcon;
 	struct dentry *dentry;
 	struct work_struct lease_break;
@@ -46,9 +49,14 @@ struct cached_fid {
 	struct cached_dirents dirents;
 };
 
+#define MAX_CACHED_FIDS 16
 struct cached_fids {
-	struct mutex cfid_list_mutex;
-	struct cached_fid *cfid;
+	/* Must be held when:
+	 * - accessing the cfids->entries list
+	 */
+	spinlock_t cfid_list_lock;
+	int num_entries;
+	struct list_head entries;
 };
 
 extern struct cached_fids *init_cached_dirs(void);
@@ -61,8 +69,6 @@ extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
 				     struct dentry *dentry,
 				     struct cached_fid **cfid);
 extern void close_cached_dir(struct cached_fid *cfid);
-extern void close_cached_dir_lease(struct cached_fid *cfid);
-extern void close_cached_dir_lease_locked(struct cached_fid *cfid);
 extern void close_all_cached_dirs(struct cifs_sb_info *cifs_sb);
 extern void invalidate_all_cached_dirs(struct cifs_tcon *tcon);
 extern int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16]);
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index bac08c20f559..a42397b52882 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2265,13 +2265,13 @@ cifs_dentry_needs_reval(struct dentry *dentry)
 		return true;
 
 	if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) {
-		mutex_lock(&cfid->fid_mutex);
+		spin_lock(&cfid->fid_lock);
 		if (cfid->time && cifs_i->time > cfid->time) {
-			mutex_unlock(&cfid->fid_mutex);
+			spin_unlock(&cfid->fid_lock);
 			close_cached_dir(cfid);
 			return false;
 		}
-		mutex_unlock(&cfid->fid_mutex);
+		spin_unlock(&cfid->fid_lock);
 		close_cached_dir(cfid);
 	}
 	/*
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 96f3b0573606..1ed4b4992025 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -787,7 +787,7 @@ 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->is_valid) {
+		if (cfid->has_lease) {
 			close_cached_dir(cfid);
 			return 0;
 		}
-- 
2.35.3

[PATCH 5/6] cifs: find and use the dentry for cached non-root directories also

[Ronnie Sahlberg]

This allows us to use cached attributes for the entries in a cached
directory for as long as a lease is held on the directory itself.
Previously we have always allowed "used cached attributes for 1 second"
but this extends this to the lifetime of the lease as well as making the
caching safer.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cached_dir.c | 70 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
index 8732903aea03..f4d7700827b3 100644
--- a/fs/cifs/cached_dir.c
+++ b/fs/cifs/cached_dir.c
@@ -5,6 +5,7 @@
  *  Copyright (c) 2022, Ronnie Sahlberg <lsahlber@redhat.com>
  */
 
+#include <linux/namei.h>
 #include "cifsglob.h"
 #include "cifsproto.h"
 #include "cifs_debug.h"
@@ -113,6 +114,50 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
 	return cfid;
 }
 
+static struct dentry *
+path_to_dentry(struct cifs_sb_info *cifs_sb, const char *full_path)
+{
+	struct dentry *dentry;
+	char *path = NULL;
+	char *s, *p;
+	char sep;
+
+	path = kstrdup(full_path, GFP_ATOMIC);
+	if (path == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	sep = CIFS_DIR_SEP(cifs_sb);
+	dentry = dget(cifs_sb->root);
+	s = path;
+
+	do {
+		struct inode *dir = d_inode(dentry);
+		struct dentry *child;
+
+		if (!S_ISDIR(dir->i_mode)) {
+			dput(dentry);
+			dentry = ERR_PTR(-ENOTDIR);
+			break;
+		}
+
+		/* skip separators */
+		while (*s == sep)
+			s++;
+		if (!*s)
+			break;
+		p = s++;
+		/* next separator */
+		while (*s && *s != sep)
+			s++;
+
+		child = lookup_positive_unlocked(p, dentry, s - p);
+		dput(dentry);
+		dentry = child;
+	} while (!IS_ERR(dentry));
+	kfree(path);
+	return dentry;
+}
+
 /*
  * Open the and cache a directory handle.
  * If error then *cfid is not initialized.
@@ -139,7 +184,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	struct dentry *dentry = NULL;
 	struct cached_fid *cfid;
 	struct cached_fids *cfids;
-	  
 
 	if (tcon == NULL || tcon->cfids == NULL || tcon->nohandlecache ||
 	    is_smb1_server(tcon->ses->server))
@@ -155,13 +199,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	if (cifs_sb->root == NULL)
 		return -ENOENT;
 
-	/*
-	 * TODO: for better caching we need to find and use the dentry also
-	 * for non-root directories.
-	 */
-	if (!strlen(path))
-		dentry = cifs_sb->root;
-
 	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
 	if (!utf16_path)
 		return -ENOMEM;
@@ -253,12 +290,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId);
 #endif /* CIFS_DEBUG2 */
 
-	cfid->tcon = tcon;
-	if (dentry) {
-		cfid->dentry = dentry;
-		dget(dentry);
-	}
-	/* BB TBD check to see if oplock level check can be removed below */
 	if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
 		goto oshr_free;
 	}
@@ -277,6 +308,17 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 				&rsp_iov[1], sizeof(struct smb2_file_all_info),
 				(char *)&cfid->file_all_info))
 		cfid->file_all_info_is_valid = true;
+
+	if (!strlen(path)) {
+		dentry = dget(cifs_sb->root);
+		cfid->dentry = dentry;
+	} else{
+		dentry = path_to_dentry(cifs_sb, path);
+		if (IS_ERR(dentry))
+			goto oshr_free;
+		cfid->dentry = dentry;
+	}
+	cfid->tcon = tcon;
 	cfid->time = jiffies;
 	cfid->is_open = true;
 	cfid->has_lease = true;
-- 
2.35.3

[PATCH 6/6] cifs: do not cache leased directories for longer than 30 seconds

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cached_dir.c | 32 ++++++++++++++++++++++++++++++--
 fs/cifs/cached_dir.h |  2 ++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
index f4d7700827b3..1c4a409bcb67 100644
--- a/fs/cifs/cached_dir.c
+++ b/fs/cifs/cached_dir.c
@@ -14,6 +14,7 @@
 
 static struct cached_fid *init_cached_dir(const char *path);
 static void free_cached_dir(struct cached_fid *cfid);
+static void smb2_cached_lease_timeout(struct work_struct *work);
 
 /*
  * Locking and reference count for cached directory handles:
@@ -321,6 +322,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	cfid->tcon = tcon;
 	cfid->time = jiffies;
 	cfid->is_open = true;
+	queue_delayed_work(cifsiod_wq, &cfid->lease_timeout, HZ * 30);
+	cfid->has_timeout = true;
 	cfid->has_lease = true;
 
 oshr_free:
@@ -447,6 +450,7 @@ void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
 		list_add(&cfid->entry, &entry);
 		cfids->num_entries--;
 		cfid->is_open = false;
+		cfid->has_timeout = false;
 		/* To prevent race with smb2_cached_lease_break() */
 		kref_get(&cfid->refcount);
 	}
@@ -455,6 +459,7 @@ void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
 	list_for_each_entry_safe(cfid, q, &entry, entry) {
 		cfid->on_list = false;
 		list_del(&cfid->entry);
+		cancel_delayed_work_sync(&cfid->lease_timeout);
 		cancel_work_sync(&cfid->lease_break);
 		if (cfid->has_lease) {
 			/*
@@ -477,12 +482,34 @@ smb2_cached_lease_break(struct work_struct *work)
 	struct cached_fid *cfid = container_of(work,
 				struct cached_fid, lease_break);
 
+	cancel_delayed_work_sync(&cfid->lease_timeout);
 	spin_lock(&cfid->cfids->cfid_list_lock);
+	if (!cfid->has_lease) {
+		spin_unlock(&cfid->cfids->cfid_list_lock);
+		return;
+	}
 	cfid->has_lease = false;
 	spin_unlock(&cfid->cfids->cfid_list_lock);
 	kref_put(&cfid->refcount, smb2_close_cached_fid);
 }
 
+static void
+smb2_cached_lease_timeout(struct work_struct *work)
+{
+	struct cached_fid *cfid = container_of(work,
+				struct cached_fid, lease_timeout.work);
+
+	spin_lock(&cfid->cfids->cfid_list_lock);
+	if (!cfid->has_timeout || !cfid->on_list) {
+		spin_unlock(&cfid->cfids->cfid_list_lock);
+		return;
+	}
+	cfid->has_timeout = false;
+	spin_unlock(&cfid->cfids->cfid_list_lock);
+
+	queue_work(cifsiod_wq, &cfid->lease_break);
+}
+
 int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
 {
 	struct cached_fids *cfids = tcon->cfids;
@@ -506,9 +533,9 @@ int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
 			cfid->on_list = false;
 			cfids->num_entries--;
 
-			queue_work(cifsiod_wq,
-				   &cfid->lease_break);
+			cfid->has_timeout = false;
 			spin_unlock(&cfids->cfid_list_lock);
+			queue_work(cifsiod_wq, &cfid->lease_break);
 			return true;
 		}
 	}
@@ -530,6 +557,7 @@ static struct cached_fid *init_cached_dir(const char *path)
 	}
 
 	INIT_WORK(&cfid->lease_break, smb2_cached_lease_break);
+	INIT_DELAYED_WORK(&cfid->lease_timeout, smb2_cached_lease_timeout);
 	INIT_LIST_HEAD(&cfid->entry);
 	INIT_LIST_HEAD(&cfid->dirents.entries);
 	mutex_init(&cfid->dirents.de_mutex);
diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
index e536304ca2ce..813cd30f7ca3 100644
--- a/fs/cifs/cached_dir.h
+++ b/fs/cifs/cached_dir.h
@@ -35,6 +35,7 @@ struct cached_fid {
 	struct cached_fids *cfids;
 	const char *path;
 	bool has_lease:1;
+	bool has_timeout:1;
 	bool is_open:1;
 	bool on_list:1;
 	bool file_all_info_is_valid:1;
@@ -45,6 +46,7 @@ struct cached_fid {
 	struct cifs_tcon *tcon;
 	struct dentry *dentry;
 	struct work_struct lease_break;
+	struct delayed_work lease_timeout;
 	struct smb2_file_all_info file_all_info;
 	struct cached_dirents dirents;
 };
-- 
2.35.3

Re: [PATCH 2/6] cifs: cifs: handlecache, only track the dentry for the root handle

[Tom Talpey]

On 8/23/2022 8:27 PM, Ronnie Sahlberg wrote:
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>   fs/cifs/cached_dir.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
> index c2f5b71a3c9f..77880470c7ea 100644
> --- a/fs/cifs/cached_dir.c
> +++ b/fs/cifs/cached_dir.c
> @@ -47,11 +47,12 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   	if (cifs_sb->root == NULL)
>   		return -ENOENT;
>   
> +	if (!strlen(path))
> +		dentry = cifs_sb->root;

Wouldn't it be safer and more efficient to simply test
"if (path[0] == 0)"?

But, why would a non-null path ever be passed, if it
always fails? Seems like a pointless call in the first
place.

> +
>   	if (strlen(path))

Simply "else"? No need to recompute strlen.

Tom.

>   		return -ENOENT;
>   
> -	dentry = cifs_sb->root;
> -
>   	cfid = &tcon->cfids->cfid;
>   	mutex_lock(&cfid->fid_mutex);
>   	if (cfid->is_valid) {
> @@ -177,7 +178,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   	cfid->tcon = tcon;
>   	cfid->is_valid = true;
>   	cfid->dentry = dentry;
> -	dget(dentry);
> +	if (dentry)
> +		dget(dentry);
>   	kref_init(&cfid->refcount);
>   
>   	/* BB TBD check to see if oplock level check can be removed below */

Re: [PATCH 4/6] cifs: start caching all directories we open and get a lease for

[Tom Talpey]

Comment on the subject.

This code doesn't cache *all* directories, it is limited to
MAX_CACHED_FIDS. And "open and get a lease for" is redundant
as well - leases are granted at open.

Suggest "Enable caching of directories for which a lease is held".

On 8/23/2022 8:27 PM, Ronnie Sahlberg wrote:
> This expands the directory caching to now cache an open handle for all
> directories (up to a maximum) and not just the root directory.
> 
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>   fs/cifs/cached_dir.c | 490 ++++++++++++++++++++++++++-----------------
>   fs/cifs/cached_dir.h |  20 +-
>   fs/cifs/inode.c      |   6 +-
>   fs/cifs/smb2ops.c    |   2 +-
>   4 files changed, 320 insertions(+), 198 deletions(-)
> 
> diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
> index 594ec4385077..8732903aea03 100644
> --- a/fs/cifs/cached_dir.c
> +++ b/fs/cifs/cached_dir.c
> @@ -11,7 +11,107 @@
>   #include "smb2proto.h"
>   #include "cached_dir.h"
>   
> -struct cached_fid *init_cached_dir(const char *path);
> +static struct cached_fid *init_cached_dir(const char *path);
> +static void free_cached_dir(struct cached_fid *cfid);
> +
> +/*
> + * Locking and reference count for cached directory handles:
> + *
> + * The main function to get a reference to a cached handle is
> + * find_or_create_cached_dir() called from open_cached_dir()
> + * These functions are protected under the cfid_list_lock spin-lock
> + * to make sure we do not race creating new references for cached dirs
> + * with deletion of expired ones.
> + *
> + * An successful open_cached_dir() will take out 2 references to the cfid if
> + * this was the very first and successful call to open the directory and
> + * it acquired a lease from the server.
> + * One reference is for the lease  and the other is for the cfid that we
> + * return. The is lease reference is tracked by cfid->has_lease.
> + * If the directory already has a handle with an active lease, then we just
> + * take out one new reference for the cfid and return it.
> + * It can happen that we have a thread that tries to open a cached directory
> + * where we have a cfid already but we do not, yet, have a working lease. In
> + * this case we will just return NULL, and this the caller will fall back to
> + * the case when no handle was available.
> + *
> + * In this model the total number of references we have on a cfid is
> + * 1 for while the handle is open and we have a lease, and one additional
> + * reference for each open instance of a cfid.
> + *
> + *
> + * Once we get a lease break (cached_dir_lease_break()) we remove the
> + * cfid from the list under the spinlock. This prevents any new threads to
> + * use it, and we also call smb2_cached_lease_break() via the work_queue
> + * in order to drop the reference we got for the lease (we drop it outside
> + * of the spin-lock.)
> + * Anytime a thread calls close_cached_dir() we also drop a reference to the
> + * cfid.
> + * When the last reference to the cfid is released smb2_close_cached_fid()
> + * will be invoked which will drop the reference ot the dentry we held for
> + * this cfid and it will also, if we the handle is open/has a lease
> + * also call SMB2_close() to close the handle on the server.
> + *
> + *
> + * Two events require special handling:
> + * invalidate_all_cached_dirs() this function is called from SMB2_tdis()
> + * and cifs_mark_open_files_invalid().
> + * In both cases the tcon is either gone already or will be shortly so
> + * we do not need to actually close the handles. They will be dropped
> + * server side as part of the tcon dropping.
> + * But we have to be careful about a potential race with a concurrent
> + * lease break so we need to take out additional refences to avoid the
> + * cfid from being freed while we are still referencing it.
> + *
> + * free_cached_dirs() which is called from tconInfoFree().
> + * This is called quite late in the umount process so there should no longer
> + * be any open handles or files and we can just free all the remaining data.
> + */

This is a helpful comment during the review phase, but it's mighty
detailed, and therefore subject to being wrong, in the future.

> +
> +static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
> +						    const char *path,
> +						    bool lookup_only)
> +{
> +	struct cached_fid *cfid;
> +
> +	spin_lock(&cfids->cfid_list_lock);
> +	list_for_each_entry(cfid, &cfids->entries, entry) {
> +		if (!strcmp(cfid->path, path)) {
> +			/*
> +			 * If it doesn't have a lease it is either not yet
> +			 * fully cached or it may be in the process of
> +			 * being deleted due to a lease break.
> +			 */
> +			if (!cfid->has_lease) {
> +				spin_unlock(&cfids->cfid_list_lock);
> +				return NULL;
> +			}
> +			kref_get(&cfid->refcount);
> +			spin_unlock(&cfids->cfid_list_lock);
> +			return cfid;
> +		}
> +	}
> +	if (lookup_only) {
> +		spin_unlock(&cfids->cfid_list_lock);
> +		return NULL;
> +	}
> +	if (cfids->num_entries >= MAX_CACHED_FIDS) {
> +		spin_unlock(&cfids->cfid_list_lock);
> +		return NULL;
> +	}
> +	cfid = init_cached_dir(path);
> +	if (cfid == NULL) {
> +		spin_unlock(&cfids->cfid_list_lock);
> +		return NULL;
> +	}
> +	cfid->cfids = cfids;
> +	cfids->num_entries++;
> +	list_add(&cfid->entry, &cfids->entries);
> +	cfid->on_list = true;
> +	kref_get(&cfid->refcount);
> +	spin_unlock(&cfids->cfid_list_lock);
> +	return cfid;
> +}
>   
>   /*
>    * Open the and cache a directory handle.
> @@ -33,62 +133,65 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   	struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
>   	struct kvec qi_iov[1];
>   	int rc, flags = 0;
> -	__le16 utf16_path = 0; /* Null - since an open of top of share */
> +	__le16 *utf16_path = NULL;
>   	u8 oplock = SMB2_OPLOCK_LEVEL_II;
>   	struct cifs_fid *pfid;
> -	struct dentry *dentry;
> +	struct dentry *dentry = NULL;
>   	struct cached_fid *cfid;
> +	struct cached_fids *cfids;
> +	
>   
> -	if (tcon == NULL || tcon->nohandlecache ||
> +	if (tcon == NULL || tcon->cfids == NULL || tcon->nohandlecache ||
>   	    is_smb1_server(tcon->ses->server))
> -		return -EOPNOTSUPP;
> +		return -ENOTSUPP;

Good catch - there's no operation here.

>   
>   	ses = tcon->ses;
>   	server = ses->server;
> +	cfids = tcon->cfids;
>   
> +	if (!server->ops->new_lease_key)
> +		return -EIO;
> +	
>   	if (cifs_sb->root == NULL)
>   		return -ENOENT;
>   
> +	/*
> +	 * TODO: for better caching we need to find and use the dentry also
> +	 * for non-root directories.
> +	 */
>   	if (!strlen(path))
>   		dentry = cifs_sb->root;
>   
> -	if (strlen(path))
> -		return -ENOENT;
> +	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
> +	if (!utf16_path)
> +		return -ENOMEM;
>   
> -	cfid = tcon->cfids->cfid;
> +	cfid = find_or_create_cached_dir(cfids, path, lookup_only);
>   	if (cfid == NULL) {
> -		cfid = init_cached_dir(path);
> -		tcon->cfids->cfid = cfid;
> +		kfree(utf16_path);
> +		return -ENOENT;
>   	}
> -	if (cfid == NULL)
> -		return -ENOMEM;
> -
> -	mutex_lock(&cfid->fid_mutex);
> -	if (cfid->is_valid) {
> -		cifs_dbg(FYI, "found a cached root file handle\n");
> +	/*
> +	 * At this point we either have a lease already and we can just
> +	 * return it. If not we are guaranteed to be the only thread accessing
> +	 * this cfid.
> +	 */
> +	if (cfid->has_lease) {
>   		*ret_cfid = cfid;
> -		kref_get(&cfid->refcount);
> -		mutex_unlock(&cfid->fid_mutex);
> +		kfree(utf16_path);
>   		return 0;
>   	}
>   
>   	/*
>   	 * We do not hold the lock for the open because in case
> -	 * SMB2_open needs to reconnect, it will end up calling
> -	 * cifs_mark_open_files_invalid() which takes the lock again
> -	 * thus causing a deadlock
> +	 * SMB2_open needs to reconnect.
> +	 * This is safe because no other thread will be able to get a ref
> +	 * to the cfid until we have finished opening the file and (possibly)
> +	 * aquired a lease.
>   	 */
> -	mutex_unlock(&cfid->fid_mutex);
> -
> -	if (lookup_only)
> -		return -ENOENT;
> -
>   	if (smb3_encryption_required(tcon))
>   		flags |= CIFS_TRANSFORM_REQ;
>   
> -	if (!server->ops->new_lease_key)
> -		return -EIO;
> -
>   	pfid = &cfid->fid;
>   	server->ops->new_lease_key(pfid);
>   
> @@ -109,7 +212,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   	oparms.reconnect = false;
>   
>   	rc = SMB2_open_init(tcon, server,
> -			    &rqst[0], &oplock, &oparms, &utf16_path);
> +			    &rqst[0], &oplock, &oparms, utf16_path);
>   	if (rc)
>   		goto oshr_free;
>   	smb2_set_next_command(tcon, &rqst[0]);
> @@ -132,47 +235,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   	rc = compound_send_recv(xid, ses, server,
>   				flags, 2, rqst,
>   				resp_buftype, rsp_iov);
> -	mutex_lock(&cfid->fid_mutex);
> -
> -	/*
> -	 * Now we need to check again as the cached root might have
> -	 * been successfully re-opened from a concurrent process
> -	 */
> -
> -	if (cfid->is_valid) {
> -		/* work was already done */
> -
> -		/* stash fids for close() later */
> -		struct cifs_fid fid = {
> -			.persistent_fid = pfid->persistent_fid,
> -			.volatile_fid = pfid->volatile_fid,
> -		};
> -
> -		/*
> -		 * caller expects this func to set the fid in cfid to valid
> -		 * cached root, so increment the refcount.
> -		 */
> -		kref_get(&cfid->refcount);
> -
> -		mutex_unlock(&cfid->fid_mutex);
> -
> -		if (rc == 0) {
> -			/* close extra handle outside of crit sec */
> -			SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
> -		}
> -		rc = 0;
> -		goto oshr_free;
> -	}
> -
> -	/* Cached root is still invalid, continue normaly */
> -
>   	if (rc) {
>   		if (rc == -EREMCHG) {
>   			tcon->need_reconnect = true;
>   			pr_warn_once("server share %s deleted\n",
>   				     tcon->treeName);
>   		}
> -		goto oshr_exit;
> +		goto oshr_free;
>   	}
>   
>   	atomic_inc(&tcon->num_remote_opens);
> @@ -185,49 +254,56 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   #endif /* CIFS_DEBUG2 */
>   
>   	cfid->tcon = tcon;
> -	cfid->is_valid = true;
> -	cfid->dentry = dentry;
> -	if (dentry)
> +	if (dentry) {
> +		cfid->dentry = dentry;
>   		dget(dentry);
> -	kref_init(&cfid->refcount);
> -
> +	}
>   	/* BB TBD check to see if oplock level check can be removed below */
> -	if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) {
> -		/*
> -		 * See commit 2f94a3125b87. Increment the refcount when we
> -		 * get a lease for root, release it if lease break occurs
> -		 */
> -		kref_get(&cfid->refcount);
> -		cfid->has_lease = true;
> -		smb2_parse_contexts(server, o_rsp,
> -				&oparms.fid->epoch,
> -				    oparms.fid->lease_key, &oplock,
> -				    NULL, NULL);
> -	} else
> -		goto oshr_exit;
> +	if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
> +		goto oshr_free;
> +	}
>   
> +	smb2_parse_contexts(server, o_rsp,
> +			    &oparms.fid->epoch,
> +			    oparms.fid->lease_key, &oplock,
> +			    NULL, NULL);
> +	
>   	qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
>   	if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
> -		goto oshr_exit;
> +		goto oshr_free;
>   	if (!smb2_validate_and_copy_iov(
>   				le16_to_cpu(qi_rsp->OutputBufferOffset),
>   				sizeof(struct smb2_file_all_info),
>   				&rsp_iov[1], sizeof(struct smb2_file_all_info),
>   				(char *)&cfid->file_all_info))
>   		cfid->file_all_info_is_valid = true;
> -
>   	cfid->time = jiffies;
> +	cfid->is_open = true;
> +	cfid->has_lease = true;
>   
> -oshr_exit:
> -	mutex_unlock(&cfid->fid_mutex);
>   oshr_free:
> +	kfree(utf16_path);
>   	SMB2_open_free(&rqst[0]);
>   	SMB2_query_info_free(&rqst[1]);
>   	free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
>   	free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> -	if (rc == 0)
> -		*ret_cfid = cfid;
> -
> +	spin_lock(&cfids->cfid_list_lock);
> +	if (!cfid->has_lease) {
> +		if (cfid->on_list) {
> +			list_del(&cfid->entry);
> +			cfid->on_list = false;
> +			cfids->num_entries--;
> +		}
> +		rc = -ENOENT;
> +	}
> +	spin_unlock(&cfids->cfid_list_lock);
> +	if (rc) {
> +		free_cached_dir(cfid);
> +		cfid = NULL;
> +	}
> +	if (rc == 0) {
> +		*ret_cfid = cfid;	
> +	}
>   	return rc;
>   }
>   
> @@ -236,20 +312,22 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
>   			      struct cached_fid **ret_cfid)
>   {
>   	struct cached_fid *cfid;
> +	struct cached_fids *cfids = tcon->cfids;
>   
> -	cfid = tcon->cfids->cfid;
> -	if (cfid == NULL)
> +	if (cfids == NULL)
>   		return -ENOENT;
>   
> -	mutex_lock(&cfid->fid_mutex);
> -	if (cfid->dentry == dentry) {
> -		cifs_dbg(FYI, "found a cached root file handle by dentry\n");
> -		*ret_cfid = cfid;
> -		kref_get(&cfid->refcount);
> -		mutex_unlock(&cfid->fid_mutex);
> -		return 0;
> +	spin_lock(&cfids->cfid_list_lock);
> +	list_for_each_entry(cfid, &cfids->entries, entry) {
> +		if (dentry && cfid->dentry == dentry) {
> +			cifs_dbg(FYI, "found a cached root file handle by dentry\n");
> +			kref_get(&cfid->refcount);
> +			*ret_cfid = cfid;	
> +			spin_unlock(&cfids->cfid_list_lock);
> +			return 0;
> +		}
>   	}
> -	mutex_unlock(&cfid->fid_mutex);
> +	spin_unlock(&cfids->cfid_list_lock);
>   	return -ENOENT;
>   }
>   
> @@ -258,63 +336,29 @@ smb2_close_cached_fid(struct kref *ref)
>   {
>   	struct cached_fid *cfid = container_of(ref, struct cached_fid,
>   					       refcount);
> -	struct cached_dirent *dirent, *q;
>   
> -	if (cfid->is_valid) {
> -		cifs_dbg(FYI, "clear cached root file handle\n");
> -		SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
> -			   cfid->fid.volatile_fid);
> +	spin_lock(&cfid->cfids->cfid_list_lock);
> +	if (cfid->on_list) {
> +		list_del(&cfid->entry);
> +		cfid->on_list = false;
> +		cfid->cfids->num_entries--;
>   	}
> +	spin_unlock(&cfid->cfids->cfid_list_lock);
>   
> -	/*
> -	 * We only check validity above to send SMB2_close,
> -	 * but we still need to invalidate these entries
> -	 * when this function is called
> -	 */
> -	cfid->is_valid = false;
> -	cfid->file_all_info_is_valid = false;
> -	cfid->has_lease = false;
> -	if (cfid->dentry) {
> -		dput(cfid->dentry);
> -		cfid->dentry = NULL;
> -	}
> -	/*
> -	 * Delete all cached dirent names
> -	 */
> -	mutex_lock(&cfid->dirents.de_mutex);
> -	list_for_each_entry_safe(dirent, q, &cfid->dirents.entries, entry) {
> -		list_del(&dirent->entry);
> -		kfree(dirent->name);
> -		kfree(dirent);
> +	dput(cfid->dentry);
> +	cfid->dentry = NULL;
> +
> +	if (cfid->is_open) {
> +		SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
> +			   cfid->fid.volatile_fid);
>   	}
> -	cfid->dirents.is_valid = 0;
> -	cfid->dirents.is_failed = 0;
> -	cfid->dirents.ctx = NULL;
> -	cfid->dirents.pos = 0;
> -	mutex_unlock(&cfid->dirents.de_mutex);
>   
> +	free_cached_dir(cfid);
>   }
>   
>   void close_cached_dir(struct cached_fid *cfid)
>   {
> -	mutex_lock(&cfid->fid_mutex);
>   	kref_put(&cfid->refcount, smb2_close_cached_fid);
> -	mutex_unlock(&cfid->fid_mutex);
> -}
> -
> -void close_cached_dir_lease_locked(struct cached_fid *cfid)
> -{
> -	if (cfid->has_lease) {
> -		cfid->has_lease = false;
> -		kref_put(&cfid->refcount, smb2_close_cached_fid);
> -	}
> -}
> -
> -void close_cached_dir_lease(struct cached_fid *cfid)
> -{
> -	mutex_lock(&cfid->fid_mutex);
> -	close_cached_dir_lease_locked(cfid);
> -	mutex_unlock(&cfid->fid_mutex);
>   }
>   
>   /*
> @@ -327,41 +371,62 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb)
>   	struct cached_fid *cfid;
>   	struct cifs_tcon *tcon;
>   	struct tcon_link *tlink;
> +	struct cached_fids *cfids;
>   
>   	for (node = rb_first(root); node; node = rb_next(node)) {
>   		tlink = rb_entry(node, struct tcon_link, tl_rbnode);
>   		tcon = tlink_tcon(tlink);
>   		if (IS_ERR(tcon))
>   			continue;
> -		cfid = tcon->cfids->cfid;
> -		if (cfid == NULL)
> +		cfids = tcon->cfids;
> +		if (cfids == NULL)
>   			continue;
> -		mutex_lock(&cfid->fid_mutex);
> -		if (cfid->dentry) {
> +		list_for_each_entry(cfid, &cfids->entries, entry) {
>   			dput(cfid->dentry);
>   			cfid->dentry = NULL;
>   		}
> -		mutex_unlock(&cfid->fid_mutex);
>   	}
>   }
>   
>   /*
> - * Invalidate and close all cached dirs when a TCON has been reset
> + * Invalidate all cached dirs when a TCON has been reset
>    * due to a session loss.
>    */
>   void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
>   {
> -	struct cached_fid *cfid = tcon->cfids->cfid;
> -
> -	if (cfid == NULL)
> -		return;
> -
> -	mutex_lock(&cfid->fid_mutex);
> -	cfid->is_valid = false;
> -	/* cached handle is not valid, so SMB2_CLOSE won't be sent below */
> -	close_cached_dir_lease_locked(cfid);
> -	memset(&cfid->fid, 0, sizeof(struct cifs_fid));
> -	mutex_unlock(&cfid->fid_mutex);
> +	struct cached_fids *cfids = tcon->cfids;
> +	struct cached_fid *cfid, *q;
> +	struct list_head entry;
> +
> +	INIT_LIST_HEAD(&entry);
> +	spin_lock(&cfids->cfid_list_lock);
> +	list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
> +		list_del(&cfid->entry);
> +		list_add(&cfid->entry, &entry);
> +		cfids->num_entries--;
> +		cfid->is_open = false;
> +		/* To prevent race with smb2_cached_lease_break() */
> +		kref_get(&cfid->refcount);
> +	}
> +	spin_unlock(&cfids->cfid_list_lock);
> +
> +	list_for_each_entry_safe(cfid, q, &entry, entry) {
> +		cfid->on_list = false;
> +		list_del(&cfid->entry);
> +		cancel_work_sync(&cfid->lease_break);
> +		if (cfid->has_lease) {
> +			/*
> +			 * We lease was never cancelled from the server so we
> +			 * we need to drop the reference.
> +			 */
> +			spin_lock(&cfids->cfid_list_lock);
> +			cfid->has_lease = false;
> +			spin_unlock(&cfids->cfid_list_lock);
> +			kref_put(&cfid->refcount, smb2_close_cached_fid);
> +		}
> +		/* Drop the extra reference opened above*/
> +		kref_put(&cfid->refcount, smb2_close_cached_fid);
> +	}
>   }
>   
>   static void
> @@ -370,51 +435,83 @@ smb2_cached_lease_break(struct work_struct *work)
>   	struct cached_fid *cfid = container_of(work,
>   				struct cached_fid, lease_break);
>   
> -	close_cached_dir_lease(cfid);
> +	spin_lock(&cfid->cfids->cfid_list_lock);
> +	cfid->has_lease = false;
> +	spin_unlock(&cfid->cfids->cfid_list_lock);
> +	kref_put(&cfid->refcount, smb2_close_cached_fid);
>   }
>   
>   int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
>   {
> -	struct cached_fid *cfid = tcon->cfids->cfid;
> +	struct cached_fids *cfids = tcon->cfids;
> +	struct cached_fid *cfid;
>   
> -	if (cfid == NULL)
> +	if (cfids == NULL)
>   		return false;
>   
> -	if (cfid->is_valid &&
> -	    !memcmp(lease_key,
> -		    cfid->fid.lease_key,
> -		    SMB2_LEASE_KEY_SIZE)) {
> -		cfid->time = 0;
> -		INIT_WORK(&cfid->lease_break,
> -			  smb2_cached_lease_break);
> -		queue_work(cifsiod_wq,
> -			   &cfid->lease_break);
> -		return true;
> +	spin_lock(&cfids->cfid_list_lock);
> +	list_for_each_entry(cfid, &cfids->entries, entry) {
> +		if (cfid->has_lease &&
> +		    !memcmp(lease_key,
> +			    cfid->fid.lease_key,
> +			    SMB2_LEASE_KEY_SIZE)) {
> +			cfid->time = 0;
> +			/*
> +			 * We found a lease remove it from the list
> +			 * so no threads threads can access it.
> +			 */
> +			list_del(&cfid->entry);
> +			cfid->on_list = false;
> +			cfids->num_entries--;
> +
> +			queue_work(cifsiod_wq,
> +				   &cfid->lease_break);
> +			spin_unlock(&cfids->cfid_list_lock);
> +			return true;
> +		}
>   	}
> +	spin_unlock(&cfids->cfid_list_lock);
>   	return false;
>   }
>   
> -struct cached_fid *init_cached_dir(const char *path)
> +static struct cached_fid *init_cached_dir(const char *path)
>   {
>   	struct cached_fid *cfid;
>   
> -	cfid = kzalloc(sizeof(*cfid), GFP_KERNEL);
> +	cfid = kzalloc(sizeof(*cfid), GFP_ATOMIC);
>   	if (!cfid)
>   		return NULL;
> -	cfid->path = kstrdup(path, GFP_KERNEL);
> +	cfid->path = kstrdup(path, GFP_ATOMIC);
>   	if (!cfid->path) {
>   		kfree(cfid);
>   		return NULL;
>   	}
>   
> +	INIT_WORK(&cfid->lease_break, smb2_cached_lease_break);
> +	INIT_LIST_HEAD(&cfid->entry);
>   	INIT_LIST_HEAD(&cfid->dirents.entries);
>   	mutex_init(&cfid->dirents.de_mutex);
> -	mutex_init(&cfid->fid_mutex);
> +	spin_lock_init(&cfid->fid_lock);
> +	kref_init(&cfid->refcount);
>   	return cfid;
>   }
>   
> -void free_cached_dir(struct cached_fid *cfid)
> +static void free_cached_dir(struct cached_fid *cfid)
>   {
> +	struct cached_dirent *dirent, *q;
> +
> +	dput(cfid->dentry);
> +	cfid->dentry = NULL;
> +
> +	/*
> +	 * Delete all cached dirent names
> +	 */
> +	list_for_each_entry_safe(dirent, q, &cfid->dirents.entries, entry) {
> +		list_del(&dirent->entry);
> +		kfree(dirent->name);
> +		kfree(dirent);
> +	}
> +
>   	kfree(cfid->path);
>   	cfid->path = NULL;
>   	kfree(cfid);
> @@ -427,15 +524,34 @@ struct cached_fids *init_cached_dirs(void)
>   	cfids = kzalloc(sizeof(*cfids), GFP_KERNEL);
>   	if (!cfids)
>   		return NULL;
> -	mutex_init(&cfids->cfid_list_mutex);
> +	spin_lock_init(&cfids->cfid_list_lock);
> +	INIT_LIST_HEAD(&cfids->entries);
>   	return cfids;
>   }
>   
> +/*
> + * Called from tconInfoFree when we are tearing down the tcon.
> + * There are no active users or open files/directories at this point.
> + */
>   void free_cached_dirs(struct cached_fids *cfids)
>   {
> -	if (cfids->cfid) {
> -		free_cached_dir(cfids->cfid);
> -		cfids->cfid = NULL;
> +	struct cached_fid *cfid, *q;
> +	struct list_head entry;
> +
> +       	INIT_LIST_HEAD(&entry);
> +	spin_lock(&cfids->cfid_list_lock);
> +	list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
> +		cfid->on_list = false;
> +		cfid->is_open = false;
> +		list_del(&cfid->entry);
> +		list_add(&cfid->entry, &entry);
> +	}
> +	spin_unlock(&cfids->cfid_list_lock);
> +
> +	list_for_each_entry_safe(cfid, q, &entry, entry) {
> +		list_del(&cfid->entry);
> +		free_cached_dir(cfid);
>   	}
> -	kfree(cfids);
> +
> + 	kfree(cfids);
>   }
> diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
> index bdf6c3866653..e536304ca2ce 100644
> --- a/fs/cifs/cached_dir.h
> +++ b/fs/cifs/cached_dir.h
> @@ -31,14 +31,17 @@ struct cached_dirents {
>   };
>   
>   struct cached_fid {
> +	struct list_head entry;
> +	struct cached_fids *cfids;
>   	const char *path;
> -	bool is_valid:1;	/* Do we have a useable root fid */
> -	bool file_all_info_is_valid:1;
>   	bool has_lease:1;
> +	bool is_open:1;
> +	bool on_list:1;
> +	bool file_all_info_is_valid:1;
>   	unsigned long time; /* jiffies of when lease was taken */
>   	struct kref refcount;
>   	struct cifs_fid fid;
> -	struct mutex fid_mutex;
> +	spinlock_t fid_lock;
>   	struct cifs_tcon *tcon;
>   	struct dentry *dentry;
>   	struct work_struct lease_break;
> @@ -46,9 +49,14 @@ struct cached_fid {
>   	struct cached_dirents dirents;
>   };
>   
> +#define MAX_CACHED_FIDS 16
>   struct cached_fids {
> -	struct mutex cfid_list_mutex;
> -	struct cached_fid *cfid;
> +	/* Must be held when:
> +	 * - accessing the cfids->entries list
> +	 */
> +	spinlock_t cfid_list_lock;
> +	int num_entries;
> +	struct list_head entries;
>   };
>   
>   extern struct cached_fids *init_cached_dirs(void);
> @@ -61,8 +69,6 @@ extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
>   				     struct dentry *dentry,
>   				     struct cached_fid **cfid);
>   extern void close_cached_dir(struct cached_fid *cfid);
> -extern void close_cached_dir_lease(struct cached_fid *cfid);
> -extern void close_cached_dir_lease_locked(struct cached_fid *cfid);
>   extern void close_all_cached_dirs(struct cifs_sb_info *cifs_sb);
>   extern void invalidate_all_cached_dirs(struct cifs_tcon *tcon);
>   extern int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16]);
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index bac08c20f559..a42397b52882 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -2265,13 +2265,13 @@ cifs_dentry_needs_reval(struct dentry *dentry)
>   		return true;
>   
>   	if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) {
> -		mutex_lock(&cfid->fid_mutex);
> +		spin_lock(&cfid->fid_lock);
>   		if (cfid->time && cifs_i->time > cfid->time) {
> -			mutex_unlock(&cfid->fid_mutex);
> +			spin_unlock(&cfid->fid_lock);
>   			close_cached_dir(cfid);
>   			return false;
>   		}
> -		mutex_unlock(&cfid->fid_mutex);
> +		spin_unlock(&cfid->fid_lock);
>   		close_cached_dir(cfid);
>   	}
>   	/*
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 96f3b0573606..1ed4b4992025 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -787,7 +787,7 @@ 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->is_valid) {
> +		if (cfid->has_lease) {
>   			close_cached_dir(cfid);
>   			return 0;
>   		}

Re: [PATCH 5/6] cifs: find and use the dentry for cached non-root directories also

[Tom Talpey]

On 8/23/2022 8:27 PM, Ronnie Sahlberg wrote:
> This allows us to use cached attributes for the entries in a cached
> directory for as long as a lease is held on the directory itself.
> Previously we have always allowed "used cached attributes for 1 second"
> but this extends this to the lifetime of the lease as well as making the
> caching safer.
> 
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>   fs/cifs/cached_dir.c | 70 +++++++++++++++++++++++++++++++++++---------
>   1 file changed, 56 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
> index 8732903aea03..f4d7700827b3 100644
> --- a/fs/cifs/cached_dir.c
> +++ b/fs/cifs/cached_dir.c
> @@ -5,6 +5,7 @@
>    *  Copyright (c) 2022, Ronnie Sahlberg <lsahlber@redhat.com>
>    */
>   
> +#include <linux/namei.h>
>   #include "cifsglob.h"
>   #include "cifsproto.h"
>   #include "cifs_debug.h"
> @@ -113,6 +114,50 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
>   	return cfid;
>   }
>   
> +static struct dentry *
> +path_to_dentry(struct cifs_sb_info *cifs_sb, const char *full_path)
> +{
> +	struct dentry *dentry;
> +	char *path = NULL;
> +	char *s, *p;
> +	char sep;
> +
> +	path = kstrdup(full_path, GFP_ATOMIC);
> +	if (path == NULL)
> +		return ERR_PTR(-ENOMEM);

Why make a copy of the path? I don't see anything in the code
below that modifies its contents...

> +
> +	sep = CIFS_DIR_SEP(cifs_sb);
> +	dentry = dget(cifs_sb->root);
> +	s = path;
> +
> +	do {
> +		struct inode *dir = d_inode(dentry);
> +		struct dentry *child;
> +
> +		if (!S_ISDIR(dir->i_mode)) {
> +			dput(dentry);
> +			dentry = ERR_PTR(-ENOTDIR);
> +			break;
> +		}
> +
> +		/* skip separators */
> +		while (*s == sep)
> +			s++;
> +		if (!*s)
> +			break;
> +		p = s++;
> +		/* next separator */
> +		while (*s && *s != sep)
> +			s++;
> +
> +		child = lookup_positive_unlocked(p, dentry, s - p);
> +		dput(dentry);
> +		dentry = child;
> +	} while (!IS_ERR(dentry));
> +	kfree(path);
> +	return dentry;
> +}
> +
>   /*
>    * Open the and cache a directory handle.
>    * If error then *cfid is not initialized.
> @@ -139,7 +184,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   	struct dentry *dentry = NULL;
>   	struct cached_fid *cfid;
>   	struct cached_fids *cfids;
> -	
>   
>   	if (tcon == NULL || tcon->cfids == NULL || tcon->nohandlecache ||
>   	    is_smb1_server(tcon->ses->server))
> @@ -155,13 +199,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   	if (cifs_sb->root == NULL)
>   		return -ENOENT;
>   
> -	/*
> -	 * TODO: for better caching we need to find and use the dentry also
> -	 * for non-root directories.
> -	 */
> -	if (!strlen(path))
> -		dentry = cifs_sb->root;

Test path[0] instead of calling strlen?

> -
>   	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
>   	if (!utf16_path)
>   		return -ENOMEM;
> @@ -253,12 +290,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   	oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId);
>   #endif /* CIFS_DEBUG2 */
>   
> -	cfid->tcon = tcon;
> -	if (dentry) {
> -		cfid->dentry = dentry;
> -		dget(dentry);
> -	}
> -	/* BB TBD check to see if oplock level check can be removed below */
>   	if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
>   		goto oshr_free;
>   	}
> @@ -277,6 +308,17 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   				&rsp_iov[1], sizeof(struct smb2_file_all_info),
>   				(char *)&cfid->file_all_info))
>   		cfid->file_all_info_is_valid = true;
> +
> +	if (!strlen(path)) {
> +		dentry = dget(cifs_sb->root);
> +		cfid->dentry = dentry;
> +	} else{
> +		dentry = path_to_dentry(cifs_sb, path);
> +		if (IS_ERR(dentry))
> +			goto oshr_free;
> +		cfid->dentry = dentry;
> +	}

Pull cfid->dentry = dentry out of the above conditionals and
just set it here for both cases.

> +	cfid->tcon = tcon;
>   	cfid->time = jiffies;
>   	cfid->is_open = true;
>   	cfid->has_lease = true;

Re: [PATCH 6/6] cifs: do not cache leased directories for longer than 30 seconds

[Tom Talpey]

On 8/23/2022 8:27 PM, Ronnie Sahlberg wrote:
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>   fs/cifs/cached_dir.c | 32 ++++++++++++++++++++++++++++++--
>   fs/cifs/cached_dir.h |  2 ++
>   2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
> index f4d7700827b3..1c4a409bcb67 100644
> --- a/fs/cifs/cached_dir.c
> +++ b/fs/cifs/cached_dir.c
> @@ -14,6 +14,7 @@
>   
>   static struct cached_fid *init_cached_dir(const char *path);
>   static void free_cached_dir(struct cached_fid *cfid);
> +static void smb2_cached_lease_timeout(struct work_struct *work);
>   
>   /*
>    * Locking and reference count for cached directory handles:
> @@ -321,6 +322,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   	cfid->tcon = tcon;
>   	cfid->time = jiffies;
>   	cfid->is_open = true;
> +	queue_delayed_work(cifsiod_wq, &cfid->lease_timeout, HZ * 30);

Wouldn't it be more efficient to implement a laundromat? There
will be MAX_CACHED_FIDS of these delayed workers, and the
timing does not need to be precise (right?).

Is it worth considering making HZ*30 into a tunable?

Tom.

> +	cfid->has_timeout = true;
>   	cfid->has_lease = true;
>   
>   oshr_free:
> @@ -447,6 +450,7 @@ void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
>   		list_add(&cfid->entry, &entry);
>   		cfids->num_entries--;
>   		cfid->is_open = false;
> +		cfid->has_timeout = false;
>   		/* To prevent race with smb2_cached_lease_break() */
>   		kref_get(&cfid->refcount);
>   	}
> @@ -455,6 +459,7 @@ void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
>   	list_for_each_entry_safe(cfid, q, &entry, entry) {
>   		cfid->on_list = false;
>   		list_del(&cfid->entry);
> +		cancel_delayed_work_sync(&cfid->lease_timeout);
>   		cancel_work_sync(&cfid->lease_break);
>   		if (cfid->has_lease) {
>   			/*
> @@ -477,12 +482,34 @@ smb2_cached_lease_break(struct work_struct *work)
>   	struct cached_fid *cfid = container_of(work,
>   				struct cached_fid, lease_break);
>   
> +	cancel_delayed_work_sync(&cfid->lease_timeout);
>   	spin_lock(&cfid->cfids->cfid_list_lock);
> +	if (!cfid->has_lease) {
> +		spin_unlock(&cfid->cfids->cfid_list_lock);
> +		return;
> +	}
>   	cfid->has_lease = false;
>   	spin_unlock(&cfid->cfids->cfid_list_lock);
>   	kref_put(&cfid->refcount, smb2_close_cached_fid);
>   }
>   
> +static void
> +smb2_cached_lease_timeout(struct work_struct *work)
> +{
> +	struct cached_fid *cfid = container_of(work,
> +				struct cached_fid, lease_timeout.work);
> +
> +	spin_lock(&cfid->cfids->cfid_list_lock);
> +	if (!cfid->has_timeout || !cfid->on_list) {
> +		spin_unlock(&cfid->cfids->cfid_list_lock);
> +		return;
> +	}
> +	cfid->has_timeout = false;
> +	spin_unlock(&cfid->cfids->cfid_list_lock);
> +
> +	queue_work(cifsiod_wq, &cfid->lease_break);
> +}
> +
>   int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
>   {
>   	struct cached_fids *cfids = tcon->cfids;
> @@ -506,9 +533,9 @@ int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
>   			cfid->on_list = false;
>   			cfids->num_entries--;
>   
> -			queue_work(cifsiod_wq,
> -				   &cfid->lease_break);
> +			cfid->has_timeout = false;
>   			spin_unlock(&cfids->cfid_list_lock);
> +			queue_work(cifsiod_wq, &cfid->lease_break);
>   			return true;
>   		}
>   	}
> @@ -530,6 +557,7 @@ static struct cached_fid *init_cached_dir(const char *path)
>   	}
>   
>   	INIT_WORK(&cfid->lease_break, smb2_cached_lease_break);
> +	INIT_DELAYED_WORK(&cfid->lease_timeout, smb2_cached_lease_timeout);
>   	INIT_LIST_HEAD(&cfid->entry);
>   	INIT_LIST_HEAD(&cfid->dirents.entries);
>   	mutex_init(&cfid->dirents.de_mutex);
> diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
> index e536304ca2ce..813cd30f7ca3 100644
> --- a/fs/cifs/cached_dir.h
> +++ b/fs/cifs/cached_dir.h
> @@ -35,6 +35,7 @@ struct cached_fid {
>   	struct cached_fids *cfids;
>   	const char *path;
>   	bool has_lease:1;
> +	bool has_timeout:1;
>   	bool is_open:1;
>   	bool on_list:1;
>   	bool file_all_info_is_valid:1;
> @@ -45,6 +46,7 @@ struct cached_fid {
>   	struct cifs_tcon *tcon;
>   	struct dentry *dentry;
>   	struct work_struct lease_break;
> +	struct delayed_work lease_timeout;
>   	struct smb2_file_all_info file_all_info;
>   	struct cached_dirents dirents;
>   };

Re: [PATCH 5/6] cifs: find and use the dentry for cached non-root directories also

[ronnie sahlberg]

On Wed, 24 Aug 2022 at 23:51, Tom Talpey <tom@talpey.com> wrote:
>
> On 8/23/2022 8:27 PM, Ronnie Sahlberg wrote:
> > This allows us to use cached attributes for the entries in a cached
> > directory for as long as a lease is held on the directory itself.
> > Previously we have always allowed "used cached attributes for 1 second"
> > but this extends this to the lifetime of the lease as well as making the
> > caching safer.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >   fs/cifs/cached_dir.c | 70 +++++++++++++++++++++++++++++++++++---------
> >   1 file changed, 56 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
> > index 8732903aea03..f4d7700827b3 100644
> > --- a/fs/cifs/cached_dir.c
> > +++ b/fs/cifs/cached_dir.c
> > @@ -5,6 +5,7 @@
> >    *  Copyright (c) 2022, Ronnie Sahlberg <lsahlber@redhat.com>
> >    */
> >
> > +#include <linux/namei.h>
> >   #include "cifsglob.h"
> >   #include "cifsproto.h"
> >   #include "cifs_debug.h"
> > @@ -113,6 +114,50 @@ static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
> >       return cfid;
> >   }
> >
> > +static struct dentry *
> > +path_to_dentry(struct cifs_sb_info *cifs_sb, const char *full_path)
> > +{
> > +     struct dentry *dentry;
> > +     char *path = NULL;
> > +     char *s, *p;
> > +     char sep;
> > +
> > +     path = kstrdup(full_path, GFP_ATOMIC);
> > +     if (path == NULL)
> > +             return ERR_PTR(-ENOMEM);
>
> Why make a copy of the path? I don't see anything in the code
> below that modifies its contents...

Thanks!
You are right. I have fixed it.

>
> > +
> > +     sep = CIFS_DIR_SEP(cifs_sb);
> > +     dentry = dget(cifs_sb->root);
> > +     s = path;
> > +
> > +     do {
> > +             struct inode *dir = d_inode(dentry);
> > +             struct dentry *child;
> > +
> > +             if (!S_ISDIR(dir->i_mode)) {
> > +                     dput(dentry);
> > +                     dentry = ERR_PTR(-ENOTDIR);
> > +                     break;
> > +             }
> > +
> > +             /* skip separators */
> > +             while (*s == sep)
> > +                     s++;
> > +             if (!*s)
> > +                     break;
> > +             p = s++;
> > +             /* next separator */
> > +             while (*s && *s != sep)
> > +                     s++;
> > +
> > +             child = lookup_positive_unlocked(p, dentry, s - p);
> > +             dput(dentry);
> > +             dentry = child;
> > +     } while (!IS_ERR(dentry));
> > +     kfree(path);
> > +     return dentry;
> > +}
> > +
> >   /*
> >    * Open the and cache a directory handle.
> >    * If error then *cfid is not initialized.
> > @@ -139,7 +184,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >       struct dentry *dentry = NULL;
> >       struct cached_fid *cfid;
> >       struct cached_fids *cfids;
> > -
> >
> >       if (tcon == NULL || tcon->cfids == NULL || tcon->nohandlecache ||
> >           is_smb1_server(tcon->ses->server))
> > @@ -155,13 +199,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >       if (cifs_sb->root == NULL)
> >               return -ENOENT;
> >
> > -     /*
> > -      * TODO: for better caching we need to find and use the dentry also
> > -      * for non-root directories.
> > -      */
> > -     if (!strlen(path))
> > -             dentry = cifs_sb->root;
>
> Test path[0] instead of calling strlen?

Done.

>
> > -
> >       utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
> >       if (!utf16_path)
> >               return -ENOMEM;
> > @@ -253,12 +290,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >       oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId);
> >   #endif /* CIFS_DEBUG2 */
> >
> > -     cfid->tcon = tcon;
> > -     if (dentry) {
> > -             cfid->dentry = dentry;
> > -             dget(dentry);
> > -     }
> > -     /* BB TBD check to see if oplock level check can be removed below */
> >       if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
> >               goto oshr_free;
> >       }
> > @@ -277,6 +308,17 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >                               &rsp_iov[1], sizeof(struct smb2_file_all_info),
> >                               (char *)&cfid->file_all_info))
> >               cfid->file_all_info_is_valid = true;
> > +
> > +     if (!strlen(path)) {
> > +             dentry = dget(cifs_sb->root);
> > +             cfid->dentry = dentry;
> > +     } else{
> > +             dentry = path_to_dentry(cifs_sb, path);
> > +             if (IS_ERR(dentry))
> > +                     goto oshr_free;
> > +             cfid->dentry = dentry;
> > +     }
>
> Pull cfid->dentry = dentry out of the above conditionals and
> just set it here for both cases.

Of course.  done.
Thanks.
>
> > +     cfid->tcon = tcon;
> >       cfid->time = jiffies;
> >       cfid->is_open = true;
> >       cfid->has_lease = true;

Re: [PATCH 2/6] cifs: cifs: handlecache, only track the dentry for the root handle

[ronnie sahlberg]

On Wed, 24 Aug 2022 at 23:35, Tom Talpey <tom@talpey.com> wrote:
>
> On 8/23/2022 8:27 PM, Ronnie Sahlberg wrote:
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >   fs/cifs/cached_dir.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
> > index c2f5b71a3c9f..77880470c7ea 100644
> > --- a/fs/cifs/cached_dir.c
> > +++ b/fs/cifs/cached_dir.c
> > @@ -47,11 +47,12 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >       if (cifs_sb->root == NULL)
> >               return -ENOENT;
> >
> > +     if (!strlen(path))
> > +             dentry = cifs_sb->root;
>
> Wouldn't it be safer and more efficient to simply test
> "if (path[0] == 0)"?

Thanks!

Right. I have changed it like so.

>
> But, why would a non-null path ever be passed, if it
> always fails? Seems like a pointless call in the first
> place.
>
> > +
> >       if (strlen(path))
>
> Simply "else"? No need to recompute strlen.

Done.
It was done this way because shortly later in the patch series some of
these checks go away
once we add the capability to cache more than just the root directory.


> Tom.
>
> >               return -ENOENT;
> >
> > -     dentry = cifs_sb->root;
> > -
> >       cfid = &tcon->cfids->cfid;
> >       mutex_lock(&cfid->fid_mutex);
> >       if (cfid->is_valid) {
> > @@ -177,7 +178,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >       cfid->tcon = tcon;
> >       cfid->is_valid = true;
> >       cfid->dentry = dentry;
> > -     dget(dentry);
> > +     if (dentry)
> > +             dget(dentry);
> >       kref_init(&cfid->refcount);
> >
> >       /* BB TBD check to see if oplock level check can be removed below */

Re: [PATCH 4/6] cifs: start caching all directories we open and get a lease for

[ronnie sahlberg]

On Wed, 24 Aug 2022 at 23:42, Tom Talpey <tom@talpey.com> wrote:
>
> Comment on the subject.
>
> This code doesn't cache *all* directories, it is limited to
> MAX_CACHED_FIDS. And "open and get a lease for" is redundant
> as well - leases are granted at open.
>
> Suggest "Enable caching of directories for which a lease is held".
Thanks!

Good suggestion. changed.

>
> On 8/23/2022 8:27 PM, Ronnie Sahlberg wrote:
> > This expands the directory caching to now cache an open handle for all
> > directories (up to a maximum) and not just the root directory.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >   fs/cifs/cached_dir.c | 490 ++++++++++++++++++++++++++-----------------
> >   fs/cifs/cached_dir.h |  20 +-
> >   fs/cifs/inode.c      |   6 +-
> >   fs/cifs/smb2ops.c    |   2 +-
> >   4 files changed, 320 insertions(+), 198 deletions(-)
> >
> > diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
> > index 594ec4385077..8732903aea03 100644
> > --- a/fs/cifs/cached_dir.c
> > +++ b/fs/cifs/cached_dir.c
> > @@ -11,7 +11,107 @@
> >   #include "smb2proto.h"
> >   #include "cached_dir.h"
> >
> > -struct cached_fid *init_cached_dir(const char *path);
> > +static struct cached_fid *init_cached_dir(const char *path);
> > +static void free_cached_dir(struct cached_fid *cfid);
> > +
> > +/*
> > + * Locking and reference count for cached directory handles:
> > + *
> > + * The main function to get a reference to a cached handle is
> > + * find_or_create_cached_dir() called from open_cached_dir()
> > + * These functions are protected under the cfid_list_lock spin-lock
> > + * to make sure we do not race creating new references for cached dirs
> > + * with deletion of expired ones.
> > + *
> > + * An successful open_cached_dir() will take out 2 references to the cfid if
> > + * this was the very first and successful call to open the directory and
> > + * it acquired a lease from the server.
> > + * One reference is for the lease  and the other is for the cfid that we
> > + * return. The is lease reference is tracked by cfid->has_lease.
> > + * If the directory already has a handle with an active lease, then we just
> > + * take out one new reference for the cfid and return it.
> > + * It can happen that we have a thread that tries to open a cached directory
> > + * where we have a cfid already but we do not, yet, have a working lease. In
> > + * this case we will just return NULL, and this the caller will fall back to
> > + * the case when no handle was available.
> > + *
> > + * In this model the total number of references we have on a cfid is
> > + * 1 for while the handle is open and we have a lease, and one additional
> > + * reference for each open instance of a cfid.
> > + *
> > + *
> > + * Once we get a lease break (cached_dir_lease_break()) we remove the
> > + * cfid from the list under the spinlock. This prevents any new threads to
> > + * use it, and we also call smb2_cached_lease_break() via the work_queue
> > + * in order to drop the reference we got for the lease (we drop it outside
> > + * of the spin-lock.)
> > + * Anytime a thread calls close_cached_dir() we also drop a reference to the
> > + * cfid.
> > + * When the last reference to the cfid is released smb2_close_cached_fid()
> > + * will be invoked which will drop the reference ot the dentry we held for
> > + * this cfid and it will also, if we the handle is open/has a lease
> > + * also call SMB2_close() to close the handle on the server.
> > + *
> > + *
> > + * Two events require special handling:
> > + * invalidate_all_cached_dirs() this function is called from SMB2_tdis()
> > + * and cifs_mark_open_files_invalid().
> > + * In both cases the tcon is either gone already or will be shortly so
> > + * we do not need to actually close the handles. They will be dropped
> > + * server side as part of the tcon dropping.
> > + * But we have to be careful about a potential race with a concurrent
> > + * lease break so we need to take out additional refences to avoid the
> > + * cfid from being freed while we are still referencing it.
> > + *
> > + * free_cached_dirs() which is called from tconInfoFree().
> > + * This is called quite late in the umount process so there should no longer
> > + * be any open handles or files and we can just free all the remaining data.
> > + */
>
> This is a helpful comment during the review phase, but it's mighty
> detailed, and therefore subject to being wrong, in the future.

I have moved the comment to the commit message instead. That way it
will not become stale
if the code changes but is still present to assist review and the
intention of the code.


>
> > +
> > +static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids,
> > +                                                 const char *path,
> > +                                                 bool lookup_only)
> > +{
> > +     struct cached_fid *cfid;
> > +
> > +     spin_lock(&cfids->cfid_list_lock);
> > +     list_for_each_entry(cfid, &cfids->entries, entry) {
> > +             if (!strcmp(cfid->path, path)) {
> > +                     /*
> > +                      * If it doesn't have a lease it is either not yet
> > +                      * fully cached or it may be in the process of
> > +                      * being deleted due to a lease break.
> > +                      */
> > +                     if (!cfid->has_lease) {
> > +                             spin_unlock(&cfids->cfid_list_lock);
> > +                             return NULL;
> > +                     }
> > +                     kref_get(&cfid->refcount);
> > +                     spin_unlock(&cfids->cfid_list_lock);
> > +                     return cfid;
> > +             }
> > +     }
> > +     if (lookup_only) {
> > +             spin_unlock(&cfids->cfid_list_lock);
> > +             return NULL;
> > +     }
> > +     if (cfids->num_entries >= MAX_CACHED_FIDS) {
> > +             spin_unlock(&cfids->cfid_list_lock);
> > +             return NULL;
> > +     }
> > +     cfid = init_cached_dir(path);
> > +     if (cfid == NULL) {
> > +             spin_unlock(&cfids->cfid_list_lock);
> > +             return NULL;
> > +     }
> > +     cfid->cfids = cfids;
> > +     cfids->num_entries++;
> > +     list_add(&cfid->entry, &cfids->entries);
> > +     cfid->on_list = true;
> > +     kref_get(&cfid->refcount);
> > +     spin_unlock(&cfids->cfid_list_lock);
> > +     return cfid;
> > +}
> >
> >   /*
> >    * Open the and cache a directory handle.
> > @@ -33,62 +133,65 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >       struct kvec open_iov[SMB2_CREATE_IOV_SIZE];
> >       struct kvec qi_iov[1];
> >       int rc, flags = 0;
> > -     __le16 utf16_path = 0; /* Null - since an open of top of share */
> > +     __le16 *utf16_path = NULL;
> >       u8 oplock = SMB2_OPLOCK_LEVEL_II;
> >       struct cifs_fid *pfid;
> > -     struct dentry *dentry;
> > +     struct dentry *dentry = NULL;
> >       struct cached_fid *cfid;
> > +     struct cached_fids *cfids;
> > +
> >
> > -     if (tcon == NULL || tcon->nohandlecache ||
> > +     if (tcon == NULL || tcon->cfids == NULL || tcon->nohandlecache ||
> >           is_smb1_server(tcon->ses->server))
> > -             return -EOPNOTSUPP;
> > +             return -ENOTSUPP;
>
> Good catch - there's no operation here.
>
> >
> >       ses = tcon->ses;
> >       server = ses->server;
> > +     cfids = tcon->cfids;
> >
> > +     if (!server->ops->new_lease_key)
> > +             return -EIO;
> > +
> >       if (cifs_sb->root == NULL)
> >               return -ENOENT;
> >
> > +     /*
> > +      * TODO: for better caching we need to find and use the dentry also
> > +      * for non-root directories.
> > +      */
> >       if (!strlen(path))
> >               dentry = cifs_sb->root;
> >
> > -     if (strlen(path))
> > -             return -ENOENT;
> > +     utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
> > +     if (!utf16_path)
> > +             return -ENOMEM;
> >
> > -     cfid = tcon->cfids->cfid;
> > +     cfid = find_or_create_cached_dir(cfids, path, lookup_only);
> >       if (cfid == NULL) {
> > -             cfid = init_cached_dir(path);
> > -             tcon->cfids->cfid = cfid;
> > +             kfree(utf16_path);
> > +             return -ENOENT;
> >       }
> > -     if (cfid == NULL)
> > -             return -ENOMEM;
> > -
> > -     mutex_lock(&cfid->fid_mutex);
> > -     if (cfid->is_valid) {
> > -             cifs_dbg(FYI, "found a cached root file handle\n");
> > +     /*
> > +      * At this point we either have a lease already and we can just
> > +      * return it. If not we are guaranteed to be the only thread accessing
> > +      * this cfid.
> > +      */
> > +     if (cfid->has_lease) {
> >               *ret_cfid = cfid;
> > -             kref_get(&cfid->refcount);
> > -             mutex_unlock(&cfid->fid_mutex);
> > +             kfree(utf16_path);
> >               return 0;
> >       }
> >
> >       /*
> >        * We do not hold the lock for the open because in case
> > -      * SMB2_open needs to reconnect, it will end up calling
> > -      * cifs_mark_open_files_invalid() which takes the lock again
> > -      * thus causing a deadlock
> > +      * SMB2_open needs to reconnect.
> > +      * This is safe because no other thread will be able to get a ref
> > +      * to the cfid until we have finished opening the file and (possibly)
> > +      * aquired a lease.
> >        */
> > -     mutex_unlock(&cfid->fid_mutex);
> > -
> > -     if (lookup_only)
> > -             return -ENOENT;
> > -
> >       if (smb3_encryption_required(tcon))
> >               flags |= CIFS_TRANSFORM_REQ;
> >
> > -     if (!server->ops->new_lease_key)
> > -             return -EIO;
> > -
> >       pfid = &cfid->fid;
> >       server->ops->new_lease_key(pfid);
> >
> > @@ -109,7 +212,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >       oparms.reconnect = false;
> >
> >       rc = SMB2_open_init(tcon, server,
> > -                         &rqst[0], &oplock, &oparms, &utf16_path);
> > +                         &rqst[0], &oplock, &oparms, utf16_path);
> >       if (rc)
> >               goto oshr_free;
> >       smb2_set_next_command(tcon, &rqst[0]);
> > @@ -132,47 +235,13 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >       rc = compound_send_recv(xid, ses, server,
> >                               flags, 2, rqst,
> >                               resp_buftype, rsp_iov);
> > -     mutex_lock(&cfid->fid_mutex);
> > -
> > -     /*
> > -      * Now we need to check again as the cached root might have
> > -      * been successfully re-opened from a concurrent process
> > -      */
> > -
> > -     if (cfid->is_valid) {
> > -             /* work was already done */
> > -
> > -             /* stash fids for close() later */
> > -             struct cifs_fid fid = {
> > -                     .persistent_fid = pfid->persistent_fid,
> > -                     .volatile_fid = pfid->volatile_fid,
> > -             };
> > -
> > -             /*
> > -              * caller expects this func to set the fid in cfid to valid
> > -              * cached root, so increment the refcount.
> > -              */
> > -             kref_get(&cfid->refcount);
> > -
> > -             mutex_unlock(&cfid->fid_mutex);
> > -
> > -             if (rc == 0) {
> > -                     /* close extra handle outside of crit sec */
> > -                     SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
> > -             }
> > -             rc = 0;
> > -             goto oshr_free;
> > -     }
> > -
> > -     /* Cached root is still invalid, continue normaly */
> > -
> >       if (rc) {
> >               if (rc == -EREMCHG) {
> >                       tcon->need_reconnect = true;
> >                       pr_warn_once("server share %s deleted\n",
> >                                    tcon->treeName);
> >               }
> > -             goto oshr_exit;
> > +             goto oshr_free;
> >       }
> >
> >       atomic_inc(&tcon->num_remote_opens);
> > @@ -185,49 +254,56 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >   #endif /* CIFS_DEBUG2 */
> >
> >       cfid->tcon = tcon;
> > -     cfid->is_valid = true;
> > -     cfid->dentry = dentry;
> > -     if (dentry)
> > +     if (dentry) {
> > +             cfid->dentry = dentry;
> >               dget(dentry);
> > -     kref_init(&cfid->refcount);
> > -
> > +     }
> >       /* BB TBD check to see if oplock level check can be removed below */
> > -     if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) {
> > -             /*
> > -              * See commit 2f94a3125b87. Increment the refcount when we
> > -              * get a lease for root, release it if lease break occurs
> > -              */
> > -             kref_get(&cfid->refcount);
> > -             cfid->has_lease = true;
> > -             smb2_parse_contexts(server, o_rsp,
> > -                             &oparms.fid->epoch,
> > -                                 oparms.fid->lease_key, &oplock,
> > -                                 NULL, NULL);
> > -     } else
> > -             goto oshr_exit;
> > +     if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
> > +             goto oshr_free;
> > +     }
> >
> > +     smb2_parse_contexts(server, o_rsp,
> > +                         &oparms.fid->epoch,
> > +                         oparms.fid->lease_key, &oplock,
> > +                         NULL, NULL);
> > +
> >       qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> >       if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
> > -             goto oshr_exit;
> > +             goto oshr_free;
> >       if (!smb2_validate_and_copy_iov(
> >                               le16_to_cpu(qi_rsp->OutputBufferOffset),
> >                               sizeof(struct smb2_file_all_info),
> >                               &rsp_iov[1], sizeof(struct smb2_file_all_info),
> >                               (char *)&cfid->file_all_info))
> >               cfid->file_all_info_is_valid = true;
> > -
> >       cfid->time = jiffies;
> > +     cfid->is_open = true;
> > +     cfid->has_lease = true;
> >
> > -oshr_exit:
> > -     mutex_unlock(&cfid->fid_mutex);
> >   oshr_free:
> > +     kfree(utf16_path);
> >       SMB2_open_free(&rqst[0]);
> >       SMB2_query_info_free(&rqst[1]);
> >       free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
> >       free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> > -     if (rc == 0)
> > -             *ret_cfid = cfid;
> > -
> > +     spin_lock(&cfids->cfid_list_lock);
> > +     if (!cfid->has_lease) {
> > +             if (cfid->on_list) {
> > +                     list_del(&cfid->entry);
> > +                     cfid->on_list = false;
> > +                     cfids->num_entries--;
> > +             }
> > +             rc = -ENOENT;
> > +     }
> > +     spin_unlock(&cfids->cfid_list_lock);
> > +     if (rc) {
> > +             free_cached_dir(cfid);
> > +             cfid = NULL;
> > +     }
> > +     if (rc == 0) {
> > +             *ret_cfid = cfid;
> > +     }
> >       return rc;
> >   }
> >
> > @@ -236,20 +312,22 @@ int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
> >                             struct cached_fid **ret_cfid)
> >   {
> >       struct cached_fid *cfid;
> > +     struct cached_fids *cfids = tcon->cfids;
> >
> > -     cfid = tcon->cfids->cfid;
> > -     if (cfid == NULL)
> > +     if (cfids == NULL)
> >               return -ENOENT;
> >
> > -     mutex_lock(&cfid->fid_mutex);
> > -     if (cfid->dentry == dentry) {
> > -             cifs_dbg(FYI, "found a cached root file handle by dentry\n");
> > -             *ret_cfid = cfid;
> > -             kref_get(&cfid->refcount);
> > -             mutex_unlock(&cfid->fid_mutex);
> > -             return 0;
> > +     spin_lock(&cfids->cfid_list_lock);
> > +     list_for_each_entry(cfid, &cfids->entries, entry) {
> > +             if (dentry && cfid->dentry == dentry) {
> > +                     cifs_dbg(FYI, "found a cached root file handle by dentry\n");
> > +                     kref_get(&cfid->refcount);
> > +                     *ret_cfid = cfid;
> > +                     spin_unlock(&cfids->cfid_list_lock);
> > +                     return 0;
> > +             }
> >       }
> > -     mutex_unlock(&cfid->fid_mutex);
> > +     spin_unlock(&cfids->cfid_list_lock);
> >       return -ENOENT;
> >   }
> >
> > @@ -258,63 +336,29 @@ smb2_close_cached_fid(struct kref *ref)
> >   {
> >       struct cached_fid *cfid = container_of(ref, struct cached_fid,
> >                                              refcount);
> > -     struct cached_dirent *dirent, *q;
> >
> > -     if (cfid->is_valid) {
> > -             cifs_dbg(FYI, "clear cached root file handle\n");
> > -             SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
> > -                        cfid->fid.volatile_fid);
> > +     spin_lock(&cfid->cfids->cfid_list_lock);
> > +     if (cfid->on_list) {
> > +             list_del(&cfid->entry);
> > +             cfid->on_list = false;
> > +             cfid->cfids->num_entries--;
> >       }
> > +     spin_unlock(&cfid->cfids->cfid_list_lock);
> >
> > -     /*
> > -      * We only check validity above to send SMB2_close,
> > -      * but we still need to invalidate these entries
> > -      * when this function is called
> > -      */
> > -     cfid->is_valid = false;
> > -     cfid->file_all_info_is_valid = false;
> > -     cfid->has_lease = false;
> > -     if (cfid->dentry) {
> > -             dput(cfid->dentry);
> > -             cfid->dentry = NULL;
> > -     }
> > -     /*
> > -      * Delete all cached dirent names
> > -      */
> > -     mutex_lock(&cfid->dirents.de_mutex);
> > -     list_for_each_entry_safe(dirent, q, &cfid->dirents.entries, entry) {
> > -             list_del(&dirent->entry);
> > -             kfree(dirent->name);
> > -             kfree(dirent);
> > +     dput(cfid->dentry);
> > +     cfid->dentry = NULL;
> > +
> > +     if (cfid->is_open) {
> > +             SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid,
> > +                        cfid->fid.volatile_fid);
> >       }
> > -     cfid->dirents.is_valid = 0;
> > -     cfid->dirents.is_failed = 0;
> > -     cfid->dirents.ctx = NULL;
> > -     cfid->dirents.pos = 0;
> > -     mutex_unlock(&cfid->dirents.de_mutex);
> >
> > +     free_cached_dir(cfid);
> >   }
> >
> >   void close_cached_dir(struct cached_fid *cfid)
> >   {
> > -     mutex_lock(&cfid->fid_mutex);
> >       kref_put(&cfid->refcount, smb2_close_cached_fid);
> > -     mutex_unlock(&cfid->fid_mutex);
> > -}
> > -
> > -void close_cached_dir_lease_locked(struct cached_fid *cfid)
> > -{
> > -     if (cfid->has_lease) {
> > -             cfid->has_lease = false;
> > -             kref_put(&cfid->refcount, smb2_close_cached_fid);
> > -     }
> > -}
> > -
> > -void close_cached_dir_lease(struct cached_fid *cfid)
> > -{
> > -     mutex_lock(&cfid->fid_mutex);
> > -     close_cached_dir_lease_locked(cfid);
> > -     mutex_unlock(&cfid->fid_mutex);
> >   }
> >
> >   /*
> > @@ -327,41 +371,62 @@ void close_all_cached_dirs(struct cifs_sb_info *cifs_sb)
> >       struct cached_fid *cfid;
> >       struct cifs_tcon *tcon;
> >       struct tcon_link *tlink;
> > +     struct cached_fids *cfids;
> >
> >       for (node = rb_first(root); node; node = rb_next(node)) {
> >               tlink = rb_entry(node, struct tcon_link, tl_rbnode);
> >               tcon = tlink_tcon(tlink);
> >               if (IS_ERR(tcon))
> >                       continue;
> > -             cfid = tcon->cfids->cfid;
> > -             if (cfid == NULL)
> > +             cfids = tcon->cfids;
> > +             if (cfids == NULL)
> >                       continue;
> > -             mutex_lock(&cfid->fid_mutex);
> > -             if (cfid->dentry) {
> > +             list_for_each_entry(cfid, &cfids->entries, entry) {
> >                       dput(cfid->dentry);
> >                       cfid->dentry = NULL;
> >               }
> > -             mutex_unlock(&cfid->fid_mutex);
> >       }
> >   }
> >
> >   /*
> > - * Invalidate and close all cached dirs when a TCON has been reset
> > + * Invalidate all cached dirs when a TCON has been reset
> >    * due to a session loss.
> >    */
> >   void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
> >   {
> > -     struct cached_fid *cfid = tcon->cfids->cfid;
> > -
> > -     if (cfid == NULL)
> > -             return;
> > -
> > -     mutex_lock(&cfid->fid_mutex);
> > -     cfid->is_valid = false;
> > -     /* cached handle is not valid, so SMB2_CLOSE won't be sent below */
> > -     close_cached_dir_lease_locked(cfid);
> > -     memset(&cfid->fid, 0, sizeof(struct cifs_fid));
> > -     mutex_unlock(&cfid->fid_mutex);
> > +     struct cached_fids *cfids = tcon->cfids;
> > +     struct cached_fid *cfid, *q;
> > +     struct list_head entry;
> > +
> > +     INIT_LIST_HEAD(&entry);
> > +     spin_lock(&cfids->cfid_list_lock);
> > +     list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
> > +             list_del(&cfid->entry);
> > +             list_add(&cfid->entry, &entry);
> > +             cfids->num_entries--;
> > +             cfid->is_open = false;
> > +             /* To prevent race with smb2_cached_lease_break() */
> > +             kref_get(&cfid->refcount);
> > +     }
> > +     spin_unlock(&cfids->cfid_list_lock);
> > +
> > +     list_for_each_entry_safe(cfid, q, &entry, entry) {
> > +             cfid->on_list = false;
> > +             list_del(&cfid->entry);
> > +             cancel_work_sync(&cfid->lease_break);
> > +             if (cfid->has_lease) {
> > +                     /*
> > +                      * We lease was never cancelled from the server so we
> > +                      * we need to drop the reference.
> > +                      */
> > +                     spin_lock(&cfids->cfid_list_lock);
> > +                     cfid->has_lease = false;
> > +                     spin_unlock(&cfids->cfid_list_lock);
> > +                     kref_put(&cfid->refcount, smb2_close_cached_fid);
> > +             }
> > +             /* Drop the extra reference opened above*/
> > +             kref_put(&cfid->refcount, smb2_close_cached_fid);
> > +     }
> >   }
> >
> >   static void
> > @@ -370,51 +435,83 @@ smb2_cached_lease_break(struct work_struct *work)
> >       struct cached_fid *cfid = container_of(work,
> >                               struct cached_fid, lease_break);
> >
> > -     close_cached_dir_lease(cfid);
> > +     spin_lock(&cfid->cfids->cfid_list_lock);
> > +     cfid->has_lease = false;
> > +     spin_unlock(&cfid->cfids->cfid_list_lock);
> > +     kref_put(&cfid->refcount, smb2_close_cached_fid);
> >   }
> >
> >   int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
> >   {
> > -     struct cached_fid *cfid = tcon->cfids->cfid;
> > +     struct cached_fids *cfids = tcon->cfids;
> > +     struct cached_fid *cfid;
> >
> > -     if (cfid == NULL)
> > +     if (cfids == NULL)
> >               return false;
> >
> > -     if (cfid->is_valid &&
> > -         !memcmp(lease_key,
> > -                 cfid->fid.lease_key,
> > -                 SMB2_LEASE_KEY_SIZE)) {
> > -             cfid->time = 0;
> > -             INIT_WORK(&cfid->lease_break,
> > -                       smb2_cached_lease_break);
> > -             queue_work(cifsiod_wq,
> > -                        &cfid->lease_break);
> > -             return true;
> > +     spin_lock(&cfids->cfid_list_lock);
> > +     list_for_each_entry(cfid, &cfids->entries, entry) {
> > +             if (cfid->has_lease &&
> > +                 !memcmp(lease_key,
> > +                         cfid->fid.lease_key,
> > +                         SMB2_LEASE_KEY_SIZE)) {
> > +                     cfid->time = 0;
> > +                     /*
> > +                      * We found a lease remove it from the list
> > +                      * so no threads threads can access it.
> > +                      */
> > +                     list_del(&cfid->entry);
> > +                     cfid->on_list = false;
> > +                     cfids->num_entries--;
> > +
> > +                     queue_work(cifsiod_wq,
> > +                                &cfid->lease_break);
> > +                     spin_unlock(&cfids->cfid_list_lock);
> > +                     return true;
> > +             }
> >       }
> > +     spin_unlock(&cfids->cfid_list_lock);
> >       return false;
> >   }
> >
> > -struct cached_fid *init_cached_dir(const char *path)
> > +static struct cached_fid *init_cached_dir(const char *path)
> >   {
> >       struct cached_fid *cfid;
> >
> > -     cfid = kzalloc(sizeof(*cfid), GFP_KERNEL);
> > +     cfid = kzalloc(sizeof(*cfid), GFP_ATOMIC);
> >       if (!cfid)
> >               return NULL;
> > -     cfid->path = kstrdup(path, GFP_KERNEL);
> > +     cfid->path = kstrdup(path, GFP_ATOMIC);
> >       if (!cfid->path) {
> >               kfree(cfid);
> >               return NULL;
> >       }
> >
> > +     INIT_WORK(&cfid->lease_break, smb2_cached_lease_break);
> > +     INIT_LIST_HEAD(&cfid->entry);
> >       INIT_LIST_HEAD(&cfid->dirents.entries);
> >       mutex_init(&cfid->dirents.de_mutex);
> > -     mutex_init(&cfid->fid_mutex);
> > +     spin_lock_init(&cfid->fid_lock);
> > +     kref_init(&cfid->refcount);
> >       return cfid;
> >   }
> >
> > -void free_cached_dir(struct cached_fid *cfid)
> > +static void free_cached_dir(struct cached_fid *cfid)
> >   {
> > +     struct cached_dirent *dirent, *q;
> > +
> > +     dput(cfid->dentry);
> > +     cfid->dentry = NULL;
> > +
> > +     /*
> > +      * Delete all cached dirent names
> > +      */
> > +     list_for_each_entry_safe(dirent, q, &cfid->dirents.entries, entry) {
> > +             list_del(&dirent->entry);
> > +             kfree(dirent->name);
> > +             kfree(dirent);
> > +     }
> > +
> >       kfree(cfid->path);
> >       cfid->path = NULL;
> >       kfree(cfid);
> > @@ -427,15 +524,34 @@ struct cached_fids *init_cached_dirs(void)
> >       cfids = kzalloc(sizeof(*cfids), GFP_KERNEL);
> >       if (!cfids)
> >               return NULL;
> > -     mutex_init(&cfids->cfid_list_mutex);
> > +     spin_lock_init(&cfids->cfid_list_lock);
> > +     INIT_LIST_HEAD(&cfids->entries);
> >       return cfids;
> >   }
> >
> > +/*
> > + * Called from tconInfoFree when we are tearing down the tcon.
> > + * There are no active users or open files/directories at this point.
> > + */
> >   void free_cached_dirs(struct cached_fids *cfids)
> >   {
> > -     if (cfids->cfid) {
> > -             free_cached_dir(cfids->cfid);
> > -             cfids->cfid = NULL;
> > +     struct cached_fid *cfid, *q;
> > +     struct list_head entry;
> > +
> > +             INIT_LIST_HEAD(&entry);
> > +     spin_lock(&cfids->cfid_list_lock);
> > +     list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
> > +             cfid->on_list = false;
> > +             cfid->is_open = false;
> > +             list_del(&cfid->entry);
> > +             list_add(&cfid->entry, &entry);
> > +     }
> > +     spin_unlock(&cfids->cfid_list_lock);
> > +
> > +     list_for_each_entry_safe(cfid, q, &entry, entry) {
> > +             list_del(&cfid->entry);
> > +             free_cached_dir(cfid);
> >       }
> > -     kfree(cfids);
> > +
> > +     kfree(cfids);
> >   }
> > diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
> > index bdf6c3866653..e536304ca2ce 100644
> > --- a/fs/cifs/cached_dir.h
> > +++ b/fs/cifs/cached_dir.h
> > @@ -31,14 +31,17 @@ struct cached_dirents {
> >   };
> >
> >   struct cached_fid {
> > +     struct list_head entry;
> > +     struct cached_fids *cfids;
> >       const char *path;
> > -     bool is_valid:1;        /* Do we have a useable root fid */
> > -     bool file_all_info_is_valid:1;
> >       bool has_lease:1;
> > +     bool is_open:1;
> > +     bool on_list:1;
> > +     bool file_all_info_is_valid:1;
> >       unsigned long time; /* jiffies of when lease was taken */
> >       struct kref refcount;
> >       struct cifs_fid fid;
> > -     struct mutex fid_mutex;
> > +     spinlock_t fid_lock;
> >       struct cifs_tcon *tcon;
> >       struct dentry *dentry;
> >       struct work_struct lease_break;
> > @@ -46,9 +49,14 @@ struct cached_fid {
> >       struct cached_dirents dirents;
> >   };
> >
> > +#define MAX_CACHED_FIDS 16
> >   struct cached_fids {
> > -     struct mutex cfid_list_mutex;
> > -     struct cached_fid *cfid;
> > +     /* Must be held when:
> > +      * - accessing the cfids->entries list
> > +      */
> > +     spinlock_t cfid_list_lock;
> > +     int num_entries;
> > +     struct list_head entries;
> >   };
> >
> >   extern struct cached_fids *init_cached_dirs(void);
> > @@ -61,8 +69,6 @@ extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon,
> >                                    struct dentry *dentry,
> >                                    struct cached_fid **cfid);
> >   extern void close_cached_dir(struct cached_fid *cfid);
> > -extern void close_cached_dir_lease(struct cached_fid *cfid);
> > -extern void close_cached_dir_lease_locked(struct cached_fid *cfid);
> >   extern void close_all_cached_dirs(struct cifs_sb_info *cifs_sb);
> >   extern void invalidate_all_cached_dirs(struct cifs_tcon *tcon);
> >   extern int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16]);
> > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > index bac08c20f559..a42397b52882 100644
> > --- a/fs/cifs/inode.c
> > +++ b/fs/cifs/inode.c
> > @@ -2265,13 +2265,13 @@ cifs_dentry_needs_reval(struct dentry *dentry)
> >               return true;
> >
> >       if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) {
> > -             mutex_lock(&cfid->fid_mutex);
> > +             spin_lock(&cfid->fid_lock);
> >               if (cfid->time && cifs_i->time > cfid->time) {
> > -                     mutex_unlock(&cfid->fid_mutex);
> > +                     spin_unlock(&cfid->fid_lock);
> >                       close_cached_dir(cfid);
> >                       return false;
> >               }
> > -             mutex_unlock(&cfid->fid_mutex);
> > +             spin_unlock(&cfid->fid_lock);
> >               close_cached_dir(cfid);
> >       }
> >       /*
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 96f3b0573606..1ed4b4992025 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -787,7 +787,7 @@ 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->is_valid) {
> > +             if (cfid->has_lease) {
> >                       close_cached_dir(cfid);
> >                       return 0;
> >               }

Re: [PATCH 6/6] cifs: do not cache leased directories for longer than 30 seconds

[ronnie sahlberg]

On Wed, 24 Aug 2022 at 23:58, Tom Talpey <tom@talpey.com> wrote:
>
> On 8/23/2022 8:27 PM, Ronnie Sahlberg wrote:
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >   fs/cifs/cached_dir.c | 32 ++++++++++++++++++++++++++++++--
> >   fs/cifs/cached_dir.h |  2 ++
> >   2 files changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
> > index f4d7700827b3..1c4a409bcb67 100644
> > --- a/fs/cifs/cached_dir.c
> > +++ b/fs/cifs/cached_dir.c
> > @@ -14,6 +14,7 @@
> >
> >   static struct cached_fid *init_cached_dir(const char *path);
> >   static void free_cached_dir(struct cached_fid *cfid);
> > +static void smb2_cached_lease_timeout(struct work_struct *work);
> >
> >   /*
> >    * Locking and reference count for cached directory handles:
> > @@ -321,6 +322,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> >       cfid->tcon = tcon;
> >       cfid->time = jiffies;
> >       cfid->is_open = true;
> > +     queue_delayed_work(cifsiod_wq, &cfid->lease_timeout, HZ * 30);
>
> Wouldn't it be more efficient to implement a laundromat? There
> will be MAX_CACHED_FIDS of these delayed workers, and the
> timing does not need to be precise (right?).

I was thinking that first but thought it would be easier to just use
delayed events on the pre-existing
work-queue. However, having two potentially racing work items turned
out to be as complex as
just having a separate thread for the tcon.
I will drop this patch for now and re-do it with a laundromat instead.
It will be more efficient
and I think it will make the logic a bit simpler too.

>
> Is it worth considering making HZ*30 into a tunable?

Maybe. I can add that when I re-spin this patch.
Recinding leases is super hard to get right. Recind them too quickly
and you miss out on potential performance.
Recind them too late and you hurt performance, at least for other clients.

Right now the caching is binary. It is either enabled, or fully disabled.
I would like to have timeouts like this to be auto-adjusted by the
module itself, because setting it "correctly"
or "optimally" will probably be super hard, to impossible, for the
average sysadmin.
Heck, I don't think I could even set a parameter like this correctly.
And that is even not taking into account that workloads change
dynamically over time so any fixed value will only
be "correct" for some/part of the time anyway. Sometimes these changes
occur over just a few minutes/hours so a fixed value is impossible to
come up with.

I think it would be possible to have this to be automatically and
dynamically adjusted.
I want to measure things like "how often do we re-open a directory
while holding a lease",   "how often is the lease broken by the
server"
and try to keep the first as high as possible while also have the
second as low, or zero.
And have this adjust automatically at runtime.

>
> Tom.
>
> > +     cfid->has_timeout = true;
> >       cfid->has_lease = true;
> >
> >   oshr_free:
> > @@ -447,6 +450,7 @@ void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
> >               list_add(&cfid->entry, &entry);
> >               cfids->num_entries--;
> >               cfid->is_open = false;
> > +             cfid->has_timeout = false;
> >               /* To prevent race with smb2_cached_lease_break() */
> >               kref_get(&cfid->refcount);
> >       }
> > @@ -455,6 +459,7 @@ void invalidate_all_cached_dirs(struct cifs_tcon *tcon)
> >       list_for_each_entry_safe(cfid, q, &entry, entry) {
> >               cfid->on_list = false;
> >               list_del(&cfid->entry);
> > +             cancel_delayed_work_sync(&cfid->lease_timeout);
> >               cancel_work_sync(&cfid->lease_break);
> >               if (cfid->has_lease) {
> >                       /*
> > @@ -477,12 +482,34 @@ smb2_cached_lease_break(struct work_struct *work)
> >       struct cached_fid *cfid = container_of(work,
> >                               struct cached_fid, lease_break);
> >
> > +     cancel_delayed_work_sync(&cfid->lease_timeout);
> >       spin_lock(&cfid->cfids->cfid_list_lock);
> > +     if (!cfid->has_lease) {
> > +             spin_unlock(&cfid->cfids->cfid_list_lock);
> > +             return;
> > +     }
> >       cfid->has_lease = false;
> >       spin_unlock(&cfid->cfids->cfid_list_lock);
> >       kref_put(&cfid->refcount, smb2_close_cached_fid);
> >   }
> >
> > +static void
> > +smb2_cached_lease_timeout(struct work_struct *work)
> > +{
> > +     struct cached_fid *cfid = container_of(work,
> > +                             struct cached_fid, lease_timeout.work);
> > +
> > +     spin_lock(&cfid->cfids->cfid_list_lock);
> > +     if (!cfid->has_timeout || !cfid->on_list) {
> > +             spin_unlock(&cfid->cfids->cfid_list_lock);
> > +             return;
> > +     }
> > +     cfid->has_timeout = false;
> > +     spin_unlock(&cfid->cfids->cfid_list_lock);
> > +
> > +     queue_work(cifsiod_wq, &cfid->lease_break);
> > +}
> > +
> >   int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
> >   {
> >       struct cached_fids *cfids = tcon->cfids;
> > @@ -506,9 +533,9 @@ int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16])
> >                       cfid->on_list = false;
> >                       cfids->num_entries--;
> >
> > -                     queue_work(cifsiod_wq,
> > -                                &cfid->lease_break);
> > +                     cfid->has_timeout = false;
> >                       spin_unlock(&cfids->cfid_list_lock);
> > +                     queue_work(cifsiod_wq, &cfid->lease_break);
> >                       return true;
> >               }
> >       }
> > @@ -530,6 +557,7 @@ static struct cached_fid *init_cached_dir(const char *path)
> >       }
> >
> >       INIT_WORK(&cfid->lease_break, smb2_cached_lease_break);
> > +     INIT_DELAYED_WORK(&cfid->lease_timeout, smb2_cached_lease_timeout);
> >       INIT_LIST_HEAD(&cfid->entry);
> >       INIT_LIST_HEAD(&cfid->dirents.entries);
> >       mutex_init(&cfid->dirents.de_mutex);
> > diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
> > index e536304ca2ce..813cd30f7ca3 100644
> > --- a/fs/cifs/cached_dir.h
> > +++ b/fs/cifs/cached_dir.h
> > @@ -35,6 +35,7 @@ struct cached_fid {
> >       struct cached_fids *cfids;
> >       const char *path;
> >       bool has_lease:1;
> > +     bool has_timeout:1;
> >       bool is_open:1;
> >       bool on_list:1;
> >       bool file_all_info_is_valid:1;
> > @@ -45,6 +46,7 @@ struct cached_fid {
> >       struct cifs_tcon *tcon;
> >       struct dentry *dentry;
> >       struct work_struct lease_break;
> > +     struct delayed_work lease_timeout;
> >       struct smb2_file_all_info file_all_info;
> >       struct cached_dirents dirents;
> >   };

Re: [PATCH 6/6] cifs: do not cache leased directories for longer than 30 seconds

[Tom Talpey]

On 8/25/2022 12:39 AM, ronnie sahlberg wrote:
> On Wed, 24 Aug 2022 at 23:58, Tom Talpey <tom@talpey.com> wrote:
>> Is it worth considering making HZ*30 into a tunable?
> 
> Maybe. I can add that when I re-spin this patch.
> Recinding leases is super hard to get right. Recind them too quickly
> and you miss out on potential performance.
> Recind them too late and you hurt performance, at least for other clients.
> 
> Right now the caching is binary. It is either enabled, or fully disabled.
> I would like to have timeouts like this to be auto-adjusted by the
> module itself, because setting it "correctly"
> or "optimally" will probably be super hard, to impossible, for the
> average sysadmin.
> Heck, I don't think I could even set a parameter like this correctly.
> And that is even not taking into account that workloads change
> dynamically over time so any fixed value will only
> be "correct" for some/part of the time anyway. Sometimes these changes
> occur over just a few minutes/hours so a fixed value is impossible to
> come up with.
> 
> I think it would be possible to have this to be automatically and
> dynamically adjusted.
> I want to measure things like "how often do we re-open a directory
> while holding a lease",   "how often is the lease broken by the
> server"
> and try to keep the first as high as possible while also have the
> second as low, or zero.
> And have this adjust automatically at runtime.

Unfortunately I think the problem is that the lease behavior is
dependent on the workload, and whether the app is sharing files.
If the lease is never recalled, obviously the caching can be
"forever". But apps banging heads over a set of shared files in
a shared directory will be nearly pointless. I'm not sure how
you can sanely automate that detection.

However, it seems to me that 30 seconds is pretty arbitrary.
It might help speed up a "tar x" or "tar c" but is it broadly
worthwhile? Would 5 seconds do the same? Dunno.

Tom.