Updates to directory caching

Updates to directory caching

[Ronnie Sahlberg]

Steve, List

Small update to the directory caching series.
The first patch is an updated to the patch at the tip of your for-next.
The patch in for-next does not have the changes Tom suggested. So please
replace it with the patch below.

The second patch adds a timeout for the leases and a laundromat thread
to return the leases when the timeout expired.

[PATCH 1/2] 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>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cached_dir.c | 63 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
index 0f62e37b4251..e2f2052dbabf 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"
@@ -59,6 +60,44 @@ 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 *path)
+{
+	struct dentry *dentry;
+	const char *s, *p;
+	char sep;
+
+	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));
+	return dentry;
+}
+
 /*
  * Open the and cache a directory handle.
  * If error then *cfid is not initialized.
@@ -86,7 +125,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	struct cached_fid *cfid;
 	struct cached_fids *cfids;
 
-
 	if (tcon == NULL || tcon->cfids == NULL || tcon->nohandlecache ||
 	    is_smb1_server(tcon->ses->server))
 		return -EOPNOTSUPP;
@@ -101,13 +139,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 (!path[0])
-		dentry = cifs_sb->root;
-
 	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
 	if (!utf16_path)
 		return -ENOMEM;
@@ -199,12 +230,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;
 	}
@@ -223,6 +248,16 @@ 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 (!path[0])
+		dentry = dget(cifs_sb->root);
+	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 2/2] cifs: Add a laundromat thread that will timeout any directory leases we have

[Ronnie Sahlberg]

after a timeout of 5 seconds. Later we will add instrumentation to tweak this value.

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

diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
index e2f2052dbabf..ffe0dea08b17 100644
--- a/fs/cifs/cached_dir.c
+++ b/fs/cifs/cached_dir.c
@@ -95,6 +95,7 @@ path_to_dentry(struct cifs_sb_info *cifs_sb, const char *path)
 		dput(dentry);
 		dentry = child;
 	} while (!IS_ERR(dentry));
+
 	return dentry;
 }
 
@@ -498,6 +499,55 @@ static void free_cached_dir(struct cached_fid *cfid)
 	kfree(cfid);
 }
 
+
+
+static int
+cifs_cfids_laundromat_thread(void *p)
+{
+	struct cached_fids *cfids = p;
+	struct cached_fid *cfid, *q;
+	struct list_head entry;
+
+	while (!kthread_should_stop()) {
+		ssleep(1);
+		INIT_LIST_HEAD(&entry);
+		if (kthread_should_stop())
+			return 0;
+		spin_lock(&cfids->cfid_list_lock);
+		list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
+			if (time_after(jiffies, cfid->time + HZ * 5)) {
+				list_del(&cfid->entry);
+				list_add(&cfid->entry, &entry);
+				cfids->num_entries--;
+			}
+		}
+		spin_unlock(&cfids->cfid_list_lock);
+
+		list_for_each_entry_safe(cfid, q, &entry, entry) {
+			cfid->on_list = false;
+			list_del(&cfid->entry);
+			/*
+			 * Cancel, and wait for the work to finish in
+			 * case we are racing with it.
+			 */
+			cancel_work_sync(&cfid->lease_break);
+			if (cfid->has_lease) {
+				/*
+				 * We lease has not yet been cancelled from
+				 * the server so 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);
+			}
+		}
+	}
+
+	return 0;
+}
+
+
 struct cached_fids *init_cached_dirs(void)
 {
 	struct cached_fids *cfids;
@@ -507,6 +557,20 @@ struct cached_fids *init_cached_dirs(void)
 		return NULL;
 	spin_lock_init(&cfids->cfid_list_lock);
 	INIT_LIST_HEAD(&cfids->entries);
+
+	/*
+	 * since we're in a cifs function already, we know that
+	 * this will succeed. No need for try_module_get().
+	 */
+	__module_get(THIS_MODULE);
+	cfids->laundromat = kthread_run(cifs_cfids_laundromat_thread,
+				  cfids, "cifsd-cfid-laundromat");
+	if (IS_ERR(cfids->laundromat)) {
+		cifs_dbg(VFS, "Failed to start cfids laundromat thread.\n");
+		kfree(cfids);
+		module_put(THIS_MODULE);
+		return NULL;
+	}
 	return cfids;
 }
 
@@ -519,6 +583,12 @@ void free_cached_dirs(struct cached_fids *cfids)
 	struct cached_fid *cfid, *q;
 	struct list_head entry;
 
+	if (cfids->laundromat) {
+		kthread_stop(cfids->laundromat);
+		cfids->laundromat = NULL;
+		module_put(THIS_MODULE);
+	}
+	
 	INIT_LIST_HEAD(&entry);
 	spin_lock(&cfids->cfid_list_lock);
 	list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
