anon_inodes.c cleanups.

PATCH In review Rusty Russell 2 participants 4 messages Apr 11, 2008 – Apr 13, 2008 VFS

lore

Share & tools

lore

Rusty Russell

author

2008-04-11 14:59 UTC #0 lore raw

PATCH 1/1 anon_inodes.c cleanups.

Arnd pointed me at anon_inode_getfd(), and the code annoyed me enough to send this patch.

Mainly because the init routine carefully checks for errors, then panics (because we shouldn't run out of memory at boot). Unfortunately, it's actually worse than simply oopsing, where we'd know what had failed.

Davide, please consider parts of this patch, at least.

Cheers,
Rusty.
----
1) anon_inode_inode can be read_mostly, same as anon_inode_mnt.
2) The IS_ERR(anon_inode_inode) check is unneeded, since we panic on boot if that were true.
3) anon_inode_mkinode has one caller, so it's a little confusing.
4) Don't clean up before panic.
5) Panic gives less information than an oops would, plus is untested.

Signed-off-by Rusty Russell <[email protected]>

fs/anon_inodes.c +18 ~~-48~~

diff -r 51bdbdc4f199 fs/anon_inodes.c
@@ -22,7 +22,7 @@
 #include <asm/uaccess.h>
 
 static struct vfsmount *anon_inode_mnt __read_mostly;
-static struct inode *anon_inode_inode;
+static struct inode *anon_inode_inode __read_mostly;
 static const struct file_operations anon_inode_fops;
 
 static int anon_inodefs_get_sb(struct file_system_type *fs_type, int flags,

@@ -78,9 +78,6 @@ int anon_inode_getfd(int *pfd, struct in
 	struct dentry *dentry;
 	struct file *file;
 	int error, fd;
-
-	if (IS_ERR(anon_inode_inode))
-		return -ENODEV;
 
 	error = get_unused_fd();
 	if (error < 0)

@@ -138,19 +135,18 @@ err_put_unused_fd:
 }
 EXPORT_SYMBOL_GPL(anon_inode_getfd);
 
-/*
- * A single inode exists for all anon_inode files. Contrary to pipes,
- * anon_inode inodes have no associated per-instance data, so we need
- * only allocate one of them.
- */
-static struct inode *anon_inode_mkinode(void)
+static int __init anon_inode_init(void)
 {
-	struct inode *inode = new_inode(anon_inode_mnt->mnt_sb);
+	register_filesystem(&anon_inode_fs_type);
+	anon_inode_mnt = kern_mount(&anon_inode_fs_type);
+	anon_inode_inode = new_inode(anon_inode_mnt->mnt_sb);
 
-	if (!inode)
-		return ERR_PTR(-ENOMEM);
-
-	inode->i_fop = &anon_inode_fops;
+	/*
+	 * A single inode exists for all anon_inode files. Contrary to pipes,
+	 * anon_inode inodes have no associated per-instance data, so we need
+	 * only allocate one of them.
+	 */
+	anon_inode_inode->i_fop = &anon_inode_fops;
 
 	/*
 	 * Mark the inode dirty from the very beginning,

@@ -158,41 +154,15 @@ static struct inode *anon_inode_mkinode(
 	 * list because mark_inode_dirty() will think
 	 * that it already _is_ on the dirty list.
 	 */
-	inode->i_state = I_DIRTY;
-	inode->i_mode = S_IRUSR | S_IWUSR;
-	inode->i_uid = current->fsuid;
-	inode->i_gid = current->fsgid;
-	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
-	return inode;
-}
-
-static int __init anon_inode_init(void)
-{
-	int error;
-
-	error = register_filesystem(&anon_inode_fs_type);
-	if (error)
-		goto err_exit;
-	anon_inode_mnt = kern_mount(&anon_inode_fs_type);
-	if (IS_ERR(anon_inode_mnt)) {
-		error = PTR_ERR(anon_inode_mnt);
-		goto err_unregister_filesystem;
-	}
-	anon_inode_inode = anon_inode_mkinode();
-	if (IS_ERR(anon_inode_inode)) {
-		error = PTR_ERR(anon_inode_inode);
-		goto err_mntput;
-	}
+	anon_inode_inode->i_state = I_DIRTY;
+	anon_inode_inode->i_mode = S_IRUSR | S_IWUSR;
+	anon_inode_inode->i_uid = current->fsuid;
+	anon_inode_inode->i_gid = current->fsgid;
+	anon_inode_inode->i_atime =
+		anon_inode_inode->i_mtime =
+		anon_inode_inode->i_ctime = CURRENT_TIME;
 
 	return 0;
-
-err_mntput:
-	mntput(anon_inode_mnt);
-err_unregister_filesystem:
-	unregister_filesystem(&anon_inode_fs_type);
-err_exit:
-	panic(KERN_ERR "anon_inode_init() failed (%d)\n", error);
 }
 
 fs_initcall(anon_inode_init);
-

Davide Libenzi

2008-04-12 01:15 UTC #1 lore raw

Quoted 8 lines · Rusty Russell

Arnd pointed me at anon_inode_getfd(), and the code annoyed me enough to send this patch.
Mainly because the init routine carefully checks for errors, then panics (because we shouldn't run out of memory at boot). Unfortunately, it's actually worse than simply oopsing, where we'd know what had failed.
1) anon_inode_inode can be read_mostly, same as anon_inode_mnt.

Sure.

Quoted 1 line

3) anon_inode_mkinode has one caller, so it's a little confusing.

