* 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
[parent not found: <20061210175652.7537.qmail@science.horizon.com>]
* 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
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).