[linux-cifs-client,2/3] cifs: add new cifs_iget_unix_basic function
diff mbox

Message ID 1238936961-6979-3-git-send-email-jlayton@redhat.com
State New, archived
Headers show

Commit Message

Jeff Layton April 5, 2009, 1:09 p.m. UTC
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(-)

Comments

Christoph Hellwig April 8, 2009, 5:05 p.m. UTC | #1
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.
Jeff Layton April 10, 2009, 1:18 p.m. UTC | #2
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.

Patch
diff mbox

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));