Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] CIFS: refactor cifs_get_inode_info()
@ 2019-11-18 20:04 Aurelien Aptel
  2019-11-18 20:47 ` Steve French
  0 siblings, 1 reply; 2+ messages in thread
From: Aurelien Aptel @ 2019-11-18 20:04 UTC (permalink / raw)
  To: linux-cifs; +Cc: smfrench, Aurelien Aptel

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@suse.com>
---
 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


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] CIFS: refactor cifs_get_inode_info()
  2019-11-18 20:04 [PATCH] CIFS: refactor cifs_get_inode_info() Aurelien Aptel
@ 2019-11-18 20:47 ` Steve French
  0 siblings, 0 replies; 2+ messages in thread
From: Steve French @ 2019-11-18 20:47 UTC (permalink / raw)
  To: Aurelien Aptel; +Cc: CIFS

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@suse.com> 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@suse.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-18 20:04 [PATCH] CIFS: refactor cifs_get_inode_info() Aurelien Aptel
2019-11-18 20:47 ` Steve French

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org
	public-inbox-index linux-cifs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git