[CFT] [JANITORIAL] Unbork fs.h

[CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

After fixing a severe (and stupid) bug, unbork.fs.h (1 of 3) now boots up and
survives updatedb on a native box.  I tried this exactly once, and I figure 
that's enough reason to call for wider testing ;)  I would strongly recommend 
that testing be restricted to a test box or an expendable root partition for 
now.  I don't know for sure it's stable yet - the possibility of eating 
filesystems is certainly there.  (OK, I just booted a second time to see if 
there's still a filesystem there... yes there is... a third time with a 
forced fsck, ok, looking promising.)

After establishing stability, the second objective of testing is to see how 
the changes to shrink_icache_memory affect the VM.  I have to admit that I 
haven't tested this at all.  Manfred Spraul thinks that kmem_cache_shrink is 
entirely overkill for shrinking the icache and that normal cache reaping 
should perform better on the whole.  He may well be right, and I have 
#ifdef'ed out the part of shrink_icache that shrinks the various inode slabs. 
If the inode cache seems to grow too much, try re-enabling that.

If you run this on UML, you'll see an error *after* the power down,
'kmem_cache_destroy: Can't free all objects'.  After discussing this, the 
concensus is that we should either get Jeff Dike to change UML to match the 
kernel's behaviour for module exit code on modules compiled into the kernel 
image (which is to skip it) or we should make sweeping changes throughout 
the system so that module initialization/cleanup code is invoked regardless 
of whether the module is actually separate from the kernel.  Guess which is 
more likely to happen ;-)

Bug Fix
-------

The bug in question should never have slipped by, but it did - the slab 
constructor for inode, init_once, was clearing the slab objects, assuming 
sizeof(struct inode), regardless of the actual size, which is obviously bad.  
I decided to fix this by invading another part of the system, slab.c, and 
adding a new function:

    void kmem_cache_clear (kmem_cache_t *cachep, void *objp);

which just clears a cache object to zero, knowing its size.  I think this 
function could be generally useful, but the immediate objective is just to 
get this patch working.  Alternatively, I could have followed the inode's 
sb->fs->inode_size pointer, and done the required calculations to infer the 
size of the object being cleared.  I avoided this approach because it's more 
fragile.  It might be slightly faster, though.

What to Expect
--------------

This patch provides the VFS mechanism for per-fs inode caches and configures 
Ext2 to use that mechanism.  All other filesystems and other users of inodes 
should continue to work as before.

When you boot with the patch applied, cat /proc/slabinfo and you will see a 
new item named 'ext2', which is the ext2-specific inode cache.  All ext2 
inodes will be allocated there, and all other inodes will be allocated in the 
inode_cache.  There will be a small savings of cache memory, since the ext2 
inodes are reduced in size by 80 bytes, from 472 to 392.  (Actually, in terms 
of saving memory, there's more to be gained by slimming down the inode common 
part.)  With the shrink_icache behavior changed, /proc/slabinfo is the place 
to watch for good or bad effects.

To apply:

  cd /your/source/tree
  cat /this/patch | patch -p0

Good luck, the sooner this gets tested the sooner I can procede with the rest 
of the unbork.

--
Daniel

--- ../2.4.16.uml.clean/arch/um/fs/hostfs/hostfs_kern.c	Wed Jan  2 04:42:37 
2002
+++ ./arch/um/fs/hostfs/hostfs_kern.c	Thu Jan  3 09:18:15 2002
@@ -378,7 +378,7 @@
 	char *name;
 	int type, err = 0, rdev;
 
-	inode = get_empty_inode();
+	inode = get_empty_inode(sb);
 	if(inode == NULL) return(NULL);
 	inode->u.hostfs_i.host_filename = NULL;
 	inode->u.hostfs_i.fd = -1;
@@ -395,7 +395,6 @@
 		kfree(name);
 	}
 	else type = HOSTFS_DIR;
-	inode->i_sb = sb;
 
 	if(type == HOSTFS_SYMLINK)
 		inode->i_op = &page_symlink_inode_operations;
--- ../2.4.16.uml.clean/drivers/block/rd.c	Mon Dec 31 02:55:36 2001
+++ ./drivers/block/rd.c	Thu Jan  3 09:18:15 2002
@@ -673,7 +673,7 @@
 #endif
 	ram_device = MKDEV(MAJOR_NR, unit);
 
-	if ((inode = get_empty_inode()) == NULL)
+	if ((inode = get_empty_inode(NULL)) == NULL)
 		return;
 	memset(&infile, 0, sizeof(infile));
 	memset(&in_dentry, 0, sizeof(in_dentry));
@@ -683,7 +683,7 @@
 	infile.f_op = &def_blk_fops;
 	init_special_inode(inode, S_IFBLK | S_IRUSR, kdev_t_to_nr(device));
 
-	if ((out_inode = get_empty_inode()) == NULL)
+	if ((out_inode = get_empty_inode(NULL)) == NULL)
 		goto free_inode;
 	memset(&outfile, 0, sizeof(outfile));
 	memset(&out_dentry, 0, sizeof(out_dentry));
--- ../2.4.16.uml.clean/fs/ext2/super.c	Sun Dec 30 10:15:56 2001
+++ ./fs/ext2/super.c	Thu Jan  3 09:18:15 2002
@@ -798,16 +798,23 @@
 	return 0;
 }
 
-static DECLARE_FSTYPE_DEV(ext2_fs_type, "ext2", ext2_read_super);
-
+static struct file_system_type ext2_fs = {
+	owner:		THIS_MODULE,
+	fs_flags:	FS_REQUIRES_DEV,
+	name:		"ext2",
+	read_super:	ext2_read_super,
+	super_size:	sizeof(struct ext2_sb_info),
+	inode_size:	sizeof(struct ext2_inode_info)
+};
+ 
 static int __init init_ext2_fs(void)
 {
-        return register_filesystem(&ext2_fs_type);
+        return register_filesystem(&ext2_fs);
 }
 
 static void __exit exit_ext2_fs(void)
 {
-	unregister_filesystem(&ext2_fs_type);
+	unregister_filesystem(&ext2_fs);
 }
 
 EXPORT_NO_SYMBOLS;
--- ../2.4.16.uml.clean/fs/inode.c	Mon Dec 31 02:55:36 2001
+++ ./fs/inode.c	Thu Jan  3 09:39:54 2002
@@ -75,29 +75,32 @@
 
 static kmem_cache_t * inode_cachep;
 
-#define alloc_inode() \
-	 ((struct inode *) kmem_cache_alloc(inode_cachep, SLAB_KERNEL))
-static void destroy_inode(struct inode *inode) 
+static inline struct inode *alloc_inode (struct super_block *sb)
 {
+	kmem_cache_t *cache = sb? sb->s_type->inode_cache: NULL;
+	return (struct inode *) kmem_cache_alloc (cache? cache: inode_cachep, 
SLAB_KERNEL);
+}
+
+static void destroy_inode (struct inode *inode) 
+{
+	struct super_block *sb = inode->i_sb;
+	kmem_cache_t *cache = sb? sb->s_type->inode_cache: NULL;
 	if (inode_has_buffers(inode))
 		BUG();
-	kmem_cache_free(inode_cachep, (inode));
+	kmem_cache_free (cache? cache: inode_cachep, inode);
 }
 
-
 /*
  * These are initializations that only need to be done
  * once, because the fields are idempotent across use
  * of the inode, so let the slab aware of that.
  */
