On Wed, 24 Aug 2022 at 23:51, Tom Talpey <tom@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> > */
> >
> > +#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;
