[PATCH 0/3] cifs: make posix codepaths handle hardlinks appropriately (RFC)

[PATCH 0/3] cifs: make posix codepaths handle hardlinks appropriately (RFC)

[Jeff Layton]

This patchset represents a first draft at making cifs detect hardlinks
at inode creation time.

Currently, cifs has a pretty broken mechanism where all dentries get a
new inode and hardlinks aren't handled at the VFS layer. Presumably this
is because getting inode numbers from windows servers has a performance
cost (separate call to the server).

The problem is that hardlinks aren't detected by the VFS so we have to
try to detect them when we think it will matter. It also means that
there isn't cache consistency between hardlinked inodes.

This patch is intended as a first stab at changing this for inodes on
posix mounts. With posix extensions we generally get a "uniqueid" value
for free from the server when we get inode attributes.

The SNIA docs state that the UniqueId is unique in the scope of the
share, and the MS docs seem to hint that it's per-machine unique value.
Either way, I think it's safe enough not to worry about collisions. With
this change, we'll probably want to make "serverino" the default (and
maybe make it hardcoded on) when posix extensions are enabled.

This set just focuses on the posix extension codepaths. Once I get this
settled, I'll take a crack at fixing up the non-posix codepaths
similarly (they'll be a bit more work -- more functions will need to
be broken up, etc).

The set here seems to work, but there's still more to be done:

1) how to handle an unexpected inode number change (consider a rename or
a delete and recreate event that happens from another client or server).
Right now, I'm not detecting this, but I think we probably need to
consider what to do for it.

2) how to deal with non-posix extensions? When serverino is disabled, I
think we'll want to use iunique() to generate new inode numbers. We
could also consider making that the default as well.

Anyway...the set isn't quite complete, but I'd like to post this to
gather comments and suggestions and as a checkpoint to make sure I'm
doing this reasonably correctly.

Jeff Layton (3):
  cifs: rename cifs_iget to cifs_root_iget
  cifs: add new cifs_iget_unix_basic function
  cifs: add cifs_iget_unix and have readdir codepath use it

 fs/cifs/cifsfs.c    |    2 +-
 fs/cifs/cifsfs.h    |   15 +++++-
 fs/cifs/cifsglob.h  |    1 +
 fs/cifs/cifsproto.h |    8 ++-
 fs/cifs/dir.c       |   12 ++--
 fs/cifs/inode.c     |  158 ++++++++++++++++++++++++++++++++++++--------------
 fs/cifs/readdir.c   |  147 +++++++++++++++++++++++++++++++++++------------
 7 files changed, 251 insertions(+), 92 deletions(-)

[PATCH 1/3] cifs: rename cifs_iget to cifs_root_iget

[Jeff Layton]