-static void init_once(void * foo, kmem_cache_t * cachep, unsigned long flags)
+static void init_once (void *p, kmem_cache_t *cache, unsigned long flags)
 {
-	struct inode * inode = (struct inode *) foo;
-
-	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
-	    SLAB_CTOR_CONSTRUCTOR)
+	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) == 
SLAB_CTOR_CONSTRUCTOR)
 	{
-		memset(inode, 0, sizeof(*inode));
+		struct inode *inode = (struct inode *) p;
+		kmem_cache_clear (cache, inode);
 		init_waitqueue_head(&inode->i_wait);
 		INIT_LIST_HEAD(&inode->i_hash);
 		INIT_LIST_HEAD(&inode->i_data.clean_pages);
@@ -710,6 +713,7 @@
 
 int shrink_icache_memory(int priority, int gfp_mask)
 {
+	struct file_system_type *fs;
 	int count = 0;
 
 	/*
@@ -725,7 +729,15 @@
 	count = inodes_stat.nr_unused / priority;
 
 	prune_icache(count);
+#if 0
+	/* Manfred thinks this isn't necessary */
 	kmem_cache_shrink(inode_cachep);
+	write_lock(&file_systems_lock);
+	for (fs = file_systems; fs; fs = fs->next)
+		kmem_cache_shrink (fs->inode_cache);
+	write_unlock(&file_systems_lock);
+	kmem_cache_shrink(inode_cachep);
+#endif
 	return 0;
 }
 
@@ -765,12 +777,14 @@
  * i_sb, i_ino, i_count, i_state and the lists have
  * been initialized elsewhere..
  */
-static void clean_inode(struct inode *inode)
+static void clean_inode (struct super_block *sb, struct inode *inode)
 {
 	static struct address_space_operations empty_aops;
 	static struct inode_operations empty_iops;
 	static struct file_operations empty_fops;
-	memset(&inode->u, 0, sizeof(inode->u));
+	unsigned given = sb? sb->s_type->inode_size: 0; // only rd.c has null sb
+	memset(&inode->u, 0, given? given: sizeof(inode->u));
+	inode->i_sb = sb;
 	inode->i_sock = 0;
 	inode->i_op = &empty_iops;
 	inode->i_fop = &empty_fops;
@@ -802,20 +816,19 @@
  * lists.
  */
  
-struct inode * get_empty_inode(void)
+struct inode *get_empty_inode (struct super_block *sb)
 {
 	static unsigned long last_ino;
-	struct inode * inode;
+	struct inode *inode;
 
 	spin_lock_prefetch(&inode_lock);
-	
-	inode = alloc_inode();
+	inode = alloc_inode (sb);
 	if (inode)
 	{
 		spin_lock(&inode_lock);
 		inodes_stat.nr_inodes++;
 		list_add(&inode->i_list, &inode_in_use);
-		inode->i_sb = NULL;
+		inode->i_sb = NULL; // need this?
 		inode->i_dev = 0;
 		inode->i_blkbits = 0;
 		inode->i_ino = ++last_ino;
@@ -823,7 +836,7 @@
 		atomic_set(&inode->i_count, 1);
 		inode->i_state = 0;
 		spin_unlock(&inode_lock);
-		clean_inode(inode);
+		clean_inode(sb, inode);
 	}
 	return inode;
 }
@@ -838,7 +851,7 @@
 {
 	struct inode * inode;
 
-	inode = alloc_inode();
+	inode = alloc_inode (sb);
 	if (inode) {
 		struct inode * old;
 
@@ -849,7 +862,6 @@
 			inodes_stat.nr_inodes++;
 			list_add(&inode->i_list, &inode_in_use);
 			list_add(&inode->i_hash, head);
-			inode->i_sb = sb;
 			inode->i_dev = sb->s_dev;
 			inode->i_blkbits = sb->s_blocksize_bits;
 			inode->i_ino = ino;
@@ -857,8 +869,7 @@
 			atomic_set(&inode->i_count, 1);
 			inode->i_state = I_LOCK;
 			spin_unlock(&inode_lock);
-
-			clean_inode(inode);
+			clean_inode(sb, inode);
 
 			/* reiserfs specific hack right here.  We don't
 			** want this to last, and are looking for VFS changes
@@ -897,6 +908,21 @@
 		wait_on_inode(inode);
 	}
 	return inode;
+}
+
+int create_inode_cache (struct file_system_type *fs)
+{
+	if (fs->inode_size)
+		if (!(fs->inode_cache = kmem_cache_create (fs->name, 
+		    fs->inode_size + sizeof(struct inode) - sizeof(get_empty_inode(0)->u),
+		    0, SLAB_HWCACHE_ALIGN, init_once, NULL)))
+			return -ENOSPC;
+	return 0;
+}
+
+int destroy_inode_cache (struct file_system_type *fs)
+{
+	return kmem_cache_destroy (fs->inode_cache)? -EBUSY: 0;
 }
 
 static inline unsigned long hash(struct super_block *sb, unsigned long i_ino)
--- ../2.4.16.uml.clean/fs/super.c	Mon Dec 31 02:55:36 2001
+++ ./fs/super.c	Thu Jan  3 09:18:15 2002
@@ -67,8 +67,8 @@
  *	Once the reference is obtained we can drop the spinlock.
  */
 
-static struct file_system_type *file_systems;
-static rwlock_t file_systems_lock = RW_LOCK_UNLOCKED;
+struct file_system_type *file_systems;
+rwlock_t file_systems_lock = RW_LOCK_UNLOCKED;
 
 /* WARNING: This can be used only if we _already_ own a reference */
 static void get_filesystem(struct file_system_type *fs)
@@ -105,10 +105,10 @@
  *	unregistered.
  */
  
-int register_filesystem(struct file_system_type * fs)
+int register_filesystem (struct file_system_type *fs)
 {
-	int res = 0;
-	struct file_system_type ** p;
+	struct file_system_type **p;
+	int err = 0;
 
 	if (!fs)
 		return -EINVAL;
@@ -118,11 +118,12 @@
 	write_lock(&file_systems_lock);
 	p = find_filesystem(fs->name);
 	if (*p)
-		res = -EBUSY;
+		err = -EBUSY;
 	else
-		*p = fs;
+		if (!(err = create_inode_cache (fs)))
+			*p = fs;
 	write_unlock(&file_systems_lock);
-	return res;
+	return err;
 }
 
 /**
@@ -137,23 +138,25 @@
  *	may be freed or reused.
  */
  
-int unregister_filesystem(struct file_system_type * fs)
+int unregister_filesystem (struct file_system_type *fs)
 {
-	struct file_system_type ** tmp;
-
+	struct file_system_type **p;
+	int err = -EINVAL;
 	write_lock(&file_systems_lock);
-	tmp = &file_systems;
-	while (*tmp) {
-		if (fs == *tmp) {
-			*tmp = fs->next;
+	p = &file_systems;
+	while (*p) {
+		if (*p == fs) {
+			if (fs->inode_cache && (err = destroy_inode_cache (fs)))
+				break;
+			*p = fs->next;
 			fs->next = NULL;
-			write_unlock(&file_systems_lock);
-			return 0;
+			err = 0;
+			break;
 		}
-		tmp = &(*tmp)->next;
+		p = &(*p)->next;
 	}
 	write_unlock(&file_systems_lock);
-	return -EINVAL;
+	return err;
 }
 
 static int fs_index(const char * __name)
--- ../2.4.16.uml.clean/include/linux/fs.h	Mon Dec 31 02:55:36 2001
+++ ./include/linux/fs.h	Thu Jan  3 09:18:15 2002
@@ -693,6 +693,9 @@
 #include <linux/cramfs_fs_sb.h>
 #include <linux/jffs2_fs_sb.h>
 
+extern struct file_system_type *file_systems;
+extern rwlock_t file_systems_lock;
+
 extern struct list_head super_blocks;
 extern spinlock_t sb_lock;
 
@@ -950,10 +953,14 @@
 	int fs_flags;
 	struct super_block *(*read_super) (struct super_block *, void *, int);
 	struct module *owner;
-	struct file_system_type * next;
+	struct file_system_type *next;
 	struct list_head fs_supers;
+	unsigned super_size, inode_size;
+	struct kmem_cache_s *inode_cache;
 };
 
+/* Backward compatible declarations, remove when all updated */
+
 #define DECLARE_FSTYPE(var,type,read,flags) \
 struct file_system_type var = { \
 	name:		type, \
@@ -1327,18 +1334,21 @@
 }
 
 extern void clear_inode(struct inode *);
-extern struct inode * get_empty_inode(void);
+extern struct inode *get_empty_inode (struct super_block *sb);
 
-static inline struct inode * new_inode(struct super_block *sb)
+static inline struct inode *new_inode (struct super_block *sb)
 {
-	struct inode *inode = get_empty_inode();
+	struct inode *inode = get_empty_inode(sb);
 	if (inode) {
-		inode->i_sb = sb;
 		inode->i_dev = sb->s_dev;
 		inode->i_blkbits = sb->s_blocksize_bits;
 	}
 	return inode;
 }
+
+int create_inode_cache (struct file_system_type *fs);
+int destroy_inode_cache (struct file_system_type *fs);
+
 extern void remove_suid(struct inode *inode);
 
 extern void insert_inode_hash(struct inode *);
--- ../2.4.16.uml.clean/include/linux/slab.h	Thu Nov 22 14:46:20 2001
+++ ./include/linux/slab.h	Thu Jan  3 09:18:47 2002
@@ -56,6 +56,7 @@
 extern int kmem_cache_shrink(kmem_cache_t *);
 extern void *kmem_cache_alloc(kmem_cache_t *, int);
 extern void kmem_cache_free(kmem_cache_t *, void *);
+extern void kmem_cache_clear(kmem_cache_t *, void *);
 
 extern void *kmalloc(size_t, int);
 extern void kfree(const void *);
--- ../2.4.16.uml.clean/mm/slab.c	Tue Sep 18 17:16:26 2001
+++ ./mm/slab.c	Thu Jan  3 09:18:23 2002
@@ -1076,6 +1076,16 @@
 	slabp->free = 0;
 }
 
+void kmem_cache_clear (kmem_cache_t *cachep, void *objp)
+{
+	unsigned size = cachep->objsize;
+#if DEBUG
+	if (cachep->flags & SLAB_RED_ZONE)
+		size -= BYTES_PER_WORD*2;
+#endif
+	memset(objp, 0, size);
+}
+
 /*
  * Grow (by 1) the number of slabs within a cache.  This is called by
  * kmem_cache_alloc() when there are no active objs left in a cache.
--- ../2.4.16.uml.clean/net/socket.c	Mon Dec 31 02:55:36 2001
+++ ./net/socket.c	Thu Jan  3 09:18:15 2002
@@ -440,11 +440,10 @@
 	struct inode * inode;
 	struct socket * sock;
 
-	inode = get_empty_inode();
+	inode = get_empty_inode(sock_mnt->mnt_sb);
 	if (!inode)
 		return NULL;
 
-	inode->i_sb = sock_mnt->mnt_sb;
 	sock = socki_lookup(inode);
 
 	inode->i_mode = S_IFSOCK|S_IRWXUGO;

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

(the same patch without linewrap)

--- ../2.4.16.uml.clean/arch/um/fs/hostfs/hostfs_kern.c	Wed Jan  2 04:42:37 2002
+++ ./arch/um/fs/hostfs/hostfs_kern.c	Thu Jan  3 09:18:15 2002
@@ -378,7 +378,7 @@
 	char *name;
 	int type, err = 0, rdev;
 
-	inode = get_empty_inode();
+	inode = get_empty_inode(sb);
 	if(inode == NULL) return(NULL);
 	inode->u.hostfs_i.host_filename = NULL;
 	inode->u.hostfs_i.fd = -1;
@@ -395,7 +395,6 @@
 		kfree(name);
 	}
 	else type = HOSTFS_DIR;
-	inode->i_sb = sb;
 
 	if(type == HOSTFS_SYMLINK)
 		inode->i_op = &page_symlink_inode_operations;
--- ../2.4.16.uml.clean/drivers/block/rd.c	Mon Dec 31 02:55:36 2001
+++ ./drivers/block/rd.c	Thu Jan  3 09:18:15 2002
@@ -673,7 +673,7 @@
 #endif
 	ram_device = MKDEV(MAJOR_NR, unit);
 
-	if ((inode = get_empty_inode()) == NULL)
+	if ((inode = get_empty_inode(NULL)) == NULL)
 		return;
 	memset(&infile, 0, sizeof(infile));
 	memset(&in_dentry, 0, sizeof(in_dentry));
@@ -683,7 +683,7 @@
 	infile.f_op = &def_blk_fops;
 	init_special_inode(inode, S_IFBLK | S_IRUSR, kdev_t_to_nr(device));
 
-	if ((out_inode = get_empty_inode()) == NULL)
+	if ((out_inode = get_empty_inode(NULL)) == NULL)
 		goto free_inode;
 	memset(&outfile, 0, sizeof(outfile));
 	memset(&out_dentry, 0, sizeof(out_dentry));
--- ../2.4.16.uml.clean/fs/ext2/super.c	Sun Dec 30 10:15:56 2001
+++ ./fs/ext2/super.c	Thu Jan  3 09:18:15 2002
@@ -798,16 +798,23 @@
 	return 0;
 }
 
-static DECLARE_FSTYPE_DEV(ext2_fs_type, "ext2", ext2_read_super);
-
+static struct file_system_type ext2_fs = {
+	owner:		THIS_MODULE,
+	fs_flags:	FS_REQUIRES_DEV,
+	name:		"ext2",
+	read_super:	ext2_read_super,
+	super_size:	sizeof(struct ext2_sb_info),
+	inode_size:	sizeof(struct ext2_inode_info)
+};
+ 
 static int __init init_ext2_fs(void)
 {
-        return register_filesystem(&ext2_fs_type);
+        return register_filesystem(&ext2_fs);
 }
 
 static void __exit exit_ext2_fs(void)
 {
-	unregister_filesystem(&ext2_fs_type);
+	unregister_filesystem(&ext2_fs);
 }
 
 EXPORT_NO_SYMBOLS;
--- ../2.4.16.uml.clean/fs/inode.c	Mon Dec 31 02:55:36 2001
+++ ./fs/inode.c	Thu Jan  3 09:39:54 2002
@@ -75,29 +75,32 @@
 
 static kmem_cache_t * inode_cachep;
 
-#define alloc_inode() \
-	 ((struct inode *) kmem_cache_alloc(inode_cachep, SLAB_KERNEL))
-static void destroy_inode(struct inode *inode) 
+static inline struct inode *alloc_inode (struct super_block *sb)
 {
+	kmem_cache_t *cache = sb? sb->s_type->inode_cache: NULL;
+	return (struct inode *) kmem_cache_alloc (cache? cache: inode_cachep, SLAB_KERNEL);
+}
+
+static void destroy_inode (struct inode *inode) 
+{
+	struct super_block *sb = inode->i_sb;
+	kmem_cache_t *cache = sb? sb->s_type->inode_cache: NULL;
 	if (inode_has_buffers(inode))
 		BUG();
-	kmem_cache_free(inode_cachep, (inode));
+	kmem_cache_free (cache? cache: inode_cachep, inode);
 }
 
-
 /*
  * These are initializations that only need to be done
  * once, because the fields are idempotent across use
  * of the inode, so let the slab aware of that.
  */
-static void init_once(void * foo, kmem_cache_t * cachep, unsigned long flags)
+static void init_once (void *p, kmem_cache_t *cache, unsigned long flags)
 {
-	struct inode * inode = (struct inode *) foo;
-
-	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
-	    SLAB_CTOR_CONSTRUCTOR)
+	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) == SLAB_CTOR_CONSTRUCTOR)
 	{
-		memset(inode, 0, sizeof(*inode));
+		struct inode *inode = (struct inode *) p;
+		kmem_cache_clear (cache, inode);
 		init_waitqueue_head(&inode->i_wait);
 		INIT_LIST_HEAD(&inode->i_hash);
 		INIT_LIST_HEAD(&inode->i_data.clean_pages);
@@ -710,6 +713,7 @@
 
 int shrink_icache_memory(int priority, int gfp_mask)
 {
+	struct file_system_type *fs;
 	int count = 0;
 
 	/*
@@ -725,7 +729,15 @@
 	count = inodes_stat.nr_unused / priority;
 
 	prune_icache(count);
+#if 0
+	/* Manfred thinks this isn't necessary */
 	kmem_cache_shrink(inode_cachep);
+	write_lock(&file_systems_lock);
+	for (fs = file_systems; fs; fs = fs->next)
+		kmem_cache_shrink (fs->inode_cache);
+	write_unlock(&file_systems_lock);
+	kmem_cache_shrink(inode_cachep);
+#endif
 	return 0;
 }
 
@@ -765,12 +777,14 @@
  * i_sb, i_ino, i_count, i_state and the lists have
  * been initialized elsewhere..
  */
-static void clean_inode(struct inode *inode)
+static void clean_inode (struct super_block *sb, struct inode *inode)
 {
 	static struct address_space_operations empty_aops;
 	static struct inode_operations empty_iops;
 	static struct file_operations empty_fops;
-	memset(&inode->u, 0, sizeof(inode->u));
+	unsigned given = sb? sb->s_type->inode_size: 0; // only rd.c has null sb
+	memset(&inode->u, 0, given? given: sizeof(inode->u));
+	inode->i_sb = sb;
 	inode->i_sock = 0;
 	inode->i_op = &empty_iops;
 	inode->i_fop = &empty_fops;
@@ -802,20 +816,19 @@
  * lists.
  */
  
-struct inode * get_empty_inode(void)
+struct inode *get_empty_inode (struct super_block *sb)
 {
 	static unsigned long last_ino;
-	struct inode * inode;
+	struct inode *inode;
 
 	spin_lock_prefetch(&inode_lock);
-	
-	inode = alloc_inode();
+	inode = alloc_inode (sb);
 	if (inode)
 	{
 		spin_lock(&inode_lock);
 		inodes_stat.nr_inodes++;
 		list_add(&inode->i_list, &inode_in_use);
-		inode->i_sb = NULL;
+		inode->i_sb = NULL; // need this?
 		inode->i_dev = 0;
 		inode->i_blkbits = 0;
 		inode->i_ino = ++last_ino;
@@ -823,7 +836,7 @@
 		atomic_set(&inode->i_count, 1);
 		inode->i_state = 0;
 		spin_unlock(&inode_lock);
-		clean_inode(inode);
+		clean_inode(sb, inode);
 	}
 	return inode;
 }
@@ -838,7 +851,7 @@
 {
 	struct inode * inode;
 
-	inode = alloc_inode();
+	inode = alloc_inode (sb);
 	if (inode) {
 		struct inode * old;
 
@@ -849,7 +862,6 @@
 			inodes_stat.nr_inodes++;
 			list_add(&inode->i_list, &inode_in_use);
 			list_add(&inode->i_hash, head);
-			inode->i_sb = sb;
 			inode->i_dev = sb->s_dev;
 			inode->i_blkbits = sb->s_blocksize_bits;
 			inode->i_ino = ino;
@@ -857,8 +869,7 @@
 			atomic_set(&inode->i_count, 1);
 			inode->i_state = I_LOCK;
 			spin_unlock(&inode_lock);
-
-			clean_inode(inode);
+			clean_inode(sb, inode);
 
 			/* reiserfs specific hack right here.  We don't
 			** want this to last, and are looking for VFS changes
@@ -897,6 +908,21 @@
 		wait_on_inode(inode);
 	}
 	return inode;
+}
+
+int create_inode_cache (struct file_system_type *fs)
+{
+	if (fs->inode_size)
+		if (!(fs->inode_cache = kmem_cache_create (fs->name, 
+		    fs->inode_size + sizeof(struct inode) - sizeof(get_empty_inode(0)->u),
+		    0, SLAB_HWCACHE_ALIGN, init_once, NULL)))
+			return -ENOSPC;
+	return 0;
+}
+
+int destroy_inode_cache (struct file_system_type *fs)
+{
+	return kmem_cache_destroy (fs->inode_cache)? -EBUSY: 0;
 }
 
 static inline unsigned long hash(struct super_block *sb, unsigned long i_ino)
--- ../2.4.16.uml.clean/fs/super.c	Mon Dec 31 02:55:36 2001
+++ ./fs/super.c	Thu Jan  3 09:18:15 2002
@@ -67,8 +67,8 @@
  *	Once the reference is obtained we can drop the spinlock.
  */
 
-static struct file_system_type *file_systems;
-static rwlock_t file_systems_lock = RW_LOCK_UNLOCKED;
+struct file_system_type *file_systems;
+rwlock_t file_systems_lock = RW_LOCK_UNLOCKED;
 
 /* WARNING: This can be used only if we _already_ own a reference */
 static void get_filesystem(struct file_system_type *fs)
@@ -105,10 +105,10 @@
  *	unregistered.
  */
  
-int register_filesystem(struct file_system_type * fs)
+int register_filesystem (struct file_system_type *fs)
 {
-	int res = 0;
-	struct file_system_type ** p;
+	struct file_system_type **p;
+	int err = 0;
 
 	if (!fs)
 		return -EINVAL;
@@ -118,11 +118,12 @@
 	write_lock(&file_systems_lock);
 	p = find_filesystem(fs->name);
 	if (*p)
-		res = -EBUSY;
+		err = -EBUSY;
 	else
-		*p = fs;
+		if (!(err = create_inode_cache (fs)))
+			*p = fs;
 	write_unlock(&file_systems_lock);
-	return res;
+	return err;
 }
 
 /**
@@ -137,23 +138,25 @@
  *	may be freed or reused.
  */
  
-int unregister_filesystem(struct file_system_type * fs)
+int unregister_filesystem (struct file_system_type *fs)
 {
-	struct file_system_type ** tmp;
-
+	struct file_system_type **p;
+	int err = -EINVAL;
 	write_lock(&file_systems_lock);
-	tmp = &file_systems;
-	while (*tmp) {
-		if (fs == *tmp) {
-			*tmp = fs->next;
+	p = &file_systems;
+	while (*p) {
+		if (*p == fs) {
+			if (fs->inode_cache && (err = destroy_inode_cache (fs)))
+				break;
+			*p = fs->next;
 			fs->next = NULL;
-			write_unlock(&file_systems_lock);
-			return 0;
+			err = 0;
+			break;
 		}
-		tmp = &(*tmp)->next;
+		p = &(*p)->next;
 	}
 	write_unlock(&file_systems_lock);
-	return -EINVAL;
+	return err;
 }
 
 static int fs_index(const char * __name)
--- ../2.4.16.uml.clean/include/linux/fs.h	Mon Dec 31 02:55:36 2001
+++ ./include/linux/fs.h	Thu Jan  3 09:18:15 2002
@@ -693,6 +693,9 @@
 #include <linux/cramfs_fs_sb.h>
 #include <linux/jffs2_fs_sb.h>
 
+extern struct file_system_type *file_systems;
+extern rwlock_t file_systems_lock;
+
 extern struct list_head super_blocks;
 extern spinlock_t sb_lock;
 
@@ -950,10 +953,14 @@
 	int fs_flags;
 	struct super_block *(*read_super) (struct super_block *, void *, int);
 	struct module *owner;
-	struct file_system_type * next;
+	struct file_system_type *next;
 	struct list_head fs_supers;
+	unsigned super_size, inode_size;
+	struct kmem_cache_s *inode_cache;
 };
 
+/* Backward compatible declarations, remove when all updated */
+
 #define DECLARE_FSTYPE(var,type,read,flags) \
 struct file_system_type var = { \
 	name:		type, \
@@ -1327,18 +1334,21 @@
 }
 
 extern void clear_inode(struct inode *);
-extern struct inode * get_empty_inode(void);
+extern struct inode *get_empty_inode (struct super_block *sb);
 
-static inline struct inode * new_inode(struct super_block *sb)
+static inline struct inode *new_inode (struct super_block *sb)
 {
-	struct inode *inode = get_empty_inode();
+	struct inode *inode = get_empty_inode(sb);
 	if (inode) {
-		inode->i_sb = sb;
 		inode->i_dev = sb->s_dev;
 		inode->i_blkbits = sb->s_blocksize_bits;
 	}
 	return inode;
 }
+
+int create_inode_cache (struct file_system_type *fs);
+int destroy_inode_cache (struct file_system_type *fs);
+
 extern void remove_suid(struct inode *inode);
 
 extern void insert_inode_hash(struct inode *);
--- ../2.4.16.uml.clean/include/linux/slab.h	Thu Nov 22 14:46:20 2001
+++ ./include/linux/slab.h	Thu Jan  3 09:18:47 2002
@@ -56,6 +56,7 @@
 extern int kmem_cache_shrink(kmem_cache_t *);
 extern void *kmem_cache_alloc(kmem_cache_t *, int);
 extern void kmem_cache_free(kmem_cache_t *, void *);
+extern void kmem_cache_clear(kmem_cache_t *, void *);
 
 extern void *kmalloc(size_t, int);
 extern void kfree(const void *);
--- ../2.4.16.uml.clean/mm/slab.c	Tue Sep 18 17:16:26 2001
+++ ./mm/slab.c	Thu Jan  3 09:18:23 2002
@@ -1076,6 +1076,16 @@
 	slabp->free = 0;
 }
 
+void kmem_cache_clear (kmem_cache_t *cachep, void *objp)
+{
+	unsigned size = cachep->objsize;
+#if DEBUG
+	if (cachep->flags & SLAB_RED_ZONE)
+		size -= BYTES_PER_WORD*2;
+#endif
+	memset(objp, 0, size);
+}
+
 /*
  * Grow (by 1) the number of slabs within a cache.  This is called by
  * kmem_cache_alloc() when there are no active objs left in a cache.
--- ../2.4.16.uml.clean/net/socket.c	Mon Dec 31 02:55:36 2001
+++ ./net/socket.c	Thu Jan  3 09:18:15 2002
@@ -440,11 +440,10 @@
 	struct inode * inode;
 	struct socket * sock;
 
-	inode = get_empty_inode();
+	inode = get_empty_inode(sock_mnt->mnt_sb);
 	if (!inode)
 		return NULL;
 
-	inode->i_sb = sock_mnt->mnt_sb;
 	sock = socki_lookup(inode);
 
 	inode->i_mode = S_IFSOCK|S_IRWXUGO;

Re: [CFT] [JANITORIAL] Unbork fs.h

[Christoph Hellwig]

In article <E16M7Gz-00015E-00@starship.berlin> you wrote:
> -	inode = get_empty_inode();
> +	inode = get_empty_inode(sb);

How about killing get_empty_inode completly and using new_inode() instead?
There should be no regularly allocated inode without a superblock.

	Christoph

-- 
Of course it doesn't work. We've performed a software upgrade.

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

On January 3, 2002 04:45 pm, Christoph Hellwig wrote:
> In article <E16M7Gz-00015E-00@starship.berlin> you wrote:
> > -	inode = get_empty_inode();
> > +	inode = get_empty_inode(sb);
> 
> How about killing get_empty_inode completly and using new_inode() instead?
> There should be no regularly allocated inode without a superblock.

There are: sock_alloc rd_load_image.  However that's a nit because the new, 
improved get_empty_inode understands the concept of null sb.  (Another thing 
we could do is require every inode to have a superblock - that's probably 
where it will go in time.)

We put this inside get_empty_inode:

	if (inode) {
		inode->i_dev = sb->s_dev;
		inode->i_blkbits = sb->s_blocksize_bits;
	}

then rename it new_inode.  But this is outside of the scope of the fs.h work 
I'm doing, don't you think?  There are a lot of things I'd like to clean up 
on the way through this, but it's probably best to just resist the temptation 
for now.

--
Daniel

Re: [CFT] [JANITORIAL] Unbork fs.h

[Alexander Viro]

On Thu, 3 Jan 2002, Daniel Phillips wrote:

> On January 3, 2002 04:45 pm, Christoph Hellwig wrote:
> > In article <E16M7Gz-00015E-00@starship.berlin> you wrote:
> > > -	inode = get_empty_inode();
> > > +	inode = get_empty_inode(sb);
> > 
> > How about killing get_empty_inode completly and using new_inode() instead?
> > There should be no regularly allocated inode without a superblock.
> 
> There are: sock_alloc rd_load_image.  However that's a nit because the new, 
> improved get_empty_inode understands the concept of null sb.  (Another thing 
> we could do is require every inode to have a superblock - that's probably 
> where it will go in time.)

It's _already_ there.  RTFS, please - sock_alloc() creates inodes with
sockfs superblock in ->i_sb and rd_load_image() just does normal open()
for device nodes on rootfs.

Please, don't reintroduce the crap we'd already killed.

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

On January 3, 2002 05:47 pm, Alexander Viro wrote:
> On Thu, 3 Jan 2002, Daniel Phillips wrote:
> > On January 3, 2002 04:45 pm, Christoph Hellwig wrote:
> > > In article <E16M7Gz-00015E-00@starship.berlin> you wrote:
> > > > -	inode = get_empty_inode();
> > > > +	inode = get_empty_inode(sb);
> > > 
> > > How about killing get_empty_inode completly and using new_inode() instead?
> > > There should be no regularly allocated inode without a superblock.
> > 
> > There are: sock_alloc rd_load_image.  However that's a nit because the new, 
> > improved get_empty_inode understands the concept of null sb.  (Another thing 
> > we could do is require every inode to have a superblock - that's probably 
> > where it will go in time.)
> 
> It's _already_ there.  RTFS, please - sock_alloc() creates inodes with
> sockfs superblock in ->i_sb and rd_load_image() just does normal open()
> for device nodes on rootfs.

Sockfs - yes, you'll see my patch already does it correctly, I was getting
a little tired when writing the previous reply... rd_load_image in 2.4.17 is
still using kdev_t there, however, no super_block anywhere to be seen.  I'll
track that change if it happens before I bring the patches forward to 2.5.

--
Daniel

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

On January 3, 2002 05:20 pm, Daniel Phillips wrote:
> On January 3, 2002 04:45 pm, Christoph Hellwig wrote:
> > In article <E16M7Gz-00015E-00@starship.berlin> you wrote:
> > > -	inode = get_empty_inode();
> > > +	inode = get_empty_inode(sb);
> > 
> > How about killing get_empty_inode completly and using new_inode() instead?
> > There should be no regularly allocated inode without a superblock.
> 
> There are: sock_alloc rd_load_image.  However that's a nit because the new, 
> improved get_empty_inode understands the concept of null sb.  (Another thing 
> we could do is require every inode to have a superblock - that's probably 
> where it will go in time.)
> 
> We put this inside get_empty_inode:
> 
> 	if (inode) {
            ^^^^^-----> whoops, getting tired, I meant (sb)
> 		inode->i_dev = sb->s_dev;
> 		inode->i_blkbits = sb->s_blocksize_bits;
> 	}
>
> then rename it new_inode.  But this is outside of the scope of the fs.h work 
> I'm doing, don't you think?  There are a lot of things I'd like to clean up 
> on the way through this, but it's probably best to just resist the temptation 
> for now.

--
Daniel

Re: [CFT] [JANITORIAL] Unbork fs.h

[Christoph Hellwig]

On Thu, Jan 03, 2002 at 05:20:12PM +0100, Daniel Phillips wrote:
> On January 3, 2002 04:45 pm, Christoph Hellwig wrote:
> > In article <E16M7Gz-00015E-00@starship.berlin> you wrote:
> > > -	inode = get_empty_inode();
> > > +	inode = get_empty_inode(sb);
> > 
> > How about killing get_empty_inode completly and using new_inode() instead?
> > There should be no regularly allocated inode without a superblock.
> 
> There are: sock_alloc rd_load_image.  However that's a nit because the new, 
> improved get_empty_inode understands the concept of null sb.

get_empty_inode is hopefully going to die in the current, non-static version.

> (Another thing 
> we could do is require every inode to have a superblock - that's probably 
> where it will go in time.)

Any inode that gets into the icache already has and superblock.
If any other are left they really should use a different allocator.

> We put this inside get_empty_inode:
> 
> 	if (inode) {
> 		inode->i_dev = sb->s_dev;
> 		inode->i_blkbits = sb->s_blocksize_bits;
> 	}
> 
> then rename it new_inode.  But this is outside of the scope of the fs.h work 
> I'm doing, don't you think?

Rename your current get_empty_inode to __get_empty_inode and mark it
static.  Add a new get_empty_inode that calls __get_empty_inode(NULL) or
better let it die.

	Christoph

-- 
Of course it doesn't work. We've performed a software upgrade.

Re: [CFT] [JANITORIAL] Unbork fs.h

[Alexander Viro]

On Thu, 3 Jan 2002, Christoph Hellwig wrote:

> In article <E16M7Gz-00015E-00@starship.berlin> you wrote:
> > -	inode = get_empty_inode();
> > +	inode = get_empty_inode(sb);
> 
> How about killing get_empty_inode completly and using new_inode() instead?
> There should be no regularly allocated inode without a superblock.

Seconded.  However, you'll need to zero out ->i_dev for objects that
traditionally have zero ->st_dev (pipes and sockets).

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

On January 3, 2002 05:45 pm, Alexander Viro wrote:
> On Thu, 3 Jan 2002, Christoph Hellwig wrote:
> 
> > In article <E16M7Gz-00015E-00@starship.berlin> you wrote:
> > > -	inode = get_empty_inode();
> > > +	inode = get_empty_inode(sb);
> > 
> > How about killing get_empty_inode completly and using new_inode() instead?
> > There should be no regularly allocated inode without a superblock.
> 
> Seconded.  However, you'll need to zero out ->i_dev for objects that
> traditionally have zero ->st_dev (pipes and sockets).

If you spell out exactly what special case treatment you'd like for i_dev, 
I'll make the changes to get rid of get_empty_inode.

--
Daniel

Re: [CFT] [JANITORIAL] Unbork fs.h

[Ion Badulescu]

Hi Daniel,

On Thu, 3 Jan 2002 13:47:01 +0100, Daniel Phillips <phillips@bonn-fries.net> wrote:

> +static struct file_system_type ext2_fs = {
> +       owner:          THIS_MODULE,
> +       fs_flags:       FS_REQUIRES_DEV,
> +       name:           "ext2",
> +       read_super:     ext2_read_super,
> +       super_size:     sizeof(struct ext2_sb_info),
> +       inode_size:     sizeof(struct ext2_inode_info)
> +};

While we're at it, can we extend this model to also include details about 
the other filesystem data structures with (potential) private info, i.e.
struct dentry and struct file? ext2 might not use them, but other 
filesystems certainly do.

> -static inline struct inode * new_inode(struct super_block *sb)
> +static inline struct inode *new_inode (struct super_block *sb)

Minor issue of coding style. I'd steer away from such gratuitious changes, 
especially since they divert from the commonly accepted practice of having 
no spaces between the name of the function and its arguments.

Thanks,
-Ion
[FiST co-maintainer]

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

On January 3, 2002 05:05 pm, Ion Badulescu wrote:
> Daniel Phillips wrote:
> 
> > +static struct file_system_type ext2_fs = {
> > +       owner:          THIS_MODULE,
> > +       fs_flags:       FS_REQUIRES_DEV,
> > +       name:           "ext2",
> > +       read_super:     ext2_read_super,
> > +       super_size:     sizeof(struct ext2_sb_info),
> > +       inode_size:     sizeof(struct ext2_inode_info)
> > +};
> 
> While we're at it, can we extend this model to also include details about 
> the other filesystem data structures with (potential) private info, i.e.
> struct dentry and struct file? ext2 might not use them, but other 
> filesystems certainly do.

Hi,

Could you be more specific about what you mean, please?

> > -static inline struct inode * new_inode(struct super_block *sb)
> > +static inline struct inode *new_inode (struct super_block *sb)
> 
> Minor issue of coding style. I'd steer away from such gratuitious changes, 
> especially since they divert from the commonly accepted practice of having 
> no spaces between the name of the function and its arguments.

That's good advice and I'm likely to adhere to it - if you can show that 
having no spaces between the name of the function and its arguments really is 
the accepted practice.  I've seen both styles on my various travels though 
the kernel, and I prefer the one with the space.  Much as I prefer to put 
spaces around '+' (but not around '.', go figure).

In general, I allow myself the indulgence of cleaning up the odd line here 
and there to be more pleasing to my eyes, so long as it's in the vicinity of 
a substantive change and doesn't introduce a new patch hunk.  You could think 
of it as a perk that takes some of the sting out of doing the grunt work.

--
Daniel

Re: [CFT] [JANITORIAL] Unbork fs.h

[Martin Dalecki]

Daniel Phillips wrote:

>On January 3, 2002 05:05 pm, Ion Badulescu wrote:
>
>>Daniel Phillips wrote:
>>
>>>+static struct file_system_type ext2_fs = {
>>>+       owner:          THIS_MODULE,
>>>+       fs_flags:       FS_REQUIRES_DEV,
>>>+       name:           "ext2",
>>>+       read_super:     ext2_read_super,
>>>+       super_size:     sizeof(struct ext2_sb_info),
>>>+       inode_size:     sizeof(struct ext2_inode_info)
>>>+};
>>>
>>While we're at it, can we extend this model to also include details about 
>>the other filesystem data structures with (potential) private info, i.e.
>>struct dentry and struct file? ext2 might not use them, but other 
>>filesystems certainly do.
>>
>
>Hi,
>
>Could you be more specific about what you mean, please?
>
>>>-static inline struct inode * new_inode(struct super_block *sb)
>>>+static inline struct inode *new_inode (struct super_block *sb)
>>>
>>Minor issue of coding style. I'd steer away from such gratuitious changes, 
>>especially since they divert from the commonly accepted practice of having 
>>no spaces between the name of the function and its arguments.
>>
>
>That's good advice and I'm likely to adhere to it - if you can show that 
>having no spaces between the name of the function and its arguments really is 
>the accepted practice. 
>
It is trust on that. Only the silly GNU indentation style introduced 
something else. Look at the "core kernel" and
not the ugly drivers around it.

Re: [CFT] [JANITORIAL] Unbork fs.h

[Arnaldo Carvalho de Melo]

Em Thu, Jan 03, 2002 at 05:28:57PM +0100, Martin Dalecki escreveu:
> >That's good advice and I'm likely to adhere to it - if you can show that 
> >having no spaces between the name of the function and its arguments really 
> >is the accepted practice. 

> It is trust on that. Only the silly GNU indentation style introduced 
> something else. Look at the "core kernel" and
> not the ugly drivers around it.

Specially _not_ the ones in drivers/scsi ;)

/me runs

- Arnaldo

Re: [CFT] [JANITORIAL] Unbork fs.h

[Arnaldo Carvalho de Melo]

Em Thu, Jan 03, 2002 at 05:34:27PM +0100, Daniel Phillips escreveu:
> On January 3, 2002 05:05 pm, Ion Badulescu wrote:
> > Daniel Phillips wrote:
> > > -static inline struct inode * new_inode(struct super_block *sb)
> > > +static inline struct inode *new_inode (struct super_block *sb)
> > 
> > Minor issue of coding style. I'd steer away from such gratuitious changes, 
> > especially since they divert from the commonly accepted practice of having 
> > no spaces between the name of the function and its arguments.
> 
> That's good advice and I'm likely to adhere to it - if you can show that 
> having no spaces between the name of the function and its arguments really is 
> the accepted practice.  I've seen both styles on my various travels though 
> the kernel, and I prefer the one with the space.  Much as I prefer to put 
> spaces around '+' (but not around '.', go figure).

Maybe CodingStyle should have an entry for this, I'd vote for this style:

static inline struct inode *new_inode(struct super_block *sb)

> In general, I allow myself the indulgence of cleaning up the odd line here 
> and there to be more pleasing to my eyes, so long as it's in the vicinity of 
> a substantive change and doesn't introduce a new patch hunk.  You could think 
> of it as a perk that takes some of the sting out of doing the grunt work.

fair, thats what I usually do as well 8)

- Arnaldo

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

On January 3, 2002 05:36 pm, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 03, 2002 at 05:34:27PM +0100, Daniel Phillips escreveu:
> > On January 3, 2002 05:05 pm, Ion Badulescu wrote:
> > > Daniel Phillips wrote:
> > > > -static inline struct inode * new_inode(struct super_block *sb)
> > > > +static inline struct inode *new_inode (struct super_block *sb)
> > > 
> > > Minor issue of coding style. I'd steer away from such gratuitious changes, 
> > > especially since they divert from the commonly accepted practice of having 
> > > no spaces between the name of the function and its arguments.
> > 
> > That's good advice and I'm likely to adhere to it - if you can show that 
> > having no spaces between the name of the function and its arguments really is 
> > the accepted practice.  I've seen both styles on my various travels though 
> > the kernel, and I prefer the one with the space.  Much as I prefer to put 
> > spaces around '+' (but not around '.', go figure).
> 
> Maybe CodingStyle should have an entry for this, I'd vote for this style:
>
> static inline struct inode *new_inode(struct super_block *sb)

OK, I'll revise it to that style.  Shall we start an official janitor's style
guide? ;-)

--
Daniel

Re: [CFT] [JANITORIAL] Unbork fs.h

[Arnaldo Carvalho de Melo]

Em Thu, Jan 03, 2002 at 06:05:19PM +0100, Daniel Phillips escreveu:
> On January 3, 2002 05:36 pm, Arnaldo Carvalho de Melo wrote:
> > Maybe CodingStyle should have an entry for this, I'd vote for this style:
> >
> > static inline struct inode *new_inode(struct super_block *sb)
> 
> OK, I'll revise it to that style.  Shall we start an official janitor's style
> guide? ;-)

Nah, I'll try and submit some patches do CodingStyle, for discussion. Hey,
people here _love_ to discuss important things like coding style, dontcha?
8)

- Arnaldo

Re: [CFT] [JANITORIAL] Unbork fs.h

[Andreas Dilger]

On Jan 03, 2002  15:07 -0200, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 03, 2002 at 06:05:19PM +0100, Daniel Phillips escreveu:
> > On January 3, 2002 05:36 pm, Arnaldo Carvalho de Melo wrote:
> > > Maybe CodingStyle should have an entry for this, I'd vote for this style:
> > >
> > > static inline struct inode *new_inode(struct super_block *sb)
> > 
> > OK, I'll revise it to that style.  Shall we start an official janitor's
> > style guide? ;-)
> 
> Nah, I'll try and submit some patches do CodingStyle, for discussion. Hey,
> people here _love_ to discuss important things like coding style, dontcha?

While you are at it, the following patch to scripts/Lindent makes it do the
right thing as well.  From what I can see, the following is more consistent
with the actual practise in the kernel code.  Sadly, indent (v2.2.4) will
break the indenting of any line with a colon (e.g. labels, named struct
initialization), so it is not possible to just run Lindent on the whole
tree.


I removed the following two options:
-bs: Put a space between sizeof and its argument.
-psl: Put the type of a procedure on the line before its name.

I added the following options:
-nbbo: don't prefer to break lines before boolean operators
-ci8: indent continuation lines 8 characters
-ncs: Do not put a space after cast operators.
-lps: Leave space between # and preprocessor directive.
-pmt: Preserve access and modification times on output files.
-npro: Do not read .indent.pro files.


diff -u linux.orig/scripts/Lindent linux/scripts/Lindent
--- linux.orig/scripts/Lindent	Wed Nov 28 23:25:14 2001
+++ linux/scripts/Lindent	Thu Jan  3 12:10:00 2002
@@ -1,2 +1,37 @@
 #!/bin/sh
-indent -kr -i8 -ts8 -sob -l80 -ss -bs -psl "$@"
+indent -kr -nbbo -ci8 -ncs -i8 -l80 -lps -pmt -npro -sob -ss -ts8 "$@"

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

On January 3, 2002 08:36 pm, Andreas Dilger wrote:
> I removed the following two options:
> -bs: Put a space between sizeof and its argument.
> -psl: Put the type of a procedure on the line before its name.
> 
> I added the following options:
> -nbbo: don't prefer to break lines before boolean operators
> -ci8: indent continuation lines 8 characters
> -ncs: Do not put a space after cast operators.

Not putting a space after a cast is gross ;)

> -lps: Leave space between # and preprocessor directive.
> -pmt: Preserve access and modification times on output files.
> -npro: Do not read .indent.pro files.

--
Daniel

Re: [CFT] [JANITORIAL] Unbork fs.h

[Andreas Dilger]

OK, for people that care, I did a quick survey of the changes I made.  My
idea is to conform to what is the current "standard" coding style, and not
necessarily what was thrown into Lindent, so that running Lindent on sources
will change as little as is necessary to make it "standard".

I also checked the l-k archives a bit to confirm the changes match the
formatting of the examples given by the King Penguin.

Sadly, lksr is not responding, so I can't look at the history of changes
to Lindent.  Is there another kernel CVS with CVSWeb that was as complete
as lksr?

On Jan 04, 2002  08:05 +0100, Daniel Phillips wrote:
> On January 3, 2002 08:36 pm, Andreas Dilger wrote:
> > I removed the following two options:
> > -bs: Put a space between sizeof and its argument.

grep -r "sizeof (" linux | wc -l,
grep -r "sizeof(" linux | wc -l:

sizeof (foo): 1611, sizeof(foo): 19364 => -bs should be removed

> > -psl: Put the type of a procedure on the line before its name.

grep -r -B2 "^{" linux | grep "^[^ ]*(" | wc -l,
grep -r -B2 "^{" linux | grep "^.* .*(" | wc -l:

int
foo(int x): 11408, int foo(int x): 57275 => -psl should be removed

> > I added the following options:
> > -nbbo: don't prefer to break lines before boolean operators

grep -r "[&|][&|][ ^I]*$" | wc -l,
grep -r "^[ ^I]*[&|][&|]" | wc -l:

	&& foo): 3338,
	(foo &&: 12003 => -nbbo should be added

> > -ci8: indent continuation lines 8 characters

Hard to measure.

> > -ncs: Do not put a space after cast operators.

grep -r "\*) [a-z_(]" . | wc -l,
grep -r "\*)[a-z_(]" . | wc -l:

(void *) foo: 11274, (void *)foo: 17062 => -ncs should be added

> Not putting a space after a cast is gross ;)

Well, it seems you are in the (slight) minority on this one.  It's not as
big a margin as the other ones, but still measurable.  I wasn't able to
find any examples from the King Penguin himself on this one.  Maybe that
means casts are evil and we should strive to rid the world of them? ;-)

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Re: [CFT] [JANITORIAL] Unbork fs.h

[Jeff Garzik]

Andreas Dilger wrote:
> Well, it seems you are in the (slight) minority on this one.  It's not as
> big a margin as the other ones, but still measurable.  I wasn't able to
> find any examples from the King Penguin himself on this one.  Maybe that
> means casts are evil and we should strive to rid the world of them? ;-)

Correct conclusion IMHO ;-)

-- 
Jeff Garzik      | Only so many songs can be sung
Building 1024    | with two lips, two lungs, and one tongue.
MandrakeSoft     |         - nomeansno

Re: [CFT] [JANITORIAL] Unbork fs.h

[J.A. Magallon]

On 20020103 Daniel Phillips wrote:
>> 
>> Maybe CodingStyle should have an entry for this, I'd vote for this style:
>>
>> static inline struct inode *new_inode(struct super_block *sb)
>
>OK, I'll revise it to that style.  Shall we start an official janitor's style
>guide? ;-)
>

Perhaps it is a silly question for kernel hackers, but I found it useful
for making code more readable...
Why dont you use things like:

typedef struct inode inode;
typedef struct super_block super_block;

so you can write things like

static inline inode* new_inode(super_block* cb)
{
	inode* ni;
	ni = (inode*)malloc(sizeof(inode));
	...
}

(ie, kill 'struct' visual pollution...)

??

Isn't it more readable ?

Just curious...

-- 
J.A. Magallon                           #  Let the source be with you...        
mailto:jamagallon@able.es
Mandrake Linux release 8.2 (Cooker) for i586
Linux werewolf 2.4.18-pre1-beo #3 SMP Thu Dec 27 10:15:27 CET 2001 i686

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

On January 4, 2002 12:25 am, J.A. Magallon wrote:
> Perhaps it is a silly question for kernel hackers, but I found it useful
> for making code more readable...
> Why dont you use things like:
> 
> typedef struct inode inode;
> typedef struct super_block super_block;
> 
> so you can write things like
> 
> static inline inode* new_inode(super_block* cb)
> {
> 	inode* ni;
> 	ni = (inode*)malloc(sizeof(inode));
> 	...
> }
> 
> (ie, kill 'struct' visual pollution...)
> 
> ??
> 
> Isn't it more readable ?
> 
> Just curious...

Needing to type 'struct' everywhere is annoying and makes for long lines.
Other than that it's harmless, and actually, the situation where you have two 
ways of spelling everything is annoying too.  Anyway, if it was to be done,
I'd spell it:

	typedef struct super_struct super;
	typedef struct inode_struct inode;

	static inline inode *new_inode(super *sb)
	{
		inode *ni = (inode *) malloc(sizeof(inode));
		...
	}

It won't happen though, because it would generate a massive diff for the sole 
reason of making things prettier, and a very high percentage of existing 
patches would break immediately.  If you're going to clean stuff up, you have 
to do it a bit at a time while you're working on other things.

--
Daniel

Re: [CFT] [JANITORIAL] Unbork fs.h

[J.A. Magallon]

On 20020104 Daniel Phillips wrote:
>
>Needing to type 'struct' everywhere is annoying and makes for long lines.
>Other than that it's harmless, and actually, the situation where you have two 
>ways of spelling everything is annoying too.  Anyway, if it was to be done,
>I'd spell it:
>
>	typedef struct super_struct super;
>	typedef struct inode_struct inode;
>
>	static inline inode *new_inode(super *sb)
>	{
>		inode *ni = (inode *) malloc(sizeof(inode));
>		...
>	}
>
>It won't happen though, because it would generate a massive diff for the sole 
>reason of making things prettier, and a very high percentage of existing 
>patches would break immediately.  If you're going to clean stuff up, you have 
>to do it a bit at a time while you're working on other things.
>

>From my point of view, this kind of changes can keep compatability and be done
in small chunks if you do something like

typedef struct inode inode_t;
typedef struct super_block super_block_t;

so old code still builds, new code can use new types and you can patch
code smoothly. And you can grep-r for both usages in the tree.

But all is a matter of preferences. I found it cleaner. Some people
hate the _t suffix. Many people prefer explicit 'struct' than opaque
types. And so on...

-- 
J.A. Magallon                           #  Let the source be with you...        
mailto:jamagallon@able.es
Mandrake Linux release 8.2 (Cooker) for i586
Linux werewolf 2.4.18-pre1-beo #1 SMP Fri Jan 4 02:25:59 CET 2002 i686

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

On January 3, 2002 05:36 pm, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 03, 2002 at 05:34:27PM +0100, Daniel Phillips escreveu:
> > On January 3, 2002 05:05 pm, Ion Badulescu wrote:
> > > Daniel Phillips wrote:
> > > > -static inline struct inode * new_inode(struct super_block *sb)
> > > > +static inline struct inode *new_inode (struct super_block *sb)
> > > 
> > > Minor issue of coding style. I'd steer away from such gratuitious changes, 
> > > especially since they divert from the commonly accepted practice of having 
> > > no spaces between the name of the function and its arguments.
> > 
> > That's good advice and I'm likely to adhere to it - if you can show that 
> > having no spaces between the name of the function and its arguments really is 
> > the accepted practice.  I've seen both styles on my various travels though 
> > the kernel, and I prefer the one with the space.  Much as I prefer to put 
> > spaces around '+' (but not around '.', go figure).
> 
> Maybe CodingStyle should have an entry for this, I'd vote for this style:
> 
> static inline struct inode *new_inode(struct super_block *sb)
> 
> > In general, I allow myself the indulgence of cleaning up the odd line here 
> > and there to be more pleasing to my eyes, so long as it's in the vicinity of 
> > a substantive change and doesn't introduce a new patch hunk.  You could think 
> > of it as a perk that takes some of the sting out of doing the grunt work.
> 
> fair, thats what I usually do as well 8)

OK, here's the new improved patch, with new improved style:

--- ../2.4.17.clean/drivers/block/rd.c	Fri Dec 21 12:41:53 2001
+++ ./drivers/block/rd.c	Thu Jan  3 10:03:49 2002
@@ -673,7 +673,7 @@
 #endif
 	ram_device = MKDEV(MAJOR_NR, unit);
 
-	if ((inode = get_empty_inode()) == NULL)
+	if ((inode = get_empty_inode(NULL)) == NULL)
 		return;
 	memset(&infile, 0, sizeof(infile));
 	memset(&in_dentry, 0, sizeof(in_dentry));
@@ -683,7 +683,7 @@
 	infile.f_op = &def_blk_fops;
 	init_special_inode(inode, S_IFBLK | S_IRUSR, kdev_t_to_nr(device));
 
-	if ((out_inode = get_empty_inode()) == NULL)
+	if ((out_inode = get_empty_inode(NULL)) == NULL)
 		goto free_inode;
 	memset(&outfile, 0, sizeof(outfile));
 	memset(&out_dentry, 0, sizeof(out_dentry));
--- ../2.4.17.clean/fs/ext2/super.c	Fri Dec 21 12:41:55 2001
+++ ./fs/ext2/super.c	Thu Jan  3 10:03:49 2002
@@ -806,16 +806,23 @@
 	return 0;
 }
 
-static DECLARE_FSTYPE_DEV(ext2_fs_type, "ext2", ext2_read_super);
-
+static struct file_system_type ext2_fs = {
+	owner:		THIS_MODULE,
+	fs_flags:	FS_REQUIRES_DEV,
+	name:		"ext2",
+	read_super:	ext2_read_super,
+	super_size:	sizeof(struct ext2_sb_info),
+	inode_size:	sizeof(struct ext2_inode_info)
+};
+ 
 static int __init init_ext2_fs(void)
 {
-        return register_filesystem(&ext2_fs_type);
+        return register_filesystem(&ext2_fs);
 }
 
 static void __exit exit_ext2_fs(void)
 {
-	unregister_filesystem(&ext2_fs_type);
+	unregister_filesystem(&ext2_fs);
 }
 
 EXPORT_NO_SYMBOLS;
--- ../2.4.17.clean/fs/inode.c	Fri Dec 21 12:41:55 2001
+++ ./fs/inode.c	Thu Jan  3 18:20:27 2002
@@ -75,29 +75,32 @@
 
 static kmem_cache_t * inode_cachep;
 
-#define alloc_inode() \
-	 ((struct inode *) kmem_cache_alloc(inode_cachep, SLAB_KERNEL))
+static inline struct inode *alloc_inode(struct super_block *sb)
+{
+	kmem_cache_t *cache = sb? sb->s_type->inode_cache: NULL;
+	return (struct inode *) kmem_cache_alloc (cache? cache: inode_cachep, SLAB_KERNEL);
+}
+
 static void destroy_inode(struct inode *inode) 
 {
+	struct super_block *sb = inode->i_sb;
+	kmem_cache_t *cache = sb? sb->s_type->inode_cache: NULL;
 	if (inode_has_buffers(inode))
 		BUG();
-	kmem_cache_free(inode_cachep, (inode));
+	kmem_cache_free (cache? cache: inode_cachep, inode);
 }
 
-
 /*
  * These are initializations that only need to be done
  * once, because the fields are idempotent across use
  * of the inode, so let the slab aware of that.
  */
-static void init_once(void * foo, kmem_cache_t * cachep, unsigned long flags)
+static void init_once(void *p, kmem_cache_t *cache, unsigned long flags)
 {
-	struct inode * inode = (struct inode *) foo;
-
-	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
-	    SLAB_CTOR_CONSTRUCTOR)
+	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) == SLAB_CTOR_CONSTRUCTOR)
 	{
-		memset(inode, 0, sizeof(*inode));
+		struct inode *inode = (struct inode *) p;
+		kmem_cache_clear (cache, inode);
 		init_waitqueue_head(&inode->i_wait);
 		INIT_LIST_HEAD(&inode->i_hash);
 		INIT_LIST_HEAD(&inode->i_data.clean_pages);
@@ -710,6 +713,7 @@
 
 int shrink_icache_memory(int priority, int gfp_mask)
 {
+	struct file_system_type *fs;
 	int count = 0;
 
 	/*
@@ -725,7 +729,15 @@
 	count = inodes_stat.nr_unused / priority;
 
 	prune_icache(count);
+#if 0
+	/* Manfred thinks this isn't necessary */
 	kmem_cache_shrink(inode_cachep);
+	write_lock(&file_systems_lock);
+	for (fs = file_systems; fs; fs = fs->next)
+		kmem_cache_shrink (fs->inode_cache);
+	write_unlock(&file_systems_lock);
+	kmem_cache_shrink(inode_cachep);
+#endif
 	return 0;
 }
 
@@ -765,12 +777,14 @@
  * i_sb, i_ino, i_count, i_state and the lists have
  * been initialized elsewhere..
  */
-static void clean_inode(struct inode *inode)
+static void clean_inode(struct super_block *sb, struct inode *inode)
 {
 	static struct address_space_operations empty_aops;
 	static struct inode_operations empty_iops;
 	static struct file_operations empty_fops;
-	memset(&inode->u, 0, sizeof(inode->u));
+	unsigned given = sb? sb->s_type->inode_size: 0; // only rd.c has null sb
+	memset(&inode->u, 0, given? given: sizeof(inode->u));
+	inode->i_sb = sb;
 	inode->i_sock = 0;
 	inode->i_op = &empty_iops;
 	inode->i_fop = &empty_fops;
@@ -802,20 +816,19 @@
  * lists.
  */
  
-struct inode * get_empty_inode(void)
+struct inode *get_empty_inode(struct super_block *sb)
 {
 	static unsigned long last_ino;
-	struct inode * inode;
+	struct inode *inode;
 
 	spin_lock_prefetch(&inode_lock);
-	
-	inode = alloc_inode();
+	inode = alloc_inode (sb);
 	if (inode)
 	{
 		spin_lock(&inode_lock);
 		inodes_stat.nr_inodes++;
 		list_add(&inode->i_list, &inode_in_use);
-		inode->i_sb = NULL;
+		inode->i_sb = NULL; // need this?
 		inode->i_dev = 0;
 		inode->i_blkbits = 0;
 		inode->i_ino = ++last_ino;
@@ -823,7 +836,7 @@
 		atomic_set(&inode->i_count, 1);
 		inode->i_state = 0;
 		spin_unlock(&inode_lock);
-		clean_inode(inode);
+		clean_inode(sb, inode);
 	}
 	return inode;
 }
@@ -838,7 +851,7 @@
 {
 	struct inode * inode;
 
-	inode = alloc_inode();
+	inode = alloc_inode (sb);
 	if (inode) {
 		struct inode * old;
 
@@ -849,7 +862,6 @@
 			inodes_stat.nr_inodes++;
 			list_add(&inode->i_list, &inode_in_use);
 			list_add(&inode->i_hash, head);
-			inode->i_sb = sb;
 			inode->i_dev = sb->s_dev;
 			inode->i_blkbits = sb->s_blocksize_bits;
 			inode->i_ino = ino;
@@ -857,8 +869,7 @@
 			atomic_set(&inode->i_count, 1);
 			inode->i_state = I_LOCK;
 			spin_unlock(&inode_lock);
-
-			clean_inode(inode);
+			clean_inode(sb, inode);
 
 			/* reiserfs specific hack right here.  We don't
 			** want this to last, and are looking for VFS changes
@@ -897,6 +908,21 @@
 		wait_on_inode(inode);
 	}
 	return inode;
+}
+
+int create_inode_cache(struct file_system_type *fs)
+{
+	if (fs->inode_size)
+		if (!(fs->inode_cache = kmem_cache_create (fs->name, 
+		    fs->inode_size + sizeof(struct inode) - sizeof(get_empty_inode(0)->u),
+		    0, SLAB_HWCACHE_ALIGN, init_once, NULL)))
+			return -ENOSPC;
+	return 0;
+}
+
+int destroy_inode_cache(struct file_system_type *fs)
+{
+	return kmem_cache_destroy (fs->inode_cache)? -EBUSY: 0;
 }
 
 static inline unsigned long hash(struct super_block *sb, unsigned long i_ino)
--- ../2.4.17.clean/fs/super.c	Fri Dec 21 12:42:03 2001
+++ ./fs/super.c	Thu Jan  3 18:05:09 2002
@@ -67,8 +67,8 @@
  *	Once the reference is obtained we can drop the spinlock.
  */
 
-static struct file_system_type *file_systems;
-static rwlock_t file_systems_lock = RW_LOCK_UNLOCKED;
+struct file_system_type *file_systems;
+rwlock_t file_systems_lock = RW_LOCK_UNLOCKED;
 
 /* WARNING: This can be used only if we _already_ own a reference */
 static void get_filesystem(struct file_system_type *fs)
@@ -105,10 +105,10 @@
  *	unregistered.
  */
  
-int register_filesystem(struct file_system_type * fs)
+int register_filesystem(struct file_system_type *fs)
 {
-	int res = 0;
-	struct file_system_type ** p;
+	struct file_system_type **p;
+	int err = 0;
 
 	if (!fs)
 		return -EINVAL;
@@ -118,11 +118,12 @@
 	write_lock(&file_systems_lock);
 	p = find_filesystem(fs->name);
 	if (*p)
-		res = -EBUSY;
+		err = -EBUSY;
 	else
-		*p = fs;
+		if (!(err = create_inode_cache (fs)))
+			*p = fs;
 	write_unlock(&file_systems_lock);
-	return res;
+	return err;
 }
 
 /**
@@ -137,23 +138,25 @@
  *	may be freed or reused.
  */
  
-int unregister_filesystem(struct file_system_type * fs)
+int unregister_filesystem(struct file_system_type *fs)
 {
-	struct file_system_type ** tmp;
-
+	struct file_system_type **p;
+	int err = -EINVAL;
 	write_lock(&file_systems_lock);
-	tmp = &file_systems;
-	while (*tmp) {
-		if (fs == *tmp) {
-			*tmp = fs->next;
+	p = &file_systems;
+	while (*p) {
+		if (*p == fs) {
+			if (fs->inode_cache && (err = destroy_inode_cache (fs)))
+				break;
+			*p = fs->next;
 			fs->next = NULL;
-			write_unlock(&file_systems_lock);
-			return 0;
+			err = 0;
+			break;
 		}
-		tmp = &(*tmp)->next;
+		p = &(*p)->next;
 	}
 	write_unlock(&file_systems_lock);
-	return -EINVAL;
+	return err;
 }
 
 static int fs_index(const char * __name)
--- ../2.4.17.clean/include/linux/fs.h	Fri Dec 21 12:42:03 2001
+++ ./include/linux/fs.h	Thu Jan  3 18:08:10 2002
@@ -691,6 +691,9 @@
 #include <linux/cramfs_fs_sb.h>
 #include <linux/jffs2_fs_sb.h>
 
+extern struct file_system_type *file_systems;
+extern rwlock_t file_systems_lock;
+
 extern struct list_head super_blocks;
 extern spinlock_t sb_lock;
 
@@ -951,10 +954,14 @@
 	int fs_flags;
 	struct super_block *(*read_super) (struct super_block *, void *, int);
 	struct module *owner;
-	struct file_system_type * next;
+	struct file_system_type *next;
 	struct list_head fs_supers;
+	unsigned super_size, inode_size;
+	struct kmem_cache_s *inode_cache;
 };
 
+/* Backward compatible declarations, remove when all updated */
+
 #define DECLARE_FSTYPE(var,type,read,flags) \
 struct file_system_type var = { \
 	name:		type, \
@@ -1328,20 +1335,21 @@
 }
 
 extern void clear_inode(struct inode *);
-extern struct inode * get_empty_inode(void);
+extern struct inode *get_empty_inode(struct super_block *sb);
 
-static inline struct inode * new_inode(struct super_block *sb)
+static inline struct inode *new_inode(struct super_block *sb)
 {
-	struct inode *inode = get_empty_inode();
+	struct inode *inode = get_empty_inode(sb);
 	if (inode) {
-		inode->i_sb = sb;
 		inode->i_dev = sb->s_dev;
 		inode->i_blkbits = sb->s_blocksize_bits;
 	}
 	return inode;
 }
-extern void remove_suid(struct inode *inode);
 
+extern int create_inode_cache(struct file_system_type *fs);
+extern int destroy_inode_cache(struct file_system_type *fs);
+extern void remove_suid(struct inode *inode);
 extern void insert_inode_hash(struct inode *);
 extern void remove_inode_hash(struct inode *);
 extern struct file * get_empty_filp(void);
--- ../2.4.17.clean/include/linux/slab.h	Fri Dec 21 12:42:04 2001
+++ ./include/linux/slab.h	Thu Jan  3 18:09:33 2002
@@ -57,6 +57,7 @@
 extern int kmem_cache_shrink(kmem_cache_t *);
 extern void *kmem_cache_alloc(kmem_cache_t *, int);
 extern void kmem_cache_free(kmem_cache_t *, void *);
+extern void kmem_cache_clear(kmem_cache_t *, void *);
 
 extern void *kmalloc(size_t, int);
 extern void kfree(const void *);
--- ../2.4.17.clean/mm/slab.c	Fri Dec 21 12:42:05 2001
+++ ./mm/slab.c	Thu Jan  3 18:08:56 2002
@@ -1078,6 +1078,16 @@
 	slabp->free = 0;
 }
 
+void kmem_cache_clear(kmem_cache_t *cachep, void *objp)
+{
+	unsigned size = cachep->objsize;
+#if DEBUG
+	if (cachep->flags & SLAB_RED_ZONE)
+		size -= BYTES_PER_WORD*2;
+#endif
+	memset(objp, 0, size);
+}
+
 /*
  * Grow (by 1) the number of slabs within a cache.  This is called by
  * kmem_cache_alloc() when there are no active objs left in a cache.
--- ../2.4.17.clean/net/socket.c	Fri Dec 21 12:42:06 2001
+++ ./net/socket.c	Thu Jan  3 10:03:49 2002
@@ -438,11 +438,10 @@
 	struct inode * inode;
 	struct socket * sock;
 
-	inode = get_empty_inode();
+	inode = get_empty_inode(sock_mnt->mnt_sb);
 	if (!inode)
 		return NULL;
 
-	inode->i_sb = sock_mnt->mnt_sb;
 	sock = socki_lookup(inode);
 
 	inode->i_mode = S_IFSOCK|S_IRWXUGO;

Re: [CFT] [JANITORIAL] Unbork fs.h

[Ion Badulescu]

On Thu, 3 Jan 2002, Daniel Phillips wrote:

> On January 3, 2002 05:05 pm, Ion Badulescu wrote:
> > Daniel Phillips wrote:
> > 
> > > +static struct file_system_type ext2_fs = {
> > > +       owner:          THIS_MODULE,
> > > +       fs_flags:       FS_REQUIRES_DEV,
> > > +       name:           "ext2",
> > > +       read_super:     ext2_read_super,
> > > +       super_size:     sizeof(struct ext2_sb_info),
> > > +       inode_size:     sizeof(struct ext2_inode_info)
> > > +};
> > 
> > While we're at it, can we extend this model to also include details about 
> > the other filesystem data structures with (potential) private info, i.e.
> > struct dentry and struct file? ext2 might not use them, but other 
> > filesystems certainly do.
> 
> Could you be more specific about what you mean, please?

If we're going to have specific slabs for each specific filesystem's inode
and superblock structures, why shouldn't we do the same with the struct
file and struct dentry? Right now, if a filesystem needs to stick some
information in there, it has to allocate and then dereference
file->private_data and dentry->d_fsdata, and I thought this cleanup was
trying to avoid that, right?

Something like this, added to the end of the filesystem declaration:
+       dentry_size:    sizeof(struct ext2_dentry_info),
+       file_size:      sizeof(struct ext2_file_info)

Obviously the above is only an example, since ext2 doesn't have those 
structures -- so it would simply use 0 instead.

>From the filesystems in the tree, only intermezzo actually allocates 
them, so it's not as pressing a matter as it was for inodes. A few others 
(ab)use the private pointer to store an integer value, without actually 
allocating a private structure.

Stackable filesystems, on the other hand, make extensive use of all these 
private data pointers to store the links to the stacking level immediately 
underneath. Alas, they're not in the tree... :-)

> That's good advice and I'm likely to adhere to it - if you can show that 
> having no spaces between the name of the function and its arguments really is 
> the accepted practice.  I've seen both styles on my various travels though 
> the kernel, and I prefer the one with the space.  Much as I prefer to put 
> spaces around '+' (but not around '.', go figure).

Well... I said "minor point" and it degenerated into a full-fledged 
thread, while nobody actually cared much about the real code... :-)

I know the issue has been hashed to death already, but basically the 
common practice I was referring to is to *have* a space after a C keyword 
(for, while, return), but *not* after a function name.

Thanks,
Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [CFT] [JANITORIAL] Unbork fs.h

[Andries.Brouwer]

    grep -r "sizeof (" linux | wc -l,
    grep -r "sizeof(" linux | wc -l:

    sizeof (foo): 1611, sizeof(foo): 19364 => -bs should be removed

    > > -psl: Put the type of a procedure on the line before its name.

    grep -r -B2 "^{" linux | grep "^[^ ]*(" | wc -l,
    grep -r -B2 "^{" linux | grep "^.* .*(" | wc -l:

    int
    foo(int x): 11408, int foo(int x): 57275 => -psl should be removed

I do not think good style is best defined by majority vote.

    grep -r "\*) [a-z_(]" . | wc -l,
    grep -r "\*)[a-z_(]" . | wc -l:

    (void *) foo: 11274, (void *)foo: 17062 => -ncs should be added

    > Not putting a space after a cast is gross ;)

    Well, it seems you are in the (slight) minority on this one.  It's not as
    big a margin as the other ones, but still measurable.  I wasn't able to
    find any examples from the King Penguin himself on this one.

Read old kernel sources.

        de = (struct minix_dir_entry *) (offset + bh->b_data);

	:"S" ((long) name),"D" ((long) buffer),"c" (len)

	if (32 != sizeof (struct minix_inode))

Re: [CFT] [JANITORIAL] Unbork fs.h

[Andreas Dilger]

On Jan 04, 2002  15:16 +0000, Andries.Brouwer@cwi.nl wrote:
>     sizeof (foo): 1611, sizeof(foo): 19364 => -bs should be removed
>
>     int
>     foo(int x): 11408, int foo(int x): 57275 => -psl should be removed
> 
> I do not think good style is best defined by majority vote.

Certainly not.  However, the Lindent style I'm trying to achieve is that
dictated by Linus.  However, CodingStyle doesn't give all of the details
of how code should be formatted, so I have to look at the code to see
what is actually there.

>     (void *) foo: 11274, (void *)foo: 17062 => -ncs should be added
> 
> Read old kernel sources.
> 
>         de = (struct minix_dir_entry *) (offset + bh->b_data);
> 
> 	:"S" ((long) name),"D" ((long) buffer),"c" (len)
> 
> 	if (32 != sizeof (struct minix_inode))

Well, that's what I was trying to do when I found out that lksr.org
(hosted by innominate.de) was not available.  It seems the -ncs
change is incorrect then (although my preference is to add it - there
doesn't seem to me to be any benefit of having the extra space).

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Re: [CFT] [JANITORIAL] Unbork fs.h

[Daniel Phillips]

On January 4, 2002 11:20 pm, Andreas Dilger wrote:
> On Jan 04, 2002  15:16 +0000, Andries.Brouwer@cwi.nl wrote:
> >     sizeof (foo): 1611, sizeof(foo): 19364 => -bs should be removed
> >
> >     int
> >     foo(int x): 11408, int foo(int x): 57275 => -psl should be removed
> > 
> > I do not think good style is best defined by majority vote.
> 
> Certainly not.  However, the Lindent style I'm trying to achieve is that
> dictated by Linus.  However, CodingStyle doesn't give all of the details
> of how code should be formatted, so I have to look at the code to see
> what is actually there.
> 
> >     (void *) foo: 11274, (void *)foo: 17062 => -ncs should be added
> > 
> > Read old kernel sources.
> > 
> >         de = (struct minix_dir_entry *) (offset + bh->b_data);
> > 
> > 	:"S" ((long) name),"D" ((long) buffer),"c" (len)
> > 
> > 	if (32 != sizeof (struct minix_inode))
> 
> Well, that's what I was trying to do when I found out that lksr.org
> (hosted by innominate.de) was not available.  It seems the -ncs
> change is incorrect then (although my preference is to add it - there
> doesn't seem to me to be any benefit of having the extra space).

Thomas Graichen told me that lksr.org will be back up 'soon'.  Innominate.org 
is back up, run by Gerrit Pape, one of a group of Berliners who call 
themselves 'exnominate' (that would include me).  In fact, innominate itself 
is back, but that's a whole nuther story.

p.s., if Andries is right - and he probably is - then it looks like Linus's 
habitual style is 'space after cast' and 'space after sizeof'.  We could 
always ask.

If we put enough work into lindent, maybe we'll come up with a Linusbot?

--
Daniel

Re: [CFT] [JANITORIAL] Unbork fs.h

[Bryan Henderson]

>>    sizeof (foo): 1611, sizeof(foo): 19364 => -bs should be removed
>> ...
>>    int
>>    foo(int x): 11408, int foo(int x): 57275 => -psl should be removed
>
>I do not think good style is best defined by majority vote.

I don't think the implication was that sizeof(foo) is better style because
more people like it.  The implication is that consistency is, in general,
good programming style and it's easier to arrive at consistency by adhering
to the majority style than by adhering to the minority style.

Of course, there are other variables that may in any particular case have
more weight than the consistency or minimal effort considerations.

And I don't see what any of this has to do with whether an option should be
removed from Lindent.  Lindent should be a tool, which means it helps a
user do whatever he wants to do.  Whether he should want to do "sizeof
(foo)" is a separate issue.

Re: [CFT] [JANITORIAL] Unbork fs.h

[Andreas Dilger]

On Jan 04, 2002  09:45 -0700, Bryan Henderson wrote:
> >>    sizeof (foo): 1611, sizeof(foo): 19364 => -bs should be removed
> >> ...
> >>    int
> >>    foo(int x): 11408, int foo(int x): 57275 => -psl should be removed
> >
> >I do not think good style is best defined by majority vote.
> 
> I don't think the implication was that sizeof(foo) is better style because
> more people like it.  The implication is that consistency is, in general,
> good programming style and it's easier to arrive at consistency by adhering
> to the majority style than by adhering to the minority style.

That was my goal.

> And I don't see what any of this has to do with whether an option should be
> removed from Lindent.  Lindent should be a tool, which means it helps a
> user do whatever he wants to do.  Whether he should want to do "sizeof
> (foo)" is a separate issue.

Well Lindent != indent.  The "indent" program can do formatting to the
wishes of the user.  However, "Lindent" is a wrapper script which is
trying to impose the will of Linus on other kernel programmers, and as
such "what the user wants to do" is of no concern.  If they don't want
to follow the "mandated" coding style, then they just don't use Lindent.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Re: [CFT] [JANITORIAL] Unbork fs.h

[Bryan Henderson]

>Well Lindent != indent.  The "indent" program can do formatting to the
>wishes of the user.  However, "Lindent" is a wrapper script which is
>trying to impose the will of Linus on other kernel programmers,

Ah.  I must have been confused (never having seen Lindent or Indent 
myself).  We're talking about options Lindent passes to Indent?  I thought 
we were talking about new options on Lindent.  Options on Lindent don't 
really make sense.

In that case, I should modify my earlier observation and say Lindent 
shouldn't follow the majority either because it defines good style _or_ 
because it achieves consistency.  It should do it because the majority 
style is evidence of what Linus likes.