[PATCH] Unionfs: stop using iget() and read_inode()

[PATCH] Unionfs: stop using iget() and read_inode()

[Erez Zadok]

From: David Howells <dhowells@redhat.com>

Stop UnionFS from using iget() and read_inode().  Replace
unionfs_read_inode() with unionfs_iget(), and call that instead of iget().
unionfs_iget() then uses iget_locked() directly and returns a proper error
code instead of an inode in the event of an error.

unionfs_fill_super() returns any error incurred when getting the root inode
instead of EINVAL.

Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

diff --git a/fs/unionfs/main.c b/fs/unionfs/main.c
index ffb0da1..89e3b31 100644
--- a/fs/unionfs/main.c
+++ b/fs/unionfs/main.c
@@ -104,9 +104,8 @@ struct dentry *unionfs_interpose(struct dentry *dentry, struct super_block *sb,
 	BUG_ON(is_negative_dentry);
 
 	/*
-	 * We allocate our new inode below, by calling iget.
-	 * iget will call our read_inode which will initialize some
-	 * of the new inode's fields
+	 * We allocate our new inode below by calling unionfs_iget,
+	 * which will initialize some of the new inode's fields
 	 */
 
 	/*
@@ -128,9 +127,9 @@ struct dentry *unionfs_interpose(struct dentry *dentry, struct super_block *sb,
 		}
 	} else {
 		/* get unique inode number for unionfs */
-		inode = iget(sb, iunique(sb, UNIONFS_ROOT_INO));
-		if (!inode) {
-			err = -EACCES;
+		inode = unionfs_iget(sb, iunique(sb, UNIONFS_ROOT_INO));
+		if (IS_ERR(inode)) {
+			err = PTR_ERR(inode);
 			goto out;
 		}
 		if (atomic_read(&inode->i_count) > 1)
diff --git a/fs/unionfs/super.c b/fs/unionfs/super.c
index 7d28045..18e506b 100644
--- a/fs/unionfs/super.c
+++ b/fs/unionfs/super.c
@@ -24,13 +24,21 @@
  */
 static struct kmem_cache *unionfs_inode_cachep;
 
-static void unionfs_read_inode(struct inode *inode)
+struct inode *unionfs_iget(struct super_block *sb, unsigned long ino)
 {
 	int size;
-	struct unionfs_inode_info *info = UNIONFS_I(inode);
+	struct unionfs_inode_info *info;
+	struct inode *inode;
 
-	unionfs_read_lock(inode->i_sb);
+	inode = iget_locked(sb, ino);
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+	if (!(inode->i_state & I_NEW))
+		return inode;
 
+	unionfs_read_lock(sb);
+
+	info = UNIONFS_I(inode);
 	memset(info, 0, offsetof(struct unionfs_inode_info, vfs_inode));
 	info->bstart = -1;
 	info->bend = -1;
@@ -46,7 +54,9 @@ static void unionfs_read_inode(struct inode *inode)
 	if (unlikely(!info->lower_inodes)) {
 		printk(KERN_CRIT "unionfs: no kernel memory when allocating "
 		       "lower-pointer array!\n");
-		BUG();
+		iget_failed(inode);
+		unionfs_read_unlock(sb);
+		return ERR_PTR(-ENOMEM);
 	}
 
 	inode->i_version++;
@@ -55,7 +65,9 @@ static void unionfs_read_inode(struct inode *inode)
 
 	inode->i_mapping->a_ops = &unionfs_aops;
 
-	unionfs_read_unlock(inode->i_sb);
+	unlock_new_inode(inode);
+	unionfs_read_unlock(sb);
+	return inode;
 }
 
 /*
@@ -1002,7 +1014,6 @@ out:
 }
 
 struct super_operations unionfs_sops = {
-	.read_inode	= unionfs_read_inode,
 	.delete_inode	= unionfs_delete_inode,
 	.put_super	= unionfs_put_super,
 	.statfs		= unionfs_statfs,
diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h
index 9b530ec..f5afae0 100644
--- a/fs/unionfs/union.h
+++ b/fs/unionfs/union.h
@@ -356,6 +356,7 @@ extern int unionfs_fsync(struct file *file, struct dentry *dentry,
 extern int unionfs_fasync(int fd, struct file *file, int flag);
 
 /* Inode operations */
+extern struct inode *unionfs_iget(struct super_block *sb, unsigned long ino);
 extern int unionfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			  struct inode *new_dir, struct dentry *new_dentry);
 extern int unionfs_unlink(struct inode *dir, struct dentry *dentry);

Re: [PATCH] Unionfs: stop using iget() and read_inode()

[Erez Zadok]

Andrew, the following small patch is critical to have after:

iget-stop-unionfs-from-using-iget-and-read_inode.patch

Question: since the above patch isn't in my unionfs.git tree (not until the
whole iget series makes it to Linus), then do you prefer if I give you a
replacement patch to the above patch, or a small incremental one; either
way, you can choose to fold the above and below patches into one.

Thanks,
Erez.


Unionfs: bugfix to initialize unionfs_inode_info node

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

diff --git a/fs/unionfs/super.c b/fs/unionfs/super.c
index 7408b0f..d6544f8 100644
--- a/fs/unionfs/super.c
+++ b/fs/unionfs/super.c
@@ -38,6 +38,7 @@ struct inode *unionfs_iget(struct super_block *sb, unsigned long ino)
 
  	unionfs_read_lock(sb);
 
+	info = UNIONFS_I(inode);
 	memset(info, 0, offsetof(struct unionfs_inode_info, vfs_inode));
 	info->bstart = -1;
 	info->bend = -1;

Re: [PATCH] Unionfs: stop using iget() and read_inode()

[Andrew Morton]

On Sun, 11 Nov 2007 13:38:04 -0500 Erez Zadok <ezk@cs.sunysb.edu> wrote:

> Andrew, the following small patch is critical to have after:
> 
> iget-stop-unionfs-from-using-iget-and-read_inode.patch

Thanks

> Question: since the above patch isn't in my unionfs.git tree (not until the
> whole iget series makes it to Linus), then do you prefer if I give you a
> replacement patch to the above patch, or a small incremental one; either
> way, you can choose to fold the above and below patches into one.

Incrementals are usually preferred.  But it's usually more convenient for the
originator to generate a new patch, and I will almost always turn that into
an incremental so that I can review what was changed.  The downside to this
is that nobody else will be able to see what was changed.

So there are pros and cons and I'm not particularly fussed either way,
really.  But if a patch is large and has already had some testing and
review and has been in -mm for a while then that's when wholesale
replacement becomes problematic: it pretty much sets us back to square one.