index e536304ca2ce..4ab9b10cd098 100644
--- a/fs/cifs/cached_dir.h
+++ b/fs/cifs/cached_dir.h
@@ -57,6 +57,7 @@ struct cached_fids {
 	spinlock_t cfid_list_lock;
 	int num_entries;
 	struct list_head entries;
+	struct task_struct *laundromat;
 };
 
 extern struct cached_fids *init_cached_dirs(void);
-- 
2.35.3

Re: [PATCH 2/2] cifs: Add a laundromat thread that will timeout any directory leases we have

[Steve French]

fixing minor whitespace error and tentatively pushed to cifs-2.6.git
for-next pending testing

On Thu, Sep 22, 2022 at 7:56 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> after a timeout of 5 seconds. Later we will add instrumentation to tweak this value.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/cached_dir.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
>  fs/cifs/cached_dir.h |  1 +
>  2 files changed, 71 insertions(+)
>
> diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c
> index e2f2052dbabf..ffe0dea08b17 100644
> --- a/fs/cifs/cached_dir.c
> +++ b/fs/cifs/cached_dir.c
> @@ -95,6 +95,7 @@ path_to_dentry(struct cifs_sb_info *cifs_sb, const char *path)
>                 dput(dentry);
>                 dentry = child;
>         } while (!IS_ERR(dentry));
> +
>         return dentry;
>  }
>
> @@ -498,6 +499,55 @@ static void free_cached_dir(struct cached_fid *cfid)
>         kfree(cfid);
>  }
>
> +
> +
> +static int
> +cifs_cfids_laundromat_thread(void *p)
> +{
> +       struct cached_fids *cfids = p;
> +       struct cached_fid *cfid, *q;
> +       struct list_head entry;
> +
> +       while (!kthread_should_stop()) {
> +               ssleep(1);
> +               INIT_LIST_HEAD(&entry);
> +               if (kthread_should_stop())
> +                       return 0;
> +               spin_lock(&cfids->cfid_list_lock);
> +               list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
> +                       if (time_after(jiffies, cfid->time + HZ * 5)) {
> +                               list_del(&cfid->entry);
> +                               list_add(&cfid->entry, &entry);
> +                               cfids->num_entries--;
> +                       }
> +               }
> +               spin_unlock(&cfids->cfid_list_lock);
> +
> +               list_for_each_entry_safe(cfid, q, &entry, entry) {
> +                       cfid->on_list = false;
> +                       list_del(&cfid->entry);
> +                       /*
> +                        * Cancel, and wait for the work to finish in
> +                        * case we are racing with it.
> +                        */
> +                       cancel_work_sync(&cfid->lease_break);
> +                       if (cfid->has_lease) {
> +                               /*
> +                                * We lease has not yet been cancelled from
> +                                * the server so 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);
> +                       }
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +
>  struct cached_fids *init_cached_dirs(void)
>  {
>         struct cached_fids *cfids;
> @@ -507,6 +557,20 @@ struct cached_fids *init_cached_dirs(void)
>                 return NULL;
>         spin_lock_init(&cfids->cfid_list_lock);
>         INIT_LIST_HEAD(&cfids->entries);
> +
> +       /*
> +        * since we're in a cifs function already, we know that
> +        * this will succeed. No need for try_module_get().
> +        */
> +       __module_get(THIS_MODULE);
> +       cfids->laundromat = kthread_run(cifs_cfids_laundromat_thread,
> +                                 cfids, "cifsd-cfid-laundromat");
> +       if (IS_ERR(cfids->laundromat)) {
> +               cifs_dbg(VFS, "Failed to start cfids laundromat thread.\n");
> +               kfree(cfids);
> +               module_put(THIS_MODULE);
> +               return NULL;
> +       }
>         return cfids;
>  }
>
> @@ -519,6 +583,12 @@ void free_cached_dirs(struct cached_fids *cfids)
>         struct cached_fid *cfid, *q;
>         struct list_head entry;
>
> +       if (cfids->laundromat) {
> +               kthread_stop(cfids->laundromat);
> +               cfids->laundromat = NULL;
> +               module_put(THIS_MODULE);
> +       }
> +
>         INIT_LIST_HEAD(&entry);
>         spin_lock(&cfids->cfid_list_lock);
>         list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
> diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h
> index e536304ca2ce..4ab9b10cd098 100644
> --- a/fs/cifs/cached_dir.h
> +++ b/fs/cifs/cached_dir.h
> @@ -57,6 +57,7 @@ struct cached_fids {
>         spinlock_t cfid_list_lock;
>         int num_entries;
>         struct list_head entries;
> +       struct task_struct *laundromat;
>  };
>
>  extern struct cached_fids *init_cached_dirs(void);
> --
> 2.35.3
>


-- 
Thanks,

Steve