| Message ID | 20220824002756.3659568-6-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] cifs: Make tcon contain a wrapper structure cached_fids instead of cached_fid | expand |
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;
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;
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;
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(-)