Re: [linux-cifs-client] review 5, was Re: projected date for mount.cifs to support DFS junction points

[Christoph Hellwig <hch@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On Fri, Feb 15, 2008 at 07:37:35PM +0300, Q (Igor Mammedov) wrote:
> Sorry guys, but I have a lot of work for the last 3 weeks,
> so I couldn't spare much time for a hobby and react quickly.

No problem.  I know this problem very well as almost all of my core
kernel contributions are spare time as well.  Thanks for keeping
up the work.

> Here is what I've done the last weekend.
> Attached:
>  fixed patch [5/5] (0001-DFS-patch-that-connects-inode-with-dfs-handling-ops.patch).
>  fixed mixed case in struct member (0002-Fixed-mixed-case-name-in-structure-dfs_info3_param.patch)

The second one is trivially correct and should be pushed to Linus asap
as small cleanup.  Patch 1 is not exactly what I had in mind, I was
thinking about factoring out the common bits of cifs-cifs_get_inode_info
and cifs-cifs_get_inode_info_unix to avoid having all the logic to
set the various options twice.  I've attached two quick and untested
patches showing what I mean.  I think in this case having the ifdef
for that two line statement setting the inode operations here is fine.
But thinking about it I'm not even sure if it's good idea to make
dfs support conditional.  Any reason it can't just be included
unconditionally?



[-- Attachment #2: cifs-cifs_get_inode_info-cleanup --]
[-- Type: text/plain, Size: 4186 bytes --]

Index: linux-2.6/fs/cifs/inode.c
===================================================================
--- linux-2.6.orig/fs/cifs/inode.c	2008-02-15 17:44:48.000000000 +0100
+++ linux-2.6/fs/cifs/inode.c	2008-02-15 17:52:04.000000000 +0100
@@ -29,6 +29,46 @@
 #include "cifs_debug.h"
 #include "cifs_fs_sb.h"
 
+
+static void cifs_set_ops(struct inode *inode)
+{
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+
+	switch (inode->i_mode & S_IFMT) {
+	case S_IFREG:
+		inode->i_op = &cifs_file_inode_ops;
+		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DIRECT_IO) {
+			if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
+				inode->i_fop = &cifs_file_direct_nobrl_ops;
+			else
+				inode->i_fop = &cifs_file_direct_ops;
+		} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
+			inode->i_fop = &cifs_file_nobrl_ops;
+		else { /* not direct, send byte range locks */
+			inode->i_fop = &cifs_file_ops;
+		}
+
+
+		/* check if server can support readpages */
+		if (cifs_sb->tcon->ses->server->maxBuf <
+				PAGE_CACHE_SIZE + MAX_CIFS_HDR_SIZE)
+			inode->i_data.a_ops = &cifs_addr_ops_smallbuf;
+		else
+			inode->i_data.a_ops = &cifs_addr_ops;
+		break;
+	case S_IFDIR:
+		inode->i_op = &cifs_dir_inode_ops;
+		inode->i_fop = &cifs_dir_ops;
+		break;
+	case S_IFLNK:
+		inode->i_op = &cifs_symlink_inode_ops;
+		break;
+	default:
+		init_special_inode(inode, inode->i_mode, inode->i_rdev);
+		break;
+	}
+}
+
 int cifs_get_inode_info_unix(struct inode **pinode,
 	const unsigned char *search_path, struct super_block *sb, int xid)
 {
@@ -178,39 +218,8 @@ int cifs_get_inode_info_unix(struct inod
 		cFYI(1, ("Size %ld and blocks %llu",
 			(unsigned long) inode->i_size,
 			(unsigned long long)inode->i_blocks));
-		if (S_ISREG(inode->i_mode)) {
-			cFYI(1, ("File inode"));
-			inode->i_op = &cifs_file_inode_ops;
-			if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DIRECT_IO) {
-				if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-					inode->i_fop =
-						&cifs_file_direct_nobrl_ops;
-				else
-					inode->i_fop = &cifs_file_direct_ops;
-			} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-				inode->i_fop = &cifs_file_nobrl_ops;
-			else /* not direct, send byte range locks */
-				inode->i_fop = &cifs_file_ops;
-
-			/* check if server can support readpages */
-			if (pTcon->ses->server->maxBuf <
-			    PAGE_CACHE_SIZE + MAX_CIFS_HDR_SIZE)
-				inode->i_data.a_ops = &cifs_addr_ops_smallbuf;
-			else
-				inode->i_data.a_ops = &cifs_addr_ops;
-		} else if (S_ISDIR(inode->i_mode)) {
-			cFYI(1, ("Directory inode"));
-			inode->i_op = &cifs_dir_inode_ops;
-			inode->i_fop = &cifs_dir_ops;
-		} else if (S_ISLNK(inode->i_mode)) {
-			cFYI(1, ("Symbolic Link inode"));
-			inode->i_op = &cifs_symlink_inode_ops;
-		/* tmp_inode->i_fop = */ /* do not need to set to anything */
-		} else {
-			cFYI(1, ("Init special inode"));
-			init_special_inode(inode, inode->i_mode,
-					   inode->i_rdev);
-		}
+
+		cifs_set_ops(inode);
 	}
 	return rc;
 }
