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

[Tom Talpey <tom@talpey.com>]

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;