| Message ID | 1383314259-4694-1-git-send-email-piastryyy@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 1, 2013 at 8:57 AM, Pavel Shilovsky <piastryyy@gmail.com> wrote: > From: Pavel Shilovsky <piastry@etersoft.ru> > > Now we treat any reparse point as a symbolic link and map it to a Unix > one that is not true in a common case due to many reparse point types > supported by SMB servers. > > Distinguish reparse point types into two groups: > 1) that can be accessed directly through a reparse point > (junctions, deduplicated files, NFS symlinks); > 2) that need to be processed manually (Windows symbolic links, DFS); > > and map only Windows symbolic links to Unix ones. > > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > --- > fs/cifs/cifsglob.h | 2 +- > fs/cifs/inode.c | 23 +++++++++++++---------- > fs/cifs/readdir.c | 40 ++++++++-------------------------------- > fs/cifs/smb1ops.c | 21 ++++++++++++++++++++- > fs/cifs/smb2inode.c | 16 ++++++++++++---- > fs/cifs/smb2proto.h | 2 +- > 6 files changed, 55 insertions(+), 49 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index a67cf12..b688f2b 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -261,7 +261,7 @@ struct smb_version_operations { > /* query path data from the server */ > int (*query_path_info)(const unsigned int, struct cifs_tcon *, > struct cifs_sb_info *, const char *, > - FILE_ALL_INFO *, bool *); > + FILE_ALL_INFO *, bool *, bool *); > /* query file data from the server */ > int (*query_file_info)(const unsigned int, struct cifs_tcon *, > struct cifs_fid *, FILE_ALL_INFO *); > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index 867b7cd..36f9ebb 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path, > /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */ > static void > cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > - struct cifs_sb_info *cifs_sb, bool adjust_tz) > + struct cifs_sb_info *cifs_sb, bool adjust_tz, > + bool symlink) > { > struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > > @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > fattr->cf_createtime = le64_to_cpu(info->CreationTime); > > fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > + > + if (symlink) { > + fattr->cf_mode = S_IFLNK; > + fattr->cf_dtype = DT_LNK; > + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > fattr->cf_dtype = DT_DIR; > /* > @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > */ > if (!tcon->unix_ext) > fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; > - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { > - fattr->cf_mode = S_IFLNK; > - fattr->cf_dtype = DT_LNK; > - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > } else { > fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > fattr->cf_dtype = DT_REG; Pavel, does this handle a case where it could be symbolic link to a directory? > @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp) > rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data); > switch (rc) { > case 0: > - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false); > + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false, > + false); > break; > case -EREMOTE: > cifs_create_dfs_fattr(&fattr, inode->i_sb); > @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > bool adjust_tz = false; > struct cifs_fattr fattr; > struct cifs_search_info *srchinf = NULL; > + bool symlink = false; > > tlink = cifs_sb_tlink(cifs_sb); > if (IS_ERR(tlink)) > @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > } > data = (FILE_ALL_INFO *)buf; > rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path, > - data, &adjust_tz); > + data, &adjust_tz, &symlink); > } > > if (!rc) { > - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb, > - adjust_tz); > + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz, > + symlink); > } else if (rc == -EREMOTE) { > cifs_create_dfs_fattr(&fattr, sb); > rc = 0; > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > index 53a75f3..5940eca 100644 > --- a/fs/cifs/readdir.c > +++ b/fs/cifs/readdir.c > @@ -134,22 +134,6 @@ out: > dput(dentry); > } > > -/* > - * Is it possible that this directory might turn out to be a DFS referral > - * once we go to try and use it? > - */ > -static bool > -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) > -{ > -#ifdef CONFIG_CIFS_DFS_UPCALL > - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > - > - if (tcon->Flags & SMB_SHARE_IS_IN_DFS) > - return true; > -#endif > - return false; > -} > - > static void > cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > { > @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > fattr->cf_dtype = DT_DIR; > - /* > - * Windows CIFS servers generally make DFS referrals look > - * like directories in FIND_* responses with the reparse > - * attribute flag also set (since DFS junctions are > - * reparse points). We must revalidate at least these > - * directory inodes before trying to use them (if > - * they are DFS we will get PATH_NOT_COVERED back > - * when queried directly and can then try to connect > - * to the DFS target) > - */ > - if (cifs_dfs_is_possible(cifs_sb) && > - (fattr->cf_cifsattrs & ATTR_REPARSE)) > - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { > - fattr->cf_mode = S_IFLNK; > - fattr->cf_dtype = DT_LNK; > } else { > fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > fattr->cf_dtype = DT_REG; > } > > + /* > + * 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 (fattr->cf_cifsattrs & ATTR_REPARSE) > + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > + > /* non-unix readdir doesn't provide nlink */ > fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; > > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > index ea99efe..47ab035 100644 > --- a/fs/cifs/smb1ops.c > +++ b/fs/cifs/smb1ops.c > @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, > static int > cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > struct cifs_sb_info *cifs_sb, const char *full_path, > - FILE_ALL_INFO *data, bool *adjustTZ) > + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink) > { > int rc; > > + *symlink = false; > + > /* could do find first instead but this returns more info */ > rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */, > cifs_sb->local_nls, cifs_sb->mnt_cifs_flags & > @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > CIFS_MOUNT_MAP_SPECIAL_CHR); > *adjustTZ = true; > } > + > + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) { > + int tmprc; > + int oplock = 0; > + __u16 netfid; > + > + /* Need to check if this is a symbolic link or not */ > + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN, > + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock, > + NULL, cifs_sb->local_nls, > + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); > + if (tmprc == -EOPNOTSUPP) > + *symlink = true; > + else > + CIFSSMBClose(xid, tcon, netfid); > + } > + > return rc; > } > > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > index 78ff88c..84c012a 100644 > --- a/fs/cifs/smb2inode.c > +++ b/fs/cifs/smb2inode.c > @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src) > int > smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > struct cifs_sb_info *cifs_sb, const char *full_path, > - FILE_ALL_INFO *data, bool *adjust_tz) > + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink) > { > int rc; > struct smb2_file_all_info *smb2_data; > > *adjust_tz = false; > + *symlink = false; > > smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2, > GFP_KERNEL); > @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > return -ENOMEM; > > rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, > - FILE_READ_ATTRIBUTES, FILE_OPEN, > - OPEN_REPARSE_POINT, smb2_data, > - SMB2_OP_QUERY_INFO); > + FILE_READ_ATTRIBUTES, FILE_OPEN, 0, > + smb2_data, SMB2_OP_QUERY_INFO); > + if (rc == -EOPNOTSUPP) { > + *symlink = true; > + /* Failed on a symbolic link - query a reparse point info */ > + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, > + FILE_READ_ATTRIBUTES, FILE_OPEN, > + OPEN_REPARSE_POINT, smb2_data, > + SMB2_OP_QUERY_INFO); > + } > if (rc) > goto out; > > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > index 313813e..b4eea10 100644 > --- a/fs/cifs/smb2proto.h > +++ b/fs/cifs/smb2proto.h > @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst, > extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > struct cifs_sb_info *cifs_sb, > const char *full_path, FILE_ALL_INFO *data, > - bool *adjust_tz); > + bool *adjust_tz, bool *symlink); > extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon, > const char *full_path, __u64 size, > struct cifs_sb_info *cifs_sb, bool set_alloc); > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 1 Nov 2013 17:57:39 +0400 Pavel Shilovsky <piastryyy@gmail.com> wrote: > From: Pavel Shilovsky <piastry@etersoft.ru> > > Now we treat any reparse point as a symbolic link and map it to a Unix > one that is not true in a common case due to many reparse point types > supported by SMB servers. > > Distinguish reparse point types into two groups: > 1) that can be accessed directly through a reparse point > (junctions, deduplicated files, NFS symlinks); > 2) that need to be processed manually (Windows symbolic links, DFS); > > and map only Windows symbolic links to Unix ones. > > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > --- > fs/cifs/cifsglob.h | 2 +- > fs/cifs/inode.c | 23 +++++++++++++---------- > fs/cifs/readdir.c | 40 ++++++++-------------------------------- > fs/cifs/smb1ops.c | 21 ++++++++++++++++++++- > fs/cifs/smb2inode.c | 16 ++++++++++++---- > fs/cifs/smb2proto.h | 2 +- > 6 files changed, 55 insertions(+), 49 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index a67cf12..b688f2b 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -261,7 +261,7 @@ struct smb_version_operations { > /* query path data from the server */ > int (*query_path_info)(const unsigned int, struct cifs_tcon *, > struct cifs_sb_info *, const char *, > - FILE_ALL_INFO *, bool *); > + FILE_ALL_INFO *, bool *, bool *); This looks much better for performance, but I think it really shines a light on what an abortion the query_path_info and query_file_info operations are. They work with a FILE_ALL_INFO struct which is SMB1 specific, so for SMB2/3 we have to convert data to something that doesn't even match the protocol. I think it would be best to base this fix on top of a patchset to fix that properly. Those functions should be changed to pass data around in a cifs_fattr. With that, you could just add a new cifs_fattr->flags value and you wouldn't need to add this extra bool * argument. > /* query file data from the server */ > int (*query_file_info)(const unsigned int, struct cifs_tcon *, > struct cifs_fid *, FILE_ALL_INFO *); > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index 867b7cd..36f9ebb 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path, > /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */ > static void > cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > - struct cifs_sb_info *cifs_sb, bool adjust_tz) > + struct cifs_sb_info *cifs_sb, bool adjust_tz, > + bool symlink) > { > struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > > @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > fattr->cf_createtime = le64_to_cpu(info->CreationTime); > > fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > + > + if (symlink) { > + fattr->cf_mode = S_IFLNK; > + fattr->cf_dtype = DT_LNK; > + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > fattr->cf_dtype = DT_DIR; > /* > @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > */ > if (!tcon->unix_ext) > fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; > - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { > - fattr->cf_mode = S_IFLNK; > - fattr->cf_dtype = DT_LNK; > - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > } else { > fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > fattr->cf_dtype = DT_REG; > @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp) > rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data); > switch (rc) { > case 0: > - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false); > + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false, > + false); > break; > case -EREMOTE: > cifs_create_dfs_fattr(&fattr, inode->i_sb); > @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > bool adjust_tz = false; > struct cifs_fattr fattr; > struct cifs_search_info *srchinf = NULL; > + bool symlink = false; > > tlink = cifs_sb_tlink(cifs_sb); > if (IS_ERR(tlink)) > @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > } > data = (FILE_ALL_INFO *)buf; > rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path, > - data, &adjust_tz); > + data, &adjust_tz, &symlink); > } > > if (!rc) { > - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb, > - adjust_tz); > + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz, > + symlink); > } else if (rc == -EREMOTE) { > cifs_create_dfs_fattr(&fattr, sb); > rc = 0; > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > index 53a75f3..5940eca 100644 > --- a/fs/cifs/readdir.c > +++ b/fs/cifs/readdir.c > @@ -134,22 +134,6 @@ out: > dput(dentry); > } > > -/* > - * Is it possible that this directory might turn out to be a DFS referral > - * once we go to try and use it? > - */ > -static bool > -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) > -{ > -#ifdef CONFIG_CIFS_DFS_UPCALL > - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > - > - if (tcon->Flags & SMB_SHARE_IS_IN_DFS) > - return true; > -#endif > - return false; > -} > - > static void > cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > { > @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > fattr->cf_dtype = DT_DIR; > - /* > - * Windows CIFS servers generally make DFS referrals look > - * like directories in FIND_* responses with the reparse > - * attribute flag also set (since DFS junctions are > - * reparse points). We must revalidate at least these > - * directory inodes before trying to use them (if > - * they are DFS we will get PATH_NOT_COVERED back > - * when queried directly and can then try to connect > - * to the DFS target) > - */ > - if (cifs_dfs_is_possible(cifs_sb) && > - (fattr->cf_cifsattrs & ATTR_REPARSE)) > - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { > - fattr->cf_mode = S_IFLNK; > - fattr->cf_dtype = DT_LNK; > } else { > fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > fattr->cf_dtype = DT_REG; > } > > + /* > + * 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 (fattr->cf_cifsattrs & ATTR_REPARSE) > + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > + > /* non-unix readdir doesn't provide nlink */ > fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; > > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > index ea99efe..47ab035 100644 > --- a/fs/cifs/smb1ops.c > +++ b/fs/cifs/smb1ops.c > @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, > static int > cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > struct cifs_sb_info *cifs_sb, const char *full_path, > - FILE_ALL_INFO *data, bool *adjustTZ) > + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink) > { > int rc; > > + *symlink = false; > + > /* could do find first instead but this returns more info */ > rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */, > cifs_sb->local_nls, cifs_sb->mnt_cifs_flags & > @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > CIFS_MOUNT_MAP_SPECIAL_CHR); > *adjustTZ = true; > } > + > + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) { > + int tmprc; > + int oplock = 0; > + __u16 netfid; > + > + /* Need to check if this is a symbolic link or not */ > + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN, > + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock, > + NULL, cifs_sb->local_nls, > + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); > + if (tmprc == -EOPNOTSUPP) > + *symlink = true; > + else > + CIFSSMBClose(xid, tcon, netfid); > + } > + > return rc; > } > > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > index 78ff88c..84c012a 100644 > --- a/fs/cifs/smb2inode.c > +++ b/fs/cifs/smb2inode.c > @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src) > int > smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > struct cifs_sb_info *cifs_sb, const char *full_path, > - FILE_ALL_INFO *data, bool *adjust_tz) > + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink) > { > int rc; > struct smb2_file_all_info *smb2_data; > > *adjust_tz = false; > + *symlink = false; > > smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2, > GFP_KERNEL); > @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > return -ENOMEM; > > rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, > - FILE_READ_ATTRIBUTES, FILE_OPEN, > - OPEN_REPARSE_POINT, smb2_data, > - SMB2_OP_QUERY_INFO); > + FILE_READ_ATTRIBUTES, FILE_OPEN, 0, > + smb2_data, SMB2_OP_QUERY_INFO); > + if (rc == -EOPNOTSUPP) { > + *symlink = true; > + /* Failed on a symbolic link - query a reparse point info */ > + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, > + FILE_READ_ATTRIBUTES, FILE_OPEN, > + OPEN_REPARSE_POINT, smb2_data, > + SMB2_OP_QUERY_INFO); > + } > if (rc) > goto out; > > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > index 313813e..b4eea10 100644 > --- a/fs/cifs/smb2proto.h > +++ b/fs/cifs/smb2proto.h > @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst, > extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > struct cifs_sb_info *cifs_sb, > const char *full_path, FILE_ALL_INFO *data, > - bool *adjust_tz); > + bool *adjust_tz, bool *symlink); > extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon, > const char *full_path, __u64 size, > struct cifs_sb_info *cifs_sb, bool set_alloc);
FILE_ALL_INFO is not SMB1 specific, we request it in SMB2/SMB3 as well and it is still a common level. See section 2.2.37 of MS-SMB2 or FSCC description at: http://msdn.microsoft.com/en-us/library/cc232117.aspx Is there a different level that ou would like to call? On Fri, Nov 1, 2013 at 12:42 PM, Jeff Layton <jlayton@redhat.com> wrote: > On Fri, 1 Nov 2013 17:57:39 +0400 > Pavel Shilovsky <piastryyy@gmail.com> wrote: > >> From: Pavel Shilovsky <piastry@etersoft.ru> >> >> Now we treat any reparse point as a symbolic link and map it to a Unix >> one that is not true in a common case due to many reparse point types >> supported by SMB servers. >> >> Distinguish reparse point types into two groups: >> 1) that can be accessed directly through a reparse point >> (junctions, deduplicated files, NFS symlinks); >> 2) that need to be processed manually (Windows symbolic links, DFS); >> >> and map only Windows symbolic links to Unix ones. >> >> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> >> --- >> fs/cifs/cifsglob.h | 2 +- >> fs/cifs/inode.c | 23 +++++++++++++---------- >> fs/cifs/readdir.c | 40 ++++++++-------------------------------- >> fs/cifs/smb1ops.c | 21 ++++++++++++++++++++- >> fs/cifs/smb2inode.c | 16 ++++++++++++---- >> fs/cifs/smb2proto.h | 2 +- >> 6 files changed, 55 insertions(+), 49 deletions(-) >> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h >> index a67cf12..b688f2b 100644 >> --- a/fs/cifs/cifsglob.h >> +++ b/fs/cifs/cifsglob.h >> @@ -261,7 +261,7 @@ struct smb_version_operations { >> /* query path data from the server */ >> int (*query_path_info)(const unsigned int, struct cifs_tcon *, >> struct cifs_sb_info *, const char *, >> - FILE_ALL_INFO *, bool *); >> + FILE_ALL_INFO *, bool *, bool *); > > > This looks much better for performance, but I think it really shines a > light on what an abortion the query_path_info and query_file_info > operations are. They work with a FILE_ALL_INFO struct which is SMB1 > specific, so for SMB2/3 we have to convert data to something that > doesn't even match the protocol. > > I think it would be best to base this fix on top of a patchset to fix > that properly. Those functions should be changed to pass data around in > a cifs_fattr. With that, you could just add a new cifs_fattr->flags > value and you wouldn't need to add this extra bool * argument. > > >> /* query file data from the server */ >> int (*query_file_info)(const unsigned int, struct cifs_tcon *, >> struct cifs_fid *, FILE_ALL_INFO *); >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c >> index 867b7cd..36f9ebb 100644 >> --- a/fs/cifs/inode.c >> +++ b/fs/cifs/inode.c >> @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path, >> /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */ >> static void >> cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, >> - struct cifs_sb_info *cifs_sb, bool adjust_tz) >> + struct cifs_sb_info *cifs_sb, bool adjust_tz, >> + bool symlink) >> { >> struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); >> >> @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, >> fattr->cf_createtime = le64_to_cpu(info->CreationTime); >> >> fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); >> - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { >> + >> + if (symlink) { >> + fattr->cf_mode = S_IFLNK; >> + fattr->cf_dtype = DT_LNK; >> + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { >> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; >> fattr->cf_dtype = DT_DIR; >> /* >> @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, >> */ >> if (!tcon->unix_ext) >> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; >> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { >> - fattr->cf_mode = S_IFLNK; >> - fattr->cf_dtype = DT_LNK; >> - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); >> } else { >> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; >> fattr->cf_dtype = DT_REG; >> @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp) >> rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data); >> switch (rc) { >> case 0: >> - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false); >> + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false, >> + false); >> break; >> case -EREMOTE: >> cifs_create_dfs_fattr(&fattr, inode->i_sb); >> @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, >> bool adjust_tz = false; >> struct cifs_fattr fattr; >> struct cifs_search_info *srchinf = NULL; >> + bool symlink = false; >> >> tlink = cifs_sb_tlink(cifs_sb); >> if (IS_ERR(tlink)) >> @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, >> } >> data = (FILE_ALL_INFO *)buf; >> rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path, >> - data, &adjust_tz); >> + data, &adjust_tz, &symlink); >> } >> >> if (!rc) { >> - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb, >> - adjust_tz); >> + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz, >> + symlink); >> } else if (rc == -EREMOTE) { >> cifs_create_dfs_fattr(&fattr, sb); >> rc = 0; >> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c >> index 53a75f3..5940eca 100644 >> --- a/fs/cifs/readdir.c >> +++ b/fs/cifs/readdir.c >> @@ -134,22 +134,6 @@ out: >> dput(dentry); >> } >> >> -/* >> - * Is it possible that this directory might turn out to be a DFS referral >> - * once we go to try and use it? >> - */ >> -static bool >> -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) >> -{ >> -#ifdef CONFIG_CIFS_DFS_UPCALL >> - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); >> - >> - if (tcon->Flags & SMB_SHARE_IS_IN_DFS) >> - return true; >> -#endif >> - return false; >> -} >> - >> static void >> cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) >> { >> @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) >> if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { >> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; >> fattr->cf_dtype = DT_DIR; >> - /* >> - * Windows CIFS servers generally make DFS referrals look >> - * like directories in FIND_* responses with the reparse >> - * attribute flag also set (since DFS junctions are >> - * reparse points). We must revalidate at least these >> - * directory inodes before trying to use them (if >> - * they are DFS we will get PATH_NOT_COVERED back >> - * when queried directly and can then try to connect >> - * to the DFS target) >> - */ >> - if (cifs_dfs_is_possible(cifs_sb) && >> - (fattr->cf_cifsattrs & ATTR_REPARSE)) >> - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; >> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { >> - fattr->cf_mode = S_IFLNK; >> - fattr->cf_dtype = DT_LNK; >> } else { >> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; >> fattr->cf_dtype = DT_REG; >> } >> >> + /* >> + * 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 (fattr->cf_cifsattrs & ATTR_REPARSE) >> + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; >> + >> /* non-unix readdir doesn't provide nlink */ >> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; >> >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c >> index ea99efe..47ab035 100644 >> --- a/fs/cifs/smb1ops.c >> +++ b/fs/cifs/smb1ops.c >> @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, >> static int >> cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, >> struct cifs_sb_info *cifs_sb, const char *full_path, >> - FILE_ALL_INFO *data, bool *adjustTZ) >> + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink) >> { >> int rc; >> >> + *symlink = false; >> + >> /* could do find first instead but this returns more info */ >> rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */, >> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags & >> @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, >> CIFS_MOUNT_MAP_SPECIAL_CHR); >> *adjustTZ = true; >> } >> + >> + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) { >> + int tmprc; >> + int oplock = 0; >> + __u16 netfid; >> + >> + /* Need to check if this is a symbolic link or not */ >> + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN, >> + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock, >> + NULL, cifs_sb->local_nls, >> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); >> + if (tmprc == -EOPNOTSUPP) >> + *symlink = true; >> + else >> + CIFSSMBClose(xid, tcon, netfid); >> + } >> + >> return rc; >> } >> >> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c >> index 78ff88c..84c012a 100644 >> --- a/fs/cifs/smb2inode.c >> +++ b/fs/cifs/smb2inode.c >> @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src) >> int >> smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, >> struct cifs_sb_info *cifs_sb, const char *full_path, >> - FILE_ALL_INFO *data, bool *adjust_tz) >> + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink) >> { >> int rc; >> struct smb2_file_all_info *smb2_data; >> >> *adjust_tz = false; >> + *symlink = false; >> >> smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2, >> GFP_KERNEL); >> @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, >> return -ENOMEM; >> >> rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, >> - FILE_READ_ATTRIBUTES, FILE_OPEN, >> - OPEN_REPARSE_POINT, smb2_data, >> - SMB2_OP_QUERY_INFO); >> + FILE_READ_ATTRIBUTES, FILE_OPEN, 0, >> + smb2_data, SMB2_OP_QUERY_INFO); >> + if (rc == -EOPNOTSUPP) { >> + *symlink = true; >> + /* Failed on a symbolic link - query a reparse point info */ >> + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, >> + FILE_READ_ATTRIBUTES, FILE_OPEN, >> + OPEN_REPARSE_POINT, smb2_data, >> + SMB2_OP_QUERY_INFO); >> + } >> if (rc) >> goto out; >> >> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h >> index 313813e..b4eea10 100644 >> --- a/fs/cifs/smb2proto.h >> +++ b/fs/cifs/smb2proto.h >> @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst, >> extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, >> struct cifs_sb_info *cifs_sb, >> const char *full_path, FILE_ALL_INFO *data, >> - bool *adjust_tz); >> + bool *adjust_tz, bool *symlink); >> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon, >> const char *full_path, __u64 size, >> struct cifs_sb_info *cifs_sb, bool set_alloc); > > > -- > Jeff Layton <jlayton@redhat.com> > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 1 Nov 2013 12:49:47 -0500 Steve French <smfrench@gmail.com> wrote: > FILE_ALL_INFO is not SMB1 specific, we request it in SMB2/SMB3 as well > and it is still a common level. See section 2.2.37 of MS-SMB2 or FSCC > description at: > > > http://msdn.microsoft.com/en-us/library/cc232117.aspx > > Is there a different level that ou would like to call? > Ok, good point... Still...I think we ought to not base this API on on-the-wire formats. It ought to use the standard container for passing file attributes around (struct cifs_fattr). In most cases where we need to call these functions we don't actually have a FILE_ALL_INFO and have to construct one. Blech... > On Fri, Nov 1, 2013 at 12:42 PM, Jeff Layton <jlayton@redhat.com> wrote: > > On Fri, 1 Nov 2013 17:57:39 +0400 > > Pavel Shilovsky <piastryyy@gmail.com> wrote: > > > >> From: Pavel Shilovsky <piastry@etersoft.ru> > >> > >> Now we treat any reparse point as a symbolic link and map it to a Unix > >> one that is not true in a common case due to many reparse point types > >> supported by SMB servers. > >> > >> Distinguish reparse point types into two groups: > >> 1) that can be accessed directly through a reparse point > >> (junctions, deduplicated files, NFS symlinks); > >> 2) that need to be processed manually (Windows symbolic links, DFS); > >> > >> and map only Windows symbolic links to Unix ones. > >> > >> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > >> --- > >> fs/cifs/cifsglob.h | 2 +- > >> fs/cifs/inode.c | 23 +++++++++++++---------- > >> fs/cifs/readdir.c | 40 ++++++++-------------------------------- > >> fs/cifs/smb1ops.c | 21 ++++++++++++++++++++- > >> fs/cifs/smb2inode.c | 16 ++++++++++++---- > >> fs/cifs/smb2proto.h | 2 +- > >> 6 files changed, 55 insertions(+), 49 deletions(-) > >> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > >> index a67cf12..b688f2b 100644 > >> --- a/fs/cifs/cifsglob.h > >> +++ b/fs/cifs/cifsglob.h > >> @@ -261,7 +261,7 @@ struct smb_version_operations { > >> /* query path data from the server */ > >> int (*query_path_info)(const unsigned int, struct cifs_tcon *, > >> struct cifs_sb_info *, const char *, > >> - FILE_ALL_INFO *, bool *); > >> + FILE_ALL_INFO *, bool *, bool *); > > > > > > This looks much better for performance, but I think it really shines a > > light on what an abortion the query_path_info and query_file_info > > operations are. They work with a FILE_ALL_INFO struct which is SMB1 > > specific, so for SMB2/3 we have to convert data to something that > > doesn't even match the protocol. > > > > I think it would be best to base this fix on top of a patchset to fix > > that properly. Those functions should be changed to pass data around in > > a cifs_fattr. With that, you could just add a new cifs_fattr->flags > > value and you wouldn't need to add this extra bool * argument. > > > > > >> /* query file data from the server */ > >> int (*query_file_info)(const unsigned int, struct cifs_tcon *, > >> struct cifs_fid *, FILE_ALL_INFO *); > >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > >> index 867b7cd..36f9ebb 100644 > >> --- a/fs/cifs/inode.c > >> +++ b/fs/cifs/inode.c > >> @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path, > >> /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */ > >> static void > >> cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > >> - struct cifs_sb_info *cifs_sb, bool adjust_tz) > >> + struct cifs_sb_info *cifs_sb, bool adjust_tz, > >> + bool symlink) > >> { > >> struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > >> > >> @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > >> fattr->cf_createtime = le64_to_cpu(info->CreationTime); > >> > >> fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > >> - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > >> + > >> + if (symlink) { > >> + fattr->cf_mode = S_IFLNK; > >> + fattr->cf_dtype = DT_LNK; > >> + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > >> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > >> fattr->cf_dtype = DT_DIR; > >> /* > >> @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > >> */ > >> if (!tcon->unix_ext) > >> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; > >> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { > >> - fattr->cf_mode = S_IFLNK; > >> - fattr->cf_dtype = DT_LNK; > >> - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > >> } else { > >> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > >> fattr->cf_dtype = DT_REG; > >> @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp) > >> rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data); > >> switch (rc) { > >> case 0: > >> - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false); > >> + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false, > >> + false); > >> break; > >> case -EREMOTE: > >> cifs_create_dfs_fattr(&fattr, inode->i_sb); > >> @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > >> bool adjust_tz = false; > >> struct cifs_fattr fattr; > >> struct cifs_search_info *srchinf = NULL; > >> + bool symlink = false; > >> > >> tlink = cifs_sb_tlink(cifs_sb); > >> if (IS_ERR(tlink)) > >> @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > >> } > >> data = (FILE_ALL_INFO *)buf; > >> rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path, > >> - data, &adjust_tz); > >> + data, &adjust_tz, &symlink); > >> } > >> > >> if (!rc) { > >> - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb, > >> - adjust_tz); > >> + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz, > >> + symlink); > >> } else if (rc == -EREMOTE) { > >> cifs_create_dfs_fattr(&fattr, sb); > >> rc = 0; > >> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > >> index 53a75f3..5940eca 100644 > >> --- a/fs/cifs/readdir.c > >> +++ b/fs/cifs/readdir.c > >> @@ -134,22 +134,6 @@ out: > >> dput(dentry); > >> } > >> > >> -/* > >> - * Is it possible that this directory might turn out to be a DFS referral > >> - * once we go to try and use it? > >> - */ > >> -static bool > >> -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) > >> -{ > >> -#ifdef CONFIG_CIFS_DFS_UPCALL > >> - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > >> - > >> - if (tcon->Flags & SMB_SHARE_IS_IN_DFS) > >> - return true; > >> -#endif > >> - return false; > >> -} > >> - > >> static void > >> cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > >> { > >> @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > >> if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > >> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > >> fattr->cf_dtype = DT_DIR; > >> - /* > >> - * Windows CIFS servers generally make DFS referrals look > >> - * like directories in FIND_* responses with the reparse > >> - * attribute flag also set (since DFS junctions are > >> - * reparse points). We must revalidate at least these > >> - * directory inodes before trying to use them (if > >> - * they are DFS we will get PATH_NOT_COVERED back > >> - * when queried directly and can then try to connect > >> - * to the DFS target) > >> - */ > >> - if (cifs_dfs_is_possible(cifs_sb) && > >> - (fattr->cf_cifsattrs & ATTR_REPARSE)) > >> - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > >> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { > >> - fattr->cf_mode = S_IFLNK; > >> - fattr->cf_dtype = DT_LNK; > >> } else { > >> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > >> fattr->cf_dtype = DT_REG; > >> } > >> > >> + /* > >> + * 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 (fattr->cf_cifsattrs & ATTR_REPARSE) > >> + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > >> + > >> /* non-unix readdir doesn't provide nlink */ > >> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; > >> > >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > >> index ea99efe..47ab035 100644 > >> --- a/fs/cifs/smb1ops.c > >> +++ b/fs/cifs/smb1ops.c > >> @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, > >> static int > >> cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > >> struct cifs_sb_info *cifs_sb, const char *full_path, > >> - FILE_ALL_INFO *data, bool *adjustTZ) > >> + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink) > >> { > >> int rc; > >> > >> + *symlink = false; > >> + > >> /* could do find first instead but this returns more info */ > >> rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */, > >> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags & > >> @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > >> CIFS_MOUNT_MAP_SPECIAL_CHR); > >> *adjustTZ = true; > >> } > >> + > >> + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) { > >> + int tmprc; > >> + int oplock = 0; > >> + __u16 netfid; > >> + > >> + /* Need to check if this is a symbolic link or not */ > >> + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN, > >> + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock, > >> + NULL, cifs_sb->local_nls, > >> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); > >> + if (tmprc == -EOPNOTSUPP) > >> + *symlink = true; > >> + else > >> + CIFSSMBClose(xid, tcon, netfid); > >> + } > >> + > >> return rc; > >> } > >> > >> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > >> index 78ff88c..84c012a 100644 > >> --- a/fs/cifs/smb2inode.c > >> +++ b/fs/cifs/smb2inode.c > >> @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src) > >> int > >> smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > >> struct cifs_sb_info *cifs_sb, const char *full_path, > >> - FILE_ALL_INFO *data, bool *adjust_tz) > >> + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink) > >> { > >> int rc; > >> struct smb2_file_all_info *smb2_data; > >> > >> *adjust_tz = false; > >> + *symlink = false; > >> > >> smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2, > >> GFP_KERNEL); > >> @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > >> return -ENOMEM; > >> > >> rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, > >> - FILE_READ_ATTRIBUTES, FILE_OPEN, > >> - OPEN_REPARSE_POINT, smb2_data, > >> - SMB2_OP_QUERY_INFO); > >> + FILE_READ_ATTRIBUTES, FILE_OPEN, 0, > >> + smb2_data, SMB2_OP_QUERY_INFO); > >> + if (rc == -EOPNOTSUPP) { > >> + *symlink = true; > >> + /* Failed on a symbolic link - query a reparse point info */ > >> + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, > >> + FILE_READ_ATTRIBUTES, FILE_OPEN, > >> + OPEN_REPARSE_POINT, smb2_data, > >> + SMB2_OP_QUERY_INFO); > >> + } > >> if (rc) > >> goto out; > >> > >> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > >> index 313813e..b4eea10 100644 > >> --- a/fs/cifs/smb2proto.h > >> +++ b/fs/cifs/smb2proto.h > >> @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst, > >> extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > >> struct cifs_sb_info *cifs_sb, > >> const char *full_path, FILE_ALL_INFO *data, > >> - bool *adjust_tz); > >> + bool *adjust_tz, bool *symlink); > >> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon, > >> const char *full_path, __u64 size, > >> struct cifs_sb_info *cifs_sb, bool set_alloc); > > > > > > -- > > Jeff Layton <jlayton@redhat.com> > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
On Fri, 1 Nov 2013 12:49:47 -0500 Steve French <smfrench@gmail.com> wrote: > FILE_ALL_INFO is not SMB1 specific, we request it in SMB2/SMB3 as well > and it is still a common level. See section 2.2.37 of MS-SMB2 or FSCC > description at: > > > http://msdn.microsoft.com/en-us/library/cc232117.aspx > > Is there a different level that ou would like to call? > Now that I look, I'm not sure that this is the case. If we get a FILE_ALL_INFO from the server, what do we need move_smb2_info_to_cifs() for? Either way, this API would be *much* cleaner if we didn't try to use on-the-wire formats to pass inode info around. That's what a cifs_fattr is for, after all... > On Fri, Nov 1, 2013 at 12:42 PM, Jeff Layton <jlayton@redhat.com> wrote: > > On Fri, 1 Nov 2013 17:57:39 +0400 > > Pavel Shilovsky <piastryyy@gmail.com> wrote: > > > >> From: Pavel Shilovsky <piastry@etersoft.ru> > >> > >> Now we treat any reparse point as a symbolic link and map it to a Unix > >> one that is not true in a common case due to many reparse point types > >> supported by SMB servers. > >> > >> Distinguish reparse point types into two groups: > >> 1) that can be accessed directly through a reparse point > >> (junctions, deduplicated files, NFS symlinks); > >> 2) that need to be processed manually (Windows symbolic links, DFS); > >> > >> and map only Windows symbolic links to Unix ones. > >> > >> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > >> --- > >> fs/cifs/cifsglob.h | 2 +- > >> fs/cifs/inode.c | 23 +++++++++++++---------- > >> fs/cifs/readdir.c | 40 ++++++++-------------------------------- > >> fs/cifs/smb1ops.c | 21 ++++++++++++++++++++- > >> fs/cifs/smb2inode.c | 16 ++++++++++++---- > >> fs/cifs/smb2proto.h | 2 +- > >> 6 files changed, 55 insertions(+), 49 deletions(-) > >> > >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > >> index a67cf12..b688f2b 100644 > >> --- a/fs/cifs/cifsglob.h > >> +++ b/fs/cifs/cifsglob.h > >> @@ -261,7 +261,7 @@ struct smb_version_operations { > >> /* query path data from the server */ > >> int (*query_path_info)(const unsigned int, struct cifs_tcon *, > >> struct cifs_sb_info *, const char *, > >> - FILE_ALL_INFO *, bool *); > >> + FILE_ALL_INFO *, bool *, bool *); > > > > > > This looks much better for performance, but I think it really shines a > > light on what an abortion the query_path_info and query_file_info > > operations are. They work with a FILE_ALL_INFO struct which is SMB1 > > specific, so for SMB2/3 we have to convert data to something that > > doesn't even match the protocol. > > > > I think it would be best to base this fix on top of a patchset to fix > > that properly. Those functions should be changed to pass data around in > > a cifs_fattr. With that, you could just add a new cifs_fattr->flags > > value and you wouldn't need to add this extra bool * argument. > > > > > >> /* query file data from the server */ > >> int (*query_file_info)(const unsigned int, struct cifs_tcon *, > >> struct cifs_fid *, FILE_ALL_INFO *); > >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > >> index 867b7cd..36f9ebb 100644 > >> --- a/fs/cifs/inode.c > >> +++ b/fs/cifs/inode.c > >> @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path, > >> /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */ > >> static void > >> cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > >> - struct cifs_sb_info *cifs_sb, bool adjust_tz) > >> + struct cifs_sb_info *cifs_sb, bool adjust_tz, > >> + bool symlink) > >> { > >> struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > >> > >> @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > >> fattr->cf_createtime = le64_to_cpu(info->CreationTime); > >> > >> fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > >> - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > >> + > >> + if (symlink) { > >> + fattr->cf_mode = S_IFLNK; > >> + fattr->cf_dtype = DT_LNK; > >> + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > >> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > >> fattr->cf_dtype = DT_DIR; > >> /* > >> @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > >> */ > >> if (!tcon->unix_ext) > >> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; > >> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { > >> - fattr->cf_mode = S_IFLNK; > >> - fattr->cf_dtype = DT_LNK; > >> - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > >> } else { > >> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > >> fattr->cf_dtype = DT_REG; > >> @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp) > >> rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data); > >> switch (rc) { > >> case 0: > >> - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false); > >> + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false, > >> + false); > >> break; > >> case -EREMOTE: > >> cifs_create_dfs_fattr(&fattr, inode->i_sb); > >> @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > >> bool adjust_tz = false; > >> struct cifs_fattr fattr; > >> struct cifs_search_info *srchinf = NULL; > >> + bool symlink = false; > >> > >> tlink = cifs_sb_tlink(cifs_sb); > >> if (IS_ERR(tlink)) > >> @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > >> } > >> data = (FILE_ALL_INFO *)buf; > >> rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path, > >> - data, &adjust_tz); > >> + data, &adjust_tz, &symlink); > >> } > >> > >> if (!rc) { > >> - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb, > >> - adjust_tz); > >> + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz, > >> + symlink); > >> } else if (rc == -EREMOTE) { > >> cifs_create_dfs_fattr(&fattr, sb); > >> rc = 0; > >> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > >> index 53a75f3..5940eca 100644 > >> --- a/fs/cifs/readdir.c > >> +++ b/fs/cifs/readdir.c > >> @@ -134,22 +134,6 @@ out: > >> dput(dentry); > >> } > >> > >> -/* > >> - * Is it possible that this directory might turn out to be a DFS referral > >> - * once we go to try and use it? > >> - */ > >> -static bool > >> -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) > >> -{ > >> -#ifdef CONFIG_CIFS_DFS_UPCALL > >> - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > >> - > >> - if (tcon->Flags & SMB_SHARE_IS_IN_DFS) > >> - return true; > >> -#endif > >> - return false; > >> -} > >> - > >> static void > >> cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > >> { > >> @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > >> if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > >> fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > >> fattr->cf_dtype = DT_DIR; > >> - /* > >> - * Windows CIFS servers generally make DFS referrals look > >> - * like directories in FIND_* responses with the reparse > >> - * attribute flag also set (since DFS junctions are > >> - * reparse points). We must revalidate at least these > >> - * directory inodes before trying to use them (if > >> - * they are DFS we will get PATH_NOT_COVERED back > >> - * when queried directly and can then try to connect > >> - * to the DFS target) > >> - */ > >> - if (cifs_dfs_is_possible(cifs_sb) && > >> - (fattr->cf_cifsattrs & ATTR_REPARSE)) > >> - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > >> - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { > >> - fattr->cf_mode = S_IFLNK; > >> - fattr->cf_dtype = DT_LNK; > >> } else { > >> fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > >> fattr->cf_dtype = DT_REG; > >> } > >> > >> + /* > >> + * 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 (fattr->cf_cifsattrs & ATTR_REPARSE) > >> + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > >> + > >> /* non-unix readdir doesn't provide nlink */ > >> fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; > >> > >> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > >> index ea99efe..47ab035 100644 > >> --- a/fs/cifs/smb1ops.c > >> +++ b/fs/cifs/smb1ops.c > >> @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, > >> static int > >> cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > >> struct cifs_sb_info *cifs_sb, const char *full_path, > >> - FILE_ALL_INFO *data, bool *adjustTZ) > >> + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink) > >> { > >> int rc; > >> > >> + *symlink = false; > >> + > >> /* could do find first instead but this returns more info */ > >> rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */, > >> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags & > >> @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > >> CIFS_MOUNT_MAP_SPECIAL_CHR); > >> *adjustTZ = true; > >> } > >> + > >> + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) { > >> + int tmprc; > >> + int oplock = 0; > >> + __u16 netfid; > >> + > >> + /* Need to check if this is a symbolic link or not */ > >> + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN, > >> + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock, > >> + NULL, cifs_sb->local_nls, > >> + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); > >> + if (tmprc == -EOPNOTSUPP) > >> + *symlink = true; > >> + else > >> + CIFSSMBClose(xid, tcon, netfid); > >> + } > >> + > >> return rc; > >> } > >> > >> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > >> index 78ff88c..84c012a 100644 > >> --- a/fs/cifs/smb2inode.c > >> +++ b/fs/cifs/smb2inode.c > >> @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src) > >> int > >> smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > >> struct cifs_sb_info *cifs_sb, const char *full_path, > >> - FILE_ALL_INFO *data, bool *adjust_tz) > >> + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink) > >> { > >> int rc; > >> struct smb2_file_all_info *smb2_data; > >> > >> *adjust_tz = false; > >> + *symlink = false; > >> > >> smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2, > >> GFP_KERNEL); > >> @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > >> return -ENOMEM; > >> > >> rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, > >> - FILE_READ_ATTRIBUTES, FILE_OPEN, > >> - OPEN_REPARSE_POINT, smb2_data, > >> - SMB2_OP_QUERY_INFO); > >> + FILE_READ_ATTRIBUTES, FILE_OPEN, 0, > >> + smb2_data, SMB2_OP_QUERY_INFO); > >> + if (rc == -EOPNOTSUPP) { > >> + *symlink = true; > >> + /* Failed on a symbolic link - query a reparse point info */ > >> + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, > >> + FILE_READ_ATTRIBUTES, FILE_OPEN, > >> + OPEN_REPARSE_POINT, smb2_data, > >> + SMB2_OP_QUERY_INFO); > >> + } > >> if (rc) > >> goto out; > >> > >> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > >> index 313813e..b4eea10 100644 > >> --- a/fs/cifs/smb2proto.h > >> +++ b/fs/cifs/smb2proto.h > >> @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst, > >> extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > >> struct cifs_sb_info *cifs_sb, > >> const char *full_path, FILE_ALL_INFO *data, > >> - bool *adjust_tz); > >> + bool *adjust_tz, bool *symlink); > >> extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon, > >> const char *full_path, __u64 size, > >> struct cifs_sb_info *cifs_sb, bool set_alloc); > > > > > > -- > > Jeff Layton <jlayton@redhat.com> > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
2013/11/1 Shirish Pargaonkar <shirishpargaonkar@gmail.com>: > Pavel, does this handle a case where it could be symbolic link > to a directory? Shirish, yes - it supports symbolic links to directories as well as to files. It treats them both as Unix symbolic links without any difference.
2013/11/1 Jeff Layton <jlayton@redhat.com>: > This looks much better for performance, but I think it really shines a > light on what an abortion the query_path_info and query_file_info > operations are. They work with a FILE_ALL_INFO struct which is SMB1 > specific, so for SMB2/3 we have to convert data to something that > doesn't even match the protocol. > > I think it would be best to base this fix on top of a patchset to fix > that properly. Those functions should be changed to pass data around in > a cifs_fattr. With that, you could just add a new cifs_fattr->flags > value and you wouldn't need to add this extra bool * argument. I agree that using cifs_fattr structure rather than FILE_ALL_INFO simplifies things and makes the code cleaner. But we have a problem in the 3.11 stable branch and I think we should fix it without many patches (code changes). We can apply this patch for stable and then convert the code to use cifs_fattr structure in query path/file info in mainline. Right?
On Sat, 2 Nov 2013 23:47:16 +0400 Pavel Shilovsky <piastryyy@gmail.com> wrote: > 2013/11/1 Jeff Layton <jlayton@redhat.com>: > > This looks much better for performance, but I think it really shines a > > light on what an abortion the query_path_info and query_file_info > > operations are. They work with a FILE_ALL_INFO struct which is SMB1 > > specific, so for SMB2/3 we have to convert data to something that > > doesn't even match the protocol. > > > > I think it would be best to base this fix on top of a patchset to fix > > that properly. Those functions should be changed to pass data around in > > a cifs_fattr. With that, you could just add a new cifs_fattr->flags > > value and you wouldn't need to add this extra bool * argument. > > I agree that using cifs_fattr structure rather than FILE_ALL_INFO > simplifies things and makes the code cleaner. But we have a problem in > the 3.11 stable branch and I think we should fix it without many > patches (code changes). We can apply this patch for stable and then > convert the code to use cifs_fattr structure in query path/file info > in mainline. Right? > If you intend to push a fix to stable for this, then that sounds like the best approach. It will make cleaning up the API harder, but as long as you're ok with that...
2013/11/3 Jeff Layton <jlayton@redhat.com>: > If you intend to push a fix to stable for this, then that sounds like > the best approach. It will make cleaning up the API harder, but as long > as you're ok with that... Yes - it was targeted for stable as well. Any acks from your side? Steve, do you have any objections to apply this patch (with Joao's "Reported-and-tested-by: Joao Correia <joaomiguelcorreia@gmail.com>" tag)?
a little large for stable but not sure yet what could be done to make it smaller On Mon, Nov 4, 2013 at 12:50 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote: > 2013/11/3 Jeff Layton <jlayton@redhat.com>: >> If you intend to push a fix to stable for this, then that sounds like >> the best approach. It will make cleaning up the API harder, but as long >> as you're ok with that... > > Yes - it was targeted for stable as well. Any acks from your side? > > Steve, do you have any objections to apply this patch (with Joao's > "Reported-and-tested-by: Joao Correia <joaomiguelcorreia@gmail.com>" > tag)? > > -- > Best regards, > Pavel Shilovsky.
On Fri, 1 Nov 2013 17:57:39 +0400 Pavel Shilovsky <piastryyy@gmail.com> wrote: > From: Pavel Shilovsky <piastry@etersoft.ru> > > Now we treat any reparse point as a symbolic link and map it to a Unix > one that is not true in a common case due to many reparse point types > supported by SMB servers. > > Distinguish reparse point types into two groups: > 1) that can be accessed directly through a reparse point > (junctions, deduplicated files, NFS symlinks); > 2) that need to be processed manually (Windows symbolic links, DFS); > > and map only Windows symbolic links to Unix ones. > > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > --- > fs/cifs/cifsglob.h | 2 +- > fs/cifs/inode.c | 23 +++++++++++++---------- > fs/cifs/readdir.c | 40 ++++++++-------------------------------- > fs/cifs/smb1ops.c | 21 ++++++++++++++++++++- > fs/cifs/smb2inode.c | 16 ++++++++++++---- > fs/cifs/smb2proto.h | 2 +- > 6 files changed, 55 insertions(+), 49 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index a67cf12..b688f2b 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -261,7 +261,7 @@ struct smb_version_operations { > /* query path data from the server */ > int (*query_path_info)(const unsigned int, struct cifs_tcon *, > struct cifs_sb_info *, const char *, > - FILE_ALL_INFO *, bool *); > + FILE_ALL_INFO *, bool *, bool *); > /* query file data from the server */ > int (*query_file_info)(const unsigned int, struct cifs_tcon *, > struct cifs_fid *, FILE_ALL_INFO *); > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index 867b7cd..36f9ebb 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path, > /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */ > static void > cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > - struct cifs_sb_info *cifs_sb, bool adjust_tz) > + struct cifs_sb_info *cifs_sb, bool adjust_tz, > + bool symlink) > { > struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > > @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > fattr->cf_createtime = le64_to_cpu(info->CreationTime); > > fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > + > + if (symlink) { > + fattr->cf_mode = S_IFLNK; > + fattr->cf_dtype = DT_LNK; > + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > fattr->cf_dtype = DT_DIR; > /* > @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, > */ > if (!tcon->unix_ext) > fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; > - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { > - fattr->cf_mode = S_IFLNK; > - fattr->cf_dtype = DT_LNK; > - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); > } else { > fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > fattr->cf_dtype = DT_REG; > @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp) > rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data); > switch (rc) { > case 0: > - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false); > + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false, > + false); > break; > case -EREMOTE: > cifs_create_dfs_fattr(&fattr, inode->i_sb); > @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > bool adjust_tz = false; > struct cifs_fattr fattr; > struct cifs_search_info *srchinf = NULL; > + bool symlink = false; > > tlink = cifs_sb_tlink(cifs_sb); > if (IS_ERR(tlink)) > @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > } > data = (FILE_ALL_INFO *)buf; > rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path, > - data, &adjust_tz); > + data, &adjust_tz, &symlink); > } > > if (!rc) { > - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb, > - adjust_tz); > + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz, > + symlink); > } else if (rc == -EREMOTE) { > cifs_create_dfs_fattr(&fattr, sb); > rc = 0; > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > index 53a75f3..5940eca 100644 > --- a/fs/cifs/readdir.c > +++ b/fs/cifs/readdir.c > @@ -134,22 +134,6 @@ out: > dput(dentry); > } > > -/* > - * Is it possible that this directory might turn out to be a DFS referral > - * once we go to try and use it? > - */ > -static bool > -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) > -{ > -#ifdef CONFIG_CIFS_DFS_UPCALL > - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > - > - if (tcon->Flags & SMB_SHARE_IS_IN_DFS) > - return true; > -#endif > - return false; > -} > - > static void > cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > { > @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > fattr->cf_dtype = DT_DIR; > - /* > - * Windows CIFS servers generally make DFS referrals look > - * like directories in FIND_* responses with the reparse > - * attribute flag also set (since DFS junctions are > - * reparse points). We must revalidate at least these > - * directory inodes before trying to use them (if > - * they are DFS we will get PATH_NOT_COVERED back > - * when queried directly and can then try to connect > - * to the DFS target) > - */ > - if (cifs_dfs_is_possible(cifs_sb) && > - (fattr->cf_cifsattrs & ATTR_REPARSE)) > - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { > - fattr->cf_mode = S_IFLNK; > - fattr->cf_dtype = DT_LNK; > } else { > fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > fattr->cf_dtype = DT_REG; > } > > + /* > + * 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 (fattr->cf_cifsattrs & ATTR_REPARSE) > + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > + > /* non-unix readdir doesn't provide nlink */ > fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; > > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c > index ea99efe..47ab035 100644 > --- a/fs/cifs/smb1ops.c > +++ b/fs/cifs/smb1ops.c > @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, > static int > cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > struct cifs_sb_info *cifs_sb, const char *full_path, > - FILE_ALL_INFO *data, bool *adjustTZ) > + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink) > { > int rc; > > + *symlink = false; > + > /* could do find first instead but this returns more info */ > rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */, > cifs_sb->local_nls, cifs_sb->mnt_cifs_flags & > @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > CIFS_MOUNT_MAP_SPECIAL_CHR); > *adjustTZ = true; > } > + > + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) { > + int tmprc; > + int oplock = 0; > + __u16 netfid; > + > + /* Need to check if this is a symbolic link or not */ > + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN, > + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock, > + NULL, cifs_sb->local_nls, > + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); > + if (tmprc == -EOPNOTSUPP) > + *symlink = true; > + else > + CIFSSMBClose(xid, tcon, netfid); > + } > + > return rc; > } > > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > index 78ff88c..84c012a 100644 > --- a/fs/cifs/smb2inode.c > +++ b/fs/cifs/smb2inode.c > @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src) > int > smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > struct cifs_sb_info *cifs_sb, const char *full_path, > - FILE_ALL_INFO *data, bool *adjust_tz) > + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink) > { > int rc; > struct smb2_file_all_info *smb2_data; > > *adjust_tz = false; > + *symlink = false; > > smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2, > GFP_KERNEL); > @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > return -ENOMEM; > > rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, > - FILE_READ_ATTRIBUTES, FILE_OPEN, > - OPEN_REPARSE_POINT, smb2_data, > - SMB2_OP_QUERY_INFO); > + FILE_READ_ATTRIBUTES, FILE_OPEN, 0, > + smb2_data, SMB2_OP_QUERY_INFO); > + if (rc == -EOPNOTSUPP) { > + *symlink = true; > + /* Failed on a symbolic link - query a reparse point info */ > + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, > + FILE_READ_ATTRIBUTES, FILE_OPEN, > + OPEN_REPARSE_POINT, smb2_data, > + SMB2_OP_QUERY_INFO); > + } > if (rc) > goto out; > > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > index 313813e..b4eea10 100644 > --- a/fs/cifs/smb2proto.h > +++ b/fs/cifs/smb2proto.h > @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst, > extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > struct cifs_sb_info *cifs_sb, > const char *full_path, FILE_ALL_INFO *data, > - bool *adjust_tz); > + bool *adjust_tz, bool *symlink); > extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon, > const char *full_path, __u64 size, > struct cifs_sb_info *cifs_sb, bool set_alloc); Looks reasonable for an interim fix I guess. I'm not thrilled with it since it doesn't help the code overall, but I guess we can live with it until we can clean up the API. Acked-by: Jeff Layton <jlayton@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index a67cf12..b688f2b 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -261,7 +261,7 @@ struct smb_version_operations { /* query path data from the server */ int (*query_path_info)(const unsigned int, struct cifs_tcon *, struct cifs_sb_info *, const char *, - FILE_ALL_INFO *, bool *); + FILE_ALL_INFO *, bool *, bool *); /* query file data from the server */ int (*query_file_info)(const unsigned int, struct cifs_tcon *, struct cifs_fid *, FILE_ALL_INFO *); diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index 867b7cd..36f9ebb 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -542,7 +542,8 @@ static int cifs_sfu_mode(struct cifs_fattr *fattr, const unsigned char *path, /* Fill a cifs_fattr struct with info from FILE_ALL_INFO */ static void cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, - struct cifs_sb_info *cifs_sb, bool adjust_tz) + struct cifs_sb_info *cifs_sb, bool adjust_tz, + bool symlink) { struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); @@ -569,7 +570,11 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, fattr->cf_createtime = le64_to_cpu(info->CreationTime); fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); - if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { + + if (symlink) { + fattr->cf_mode = S_IFLNK; + fattr->cf_dtype = DT_LNK; + } else if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; fattr->cf_dtype = DT_DIR; /* @@ -578,10 +583,6 @@ cifs_all_info_to_fattr(struct cifs_fattr *fattr, FILE_ALL_INFO *info, */ if (!tcon->unix_ext) fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { - fattr->cf_mode = S_IFLNK; - fattr->cf_dtype = DT_LNK; - fattr->cf_nlink = le32_to_cpu(info->NumberOfLinks); } else { fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; fattr->cf_dtype = DT_REG; @@ -626,7 +627,8 @@ cifs_get_file_info(struct file *filp) rc = server->ops->query_file_info(xid, tcon, &cfile->fid, &find_data); switch (rc) { case 0: - cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false); + cifs_all_info_to_fattr(&fattr, &find_data, cifs_sb, false, + false); break; case -EREMOTE: cifs_create_dfs_fattr(&fattr, inode->i_sb); @@ -673,6 +675,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, bool adjust_tz = false; struct cifs_fattr fattr; struct cifs_search_info *srchinf = NULL; + bool symlink = false; tlink = cifs_sb_tlink(cifs_sb); if (IS_ERR(tlink)) @@ -702,12 +705,12 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, } data = (FILE_ALL_INFO *)buf; rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path, - data, &adjust_tz); + data, &adjust_tz, &symlink); } if (!rc) { - cifs_all_info_to_fattr(&fattr, (FILE_ALL_INFO *)data, cifs_sb, - adjust_tz); + cifs_all_info_to_fattr(&fattr, data, cifs_sb, adjust_tz, + symlink); } else if (rc == -EREMOTE) { cifs_create_dfs_fattr(&fattr, sb); rc = 0; diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c index 53a75f3..5940eca 100644 --- a/fs/cifs/readdir.c +++ b/fs/cifs/readdir.c @@ -134,22 +134,6 @@ out: dput(dentry); } -/* - * Is it possible that this directory might turn out to be a DFS referral - * once we go to try and use it? - */ -static bool -cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) -{ -#ifdef CONFIG_CIFS_DFS_UPCALL - struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); - - if (tcon->Flags & SMB_SHARE_IS_IN_DFS) - return true; -#endif - return false; -} - static void cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) { @@ -159,27 +143,19 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; fattr->cf_dtype = DT_DIR; - /* - * Windows CIFS servers generally make DFS referrals look - * like directories in FIND_* responses with the reparse - * attribute flag also set (since DFS junctions are - * reparse points). We must revalidate at least these - * directory inodes before trying to use them (if - * they are DFS we will get PATH_NOT_COVERED back - * when queried directly and can then try to connect - * to the DFS target) - */ - if (cifs_dfs_is_possible(cifs_sb) && - (fattr->cf_cifsattrs & ATTR_REPARSE)) - fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; - } else if (fattr->cf_cifsattrs & ATTR_REPARSE) { - fattr->cf_mode = S_IFLNK; - fattr->cf_dtype = DT_LNK; } else { fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; fattr->cf_dtype = DT_REG; } + /* + * 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 (fattr->cf_cifsattrs & ATTR_REPARSE) + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; + /* non-unix readdir doesn't provide nlink */ fattr->cf_flags |= CIFS_FATTR_UNKNOWN_NLINK; diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c index ea99efe..47ab035 100644 --- a/fs/cifs/smb1ops.c +++ b/fs/cifs/smb1ops.c @@ -534,10 +534,12 @@ cifs_is_path_accessible(const unsigned int xid, struct cifs_tcon *tcon, static int cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, struct cifs_sb_info *cifs_sb, const char *full_path, - FILE_ALL_INFO *data, bool *adjustTZ) + FILE_ALL_INFO *data, bool *adjustTZ, bool *symlink) { int rc; + *symlink = false; + /* could do find first instead but this returns more info */ rc = CIFSSMBQPathInfo(xid, tcon, full_path, data, 0 /* not legacy */, cifs_sb->local_nls, cifs_sb->mnt_cifs_flags & @@ -554,6 +556,23 @@ cifs_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, CIFS_MOUNT_MAP_SPECIAL_CHR); *adjustTZ = true; } + + if (!rc && (le32_to_cpu(data->Attributes) & ATTR_REPARSE)) { + int tmprc; + int oplock = 0; + __u16 netfid; + + /* Need to check if this is a symbolic link or not */ + tmprc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN, + FILE_READ_ATTRIBUTES, 0, &netfid, &oplock, + NULL, cifs_sb->local_nls, + cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); + if (tmprc == -EOPNOTSUPP) + *symlink = true; + else + CIFSSMBClose(xid, tcon, netfid); + } + return rc; } diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c index 78ff88c..84c012a 100644 --- a/fs/cifs/smb2inode.c +++ b/fs/cifs/smb2inode.c @@ -123,12 +123,13 @@ move_smb2_info_to_cifs(FILE_ALL_INFO *dst, struct smb2_file_all_info *src) int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, struct cifs_sb_info *cifs_sb, const char *full_path, - FILE_ALL_INFO *data, bool *adjust_tz) + FILE_ALL_INFO *data, bool *adjust_tz, bool *symlink) { int rc; struct smb2_file_all_info *smb2_data; *adjust_tz = false; + *symlink = false; smb2_data = kzalloc(sizeof(struct smb2_file_all_info) + MAX_NAME * 2, GFP_KERNEL); @@ -136,9 +137,16 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, return -ENOMEM; rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, - FILE_READ_ATTRIBUTES, FILE_OPEN, - OPEN_REPARSE_POINT, smb2_data, - SMB2_OP_QUERY_INFO); + FILE_READ_ATTRIBUTES, FILE_OPEN, 0, + smb2_data, SMB2_OP_QUERY_INFO); + if (rc == -EOPNOTSUPP) { + *symlink = true; + /* Failed on a symbolic link - query a reparse point info */ + rc = smb2_open_op_close(xid, tcon, cifs_sb, full_path, + FILE_READ_ATTRIBUTES, FILE_OPEN, + OPEN_REPARSE_POINT, smb2_data, + SMB2_OP_QUERY_INFO); + } if (rc) goto out; diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h index 313813e..b4eea10 100644 --- a/fs/cifs/smb2proto.h +++ b/fs/cifs/smb2proto.h @@ -61,7 +61,7 @@ extern void move_smb2_info_to_cifs(FILE_ALL_INFO *dst, extern int smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, struct cifs_sb_info *cifs_sb, const char *full_path, FILE_ALL_INFO *data, - bool *adjust_tz); + bool *adjust_tz, bool *symlink); extern int smb2_set_path_size(const unsigned int xid, struct cifs_tcon *tcon, const char *full_path, __u64 size, struct cifs_sb_info *cifs_sb, bool set_alloc);