@@ -546,36 +555,7 @@ int cifs_get_inode_info(struct inode **p
 			atomic_set(&cifsInfo->inUse, 1);
 		}
 
-		if (S_ISREG(inode->i_mode)) {
-			cFYI(1, ("File inode"));
-			inode->i_op = &cifs_file_inode_ops;
-			if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DIRECT_IO) {
-				if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-					inode->i_fop =
-						&cifs_file_direct_nobrl_ops;
-				else
-					inode->i_fop = &cifs_file_direct_ops;
-			} else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
-				inode->i_fop = &cifs_file_nobrl_ops;
-			else /* not direct, send byte range locks */
-				inode->i_fop = &cifs_file_ops;
-
-			if (pTcon->ses->server->maxBuf <
-			     PAGE_CACHE_SIZE + MAX_CIFS_HDR_SIZE)
-				inode->i_data.a_ops = &cifs_addr_ops_smallbuf;
-			else
-				inode->i_data.a_ops = &cifs_addr_ops;
-		} else if (S_ISDIR(inode->i_mode)) {
-			cFYI(1, ("Directory inode"));
-			inode->i_op = &cifs_dir_inode_ops;
-			inode->i_fop = &cifs_dir_ops;
-		} else if (S_ISLNK(inode->i_mode)) {
-			cFYI(1, ("Symbolic Link inode"));
-			inode->i_op = &cifs_symlink_inode_ops;
-		} else {
-			init_special_inode(inode, inode->i_mode,
-					   inode->i_rdev);
-		}
+		cifs_set_ops(inode);
 	}
 	kfree(buf);
 	return rc;

[-- Attachment #3: cifs-cifs_get_inode_info-cleanup-2 --]
[-- Type: text/plain, Size: 3870 bytes --]

Index: linux-2.6/fs/cifs/inode.c
===================================================================
--- linux-2.6.orig/fs/cifs/inode.c	2008-02-15 17:52:26.000000000 +0100
+++ linux-2.6/fs/cifs/inode.c	2008-02-15 18:01:35.000000000 +0100
@@ -69,6 +69,35 @@ static void cifs_set_ops(struct inode *i
 	}
 }
 
