Message ID | 20220809021156.3086869-6-lsahlber@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/9] cifs: Move cached-dir functions into a separate file | expand |
Ronnie Sahlberg <lsahlber@redhat.com> writes: > cfids will soon keep a list of cached fids so we should not access this > directly from outside of cached_dir.c > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/cached_dir.c | 10 ++++++---- > fs/cifs/cached_dir.h | 2 +- > fs/cifs/readdir.c | 4 ++-- > fs/cifs/smb2inode.c | 2 +- > fs/cifs/smb2ops.c | 18 ++++++++++++++---- > 5 files changed, 24 insertions(+), 12 deletions(-) Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Tentatively merged this patch (after rebasing it and fixing a minor checkpatch problem). Added Paulo's RB Skipped adding patch 4 though (cifs: Make tcon contain a wrapper structure cached_fids instead of cached_fid) - let me know if problem skipping that patch in the short term On Thu, Aug 11, 2022 at 8:20 AM Paulo Alcantara <pc@cjr.nz> wrote: > > Ronnie Sahlberg <lsahlber@redhat.com> writes: > > > cfids will soon keep a list of cached fids so we should not access this > > directly from outside of cached_dir.c > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/cached_dir.c | 10 ++++++---- > > fs/cifs/cached_dir.h | 2 +- > > fs/cifs/readdir.c | 4 ++-- > > fs/cifs/smb2inode.c | 2 +- > > fs/cifs/smb2ops.c | 18 ++++++++++++++---- > > 5 files changed, 24 insertions(+), 12 deletions(-) > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
updated patch attached On Thu, Aug 11, 2022 at 7:56 PM Steve French <smfrench@gmail.com> wrote: > > Tentatively merged this patch (after rebasing it and fixing a minor > checkpatch problem). Added Paulo's RB > > Skipped adding patch 4 though (cifs: Make tcon contain a wrapper > structure cached_fids instead of cached_fid) - let me know if problem > skipping that patch in the short term > > On Thu, Aug 11, 2022 at 8:20 AM Paulo Alcantara <pc@cjr.nz> wrote: > > > > Ronnie Sahlberg <lsahlber@redhat.com> writes: > > > > > cfids will soon keep a list of cached fids so we should not access this > > > directly from outside of cached_dir.c > > > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > --- > > > fs/cifs/cached_dir.c | 10 ++++++---- > > > fs/cifs/cached_dir.h | 2 +- > > > fs/cifs/readdir.c | 4 ++-- > > > fs/cifs/smb2inode.c | 2 +- > > > fs/cifs/smb2ops.c | 18 ++++++++++++++---- > > > 5 files changed, 24 insertions(+), 12 deletions(-) > > > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > > > > -- > Thanks, > > Steve
forgot to mention the obvious - the way to tell that this would be helpful is simply looking at "/proc/fs/cifs/Stats" to see number of opens vs. other operations. Can also see pretty easily from simple trace commands ("trace-cmd record -e smb3_open* smb3_close*") etc On Thu, Aug 11, 2022 at 7:57 PM Steve French <smfrench@gmail.com> wrote: > > updated patch attached > > > On Thu, Aug 11, 2022 at 7:56 PM Steve French <smfrench@gmail.com> wrote: > > > > Tentatively merged this patch (after rebasing it and fixing a minor > > checkpatch problem). Added Paulo's RB > > > > Skipped adding patch 4 though (cifs: Make tcon contain a wrapper > > structure cached_fids instead of cached_fid) - let me know if problem > > skipping that patch in the short term > > > > On Thu, Aug 11, 2022 at 8:20 AM Paulo Alcantara <pc@cjr.nz> wrote: > > > > > > Ronnie Sahlberg <lsahlber@redhat.com> writes: > > > > > > > cfids will soon keep a list of cached fids so we should not access this > > > > directly from outside of cached_dir.c > > > > > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > > --- > > > > fs/cifs/cached_dir.c | 10 ++++++---- > > > > fs/cifs/cached_dir.h | 2 +- > > > > fs/cifs/readdir.c | 4 ++-- > > > > fs/cifs/smb2inode.c | 2 +- > > > > fs/cifs/smb2ops.c | 18 ++++++++++++++---- > > > > 5 files changed, 24 insertions(+), 12 deletions(-) > > > > > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > > > > > > > > -- > > Thanks, > > > > Steve > > > > -- > Thanks, > > Steve
And one more obvious thing ... this also fixes the problem where safe caching gets unintionally turned off (deferred close, handle leases) by users who disable actimeo (which is unsafe metadata caching). So setting actimeo=0 which some workloads do to make sure no stale mtime/filesize would end up unintentionally hurting safe handle caching (deferred close). This timer should be controlled separately than actimeo On Thu, Aug 11, 2022 at 8:26 PM Steve French <smfrench@gmail.com> wrote: > > forgot to mention the obvious - the way to tell that this would be > helpful is simply looking at "/proc/fs/cifs/Stats" to see number of > opens vs. other operations. Can also see pretty easily from simple > trace commands ("trace-cmd record -e smb3_open* smb3_close*") etc > > On Thu, Aug 11, 2022 at 7:57 PM Steve French <smfrench@gmail.com> wrote: > > > > updated patch attached > > > > > > On Thu, Aug 11, 2022 at 7:56 PM Steve French <smfrench@gmail.com> wrote: > > > > > > Tentatively merged this patch (after rebasing it and fixing a minor > > > checkpatch problem). Added Paulo's RB > > > > > > Skipped adding patch 4 though (cifs: Make tcon contain a wrapper > > > structure cached_fids instead of cached_fid) - let me know if problem > > > skipping that patch in the short term > > > > > > On Thu, Aug 11, 2022 at 8:20 AM Paulo Alcantara <pc@cjr.nz> wrote: > > > > > > > > Ronnie Sahlberg <lsahlber@redhat.com> writes: > > > > > > > > > cfids will soon keep a list of cached fids so we should not access this > > > > > directly from outside of cached_dir.c > > > > > > > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > > > --- > > > > > fs/cifs/cached_dir.c | 10 ++++++---- > > > > > fs/cifs/cached_dir.h | 2 +- > > > > > fs/cifs/readdir.c | 4 ++-- > > > > > fs/cifs/smb2inode.c | 2 +- > > > > > fs/cifs/smb2ops.c | 18 ++++++++++++++---- > > > > > 5 files changed, 24 insertions(+), 12 deletions(-) > > > > > > > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > > > > > > > > > > > > -- > > > Thanks, > > > > > > Steve > > > > > > > > -- > > Thanks, > > > > Steve > > > > -- > Thanks, > > Steve
diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c index 604ac444ad25..1fb80b23bbeb 100644 --- a/fs/cifs/cached_dir.c +++ b/fs/cifs/cached_dir.c @@ -16,9 +16,9 @@ * If error then *cfid is not initialized. */ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, - const char *path, - struct cifs_sb_info *cifs_sb, - struct cached_fid **ret_cfid) + const char *path, + struct cifs_sb_info *cifs_sb, + bool lookup_only, struct cached_fid **ret_cfid) { struct cifs_ses *ses; struct TCP_Server_Info *server; @@ -68,9 +68,11 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, * cifs_mark_open_files_invalid() which takes the lock again * thus causing a deadlock */ - mutex_unlock(&cfid->fid_mutex); + if (lookup_only) + return -ENOENT; + if (smb3_encryption_required(tcon)) flags |= CIFS_TRANSFORM_REQ; diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h index 4ea30a5ba4e7..5a384fad2432 100644 --- a/fs/cifs/cached_dir.h +++ b/fs/cifs/cached_dir.h @@ -54,7 +54,7 @@ extern void free_cached_dirs(struct cached_fids *cfids); extern int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, const char *path, struct cifs_sb_info *cifs_sb, - struct cached_fid **cfid); + bool lookup_only, struct cached_fid **cfid); extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon, struct dentry *dentry, struct cached_fid **cfid); diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c index a06072ae6c7e..2eece8a07c11 100644 --- a/fs/cifs/readdir.c +++ b/fs/cifs/readdir.c @@ -1072,7 +1072,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx) tcon = tlink_tcon(cifsFile->tlink); } - rc = open_cached_dir(xid, tcon, full_path, cifs_sb, &cfid); + rc = open_cached_dir(xid, tcon, full_path, cifs_sb, false, &cfid); cifs_put_tlink(tlink); if (rc) goto cache_not_found; @@ -1143,7 +1143,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx) tcon = tlink_tcon(cifsFile->tlink); rc = find_cifs_entry(xid, tcon, ctx->pos, file, full_path, ¤t_entry, &num_to_fill); - open_cached_dir(xid, tcon, full_path, cifs_sb, &cfid); + open_cached_dir(xid, tcon, full_path, cifs_sb, false, &cfid); if (rc) { cifs_dbg(FYI, "fce error %d\n", rc); goto rddir2_exit; diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c index 9696184a09e3..b83f59051b26 100644 --- a/fs/cifs/smb2inode.c +++ b/fs/cifs/smb2inode.c @@ -516,7 +516,7 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, if (strcmp(full_path, "")) rc = -ENOENT; else - rc = open_cached_dir(xid, tcon, full_path, cifs_sb, &cfid); + rc = open_cached_dir(xid, tcon, full_path, cifs_sb, false, &cfid); /* If it is a root and its handle is cached then use it */ if (!rc) { if (cfid->file_all_info_is_valid) { diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 780ada4d8f6e..4727ee537f11 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -720,7 +720,7 @@ smb3_qfs_tcon(const unsigned int xid, struct cifs_tcon *tcon, oparms.fid = &fid; oparms.reconnect = false; - rc = open_cached_dir(xid, tcon, "", cifs_sb, &cfid); + rc = open_cached_dir(xid, tcon, "", cifs_sb, false, &cfid); if (rc == 0) memcpy(&fid, &cfid->fid, sizeof(struct cifs_fid)); else @@ -783,9 +783,16 @@ smb2_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, __u8 oplock = SMB2_OPLOCK_LEVEL_NONE; struct cifs_open_parms oparms; struct cifs_fid fid; + struct cached_fid *cfid; - if ((*full_path == 0) && tcon->cfids->cfid.is_valid) - return 0; + rc = open_cached_dir(xid, tcon, full_path, cifs_sb, true, &cfid); + if (!rc) { + if (cfid->is_valid) { + close_cached_dir(cfid); + return 0; + } + close_cached_dir(cfid); + } utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb); if (!utf16_path) @@ -2431,8 +2438,11 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon, resp_buftype[0] = resp_buftype[1] = resp_buftype[2] = CIFS_NO_BUFFER; memset(rsp_iov, 0, sizeof(rsp_iov)); + /* + * We can only call this for things we know are directories. + */ if (!strcmp(path, "")) - open_cached_dir(xid, tcon, path, cifs_sb, &cfid); /* cfid null if open dir failed */ + open_cached_dir(xid, tcon, path, cifs_sb, false, &cfid); /* cfid null if open dir failed */ memset(&open_iov, 0, sizeof(open_iov)); rqst[0].rq_iov = open_iov;
cfids will soon keep a list of cached fids so we should not access this directly from outside of cached_dir.c Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/cached_dir.c | 10 ++++++---- fs/cifs/cached_dir.h | 2 +- fs/cifs/readdir.c | 4 ++-- fs/cifs/smb2inode.c | 2 +- fs/cifs/smb2ops.c | 18 ++++++++++++++---- 5 files changed, 24 insertions(+), 12 deletions(-)