Skip to content

Commit

Permalink
smb: client: stop revalidating reparse points unnecessarily
Browse files Browse the repository at this point in the history 
Query dir responses don't provide enough information on reparse points
such as major/minor numbers and symlink targets other than reparse
tags, however we don't need to unconditionally revalidate them only
because they are reparse points.  Instead, revalidate them only when
their ctime or reparse tag has changed.

For instance, Windows Server updates ctime of reparse points when
their data have changed.

Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
  • Loading branch information
Paulo Alcantara authored and Steve French committed Jan 7, 2024
1 parent 6ebfede commit 6d03998
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 81 deletions.
1 change: 1 addition & 0 deletions fs/smb/client/cifsglob.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1565,6 +1565,7 @@ struct cifsInodeInfo {
spinlock_t deferred_lock; /* protection on deferred list */
bool lease_granted; /* Flag to indicate whether lease or oplock is granted. */
char *symlink_target;
__u32 reparse_tag;
};

static inline struct cifsInodeInfo *
Expand Down
4 changes: 3 additions & 1 deletion fs/smb/client/inode.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
inode->i_mode = fattr->cf_mode;

cifs_i->cifsAttrs = fattr->cf_cifsattrs;
cifs_i->reparse_tag = fattr->cf_cifstag;

if (fattr->cf_flags & CIFS_FATTR_NEED_REVAL)
cifs_i->time = 0;
Expand Down Expand Up @@ -209,7 +210,7 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
inode->i_blocks = (512 - 1 + fattr->cf_bytes) >> 9;
}

if (S_ISLNK(fattr->cf_mode)) {
if (S_ISLNK(fattr->cf_mode) && fattr->cf_symlink_target) {
kfree(cifs_i->symlink_target);
cifs_i->symlink_target = fattr->cf_symlink_target;
fattr->cf_symlink_target = NULL;
Expand Down Expand Up @@ -1120,6 +1121,7 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
else
cifs_open_info_to_fattr(fattr, data, sb);
out:
fattr->cf_cifstag = data->reparse.tag;
free_rsp_buf(rsp_buftype, rsp_iov.iov_base);
return rc;
}
Expand Down
133 changes: 53 additions & 80 deletions fs/smb/client/readdir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,23 @@ static inline void dump_cifs_file_struct(struct file *file, char *label)
}
#endif /* DEBUG2 */

/*
* Match a reparse point inode if reparse tag and ctime haven't changed.
*
* Windows Server updates ctime of reparse points when their data have changed.
* The server doesn't allow changing reparse tags from existing reparse points,
* though it's worth checking.
*/
static inline bool reparse_inode_match(struct inode *inode,
struct cifs_fattr *fattr)
{
struct timespec64 ctime = inode_get_ctime(inode);

return (CIFS_I(inode)->cifsAttrs & ATTR_REPARSE) &&
CIFS_I(inode)->reparse_tag == fattr->cf_cifstag &&
timespec64_equal(&ctime, &fattr->cf_ctime);
}

/*
* Attempt to preload the dcache with the results from the FIND_FIRST/NEXT
*
Expand All @@ -71,6 +88,7 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
struct super_block *sb = parent->d_sb;
struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
int rc;

cifs_dbg(FYI, "%s: for %s\n", __func__, name->name);

Expand All @@ -82,9 +100,11 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
* We'll end up doing an on the wire call either way and
* this spares us an invalidation.
*/
if (fattr->cf_flags & CIFS_FATTR_NEED_REVAL)
return;
retry:
if ((fattr->cf_cifsattrs & ATTR_REPARSE) ||
(fattr->cf_flags & CIFS_FATTR_NEED_REVAL))
return;

dentry = d_alloc_parallel(parent, name, &wq);
}
if (IS_ERR(dentry))
Expand All @@ -104,12 +124,34 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM))
fattr->cf_uniqueid = CIFS_I(inode)->uniqueid;

/* update inode in place
* if both i_ino and i_mode didn't change */
if (CIFS_I(inode)->uniqueid == fattr->cf_uniqueid &&
cifs_fattr_to_inode(inode, fattr) == 0) {
dput(dentry);
return;
/*
* Update inode in place if both i_ino and i_mode didn't
* change.
*/
if (CIFS_I(inode)->uniqueid == fattr->cf_uniqueid) {
/*
* Query dir responses don't provide enough
* information about reparse points other than
* their reparse tags. Save an invalidation by
* not clobbering the existing mode, size and
* symlink target (if any) when reparse tag and
* ctime haven't changed.
*/
rc = 0;
if (fattr->cf_cifsattrs & ATTR_REPARSE) {
if (likely(reparse_inode_match(inode, fattr))) {
fattr->cf_mode = inode->i_mode;
fattr->cf_eof = CIFS_I(inode)->server_eof;
fattr->cf_symlink_target = NULL;
} else {
CIFS_I(inode)->time = 0;
rc = -ESTALE;
}
}
if (!rc && !cifs_fattr_to_inode(inode, fattr)) {
dput(dentry);
return;
}
}
}
d_invalidate(dentry);
Expand All @@ -127,29 +169,6 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
dput(dentry);
}

