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