The current cifs_iget isn't suitable for anything but the root inode.
Rename it with a more appropriate name.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsfs.c |    2 +-
 fs/cifs/cifsfs.h |    2 +-
 fs/cifs/inode.c  |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 34f5701..f0fb524 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -148,7 +148,7 @@ cifs_read_super(struct super_block *sb, void *data,
 #endif
 	sb->s_blocksize = CIFS_MAX_MSGSIZE;
 	sb->s_blocksize_bits = 14;	/* default 2**14 = CIFS_MAX_MSGSIZE */
-	inode = cifs_iget(sb, ROOT_I);
+	inode = cifs_root_iget(sb, ROOT_I);
 
 	if (IS_ERR(inode)) {
 		rc = PTR_ERR(inode);
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 77e190d..fa3694e 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -36,7 +36,7 @@ extern void cifs_read_inode(struct inode *);
 
 /* Functions related to inodes */
 extern const struct inode_operations cifs_dir_inode_ops;
-extern struct inode *cifs_iget(struct super_block *, unsigned long);
+extern struct inode *cifs_root_iget(struct super_block *, unsigned long);
 extern int cifs_create(struct inode *, struct dentry *, int,
 		       struct nameidata *);
 extern struct dentry *cifs_lookup(struct inode *, struct dentry *,
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 8faf275..b67c527 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -699,7 +699,7 @@ char *cifs_build_path_to_root(struct cifs_sb_info *cifs_sb)
 }
 
 /* gets root inode */
-struct inode *cifs_iget(struct super_block *sb, unsigned long ino)
+struct inode *cifs_root_iget(struct super_block *sb, unsigned long ino)
 {
 	int xid;
 	struct cifs_sb_info *cifs_sb;
-- 
1.6.0.6

[PATCH 2/3] cifs: add new cifs_iget_unix_basic function

[Jeff Layton]

Add a new cifs_iget_unix function that uses iget5_locked to identify
inodes. This will compare inodes based on the UniqueId value that
the server provides when unix extensions are enabled. We also have
mounts with unix extensions use that value in the i_ino field (after
hashing it down to 32-bits on a 32-bit arches).

With this, we should then have proper hardlink detection and can
eventually get rid of some nasty CIFS-specific hacks for handing
them.

Fixing the readdir codepath will be done in a later patch.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsfs.h    |   13 +++++
 fs/cifs/cifsglob.h  |    1 +
 fs/cifs/cifsproto.h |    4 +-
 fs/cifs/dir.c       |   12 ++---
 fs/cifs/inode.c     |  138 +++++++++++++++++++++++++++++++++++----------------
 5 files changed, 117 insertions(+), 51 deletions(-)

diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index fa3694e..aa4ffc0 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -24,6 +24,19 @@
 
 #define ROOT_I 2
 
+/*
+ * ino_t is 32-bits on 32-bit arch. We have to squash the 64-bit value down
+ * so that it will fit.
+ */
+static inline ino_t
+cifs_uniqueid_to_ino_t(u64 fileid)
+{
+	ino_t ino = (ino_t) fileid;
+	if (sizeof(ino_t) < sizeof(u64))
+		ino ^= fileid >> (sizeof(u64)-sizeof(ino_t)) * 8;
+	return ino;
+}
+
 extern struct file_system_type cifs_fs_type;
 extern const struct address_space_operations cifs_addr_ops;
 extern const struct address_space_operations cifs_addr_ops_smallbuf;
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 7ae1986..1bada66 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -371,6 +371,7 @@ struct cifsInodeInfo {
 	bool oplockPending:1;
 	bool delete_pending:1;		/* DELETE_ON_CLOSE is set */
 	u64  server_eof;		/* current file size on server */
+	u64  uniqueid;			/* server inode number */
 	struct inode vfs_inode;
 };
 
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 4167716..51dfd51 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -99,8 +99,10 @@ extern int cifs_posix_open(char *full_path, struct inode **pinode,
 			   struct super_block *sb, int mode, int oflags,
 			   int *poplock, __u16 *pnetfid, int xid);
 extern void posix_fill_in_inode(struct inode *tmp_inode,
-				FILE_UNIX_BASIC_INFO *pData, int isNewInode);
+				FILE_UNIX_BASIC_INFO *pData);
 extern struct inode *cifs_new_inode(struct super_block *sb, __u64 *inum);
+extern struct inode *cifs_iget_unix_basic(struct super_block *sb,
+				FILE_UNIX_BASIC_INFO *info);
 extern int cifs_get_inode_info(struct inode **pinode,
 			const unsigned char *search_path,
 			FILE_ALL_INFO *pfile_info,
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index d958044..be639ee 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -187,18 +187,16 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
 	if (!pinode)
 		goto posix_open_ret; /* caller does not need info */
 
-	if (*pinode == NULL) {
-		__u64 unique_id = le64_to_cpu(presp_data->UniqueId);
-		*pinode = cifs_new_inode(sb, &unique_id);
-	}
+	if (*pinode == NULL)
+		*pinode = cifs_iget_unix_basic(sb, presp_data);
 	/* else an inode was passed in. Update its info, don't create one */
 
 	/* We do not need to close the file if new_inode fails since
 	   the caller will retry qpathinfo as long as inode is null */
-	if (*pinode == NULL)
+	if (IS_ERR(*pinode)) {
+		rc = PTR_ERR(*pinode);
 		goto posix_open_ret;
-
-	posix_fill_in_inode(*pinode, presp_data, 1);
+	}
 
 posix_open_ret:
 	kfree(presp_data);
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index b67c527..c30c318 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -272,15 +272,11 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 	} else if (rc)
 		goto cgiiu_exit;
 
-	num_of_bytes = le64_to_cpu(find_data.NumOfBytes);
-	end_of_file = le64_to_cpu(find_data.EndOfFile);
-
 	/* get new inode */
 	if (*pinode == NULL) {
-		__u64 unique_id = le64_to_cpu(find_data.UniqueId);
-		*pinode = cifs_new_inode(sb, &unique_id);
-		if (*pinode == NULL) {
-			rc = -ENOMEM;
+		*pinode = cifs_iget_unix_basic(sb, &find_data);
+		if (IS_ERR(*pinode)) {
+			rc = PTR_ERR(*pinode);
 			goto cgiiu_exit;
 		}
 	}
@@ -291,18 +287,18 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 	cFYI(1, ("Old time %ld", cifsInfo->time));
 	cifsInfo->time = jiffies;
 	cFYI(1, ("New time %ld", cifsInfo->time));
-	/* this is ok to set on every inode revalidate */
-	atomic_set(&cifsInfo->inUse, 1);
 
-	cifs_unix_info_to_inode(inode, &find_data, 0);
+	if (is_dfs_referral)
+		cifs_set_ops(inode, true);
 
+	num_of_bytes = le64_to_cpu(find_data.NumOfBytes);
+	end_of_file = le64_to_cpu(find_data.EndOfFile);
 	if (num_of_bytes < end_of_file)
 		cFYI(1, ("allocation size less than end of file"));
 	cFYI(1, ("Size %ld and blocks %llu",
 		(unsigned long) inode->i_size,
 		(unsigned long long)inode->i_blocks));
 
-	cifs_set_ops(inode, is_dfs_referral);
 cgiiu_exit:
 	return rc;
 }
@@ -698,33 +694,102 @@ char *cifs_build_path_to_root(struct cifs_sb_info *cifs_sb)
 	return full_path;
 }
 
+/*
+ * Compare the UniqueID in the FILE_UNIX_BASIC_INFO with the one associated
+ * with the inode
+ */
+static int
+cifs_find_inode(struct inode *inode, void *opaque)
+{
+	u64 uniqueid = *((u64 *) opaque);
+
+	if (CIFS_I(inode)->uniqueid != uniqueid)
+		return 0;
+
+	return 1;
+}
+
+static int
+cifs_init_locked(struct inode *inode, void *opaque)
+{
+	u64 uniqueid = *((u64 *) opaque);
+
+	CIFS_I(inode)->uniqueid = uniqueid;
+	return 0;
+}
+
+/*
+ * Given a FILE_UNIX_BASIC_INFO struct, get the inode that matches the
+ * UniqueId or create a new inode.
+ */
+struct inode *
+cifs_iget_locked(struct super_block *sb, u64 uniqueid)
+{
+	unsigned long hash;
+	struct inode *inode;
+
+	cFYI(1,("looking for uniqueid=%lu\n", uniqueid));
+	/* must hash down to 32-bits on 32-bit arch */
+	hash = cifs_uniqueid_to_ino_t(uniqueid);
+
+	inode = iget5_locked(sb, hash, cifs_find_inode, cifs_init_locked,
+			     &uniqueid);
+
+	if (inode && (inode->i_state & I_NEW))
+		inode->i_ino = hash;
+
+	return inode;
+}
+
+struct inode *
+cifs_iget_unix_basic(struct super_block *sb, FILE_UNIX_BASIC_INFO *info)
+{
+	struct inode *inode;
+
+	inode = cifs_iget_locked(sb, le64_to_cpu(info->UniqueId));
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+
+	/* update inode since we have info in hand */
+	posix_fill_in_inode(inode, info);
+
+	if (inode->i_state & I_NEW)
+		unlock_new_inode(inode);
+
+	return inode;
+}
+
 /* gets root inode */
 struct inode *cifs_root_iget(struct super_block *sb, unsigned long ino)
 {
 	int xid;
 	struct cifs_sb_info *cifs_sb;
-	struct inode *inode;
+	struct inode *inode = NULL;
 	long rc;
 	char *full_path;
 
-	inode = iget_locked(sb, ino);
-	if (!inode)
-		return ERR_PTR(-ENOMEM);
-	if (!(inode->i_state & I_NEW))
-		return inode;
-
-	cifs_sb = CIFS_SB(inode->i_sb);
+	cifs_sb = CIFS_SB(sb);
 	full_path = cifs_build_path_to_root(cifs_sb);
 	if (full_path == NULL)
 		return ERR_PTR(-ENOMEM);
 
 	xid = GetXid();
-	if (cifs_sb->tcon->unix_ext)
-		rc = cifs_get_inode_info_unix(&inode, full_path, inode->i_sb,
-						xid);
-	else
+	if (cifs_sb->tcon->unix_ext) {
+		rc = cifs_get_inode_info_unix(&inode, full_path, sb, xid);
+		if (!inode)
+			return ERR_PTR(-ENOMEM);
+	} else {
+		inode = iget_locked(sb, ino);
+		if (!inode)
+			return ERR_PTR(-ENOMEM);
+		if (!(inode->i_state & I_NEW))
+			return inode;
+
 		rc = cifs_get_inode_info(&inode, full_path, NULL, inode->i_sb,
 						xid, NULL);
+		unlock_new_inode(inode);
+	}
+
 	if (rc && cifs_sb->tcon->ipc) {
 		cFYI(1, ("ipc connection - fake read inode"));
 		inode->i_mode |= S_IFDIR;
@@ -740,7 +805,6 @@ struct inode *cifs_root_iget(struct super_block *sb, unsigned long ino)
 		return ERR_PTR(rc);
 	}
 
-	unlock_new_inode(inode);
 
 	kfree(full_path);
 	/* can not call macro FreeXid here since in a void func
@@ -1056,8 +1120,7 @@ out_reval:
 	return rc;
 }
 
-void posix_fill_in_inode(struct inode *tmp_inode,
-	FILE_UNIX_BASIC_INFO *pData, int isNewInode)
+void posix_fill_in_inode(struct inode *tmp_inode, FILE_UNIX_BASIC_INFO *pData)
 {
 	struct cifsInodeInfo *cifsInfo = CIFS_I(tmp_inode);
 	loff_t local_size;
@@ -1073,15 +1136,11 @@ void posix_fill_in_inode(struct inode *tmp_inode,
 	cifs_unix_info_to_inode(tmp_inode, pData, 1);
 	cifs_set_ops(tmp_inode, false);
 
-	if (!S_ISREG(tmp_inode->i_mode))
-		return;
-
 	/*
-	 * No sense invalidating pages for new inode
-	 * since we we have not started caching
-	 * readahead file data yet.
+	 * No sense invalidating pages for new inode since we we have not
+	 * started caching readahead file data yet.
 	 */
-	if (isNewInode)
+	if (!S_ISREG(tmp_inode->i_mode) || tmp_inode->i_state & I_NEW)
 		return;
 
 	if (timespec_equal(&tmp_inode->i_mtime, &local_mtime) &&
@@ -1089,7 +1148,7 @@ void posix_fill_in_inode(struct inode *tmp_inode,
 		cFYI(1, ("inode exists but unchanged"));
 	} else {
 		/* file may have changed on server */
-		cFYI(1, ("invalidate inode, readdir detected change"));
+		cFYI(1, ("invalidate inode. change detected"));
 		invalidate_remote_inode(tmp_inode);
 	}
 }
@@ -1140,7 +1199,6 @@ int cifs_mkdir(struct inode *inode, struct dentry *direntry, int mode)
 			cFYI(1, ("posix mkdir returned 0x%x", rc));
 			d_drop(direntry);
 		} else {
-			__u64 unique_id;
 			if (pInfo->Type == cpu_to_le32(-1)) {
 				/* no return info, go query for it */
 				kfree(pInfo);
@@ -1154,20 +1212,14 @@ int cifs_mkdir(struct inode *inode, struct dentry *direntry, int mode)
 			else
 				direntry->d_op = &cifs_dentry_ops;
 
-			unique_id = le64_to_cpu(pInfo->UniqueId);
-			newinode = cifs_new_inode(inode->i_sb, &unique_id);
-			if (newinode == NULL) {
+			newinode = cifs_iget_unix_basic(inode->i_sb, pInfo);
+			if (IS_ERR(newinode)) {
 				kfree(pInfo);
 				goto mkdir_get_info;
 			}
 
-			newinode->i_nlink = 2;
 			d_instantiate(direntry, newinode);
 
-			/* we already checked in POSIXCreate whether
-			   frame was long enough */
-			posix_fill_in_inode(direntry->d_inode,
-					pInfo, 1 /* NewInode */);
 #ifdef CONFIG_CIFS_DEBUG2
 			cFYI(1, ("instantiated dentry %p %s to inode %p",
 				direntry, direntry->d_name.name, newinode));
-- 
1.6.0.6

[PATCH 3/3] cifs: add cifs_iget_unix and have readdir codepath use it

[Jeff Layton]

Add a cifs_iget_unix function similar to cifs_iget_unix_basic. This
will ID inodes based on the UniqueId value in a FILE_INFO_UNIX struct.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsproto.h |    4 ++
 fs/cifs/inode.c     |   18 ++++++
 fs/cifs/readdir.c   |  147 +++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 131 insertions(+), 38 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 51dfd51..53b79ad 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -100,9 +100,13 @@ extern int cifs_posix_open(char *full_path, struct inode **pinode,
 			   int *poplock, __u16 *pnetfid, int xid);
 extern void posix_fill_in_inode(struct inode *tmp_inode,
 				FILE_UNIX_BASIC_INFO *pData);
+extern void unix_fill_in_inode(struct inode *tmp_inode,
+				FILE_UNIX_INFO *pData);
 extern struct inode *cifs_new_inode(struct super_block *sb, __u64 *inum);
 extern struct inode *cifs_iget_unix_basic(struct super_block *sb,
 				FILE_UNIX_BASIC_INFO *info);
+extern struct inode *cifs_iget_unix(struct super_block *sb,
+				FILE_UNIX_INFO *info);
 extern int cifs_get_inode_info(struct inode **pinode,
 			const unsigned char *search_path,
 			FILE_ALL_INFO *pfile_info,
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index c30c318..ac7336a 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -759,6 +759,24 @@ cifs_iget_unix_basic(struct super_block *sb, FILE_UNIX_BASIC_INFO *info)
 	return inode;
 }
 
+struct inode *
+cifs_iget_unix(struct super_block *sb, FILE_UNIX_INFO *info)
+{
+	struct inode *inode;
+
+	inode = cifs_iget_locked(sb, le64_to_cpu(info->UniqueId));
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+
+	/* update inode since we have info in hand */
+	unix_fill_in_inode(inode, info);
+
+	if (inode->i_state & I_NEW)
+		unlock_new_inode(inode);
+
+	return inode;
+}
+
 /* gets root inode */
 struct inode *cifs_root_iget(struct super_block *sb, unsigned long ino)
 {
diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 1a8be62..4b59ccb 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -56,6 +56,80 @@ static inline void dump_cifs_file_struct(struct file *file, char *label)
 }
 #endif /* DEBUG2 */
 
+static unsigned int
+cifs_dentry_type_unix(u32 type)
+{
+	switch (type) {
+	case UNIX_FILE:
+		return DT_REG;
+	case UNIX_SYMLINK:
+		return DT_LNK;
+	case UNIX_DIR:
+		return DT_DIR;
+	case UNIX_CHARDEV:
+		return DT_CHR;
+	case UNIX_BLOCKDEV:
+		return DT_BLK;
+	case UNIX_FIFO:
+		return DT_FIFO;
+	case UNIX_SOCKET:
+		return DT_SOCK;
+	default:
+		/* safest to just call it a file */
+		cFYI(1, ("unknown inode type %d", type));
+		return DT_REG;
+	}
+}
+
+/*
+ * Find the dentry that matches "name". If there isn't one, create
+ * one. If it's a negative dentry, then drop it and recreate it.
+ */
+static struct dentry *
+cifs_readdir_lookup_unix(struct dentry *parent, struct qstr *name,
+			 FILE_UNIX_INFO *info)
+{
+	struct dentry *dentry, *alias;
+	struct inode *inode;
+	struct super_block *sb = parent->d_inode->i_sb;
+
+	cFYI(1, ("For %s", name->name));
+
+	dentry = d_lookup(parent, name);
+	if (dentry) {
+		/* BB: check for inode number change */
+		if (dentry->d_inode != NULL)
+			return dentry;
+		d_drop(dentry);
+		dput(dentry);
+	}
+
+	dentry = d_alloc(parent, name);
+	if (dentry == NULL)
+		return NULL;
+
+	inode = cifs_iget_unix(sb, info);
+	if (IS_ERR(inode)) {
+		dput(dentry);
+		return NULL;
+	}
+
+	if (CIFS_SB(sb)->tcon->nocase)
+		dentry->d_op = &cifs_ci_dentry_ops;
+	else
+		dentry->d_op = &cifs_dentry_ops;
+
+	alias = d_materialise_unique(dentry, inode);
+	if (alias != NULL) {
+		dput(dentry);
+		if (IS_ERR(alias))
+			return NULL;
+		dentry = alias;
+	}
+
+	return dentry;
+}
+
 /* Returns 1 if new inode created, 2 if both dentry and inode were */
 /* Might check in the future if inode number changed so we can rehash inode */
 static int
@@ -69,7 +143,6 @@ construct_dentry(struct qstr *qstring, struct file *file,
 
 	cFYI(1, ("For %s", qstring->name));
 
-	qstring->hash = full_name_hash(qstring->name, qstring->len);
 	tmp_dentry = d_lookup(file->f_path.dentry, qstring);
 	if (tmp_dentry) {
 		/* BB: overwrite old name? i.e. tmp_dentry->d_name and
@@ -304,8 +377,7 @@ static void fill_in_inode(struct inode *tmp_inode, int new_buf_type,
 	}
 }
 
-static void unix_fill_in_inode(struct inode *tmp_inode,
-	FILE_UNIX_INFO *pfindData, unsigned int *pobject_type, int isNewInode)
+void unix_fill_in_inode(struct inode *tmp_inode, FILE_UNIX_INFO *pfindData)
 {
 	loff_t local_size;
 	struct timespec local_mtime;
@@ -335,33 +407,25 @@ static void unix_fill_in_inode(struct inode *tmp_inode,
 	   to avoid strange results if bits above were corrupt */
 	tmp_inode->i_mode &= ~S_IFMT;
 	if (type == UNIX_FILE) {
-		*pobject_type = DT_REG;
 		tmp_inode->i_mode |= S_IFREG;
 	} else if (type == UNIX_SYMLINK) {
-		*pobject_type = DT_LNK;
 		tmp_inode->i_mode |= S_IFLNK;
 	} else if (type == UNIX_DIR) {
-		*pobject_type = DT_DIR;
 		tmp_inode->i_mode |= S_IFDIR;
 	} else if (type == UNIX_CHARDEV) {
-		*pobject_type = DT_CHR;
 		tmp_inode->i_mode |= S_IFCHR;
 		tmp_inode->i_rdev = MKDEV(le64_to_cpu(pfindData->DevMajor),
 				le64_to_cpu(pfindData->DevMinor) & MINORMASK);
 	} else if (type == UNIX_BLOCKDEV) {
-		*pobject_type = DT_BLK;
 		tmp_inode->i_mode |= S_IFBLK;
 		tmp_inode->i_rdev = MKDEV(le64_to_cpu(pfindData->DevMajor),
 				le64_to_cpu(pfindData->DevMinor) & MINORMASK);
 	} else if (type == UNIX_FIFO) {
-		*pobject_type = DT_FIFO;
 		tmp_inode->i_mode |= S_IFIFO;
 	} else if (type == UNIX_SOCKET) {
-		*pobject_type = DT_SOCK;
 		tmp_inode->i_mode |= S_IFSOCK;
 	} else {
 		/* safest to just call it a file */
-		*pobject_type = DT_REG;
 		tmp_inode->i_mode |= S_IFREG;
 		cFYI(1, ("unknown inode type %d", type));
 	}
@@ -410,7 +474,7 @@ static void unix_fill_in_inode(struct inode *tmp_inode,
 		else
 			tmp_inode->i_data.a_ops = &cifs_addr_ops;
 
-		if (isNewInode)
+		if (tmp_inode->i_state & I_NEW)
 			return; /* No sense invalidating pages for new inode
 				   since we have not started caching readahead
 				   file data for it yet */
@@ -939,36 +1003,43 @@ static int cifs_filldir(char *pfindEntry, struct file *file,
 		return rc;
 
 	/* only these two infolevels return valid inode numbers */
-	if (pCifsF->srch_inf.info_level == SMB_FIND_FILE_UNIX ||
-	    pCifsF->srch_inf.info_level == SMB_FIND_FILE_ID_FULL_DIR_INFO)
-		rc = construct_dentry(&qstring, file, &tmp_inode, &tmp_dentry,
-					&inum);
-	else
-		rc = construct_dentry(&qstring, file, &tmp_inode, &tmp_dentry,
-					NULL);
+	if (pCifsF->srch_inf.info_level == SMB_FIND_FILE_UNIX) {
+		FILE_UNIX_INFO *info = (FILE_UNIX_INFO *) pfindEntry;
 
-	if ((tmp_inode == NULL) || (tmp_dentry == NULL))
-		return -ENOMEM;
+		tmp_dentry = cifs_readdir_lookup_unix(file->f_dentry, &qstring,
+						      info);
+		if (!tmp_dentry)
+			return -ENOMEM;
 
-	/* we pass in rc below, indicating whether it is a new inode,
-	   so we can figure out whether to invalidate the inode cached
-	   data if the file has changed */
-	if (pCifsF->srch_inf.info_level == SMB_FIND_FILE_UNIX)
-		unix_fill_in_inode(tmp_inode,
-				   (FILE_UNIX_INFO *)pfindEntry,
-				   &obj_type, rc);
-	else if (pCifsF->srch_inf.info_level == SMB_FIND_FILE_INFO_STANDARD)
-		fill_in_inode(tmp_inode, 0 /* old level 1 buffer type */,
-				pfindEntry, &obj_type, rc);
-	else
-		fill_in_inode(tmp_inode, 1 /* NT */, pfindEntry, &obj_type, rc);
+		obj_type = cifs_dentry_type_unix(le32_to_cpu(info->Type));
+	} else {
+		if (pCifsF->srch_inf.info_level ==
+		    SMB_FIND_FILE_ID_FULL_DIR_INFO)
+			rc = construct_dentry(&qstring, file, &tmp_inode,
+						&tmp_dentry, &inum);
+		else
+			rc = construct_dentry(&qstring, file, &tmp_inode,
+						&tmp_dentry, NULL);
 
-	if (rc) /* new inode - needs to be tied to dentry */ {
-		d_instantiate(tmp_dentry, tmp_inode);
-		if (rc == 2)
-			d_rehash(tmp_dentry);
-	}
+		if ((tmp_inode == NULL) || (tmp_dentry == NULL))
+			return -ENOMEM;
+
+		/* we pass in rc below, indicating whether it is a new inode,
+		 * so we can figure out whether to invalidate the inode cached
+		 * data if the file has changed
+		 */
+		if (pCifsF->srch_inf.info_level == SMB_FIND_FILE_INFO_STANDARD)
+			fill_in_inode(tmp_inode, 0, pfindEntry, &obj_type, rc);
+		else
+			fill_in_inode(tmp_inode, 1, pfindEntry, &obj_type, rc);
 
+		/* new inode - needs to be tied to dentry */
+		if (rc) {
+			d_instantiate(tmp_dentry, tmp_inode);
+			if (rc == 2)
+				d_rehash(tmp_dentry);
+		}
+	}
 
 	rc = filldir(direntry, qstring.name, qstring.len, file->f_pos,
 		     tmp_inode->i_ino, obj_type);
-- 
1.6.0.6

Re: [linux-cifs-client] [PATCH 1/3] cifs: rename cifs_iget to cifs_root_iget

[Christoph Hellwig]

On Sun, Apr 05, 2009 at 09:09:19AM -0400, Jeff Layton wrote:
> The current cifs_iget isn't suitable for anything but the root inode.
> Rename it with a more appropriate name.

And using it for the root inode is stupid, too as it can't be in cache.

The root inode is the case where you can easily use new_inode.

Re: [linux-cifs-client] [PATCH 1/3] cifs: rename cifs_iget to cifs_root_iget

[Jeff Layton]

On Sun, 5 Apr 2009 09:42:14 -0400
Christoph Hellwig <hch@infradead.org> wrote:

> On Sun, Apr 05, 2009 at 09:09:19AM -0400, Jeff Layton wrote:
> > The current cifs_iget isn't suitable for anything but the root inode.
> > Rename it with a more appropriate name.
> 
> And using it for the root inode is stupid, too as it can't be in cache.
> 
> The root inode is the case where you can easily use new_inode.
> 

Yep. We might want to just get rid of that eventually and move the IPC
stuff elsewhere. With the follow-on patches here, I think using iget
makes the code a little simpler so I'll leave this in place for now.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [linux-cifs-client] [PATCH 2/3] cifs: add new cifs_iget_unix_basic function

[Christoph Hellwig]

On Sun, Apr 05, 2009 at 09:09:20AM -0400, Jeff Layton wrote:
> Add a new cifs_iget_unix function that uses iget5_locked to identify
> inodes. This will compare inodes based on the UniqueId value that
> the server provides when unix extensions are enabled. We also have
> mounts with unix extensions use that value in the i_ino field (after
> hashing it down to 32-bits on a 32-bit arches).

Note that i_ino and ino_t aren't actually all that important.  You
already do the iget comparisms based on the internal uniqueid, so the
only thing i_ino is used for is filling out the inode value in
generic_fattr.  I would suggest to fill it out explicitly in
cifs_getattr so that you can actually return a 64bit ino there which
is supported by stat64, possibly under and inode64 mount option.

> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index d958044..be639ee 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -187,18 +187,16 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>  	if (!pinode)
>  		goto posix_open_ret; /* caller does not need info */
>  
> -	if (*pinode == NULL) {
> -		__u64 unique_id = le64_to_cpu(presp_data->UniqueId);
> -		*pinode = cifs_new_inode(sb, &unique_id);
> -	}
> +	if (*pinode == NULL)
> +		*pinode = cifs_iget_unix_basic(sb, presp_data);
>  	/* else an inode was passed in. Update its info, don't create one */

Not directly related to this patch, but cifs_posix_open really needs
some restructuring.  The code up to the !pinode check should be the
basic underlying helper, and the call to cifs_iget_unix_basic and
posix_fill_in_inode should be moved to those callers that acutally need
it.

Re: [linux-cifs-client] [PATCH 2/3] cifs: add new cifs_iget_unix_basic function

[Jeff Layton]

On Wed, 8 Apr 2009 13:05:31 -0400
Christoph Hellwig <hch@infradead.org> wrote:

> On Sun, Apr 05, 2009 at 09:09:20AM -0400, Jeff Layton wrote:
> > Add a new cifs_iget_unix function that uses iget5_locked to identify
> > inodes. This will compare inodes based on the UniqueId value that
> > the server provides when unix extensions are enabled. We also have
> > mounts with unix extensions use that value in the i_ino field (after
> > hashing it down to 32-bits on a 32-bit arches).
> 
> Note that i_ino and ino_t aren't actually all that important.  You
> already do the iget comparisms based on the internal uniqueid, so the
> only thing i_ino is used for is filling out the inode value in
> generic_fattr.  I would suggest to fill it out explicitly in
> cifs_getattr so that you can actually return a 64bit ino there which
> is supported by stat64, possibly under and inode64 mount option.
> 

Thanks, Christoph. That sounds reasonable. I'll do that with the next
iteration.

> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index d958044..be639ee 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -187,18 +187,16 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
> >  	if (!pinode)
> >  		goto posix_open_ret; /* caller does not need info */
> >  
> > -	if (*pinode == NULL) {
> > -		__u64 unique_id = le64_to_cpu(presp_data->UniqueId);
> > -		*pinode = cifs_new_inode(sb, &unique_id);
> > -	}
> > +	if (*pinode == NULL)
> > +		*pinode = cifs_iget_unix_basic(sb, presp_data);
> >  	/* else an inode was passed in. Update its info, don't create one */
> 
> Not directly related to this patch, but cifs_posix_open really needs
> some restructuring.  The code up to the !pinode check should be the
> basic underlying helper, and the call to cifs_iget_unix_basic and
> posix_fill_in_inode should be moved to those callers that acutally need
> it.
> 

Agreed, and that's not the only place. The current interfaces for this
stuff seem very ad-hoc. A primary goal that have with this patchset is
to not break anything and to keep the changes to a minimum, but I'd
like to see these cleaned up too.

-- 
Jeff Layton <jlayton@redhat.com>