* Re: [PATCH 2/3] ensure unique i_ino in filesystems without permanent
[not found] <20061210175652.7537.qmail@science.horizon.com>
@ 2006-12-12 16:46 ` Jeff Layton
2006-12-12 18:02 ` linux
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Layton @ 2006-12-12 16:46 UTC (permalink / raw)
To: linux; +Cc: linux-kernel, linux-fsdevel
linux@horizon.com wrote:
> I'm very fond of <stdbool.h>, since it explicitly documents that there
> are exactly two options. It also allows the compiler a few minor
> opportunities for optimization, but that's not as big a deal.
>
> static int __simple_fill_super(struct super_block *s, int magic,
> struct tree_descr *files, bool sequential)
>
> Although "true" and "false" aren't the most meaningful #defines either,
> at least they indicate that it's one of two choices, and there is no
> option "2" to sorry about.
>
> Given your wrappr function names, "bool registered" is another option.
>
> You might want to make simple_fill_super() and registered_fill_super()
> inline functions or #defines rather than making them separate functions.
> Or is there a particular need for a function pointer? The code size
> is negligible, and it saves stack space.
>
Good catch on the inlining. I had meant to do that and missed it. The point
about the naming of the flag is a good one too. How about this patch? I've
tested it to see that it builds, but don't have good place to test this one
to see that it works. A similar patch did work on a 2.6.18 derivative kernel.
Signed-off-by: Jeff Layton <jlayton@redhat.com>
--- linux-2.6/fs/debugfs/inode.c.super 2006-12-12 08:46:46.000000000 -0500
+++ linux-2.6/fs/debugfs/inode.c 2006-12-12 08:54:14.000000000 -0500
@@ -107,7 +107,7 @@
{
static struct tree_descr debug_files[] = {{""}};
- return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
+ return registered_fill_super(sb, DEBUGFS_MAGIC, debug_files);
}
static int debug_get_sb(struct file_system_type *fs_type,
--- linux-2.6/fs/fuse/control.c.super 2006-12-12 08:46:46.000000000 -0500
+++ linux-2.6/fs/fuse/control.c 2006-12-12 08:54:14.000000000 -0500
@@ -163,7 +163,7 @@
struct fuse_conn *fc;
int err;
- err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
+ err = registered_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
if (err)
return err;
--- linux-2.6/fs/libfs.c.super 2006-12-12 08:46:46.000000000 -0500
+++ linux-2.6/fs/libfs.c 2006-12-12 11:31:20.000000000 -0500
@@ -215,7 +215,7 @@
s->s_op = ops ? ops : &default_ops;
s->s_time_gran = 1;
root = new_inode(s);
- if (!root)
+ if (!root || iunique_register(root, 0))
goto Enomem;
root->i_mode = S_IFDIR | S_IRUSR | S_IWUSR;
root->i_uid = root->i_gid = 0;
@@ -356,7 +356,8 @@
return 0;
}
-int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
+static int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, bool registered)
{
static struct super_operations s_ops = {.statfs = simple_statfs};
struct inode *inode;
@@ -380,6 +381,12 @@
inode->i_op = &simple_dir_inode_operations;
inode->i_fop = &simple_dir_operations;
inode->i_nlink = 2;
+ /*
+ * set this as high as a 32 bit val as possible to avoid collisions.
+ * This is also well above the highest value that iunique_register
+ * will assign to an inode
+ */
+ inode->i_ino = 0xffffffff;
root = d_alloc_root(inode);
if (!root) {
iput(inode);
@@ -394,12 +401,15 @@
inode = new_inode(s);
if (!inode)
goto out;
+ if (!registered)
+ inode->i_ino = i;
+ else if (iunique_register(inode, 0))
+ goto out;
inode->i_mode = S_IFREG | files->mode;
inode->i_uid = inode->i_gid = 0;
inode->i_blocks = 0;
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
inode->i_fop = files->ops;
- inode->i_ino = i;
d_add(dentry, inode);
}
s->s_root = root;
@@ -410,6 +420,30 @@
return -ENOMEM;
}
+/*
+ * Fill a superblock with a standard set of fields, and add the entries in the
+ * "files" struct. Assign i_ino values to the files sequentially. This function
+ * is appropriate for filesystems that need a particular i_ino value assigned
+ * to a particular "files" entry.
+ */
+inline int simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, false);
+}
+
+/*
+ * Just like simple_fill_super, but does an iunique_register on the inodes
+ * created for "files" entries. This function is appropriate when you don't
+ * need a particular i_ino value assigned to each files entry, and when the
+ * filesystem will have other registered inodes.
+ */
+inline int registered_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, true);
+}
+
static DEFINE_SPINLOCK(pin_fs_lock);
int simple_pin_fs(struct file_system_type *type, struct vfsmount **mount, int *count)
@@ -619,6 +653,7 @@
EXPORT_SYMBOL(simple_empty);
EXPORT_SYMBOL(d_alloc_name);
EXPORT_SYMBOL(simple_fill_super);
+EXPORT_SYMBOL(registered_fill_super);
EXPORT_SYMBOL(simple_getattr);
EXPORT_SYMBOL(simple_link);
EXPORT_SYMBOL(simple_lookup);
--- linux-2.6/include/linux/fs.h.super 2006-12-12 08:53:34.000000000 -0500
+++ linux-2.6/include/linux/fs.h 2006-12-12 08:54:14.000000000 -0500
@@ -1879,7 +1879,10 @@
extern struct inode_operations simple_dir_inode_operations;
struct tree_descr { char *name; const struct file_operations *ops; int mode; };
struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
+extern int simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files);
+extern int registered_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files);
extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
extern void simple_release_fs(struct vfsmount **mount, int *count);
--- linux-2.6/security/inode.c.super 2006-12-12 08:46:47.000000000 -0500
+++ linux-2.6/security/inode.c 2006-12-12 08:54:14.000000000 -0500
@@ -130,7 +130,7 @@
{
static struct tree_descr files[] = {{""}};
- return simple_fill_super(sb, SECURITYFS_MAGIC, files);
+ return registered_fill_super(sb, SECURITYFS_MAGIC, files);
}
static int get_sb(struct file_system_type *fs_type,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] ensure unique i_ino in filesystems without permanent
2006-12-12 16:46 ` Jeff Layton
@ 2006-12-12 18:02 ` linux
2006-12-12 19:19 ` Jeff Layton
0 siblings, 1 reply; 7+ messages in thread
From: linux @ 2006-12-12 18:02 UTC (permalink / raw)
To: jlayton, linux; +Cc: linux-fsdevel, linux-kernel
> Good catch on the inlining. I had meant to do that and missed it.
Er... if you want it to *be* inlined, you have to put it into the .h
file so the compiler knows about it at the call site. "static inline"
tells gcc not avoid emitting a callable version.
Something like this the following. (You'll also need to add
a "#include <stdbool.h>", unless you expand the "bool", "false" and
"true" macros to their values "_Bool", "0" and "1" by hand.)
--- linux-2.6/include/linux/fs.h.super 2006-12-12 08:53:34.000000000 -0500
+++ linux-2.6/include/linux/fs.h 2006-12-12 08:54:14.000000000 -0500
@@ -1879,7 +1879,32 @@
extern struct inode_operations simple_dir_inode_operations;
struct tree_descr { char *name; const struct file_operations *ops; int mode; };
struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
+extern int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, bool registered);
extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
extern void simple_release_fs(struct vfsmount **mount, int *count);
+/*
+ * Fill a superblock with a standard set of fields, and add the entries in the
+ * "files" struct. Assign i_ino values to the files sequentially. This function
+ * is appropriate for filesystems that need a particular i_ino value assigned
+ * to a particular "files" entry.
+ */
+static inline int simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, false);
+}
+
+/*
+ * Just like simple_fill_super, but does an iunique_register on the inodes
+ * created for "files" entries. This function is appropriate when you don't
+ * need a particular i_ino value assigned to each files entry, and when the
+ * filesystem will have other registered inodes.
+ */
+static inline int registered_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, true);
+}
+
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] ensure unique i_ino in filesystems without permanent
2006-12-12 18:02 ` linux
@ 2006-12-12 19:19 ` Jeff Layton
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Layton @ 2006-12-12 19:19 UTC (permalink / raw)
To: linux; +Cc: linux-fsdevel, linux-kernel
linux@horizon.com wrote:
>> Good catch on the inlining. I had meant to do that and missed it.
>
> Er... if you want it to *be* inlined, you have to put it into the .h
> file so the compiler knows about it at the call site. "static inline"
> tells gcc not avoid emitting a callable version.
>
> Something like this the following. (You'll also need to add
> a "#include <stdbool.h>", unless you expand the "bool", "false" and
> "true" macros to their values "_Bool", "0" and "1" by hand.)
>
Doh! Thanks for explaining that. Here's a respun patch with your suggestion
incorporated. Seems to build correctly without stdbool.h. In fact, I don't see
a stdbool.h in Linus' tree as of this morning. Are you sure that it's needed?
Signed-off-by: Jeff Layton <jlayton@redhat.com>
--- linux-2.6/fs/debugfs/inode.c.super
+++ linux-2.6/fs/debugfs/inode.c
@@ -107,7 +107,7 @@ static int debug_fill_super(struct super
{
static struct tree_descr debug_files[] = {{""}};
- return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
+ return registered_fill_super(sb, DEBUGFS_MAGIC, debug_files);
}
static int debug_get_sb(struct file_system_type *fs_type,
--- linux-2.6/fs/fuse/control.c.super
+++ linux-2.6/fs/fuse/control.c
@@ -163,7 +163,7 @@ static int fuse_ctl_fill_super(struct su
struct fuse_conn *fc;
int err;
- err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
+ err = registered_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
if (err)
return err;
--- linux-2.6/fs/libfs.c.super
+++ linux-2.6/fs/libfs.c
@@ -215,7 +215,7 @@ int get_sb_pseudo(struct file_system_typ
s->s_op = ops ? ops : &default_ops;
s->s_time_gran = 1;
root = new_inode(s);
- if (!root)
+ if (!root || iunique_register(root, 0))
goto Enomem;
root->i_mode = S_IFDIR | S_IRUSR | S_IWUSR;
root->i_uid = root->i_gid = 0;
@@ -356,7 +356,8 @@ int simple_commit_write(struct file *fil
return 0;
}
-int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
+int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, bool registered)
{
static struct super_operations s_ops = {.statfs = simple_statfs};
struct inode *inode;
@@ -380,6 +381,12 @@ int simple_fill_super(struct super_block
inode->i_op = &simple_dir_inode_operations;
inode->i_fop = &simple_dir_operations;
inode->i_nlink = 2;
+ /*
+ * set this as high as a 32 bit val as possible to avoid collisions.
+ * This is also well above the highest value that iunique_register
+ * will assign to an inode
+ */
+ inode->i_ino = 0xffffffff;
root = d_alloc_root(inode);
if (!root) {
iput(inode);
@@ -394,12 +401,15 @@ int simple_fill_super(struct super_block
inode = new_inode(s);
if (!inode)
goto out;
+ if (!registered)
+ inode->i_ino = i;
+ else if (iunique_register(inode, 0))
+ goto out;
inode->i_mode = S_IFREG | files->mode;
inode->i_uid = inode->i_gid = 0;
inode->i_blocks = 0;
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
inode->i_fop = files->ops;
- inode->i_ino = i;
d_add(dentry, inode);
}
s->s_root = root;
--- linux-2.6/include/linux/fs.h.super
+++ linux-2.6/include/linux/fs.h
@@ -1879,10 +1879,35 @@ extern const struct file_operations simp
extern struct inode_operations simple_dir_inode_operations;
struct tree_descr { char *name; const struct file_operations *ops; int mode; };
struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
+extern int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, bool registered);
extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
extern void simple_release_fs(struct vfsmount **mount, int *count);
+/*
+ * Fill a superblock with a standard set of fields, and add the entries in the
+ * "files" struct. Assign i_ino values to the files sequentially. This function
+ * is appropriate for filesystems that need a particular i_ino value assigned
+ * to a particular "files" entry.
+ */
+static inline int simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, false);
+}
+
+/*
+ * Just like simple_fill_super, but does an iunique_register on the inodes
+ * created for "files" entries. This function is appropriate when you don't
+ * need a particular i_ino value assigned to each files entry, and when the
+ * filesystem will have other registered inodes.
+ */
+static inline int registered_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, true);
+}
+
extern ssize_t simple_read_from_buffer(void __user *, size_t, loff_t *, const void *, size_t);
#ifdef CONFIG_MIGRATION
--- linux-2.6/security/inode.c.super
+++ linux-2.6/security/inode.c
@@ -130,7 +130,7 @@ static int fill_super(struct super_block
{
static struct tree_descr files[] = {{""}};
- return simple_fill_super(sb, SECURITYFS_MAGIC, files);
+ return registered_fill_super(sb, SECURITYFS_MAGIC, files);
}
static int get_sb(struct file_system_type *fs_type,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] ensure unique i_ino in filesystems without permanent
[not found] <20061212194708.8359.qmail@science.horizon.com>
@ 2006-12-12 20:06 ` Jeff Layton
2006-12-12 20:36 ` Peter Staubach
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Layton @ 2006-12-12 20:06 UTC (permalink / raw)
To: linux; +Cc: linux-fsdevel, linux-kernel
linux@horizon.com wrote:
>> Doh! Thanks for explaining that. Here's a respun patch with your suggestion
>> incorporated. Seems to build correctly without stdbool.h. In fact, I don't see
>> a stdbool.h in Linus' tree as of this morning. Are you sure that it's needed?
>
> include/linux/types.h:36:typedef _Bool bool;
>
> It's already in there. And <linux/stddef.h> has false and true.
>
> Sorry; I write more user-level code.
>
> The'd be built-in, except that C99 didn't want to add a kewords that
> might conflict with existing code.
Ok, that makes sense. Though I just found a bug that I missed. I forgot to change the
EXPORT_SYMBOL in libfs.c. This should fix it:
Signed-off-by: Jeff Layton <jlayton@redhat.com>
--- linux-2.6/fs/debugfs/inode.c.super
+++ linux-2.6/fs/debugfs/inode.c
@@ -107,7 +107,7 @@ static int debug_fill_super(struct super
{
static struct tree_descr debug_files[] = {{""}};
- return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
+ return registered_fill_super(sb, DEBUGFS_MAGIC, debug_files);
}
static int debug_get_sb(struct file_system_type *fs_type,
--- linux-2.6/fs/fuse/control.c.super
+++ linux-2.6/fs/fuse/control.c
@@ -163,7 +163,7 @@ static int fuse_ctl_fill_super(struct su
struct fuse_conn *fc;
int err;
- err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
+ err = registered_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
if (err)
return err;
--- linux-2.6/fs/libfs.c.super
+++ linux-2.6/fs/libfs.c
@@ -215,7 +215,7 @@ int get_sb_pseudo(struct file_system_typ
s->s_op = ops ? ops : &default_ops;
s->s_time_gran = 1;
root = new_inode(s);
- if (!root)
+ if (!root || iunique_register(root, 0))
goto Enomem;
root->i_mode = S_IFDIR | S_IRUSR | S_IWUSR;
root->i_uid = root->i_gid = 0;
@@ -356,7 +356,8 @@ int simple_commit_write(struct file *fil
return 0;
}
-int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
+int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, bool registered)
{
static struct super_operations s_ops = {.statfs = simple_statfs};
struct inode *inode;
@@ -380,6 +381,12 @@ int simple_fill_super(struct super_block
inode->i_op = &simple_dir_inode_operations;
inode->i_fop = &simple_dir_operations;
inode->i_nlink = 2;
+ /*
+ * set this as high as a 32 bit val as possible to avoid collisions.
+ * This is also well above the highest value that iunique_register
+ * will assign to an inode
+ */
+ inode->i_ino = 0xffffffff;
root = d_alloc_root(inode);
if (!root) {
iput(inode);
@@ -394,12 +401,15 @@ int simple_fill_super(struct super_block
inode = new_inode(s);
if (!inode)
goto out;
+ if (!registered)
+ inode->i_ino = i;
+ else if (iunique_register(inode, 0))
+ goto out;
inode->i_mode = S_IFREG | files->mode;
inode->i_uid = inode->i_gid = 0;
inode->i_blocks = 0;
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
inode->i_fop = files->ops;
- inode->i_ino = i;
d_add(dentry, inode);
}
s->s_root = root;
@@ -618,7 +628,7 @@ EXPORT_SYMBOL(simple_dir_inode_operation
EXPORT_SYMBOL(simple_dir_operations);
EXPORT_SYMBOL(simple_empty);
EXPORT_SYMBOL(d_alloc_name);
-EXPORT_SYMBOL(simple_fill_super);
+EXPORT_SYMBOL(__simple_fill_super);
EXPORT_SYMBOL(simple_getattr);
EXPORT_SYMBOL(simple_link);
EXPORT_SYMBOL(simple_lookup);
--- linux-2.6/include/linux/fs.h.super
+++ linux-2.6/include/linux/fs.h
@@ -1879,10 +1879,35 @@ extern const struct file_operations simp
extern struct inode_operations simple_dir_inode_operations;
struct tree_descr { char *name; const struct file_operations *ops; int mode; };
struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
+extern int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, bool registered);
extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
extern void simple_release_fs(struct vfsmount **mount, int *count);
+/*
+ * Fill a superblock with a standard set of fields, and add the entries in the
+ * "files" struct. Assign i_ino values to the files sequentially. This function
+ * is appropriate for filesystems that need a particular i_ino value assigned
+ * to a particular "files" entry.
+ */
+static inline int simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, false);
+}
+
+/*
+ * Just like simple_fill_super, but does an iunique_register on the inodes
+ * created for "files" entries. This function is appropriate when you don't
+ * need a particular i_ino value assigned to each files entry, and when the
+ * filesystem will have other registered inodes.
+ */
+static inline int registered_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, true);
+}
+
extern ssize_t simple_read_from_buffer(void __user *, size_t, loff_t *, const void *, size_t);
#ifdef CONFIG_MIGRATION
--- linux-2.6/security/inode.c.super
+++ linux-2.6/security/inode.c
@@ -130,7 +130,7 @@ static int fill_super(struct super_block
{
static struct tree_descr files[] = {{""}};
- return simple_fill_super(sb, SECURITYFS_MAGIC, files);
+ return registered_fill_super(sb, SECURITYFS_MAGIC, files);
}
static int get_sb(struct file_system_type *fs_type,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] ensure unique i_ino in filesystems without permanent
2006-12-12 20:06 ` [PATCH 2/3] ensure unique i_ino in filesystems without permanent Jeff Layton
@ 2006-12-12 20:36 ` Peter Staubach
2006-12-12 22:45 ` Jeff Layton
0 siblings, 1 reply; 7+ messages in thread
From: Peter Staubach @ 2006-12-12 20:36 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux, linux-fsdevel, linux-kernel
Jeff Layton wrote:
> linux@horizon.com wrote:
> >> Doh! Thanks for explaining that. Here's a respun patch with your
> suggestion
> >> incorporated. Seems to build correctly without stdbool.h. In fact,
> I don't see
> >> a stdbool.h in Linus' tree as of this morning. Are you sure that
> it's needed?
> >
> > include/linux/types.h:36:typedef _Bool bool;
> >
> > It's already in there. And <linux/stddef.h> has false and true.
> >
> > Sorry; I write more user-level code.
> >
> > The'd be built-in, except that C99 didn't want to add a kewords that
> > might conflict with existing code.
>
> Ok, that makes sense. Though I just found a bug that I missed. I
> forgot to change the
> EXPORT_SYMBOL in libfs.c. This should fix it:
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>
> --- linux-2.6/fs/debugfs/inode.c.super
> +++ linux-2.6/fs/debugfs/inode.c
> @@ -107,7 +107,7 @@ static int debug_fill_super(struct super
> {
> static struct tree_descr debug_files[] = {{""}};
>
> - return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
> + return registered_fill_super(sb, DEBUGFS_MAGIC, debug_files);
> }
>
> static int debug_get_sb(struct file_system_type *fs_type,
> --- linux-2.6/fs/fuse/control.c.super
> +++ linux-2.6/fs/fuse/control.c
> @@ -163,7 +163,7 @@ static int fuse_ctl_fill_super(struct su
> struct fuse_conn *fc;
> int err;
>
> - err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
> + err = registered_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
> if (err)
> return err;
>
> --- linux-2.6/fs/libfs.c.super
> +++ linux-2.6/fs/libfs.c
> @@ -215,7 +215,7 @@ int get_sb_pseudo(struct file_system_typ
> s->s_op = ops ? ops : &default_ops;
> s->s_time_gran = 1;
> root = new_inode(s);
> - if (!root)
> + if (!root || iunique_register(root, 0))
> goto Enomem;
> root->i_mode = S_IFDIR | S_IRUSR | S_IWUSR;
> root->i_uid = root->i_gid = 0;
> @@ -356,7 +356,8 @@ int simple_commit_write(struct file *fil
> return 0;
> }
>
If iunique_register() fails, does not this create a memory leak
because root will need to get iput()'d?
> -int simple_fill_super(struct super_block *s, int magic, struct
> tree_descr *files)
> +int __simple_fill_super(struct super_block *s, int magic,
> + struct tree_descr *files, bool registered)
> {
> static struct super_operations s_ops = {.statfs = simple_statfs};
> struct inode *inode;
> @@ -380,6 +381,12 @@ int simple_fill_super(struct super_block
> inode->i_op = &simple_dir_inode_operations;
> inode->i_fop = &simple_dir_operations;
> inode->i_nlink = 2;
> + /*
> + * set this as high as a 32 bit val as possible to avoid collisions.
> + * This is also well above the highest value that iunique_register
> + * will assign to an inode
> + */
> + inode->i_ino = 0xffffffff;
> root = d_alloc_root(inode);
> if (!root) {
> iput(inode);
> @@ -394,12 +401,15 @@ int simple_fill_super(struct super_block
> inode = new_inode(s);
> if (!inode)
> goto out;
> + if (!registered)
> + inode->i_ino = i;
> + else if (iunique_register(inode, 0))
> + goto out;
Does this not create a memory leak as well?
Thanx...
ps
> inode->i_mode = S_IFREG | files->mode;
> inode->i_uid = inode->i_gid = 0;
> inode->i_blocks = 0;
> inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> inode->i_fop = files->ops;
> - inode->i_ino = i;
> d_add(dentry, inode);
> }
> s->s_root = root;
> @@ -618,7 +628,7 @@ EXPORT_SYMBOL(simple_dir_inode_operation
> EXPORT_SYMBOL(simple_dir_operations);
> EXPORT_SYMBOL(simple_empty);
> EXPORT_SYMBOL(d_alloc_name);
> -EXPORT_SYMBOL(simple_fill_super);
> +EXPORT_SYMBOL(__simple_fill_super);
> EXPORT_SYMBOL(simple_getattr);
> EXPORT_SYMBOL(simple_link);
> EXPORT_SYMBOL(simple_lookup);
> --- linux-2.6/include/linux/fs.h.super
> +++ linux-2.6/include/linux/fs.h
> @@ -1879,10 +1879,35 @@ extern const struct file_operations simp
> extern struct inode_operations simple_dir_inode_operations;
> struct tree_descr { char *name; const struct file_operations *ops;
> int mode; };
> struct dentry *d_alloc_name(struct dentry *, const char *);
> -extern int simple_fill_super(struct super_block *, int, struct
> tree_descr *);
> +extern int __simple_fill_super(struct super_block *s, int magic,
> + struct tree_descr *files, bool registered);
> extern int simple_pin_fs(struct file_system_type *, struct vfsmount
> **mount, int *count);
> extern void simple_release_fs(struct vfsmount **mount, int *count);
>
> +/*
> + * Fill a superblock with a standard set of fields, and add the
> entries in the
> + * "files" struct. Assign i_ino values to the files sequentially.
> This function
> + * is appropriate for filesystems that need a particular i_ino value
> assigned
> + * to a particular "files" entry.
> + */
> +static inline int simple_fill_super(struct super_block *s, int magic,
> + struct tree_descr *files)
> +{
> + return __simple_fill_super(s, magic, files, false);
> +}
> +
> +/*
> + * Just like simple_fill_super, but does an iunique_register on the
> inodes
> + * created for "files" entries. This function is appropriate when you
> don't
> + * need a particular i_ino value assigned to each files entry, and
> when the
> + * filesystem will have other registered inodes.
> + */
> +static inline int registered_fill_super(struct super_block *s, int
> magic,
> + struct tree_descr *files)
> +{
> + return __simple_fill_super(s, magic, files, true);
> +}
> +
> extern ssize_t simple_read_from_buffer(void __user *, size_t, loff_t
> *, const void *, size_t);
>
> #ifdef CONFIG_MIGRATION
> --- linux-2.6/security/inode.c.super
> +++ linux-2.6/security/inode.c
> @@ -130,7 +130,7 @@ static int fill_super(struct super_block
> {
> static struct tree_descr files[] = {{""}};
>
> - return simple_fill_super(sb, SECURITYFS_MAGIC, files);
> + return registered_fill_super(sb, SECURITYFS_MAGIC, files);
> }
>
> static int get_sb(struct file_system_type *fs_type,
> -
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] ensure unique i_ino in filesystems without permanent
2006-12-12 20:36 ` Peter Staubach
@ 2006-12-12 22:45 ` Jeff Layton
2006-12-12 22:50 ` Jeff Layton
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Layton @ 2006-12-12 22:45 UTC (permalink / raw)
To: Peter Staubach; +Cc: linux-fsdevel, linux-kernel
Peter Staubach wrote:
>
> If iunique_register() fails, does not this create a memory leak
> because root will need to get iput()'d?
>
Good point, and now that we have a wrapper for new_inode that handles this
error transparently, both places are easy to fix. This patch should do it:
Signed-off-by: Jeff Layton <jlayton@redhat.com>
--- linux-2.6/fs/debugfs/inode.c.super
+++ linux-2.6/fs/debugfs/inode.c
@@ -107,7 +107,7 @@ static int debug_fill_super(struct super
{
static struct tree_descr debug_files[] = {{""}};
- return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
+ return registered_fill_super(sb, DEBUGFS_MAGIC, debug_files);
}
static int debug_get_sb(struct file_system_type *fs_type,
--- linux-2.6/fs/fuse/control.c.super
+++ linux-2.6/fs/fuse/control.c
@@ -163,7 +163,7 @@ static int fuse_ctl_fill_super(struct su
struct fuse_conn *fc;
int err;
- err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
+ err = registered_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
if (err)
return err;
--- linux-2.6/fs/libfs.c.super
+++ linux-2.6/fs/libfs.c
@@ -214,7 +214,7 @@ int get_sb_pseudo(struct file_system_typ
s->s_magic = magic;
s->s_op = ops ? ops : &default_ops;
s->s_time_gran = 1;
- root = new_inode(s);
+ root = new_registered_inode(s, 0);
if (!root)
goto Enomem;
root->i_mode = S_IFDIR | S_IRUSR | S_IWUSR;
@@ -356,7 +356,8 @@ int simple_commit_write(struct file *fil
return 0;
}
-int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
+int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, bool registered)
{
static struct super_operations s_ops = {.statfs = simple_statfs};
struct inode *inode;
@@ -380,6 +381,12 @@ int simple_fill_super(struct super_block
inode->i_op = &simple_dir_inode_operations;
inode->i_fop = &simple_dir_operations;
inode->i_nlink = 2;
+ /*
+ * set this as high as a 32 bit val as possible to avoid collisions.
+ * This is also well above the highest value that iunique_register
+ * will assign to an inode
+ */
+ inode->i_ino = 0xffffffff;
root = d_alloc_root(inode);
if (!root) {
iput(inode);
@@ -391,7 +398,12 @@ int simple_fill_super(struct super_block
dentry = d_alloc_name(root, files->name);
if (!dentry)
goto out;
- inode = new_inode(s);
+ if (registered)
+ inode = new_registered_inode(s, 0);
+ else {
+ inode = new_inode(s);
+ inode->i_ino = i;
+ }
if (!inode)
goto out;
inode->i_mode = S_IFREG | files->mode;
@@ -399,7 +411,6 @@ int simple_fill_super(struct super_block
inode->i_blocks = 0;
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
inode->i_fop = files->ops;
- inode->i_ino = i;
d_add(dentry, inode);
}
s->s_root = root;
@@ -618,7 +629,7 @@ EXPORT_SYMBOL(simple_dir_inode_operation
EXPORT_SYMBOL(simple_dir_operations);
EXPORT_SYMBOL(simple_empty);
EXPORT_SYMBOL(d_alloc_name);
-EXPORT_SYMBOL(simple_fill_super);
+EXPORT_SYMBOL(__simple_fill_super);
EXPORT_SYMBOL(simple_getattr);
EXPORT_SYMBOL(simple_link);
EXPORT_SYMBOL(simple_lookup);
--- linux-2.6/include/linux/fs.h.super
+++ linux-2.6/include/linux/fs.h
@@ -1879,10 +1879,35 @@ extern const struct file_operations simp
extern struct inode_operations simple_dir_inode_operations;
struct tree_descr { char *name; const struct file_operations *ops; int mode; };
struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
+extern int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, bool registered);
extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
extern void simple_release_fs(struct vfsmount **mount, int *count);
+/*
+ * Fill a superblock with a standard set of fields, and add the entries in the
+ * "files" struct. Assign i_ino values to the files sequentially. This function
+ * is appropriate for filesystems that need a particular i_ino value assigned
+ * to a particular "files" entry.
+ */
+static inline int simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, false);
+}
+
+/*
+ * Just like simple_fill_super, but does an iunique_register on the inodes
+ * created for "files" entries. This function is appropriate when you don't
+ * need a particular i_ino value assigned to each files entry, and when the
+ * filesystem will have other registered inodes.
+ */
+static inline int registered_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, true);
+}
+
extern ssize_t simple_read_from_buffer(void __user *, size_t, loff_t *, const void *, size_t);
#ifdef CONFIG_MIGRATION
--- linux-2.6/security/inode.c.super
+++ linux-2.6/security/inode.c
@@ -130,7 +130,7 @@ static int fill_super(struct super_block
{
static struct tree_descr files[] = {{""}};
- return simple_fill_super(sb, SECURITYFS_MAGIC, files);
+ return registered_fill_super(sb, SECURITYFS_MAGIC, files);
}
static int get_sb(struct file_system_type *fs_type,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] ensure unique i_ino in filesystems without permanent
2006-12-12 22:45 ` Jeff Layton
@ 2006-12-12 22:50 ` Jeff Layton
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Layton @ 2006-12-12 22:50 UTC (permalink / raw)
To: Peter Staubach; +Cc: linux-fsdevel, linux-kernel
Jeff Layton wrote:
> Peter Staubach wrote:
> >
> > If iunique_register() fails, does not this create a memory leak
> > because root will need to get iput()'d?
> >
>
> Good point, and now that we have a wrapper for new_inode that handles this
> error transparently, both places are easy to fix. This patch should do it:
I really need to proofread better before sending. That patch would crash if
new_inode returned NULL. This should be correct:
Signed-off-by: Jeff Layton <jlayton@redhat.com>
--- linux-2.6/fs/debugfs/inode.c.super
+++ linux-2.6/fs/debugfs/inode.c
@@ -107,7 +107,7 @@ static int debug_fill_super(struct super
{
static struct tree_descr debug_files[] = {{""}};
- return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
+ return registered_fill_super(sb, DEBUGFS_MAGIC, debug_files);
}
static int debug_get_sb(struct file_system_type *fs_type,
--- linux-2.6/fs/fuse/control.c.super
+++ linux-2.6/fs/fuse/control.c
@@ -163,7 +163,7 @@ static int fuse_ctl_fill_super(struct su
struct fuse_conn *fc;
int err;
- err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
+ err = registered_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
if (err)
return err;
--- linux-2.6/fs/libfs.c.super
+++ linux-2.6/fs/libfs.c
@@ -214,7 +214,7 @@ int get_sb_pseudo(struct file_system_typ
s->s_magic = magic;
s->s_op = ops ? ops : &default_ops;
s->s_time_gran = 1;
- root = new_inode(s);
+ root = new_registered_inode(s, 0);
if (!root)
goto Enomem;
root->i_mode = S_IFDIR | S_IRUSR | S_IWUSR;
@@ -356,7 +356,8 @@ int simple_commit_write(struct file *fil
return 0;
}
-int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
+int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, bool registered)
{
static struct super_operations s_ops = {.statfs = simple_statfs};
struct inode *inode;
@@ -380,6 +381,12 @@ int simple_fill_super(struct super_block
inode->i_op = &simple_dir_inode_operations;
inode->i_fop = &simple_dir_operations;
inode->i_nlink = 2;
+ /*
+ * set this as high as a 32 bit val as possible to avoid collisions.
+ * This is also well above the highest value that iunique_register
+ * will assign to an inode
+ */
+ inode->i_ino = 0xffffffff;
root = d_alloc_root(inode);
if (!root) {
iput(inode);
@@ -391,15 +398,21 @@ int simple_fill_super(struct super_block
dentry = d_alloc_name(root, files->name);
if (!dentry)
goto out;
- inode = new_inode(s);
+ if (registered)
+ inode = new_registered_inode(s, 0);
+ else
+ inode = new_inode(s);
+
if (!inode)
goto out;
+
+ if (!registered)
+ inode->i_ino = i;
inode->i_mode = S_IFREG | files->mode;
inode->i_uid = inode->i_gid = 0;
inode->i_blocks = 0;
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
inode->i_fop = files->ops;
- inode->i_ino = i;
d_add(dentry, inode);
}
s->s_root = root;
@@ -618,7 +631,7 @@ EXPORT_SYMBOL(simple_dir_inode_operation
EXPORT_SYMBOL(simple_dir_operations);
EXPORT_SYMBOL(simple_empty);
EXPORT_SYMBOL(d_alloc_name);
-EXPORT_SYMBOL(simple_fill_super);
+EXPORT_SYMBOL(__simple_fill_super);
EXPORT_SYMBOL(simple_getattr);
EXPORT_SYMBOL(simple_link);
EXPORT_SYMBOL(simple_lookup);
--- linux-2.6/include/linux/fs.h.super
+++ linux-2.6/include/linux/fs.h
@@ -1879,10 +1879,35 @@ extern const struct file_operations simp
extern struct inode_operations simple_dir_inode_operations;
struct tree_descr { char *name; const struct file_operations *ops; int mode; };
struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
+extern int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, bool registered);
extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
extern void simple_release_fs(struct vfsmount **mount, int *count);
+/*
+ * Fill a superblock with a standard set of fields, and add the entries in the
+ * "files" struct. Assign i_ino values to the files sequentially. This function
+ * is appropriate for filesystems that need a particular i_ino value assigned
+ * to a particular "files" entry.
+ */
+static inline int simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, false);
+}
+
+/*
+ * Just like simple_fill_super, but does an iunique_register on the inodes
+ * created for "files" entries. This function is appropriate when you don't
+ * need a particular i_ino value assigned to each files entry, and when the
+ * filesystem will have other registered inodes.
+ */
+static inline int registered_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, true);
+}
+
extern ssize_t simple_read_from_buffer(void __user *, size_t, loff_t *, const void *, size_t);
#ifdef CONFIG_MIGRATION
--- linux-2.6/security/inode.c.super
+++ linux-2.6/security/inode.c
@@ -130,7 +130,7 @@ static int fill_super(struct super_block
{
static struct tree_descr files[] = {{""}};
- return simple_fill_super(sb, SECURITYFS_MAGIC, files);
+ return registered_fill_super(sb, SECURITYFS_MAGIC, files);
}
static int get_sb(struct file_system_type *fs_type,
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2006-12-12 22:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20061212194708.8359.qmail@science.horizon.com>
2006-12-12 20:06 ` [PATCH 2/3] ensure unique i_ino in filesystems without permanent Jeff Layton
2006-12-12 20:36 ` Peter Staubach
2006-12-12 22:45 ` Jeff Layton
2006-12-12 22:50 ` Jeff Layton
[not found] <20061210175652.7537.qmail@science.horizon.com>
2006-12-12 16:46 ` Jeff Layton
2006-12-12 18:02 ` linux
2006-12-12 19:19 ` Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).