+static int cifs_get_inode_info_remote(struct cifs_sb_info *cifs_sb,
+		const unsigned char *search_path, int xid)
+{
+	struct cifsTconInfo *tcon = cifs_sb->tcon;
+	char *tmp_path;
+	size_t tmp_path_len;
+	int rc;
+
+	tmp_path_len = strnlen(tcon->treeName, MAX_TREE_SIZE + 1) +
+		       strnlen(search_path, MAX_PATHCONF) + 1;
+
+	tmp_path = kmalloc(tmp_path_len, GFP_KERNEL);
+	if (!tmp_path)
+		return -ENOMEM;
+
+	/*
+	 * Have to skip first of the double backslash of the UNC name.
+	 */
+	strncpy(tmp_path, tcon->treeName, MAX_TREE_SIZE);
+	strncat(tmp_path, search_path, MAX_PATHCONF);
+
+	rc = connect_to_dfs_path(xid, tcon->ses, /* treename + */ tmp_path,
+				 cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
+				 		CIFS_MOUNT_MAP_SPECIAL_CHR);
+
+	kfree(tmp_path);
+	return rc;
+}
+
 int cifs_get_inode_info_unix(struct inode **pinode,
 	const unsigned char *search_path, struct super_block *sb, int xid)
 {
@@ -77,7 +106,6 @@ int cifs_get_inode_info_unix(struct inod
 	struct cifsTconInfo *pTcon;
 	struct inode *inode;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
-	char *tmp_path;
 
 	pTcon = cifs_sb->tcon;
 	cFYI(1, ("Getting info on %s", search_path));
@@ -87,32 +115,12 @@ int cifs_get_inode_info_unix(struct inod
 					CIFS_MOUNT_MAP_SPECIAL_CHR);
 /*	dump_mem("\nUnixQPathInfo return data", &findData,
 		 sizeof(findData)); */
-	if (rc) {
-		if (rc == -EREMOTE) {
-			tmp_path =
-			    kmalloc(strnlen(pTcon->treeName,
-					    MAX_TREE_SIZE + 1) +
-				    strnlen(search_path, MAX_PATHCONF) + 1,
-				    GFP_KERNEL);
-			if (tmp_path == NULL)
-				return -ENOMEM;
-
-			/* have to skip first of the double backslash of
-			   UNC name */
-			strncpy(tmp_path, pTcon->treeName, MAX_TREE_SIZE);
-			strncat(tmp_path, search_path, MAX_PATHCONF);
-			rc = connect_to_dfs_path(xid, pTcon->ses,
-						 /* treename + */ tmp_path,
-						 cifs_sb->local_nls,
-						 cifs_sb->mnt_cifs_flags &
-						    CIFS_MOUNT_MAP_SPECIAL_CHR);
-			kfree(tmp_path);
 
-			/* BB fix up inode etc. */
-		} else if (rc) {
-			return rc;
-		}
-	} else {
+	if (rc == -EREMOTE)
+		return cifs_get_inode_info_remote(cifs_sb, search_path, xid);
+	else if (rc)
+		return rc;
+	else {
 		struct cifsInodeInfo *cifsInfo;
 		__u32 type = le32_to_cpu(findData.Type);
 		__u64 num_of_bytes = le64_to_cpu(findData.NumOfBytes);
@@ -335,7 +343,6 @@ int cifs_get_inode_info(struct inode **p
 	struct cifsTconInfo *pTcon;
 	struct inode *inode;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
-	char *tmp_path;
 	char *buf = NULL;
 	int adjustTZ = FALSE;
 
@@ -372,33 +379,9 @@ int cifs_get_inode_info(struct inode **p
 		}
 	}
 	/* dump_mem("\nQPathInfo return data",&findData, sizeof(findData)); */
-	if (rc) {
-		if (rc == -EREMOTE) {
-			tmp_path =
-			    kmalloc(strnlen
-				    (pTcon->treeName,
-				     MAX_TREE_SIZE + 1) +
-				    strnlen(search_path, MAX_PATHCONF) + 1,
-				    GFP_KERNEL);
-			if (tmp_path == NULL) {
-				kfree(buf);
-				return -ENOMEM;
-			}
-
-			strncpy(tmp_path, pTcon->treeName, MAX_TREE_SIZE);
-			strncat(tmp_path, search_path, MAX_PATHCONF);
-			rc = connect_to_dfs_path(xid, pTcon->ses,
-						 /* treename + */ tmp_path,
-						 cifs_sb->local_nls,
-						 cifs_sb->mnt_cifs_flags &
-						   CIFS_MOUNT_MAP_SPECIAL_CHR);
-			kfree(tmp_path);
-			/* BB fix up inode etc. */
-		} else if (rc) {
-			kfree(buf);
-			return rc;
-		}
-	} else {
+	if (rc == -EREMOTE)
+		rc = cifs_get_inode_info_remote(cifs_sb, search_path, xid);
+	else if (!rc) {
 		struct cifsInodeInfo *cifsInfo;
 		__u32 attr = le32_to_cpu(pfindData->Attributes);