static bool reparse_file_needs_reval(const struct cifs_fattr *fattr)
{
if (!(fattr->cf_cifsattrs & ATTR_REPARSE))
return false;
/*
* The DFS tags should be only intepreted by server side as per
* MS-FSCC 2.1.2.1, but let's include them anyway.
*
* Besides, if cf_cifstag is unset (0), then we still need it to be
* revalidated to know exactly what reparse point it is.
*/
switch (fattr->cf_cifstag) {
case IO_REPARSE_TAG_DFS:
case IO_REPARSE_TAG_DFSR:
case IO_REPARSE_TAG_SYMLINK:
case IO_REPARSE_TAG_NFS:
case IO_REPARSE_TAG_MOUNT_POINT:
case 0:
return true;
}
return false;
}

static void
cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
{
Expand Down Expand Up @@ -181,14 +200,6 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb)
}

out_reparse:
/*
* We need to revalidate it further to make a decision about whether it
* is a symbolic link, DFS referral or a reparse point with a direct
* access like junctions, deduplicated files, NFS symlinks.
*/
if (reparse_file_needs_reval(fattr))
fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;

/* non-unix readdir doesn't provide nlink */
fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK;

Expand Down Expand Up @@ -269,9 +280,6 @@ cifs_posix_to_fattr(struct cifs_fattr *fattr, struct smb2_posix_info *info,
fattr->cf_dtype = DT_REG;
}

if (reparse_file_needs_reval(fattr))
fattr->cf_flags |= CIFS_FATTR_NEED_REVAL;

sid_to_id(cifs_sb, &parsed.owner, fattr, SIDOWNER);
sid_to_id(cifs_sb, &parsed.group, fattr, SIDGROUP);
}
Expand Down Expand Up @@ -331,38 +339,6 @@ cifs_std_info_to_fattr(struct cifs_fattr *fattr, FIND_FILE_STANDARD_INFO *info,
cifs_fill_common_info(fattr, cifs_sb);
}

/* BB eventually need to add the following helper function to
resolve NT_STATUS_STOPPED_ON_SYMLINK return code when
we try to do FindFirst on (NTFS) directory symlinks */
/*
int get_symlink_reparse_path(char *full_path, struct cifs_sb_info *cifs_sb,
unsigned int xid)
{
__u16 fid;
int len;
int oplock = 0;
int rc;
struct cifs_tcon *ptcon = cifs_sb_tcon(cifs_sb);
char *tmpbuffer;
rc = CIFSSMBOpen(xid, ptcon, full_path, FILE_OPEN, GENERIC_READ,
OPEN_REPARSE_POINT, &fid, &oplock, NULL,
cifs_sb->local_nls,
cifs_remap(cifs_sb);
if (!rc) {
tmpbuffer = kmalloc(maxpath);
rc = CIFSSMBQueryReparseLinkInfo(xid, ptcon, full_path,
tmpbuffer,
maxpath -1,
fid,
cifs_sb->local_nls);
if (CIFSSMBClose(xid, ptcon, fid)) {
cifs_dbg(FYI, "Error closing temporary reparsepoint open\n");
}
}
}
*/

static int
_initiate_cifs_search(const unsigned int xid, struct file *file,
const char *full_path)
Expand Down Expand Up @@ -431,13 +407,10 @@ _initiate_cifs_search(const unsigned int xid, struct file *file,
&cifsFile->fid, search_flags,
&cifsFile->srch_inf);

if (rc == 0)
if (rc == 0) {
cifsFile->invalidHandle = false;
/* BB add following call to handle readdir on new NTFS symlink errors
else if STATUS_STOPPED_ON_SYMLINK
call get_symlink_reparse_path and retry with new path */
else if ((rc == -EOPNOTSUPP) &&
(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)) {
} else if ((rc == -EOPNOTSUPP) &&
(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)) {
cifs_sb->mnt_cifs_flags &= ~CIFS_MOUNT_SERVER_INUM;
goto ffirst_retry;
}
Expand Down

0 comments on commit 6d03998

Please sign in to comment.