Hmm? The function groups the code necessary to create the anonfds inode. If every function that has one call site would be inlined, we'd have monster long functions. Functions also have the purpose to group some code that does some task, into a single unit (and the function name hopefully gives an hint about what's doing). The compiler (not that in this case really matter, since it's not even a slow-path, is a once-run path) may take care of inlining, if sees that appropriate.

Quoted 4 lines

2) The IS_ERR(anon_inode_inode) check is unneeded, since we panic on boot if that were true.
4) Don't clean up before panic.
5) Panic gives less information than an oops would, plus is untested.

I remember we changed the failure-path of anonfds a couple of times along the way, but I can't find email traces about why we did it. So, I prefer error-checked code instead of oopses, and given that the anonfds subsystem is not a required one for most of the components of the kernel/userspace, I'd rather prefer to drop the panic().
The counter reasoning may be that, as today, if anonfds code fails it's almost 100% for ENOMEM, so every other following code will likely fail as well.
I still prefer the report-error way instead of the oops generated by not checking return codes, and leave to more critical subsystems to nail the system if necessary.

Anyway, I'll let this handle with Al (cc-ed now). The ananofds interface has been changed to remove the inode** and file** parameters (noone but KVM was using them), and Al already has those changes in his vfs tree (plus fixes for KVM, I think).

- Davide

Rusty Russell

author

2008-04-12 18:26 UTC #2 lore raw

Quoted 14 lines · On Saturday 12 April 2008 11:15:26 Davide Libenzi wrote:

Quoted 8 lines · Rusty Russell
Arnd pointed me at anon_inode_getfd(), and the code annoyed me enough to send this patch.
Mainly because the init routine carefully checks for errors, then panics (because we shouldn't run out of memory at boot). Unfortunately, it's actually worse than simply oopsing, where we'd know what had failed.
1) anon_inode_inode can be read_mostly, same as anon_inode_mnt.
Sure.
Quoted 1 line
3) anon_inode_mkinode has one caller, so it's a little confusing.

Quoted 7 lines

Hmm? The function groups the code necessary to create the anonfds inode. If every function that has one call site would be inlined, we'd have monster long functions. Functions also have the purpose to group some code that does some task, into a single unit (and the function name hopefully gives an hint about what's doing). The compiler (not that in this case really matter, since it's not even a slow-path, is a once-run path) may take care of inlining, if sees that appropriate.

If you'd called it, say, "setup_anon_inode()", it would be fine. It seems overly generic unless you planned on calling it elsewhere.

Quoted 10 lines

Quoted 4 lines
2) The IS_ERR(anon_inode_inode) check is unneeded, since we panic on boot if that were true.
4) Don't clean up before panic.
5) Panic gives less information than an oops would, plus is untested.
I remember we changed the failure-path of anonfds a couple of times along the way, but I can't find email traces about why we did it. So, I prefer error-checked code instead of oopses, and given that the anonfds subsystem is not a required one for most of the components of the kernel/userspace, I'd rather prefer to drop the panic().

We've seen this debate before, and I'm firmly on the "don't turn oopses into errors on boot paths" side. I know others disagree.

Given that it should never happen, I'd argue the highest priority minimal amount of code, and second is ease of debugging if it ever did happen to someone. Oopsing has those features.

Quoted 4 lines

Anyway, I'll let this handle with Al (cc-ed now). The ananofds interface has been changed to remove the inode** and file** parameters (noone but KVM was using them), and Al already has those changes in his vfs tree (plus fixes for KVM, I think).

OK, fine.

Thanks,
Rusty.

Davide Libenzi

2008-04-13 20:38 UTC #3 lore raw

Quoted 23 lines · Rusty Russell

Quoted 14 lines · On Saturday 12 April 2008 11:15:26 Davide Libenzi wrote:
Quoted 8 lines · Rusty Russell
Arnd pointed me at anon_inode_getfd(), and the code annoyed me enough to send this patch.
Mainly because the init routine carefully checks for errors, then panics (because we shouldn't run out of memory at boot). Unfortunately, it's actually worse than simply oopsing, where we'd know what had failed.
1) anon_inode_inode can be read_mostly, same as anon_inode_mnt.
Sure.
Quoted 1 line
3) anon_inode_mkinode has one caller, so it's a little confusing.
Quoted 7 lines
Hmm? The function groups the code necessary to create the anonfds inode. If every function that has one call site would be inlined, we'd have monster long functions. Functions also have the purpose to group some code that does some task, into a single unit (and the function name hopefully gives an hint about what's doing). The compiler (not that in this case really matter, since it's not even a slow-path, is a once-run path) may take care of inlining, if sees that appropriate.

Quoted 2 lines

If you'd called it, say, "setup_anon_inode()", it would be fine. It seems overly generic unless you planned on calling it elsewhere.

That's fine with me. I'll wait for Al's tree to get merged and I'll change the name (and the read_mostly bits).

Quoted 11 lines

Quoted 10 lines
Quoted 4 lines
2) The IS_ERR(anon_inode_inode) check is unneeded, since we panic on boot if that were true.
4) Don't clean up before panic.
5) Panic gives less information than an oops would, plus is untested.
I remember we changed the failure-path of anonfds a couple of times along the way, but I can't find email traces about why we did it. So, I prefer error-checked code instead of oopses, and given that the anonfds subsystem is not a required one for most of the components of the kernel/userspace, I'd rather prefer to drop the panic().

Quoted 6 lines

We've seen this debate before, and I'm firmly on the "don't turn oopses into errors on boot paths" side. I know others disagree.
Given that it should never happen, I'd argue the highest priority minimal amount of code, and second is ease of debugging if it ever did happen to someone. Oopsing has those features.

This, I'd prefer to have the bounce error back to do_initcalls() instead of nailing the system (given the non criticality of the anonfds subsystem).

- Davide