Removed one extra newline (trivial change) and tentatively pushed to for-next so I could run the buildbot with it: http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/283 On Mon, Nov 18, 2019 at 2:04 PM Aurelien Aptel <aaptel@xxxxxxxx> wrote: > > Make logic of cifs_get_inode() much clearer by moving code to sub > functions and adding comments. > > Document the steps this function does. > > cifs_get_inode_info() gets and updates a file inode metadata from its > file path. > > * If caller already has raw info data from server they can pass it. > * If inode already exists (just need to update) caller can pass it. > > Step 1: get raw data from server if none was passed > Step 2: parse raw data into intermediate internal cifs_fattr struct > Step 3: set fattr uniqueid which is later used for inode number. This > can sometime be done from raw data > Step 4: tweak fattr according to mount options (file_mode, acl to mode > bits, uid, gid, etc) > Step 5: update or create inode from final fattr struct > > * add is_smb1_server() helper > * add is_inode_cache_good() helper > * move SMB1-backupcreds-getinfo-retry to separate func > cifs_backup_query_path_info(). > * move set-uniqueid code to separate func cifs_set_fattr_ino() > * don't clobber uniqueid from backup cred retry > * fix some probable corner cases memleaks > > Signed-off-by: Aurelien Aptel <aaptel@xxxxxxxx> > --- > fs/cifs/cifsglob.h | 6 + > fs/cifs/inode.c | 338 +++++++++++++++++++++++++++++++---------------------- > 2 files changed, 206 insertions(+), 138 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 5bbd7d4ba58e..811448fc3781 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1984,4 +1984,10 @@ extern struct smb_version_values smb302_values; > #define ALT_SMB311_VERSION_STRING "3.11" > extern struct smb_version_operations smb311_operations; > extern struct smb_version_values smb311_values; > + > +static inline bool is_smb1_server(struct TCP_Server_Info *server) > +{ > + return strcmp(server->vals->version_string, SMB1_VERSION_STRING) == 0; > +} > + > #endif /* _CIFS_GLOB_H */ > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index df9377828e2f..1f6d174b3041 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -297,7 +297,7 @@ cifs_unix_basic_to_fattr(struct cifs_fattr *fattr, FILE_UNIX_BASIC_INFO *info, > fattr->cf_uid = uid; > } > } > - > + > fattr->cf_gid = cifs_sb->mnt_gid; > if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_OVERR_GID)) { > u64 id = le64_to_cpu(info->Gid); > @@ -727,22 +727,138 @@ static __u64 simple_hashstr(const char *str) > return hash; > } > > +/** > + * cifs_backup_query_path_info - SMB1 fallback code to get ino > + * > + * Fallback code to get file metadata when we don't have access to > + * @full_path (EACCESS) and have backup creds. > + * > + * @data will be set to search info result buffer > + * @resp_buf will be set to cifs resp buf and needs to be freed with > + * cifs_buf_release() when done with @data. > + */ > +static int > +cifs_backup_query_path_info(int xid, > + struct cifs_tcon *tcon, > + struct super_block *sb, > + const char *full_path, > + void **resp_buf, > + FILE_ALL_INFO **data) > +{ > + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > + struct cifs_search_info info = {0}; > + u16 flags; > + int rc; > + > + *resp_buf = NULL; > + info.endOfSearch = false; > + if (tcon->unix_ext) > + info.info_level = SMB_FIND_FILE_UNIX; > + else if ((tcon->ses->capabilities & > + tcon->ses->server->vals->cap_nt_find) == 0) > + info.info_level = SMB_FIND_FILE_INFO_STANDARD; > + else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) > + info.info_level = SMB_FIND_FILE_ID_FULL_DIR_INFO; > + else /* no srvino useful for fallback to some netapp */ > + info.info_level = SMB_FIND_FILE_DIRECTORY_INFO; > + > + flags = CIFS_SEARCH_CLOSE_ALWAYS | > + CIFS_SEARCH_CLOSE_AT_END | > + CIFS_SEARCH_BACKUP_SEARCH; > + > + rc = CIFSFindFirst(xid, tcon, full_path, > + cifs_sb, NULL, flags, &info, false); > + if (rc) > + return rc; > + > + *resp_buf = (void *)info.ntwrk_buf_start; > + *data = (FILE_ALL_INFO *)info.srch_entries_start; > + return 0; > +} > + > +static void > +cifs_set_fattr_ino(int xid, > + struct cifs_tcon *tcon, > + struct super_block *sb, > + struct inode **inode, > + const char *full_path, > + FILE_ALL_INFO *data, > + struct cifs_fattr *fattr) > +{ > + struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > + struct TCP_Server_Info *server = tcon->ses->server; > + int rc; > + > + if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)) { > + if (*inode) > + fattr->cf_uniqueid = CIFS_I(*inode)->uniqueid; > + else > + fattr->cf_uniqueid = iunique(sb, ROOT_I); > + return; > + } > + > + /* > + * If we have an inode pass a NULL tcon to ensure we don't > + * make a round trip to the server. This only works for SMB2+. > + */ > + rc = server->ops->get_srv_inum(xid, > + *inode ? NULL : tcon, > + cifs_sb, full_path, > + &fattr->cf_uniqueid, > + data); > + if (rc) { > + /* > + * If that fails reuse existing ino or generate one > + * and disable server ones > + */ > + if (*inode) > + fattr->cf_uniqueid = CIFS_I(*inode)->uniqueid; > + else { > + fattr->cf_uniqueid = iunique(sb, ROOT_I); > + cifs_autodisable_serverino(cifs_sb); > + } > + return; > + } > + > + /* If no errors, check for zero root inode (invalid) */ > + if (fattr->cf_uniqueid == 0 && strlen(full_path) == 0) { > + cifs_dbg(FYI, "Invalid (0) inodenum\n"); > + if (*inode) { > + /* reuse */ > + fattr->cf_uniqueid = CIFS_I(*inode)->uniqueid; > + } else { > + /* make an ino by hashing the UNC */ > + fattr->cf_flags |= CIFS_FATTR_FAKE_ROOT_INO; > + fattr->cf_uniqueid = simple_hashstr(tcon->treeName); > + } > + } > +} > + > +static inline bool is_inode_cache_good(struct inode *ino) > +{ > + return ino && CIFS_CACHE_READ(CIFS_I(ino)) && CIFS_I(ino)->time != 0; > +} > + > int > -cifs_get_inode_info(struct inode **inode, const char *full_path, > - FILE_ALL_INFO *data, struct super_block *sb, int xid, > +cifs_get_inode_info(struct inode **inode, > + const char *full_path, > + FILE_ALL_INFO *in_data, > + struct super_block *sb, int xid, > const struct cifs_fid *fid) > { > - __u16 srchflgs; > - int rc = 0, tmprc = ENOSYS; > + > struct cifs_tcon *tcon; > struct TCP_Server_Info *server; > struct tcon_link *tlink; > struct cifs_sb_info *cifs_sb = CIFS_SB(sb); > - char *buf = NULL; > bool adjust_tz = false; > - struct cifs_fattr fattr; > - struct cifs_search_info *srchinf = NULL; > + struct cifs_fattr fattr = {0}; > bool symlink = false; > + FILE_ALL_INFO *data = in_data; > + FILE_ALL_INFO *tmp_data = NULL; > + void *smb1_backup_rsp_buf = NULL; > + int rc = 0; > + int tmprc = 0; > > tlink = cifs_sb_tlink(cifs_sb); > if (IS_ERR(tlink)) > @@ -750,142 +866,88 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > tcon = tlink_tcon(tlink); > server = tcon->ses->server; > > - cifs_dbg(FYI, "Getting info on %s\n", full_path); > + /* > + * 1. Fetch file metadata if not provided (data) > + */ > > - if ((data == NULL) && (*inode != NULL)) { > - if (CIFS_CACHE_READ(CIFS_I(*inode)) && > - CIFS_I(*inode)->time != 0) { > + if (!data) { > + if (is_inode_cache_good(*inode)) { > cifs_dbg(FYI, "No need to revalidate cached inode sizes\n"); > - goto cgii_exit; > - } > - } > - > - /* if inode info is not passed, get it from server */ > - if (data == NULL) { > - if (!server->ops->query_path_info) { > - rc = -ENOSYS; > - goto cgii_exit; > + goto out; > } > - buf = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL); > - if (buf == NULL) { > + tmp_data = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL); > + if (!tmp_data) { > rc = -ENOMEM; > - goto cgii_exit; > + goto out; > } > - data = (FILE_ALL_INFO *)buf; > - rc = server->ops->query_path_info(xid, tcon, cifs_sb, full_path, > - data, &adjust_tz, &symlink); > + rc = server->ops->query_path_info(xid, tcon, cifs_sb, > + full_path, tmp_data, > + &adjust_tz, &symlink); > + data = tmp_data; > } > > - if (!rc) { > - cifs_all_info_to_fattr(&fattr, data, sb, adjust_tz, > - symlink); > - } else if (rc == -EREMOTE) { > + /* > + * 2. Convert it to internal cifs metadata (fattr) > + */ > + > + switch (rc) { > + case 0: > + cifs_all_info_to_fattr(&fattr, data, sb, adjust_tz, symlink); > + break; > + case -EREMOTE: > + /* DFS link, no metadata available on this server */ > cifs_create_dfs_fattr(&fattr, sb); > rc = 0; > - } else if ((rc == -EACCES) && backup_cred(cifs_sb) && > - (strcmp(server->vals->version_string, SMB1_VERSION_STRING) > - == 0)) { > + break; > + case -EACCES: > /* > - * For SMB2 and later the backup intent flag is already > - * sent if needed on open and there is no path based > - * FindFirst operation to use to retry with > + * perm errors, try again with backup flags if possible > + * > + * For SMB2 and later the backup intent flag > + * is already sent if needed on open and there > + * is no path based FindFirst operation to use > + * to retry with > */ > + if (backup_cred(cifs_sb) && is_smb1_server(server)) { > + /* for easier reading */ > + FILE_DIRECTORY_INFO *fdi; > + SEARCH_ID_FULL_DIR_INFO *si; > + > + rc = cifs_backup_query_path_info(xid, tcon, sb, > + full_path, > + &smb1_backup_rsp_buf, > + &data); > + if (rc) > + goto out; > > - srchinf = kzalloc(sizeof(struct cifs_search_info), > - GFP_KERNEL); > - if (srchinf == NULL) { > - rc = -ENOMEM; > - goto cgii_exit; > - } > + fdi = (FILE_DIRECTORY_INFO *)data; > + si = (SEARCH_ID_FULL_DIR_INFO *)data; > > - srchinf->endOfSearch = false; > - if (tcon->unix_ext) > - srchinf->info_level = SMB_FIND_FILE_UNIX; > - else if ((tcon->ses->capabilities & > - tcon->ses->server->vals->cap_nt_find) == 0) > - srchinf->info_level = SMB_FIND_FILE_INFO_STANDARD; > - else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) > - srchinf->info_level = SMB_FIND_FILE_ID_FULL_DIR_INFO; > - else /* no srvino useful for fallback to some netapp */ > - srchinf->info_level = SMB_FIND_FILE_DIRECTORY_INFO; > - > - srchflgs = CIFS_SEARCH_CLOSE_ALWAYS | > - CIFS_SEARCH_CLOSE_AT_END | > - CIFS_SEARCH_BACKUP_SEARCH; > - > - rc = CIFSFindFirst(xid, tcon, full_path, > - cifs_sb, NULL, srchflgs, srchinf, false); > - if (!rc) { > - data = (FILE_ALL_INFO *)srchinf->srch_entries_start; > + cifs_dir_info_to_fattr(&fattr, fdi, cifs_sb); > + fattr.cf_uniqueid = le64_to_cpu(si->UniqueId); > + /* uniqueid set, skip get inum step */ > + goto handle_mnt_opt; > + } else { > + /* nothing we can do, bail out */ > + goto out; > + } > + break; > + default: > + cifs_dbg(FYI, "%s: unhandled err rc %d\n", __func__, rc); > + goto out; > + } > > - cifs_dir_info_to_fattr(&fattr, > - (FILE_DIRECTORY_INFO *)data, cifs_sb); > - fattr.cf_uniqueid = le64_to_cpu( > - ((SEARCH_ID_FULL_DIR_INFO *)data)->UniqueId); > + /* > + * 3. Get or update inode number (fattr.cf_uniqueid) > + */ > > - cifs_buf_release(srchinf->ntwrk_buf_start); > - } > - kfree(srchinf); > - if (rc) > - goto cgii_exit; > - } else > - goto cgii_exit; > + cifs_set_fattr_ino(xid, tcon, sb, inode, full_path, data, &fattr); > > /* > - * If an inode wasn't passed in, then get the inode number > - * > - * Is an i_ino of zero legal? Can we use that to check if the server > - * supports returning inode numbers? Are there other sanity checks we > - * can use to ensure that the server is really filling in that field? > + * 4. Tweak fattr based on mount options > */ > - if (*inode == NULL) { > - if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) { > - if (server->ops->get_srv_inum) > - tmprc = server->ops->get_srv_inum(xid, > - tcon, cifs_sb, full_path, > - &fattr.cf_uniqueid, data); > - if (tmprc) { > - cifs_dbg(FYI, "GetSrvInodeNum rc %d\n", > - tmprc); > - fattr.cf_uniqueid = iunique(sb, ROOT_I); > - cifs_autodisable_serverino(cifs_sb); > - } else if ((fattr.cf_uniqueid == 0) && > - strlen(full_path) == 0) { > - /* some servers ret bad root ino ie 0 */ > - cifs_dbg(FYI, "Invalid (0) inodenum\n"); > - fattr.cf_flags |= > - CIFS_FATTR_FAKE_ROOT_INO; > - fattr.cf_uniqueid = > - simple_hashstr(tcon->treeName); > - } > - } else > - fattr.cf_uniqueid = iunique(sb, ROOT_I); > - } else { > - if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) > - && server->ops->get_srv_inum) { > - /* > - * Pass a NULL tcon to ensure we don't make a round > - * trip to the server. This only works for SMB2+. > - */ > - tmprc = server->ops->get_srv_inum(xid, > - NULL, cifs_sb, full_path, > - &fattr.cf_uniqueid, data); > - if (tmprc) > - fattr.cf_uniqueid = CIFS_I(*inode)->uniqueid; > - else if ((fattr.cf_uniqueid == 0) && > - strlen(full_path) == 0) { > - /* > - * Reuse existing root inode num since > - * inum zero for root causes ls of . and .. to > - * not be returned > - */ > - cifs_dbg(FYI, "Srv ret 0 inode num for root\n"); > - fattr.cf_uniqueid = CIFS_I(*inode)->uniqueid; > - } > - } else > - fattr.cf_uniqueid = CIFS_I(*inode)->uniqueid; > - } > > +handle_mnt_opt: > /* query for SFU type info if supported and needed */ > if (fattr.cf_cifsattrs & ATTR_SYSTEM && > cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) { > @@ -900,16 +962,15 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > full_path, fid); > if (rc) { > cifs_dbg(FYI, "%s: Get mode from SID failed. rc=%d\n", > - __func__, rc); > - goto cgii_exit; > + __func__, rc); > + goto out; > } > } else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL) { > rc = cifs_acl_to_fattr(cifs_sb, &fattr, *inode, false, > - full_path, fid); > - if (rc) { > + full_path, fid); if (rc) { > cifs_dbg(FYI, "%s: Getting ACL failed with error: %d\n", > __func__, rc); > - goto cgii_exit; > + goto out; > } > } > > @@ -925,6 +986,10 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > cifs_dbg(FYI, "check_mf_symlink: %d\n", tmprc); > } > > + /* > + * 5. Update inode with final fattr data > + */ > + > if (!*inode) { > *inode = cifs_iget(sb, &fattr); > if (!*inode) > @@ -937,7 +1002,7 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > CIFS_I(*inode)->uniqueid != fattr.cf_uniqueid)) { > CIFS_I(*inode)->time = 0; /* force reval */ > rc = -ESTALE; > - goto cgii_exit; > + goto out; > } > > /* if filetype is different, return error */ > @@ -945,18 +1010,15 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, > (fattr.cf_mode & S_IFMT))) { > CIFS_I(*inode)->time = 0; /* force reval */ > rc = -ESTALE; > - goto cgii_exit; > + goto out; > } > > cifs_fattr_to_inode(*inode, &fattr); > } > - > -cgii_exit: > - if ((*inode) && ((*inode)->i_ino == 0)) > - cifs_dbg(FYI, "inode number of zero returned\n"); > - > - kfree(buf); > +out: > + cifs_buf_release(smb1_backup_rsp_buf); > cifs_put_tlink(tlink); > + kfree(tmp_data); > return rc; > } > > -- > 2.16.4 > -- Thanks, Steve
