hundreds of mount --bind mountpoints?

DISCUSSION Ed Tomlinson 5 participants 10 messages Apr 24, 2001 – Apr 26, 2001

Subsystems (2)

VFS HFS filesystem

lore

Share & tools

lore

Ed Tomlinson

author

2001-04-24 00:16 UTC #0 lore raw

Quoted 5 lines · Andreas Dilger

Consider, when I was doing some fs benchmark, my inode slab cache was over 120k items on a 128MB machine. At 480 butes per inode, this is almost 58 MB, close to half of RAM. Reducing this to exactly ext2 sized inodes would save (50 - 27) * 4 * 120k = 11MB of memory (on 32-bit systems)!!! (This assumes nfs_inode_info is the largest).

Was this with a recient kernel (post Alexander Viro's dcache pressure fix)? If not I suggest rerunning the benchmark. I had/have a patch to apply pressure to the dcache and icache from kswapd but its not been needed here since the above fix.

Ed Tomlinson <[email protected]>

Andreas Dilger

2001-04-24 03:57 UTC #1 lore raw

Quoted 10 lines · Ed Tomlinson

Quoted 5 lines
Consider, when I was doing some fs benchmark, my inode slab cache was over 120k items on a 128MB machine. At 480 butes per inode, this is almost 58 MB, close to half of RAM. Reducing this to exactly ext2 sized inodes would save (50 - 27) * 4 * 120k = 11MB of memory (on 32-bit systems)!!! (This assumes nfs_inode_info is the largest).
Was this with a recient kernel (post Alexander Viro's dcache pressure fix)? If not I suggest rerunning the benchmark. I had/have a patch to apply pressure to the dcache and icache from kswapd but its not been needed here since the above fix.

Actually, it had the dcache patch but I'm not aware of a patch from Al to change icache behaviour. In any case, changing the icache behaviour is not what I'm getting at here - having the union of all private inode structs in the generic inode is a huge waste of RAM. Even for filesystems that are heavy NFS users, they will likely still have a considerable amount of local filesystem space (excluding NFS root systems, which are very few).

Al posted a patch to the NFS code which removes nfs_inode_info from the inode union. Since it is (AFAIK) the largest member of the union, we have just saved 24 bytes per inode (hfs_inode_info is also rather large). If we removed hfs_inode_info as well, we would save 108 bytes per inode, about 22% ({ext2,affs,ufs}_inode_info are all about the same size).

No point in punishing all users for filesystems they don't necessarily use. Even for people that DO use NFS and/or HFS, they are probably still wasting 10k inodes * 108 bytes = 1MB of RAM for no good reason (because most of their inodes are probably not NFS and/or HFS).

Cheers, Andreas

Signature · 3 lines

Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Alexander Viro

2001-04-24 10:01 UTC #2 lore raw

Quoted 5 lines · Andreas Dilger

Al posted a patch to the NFS code which removes nfs_inode_info from the inode union. Since it is (AFAIK) the largest member of the union, we have just saved 24 bytes per inode (hfs_inode_info is also rather large). If we removed hfs_inode_info as well, we would save 108 bytes per inode, about 22% ({ext2,affs,ufs}_inode_info are all about the same size).

For fsck sake! HFS patch. Time: 14 minutes, including checking that sucker builds (it had most of the accesses to ->u.hfs_i already encapsulated).

What I really don't understand is why the hell people keep coming up with the grand and convoluted plans of removing the inode bloat and nobleedinone of them actually cared to sit down and do the simplest variant possible.

I can certainly go through the rest of filesystems and even do a testing for most of them, but WTF? Could the rest of you please join the show? It's not a fscking rocket science - encapsulate accesses to ->u.foofs_i into inlined function, find ->read_inode, find places that do get_empty_inode() or new_inode(), add allocation there, add freeing to ->clear_inode() (defining one if needed), change that inlined function so that it would return ->u.generic_ip and you are done. Clean the results up and test them. Furrfu...

It's not like it was a global change that affected the whole kernel - at every step changes are local to one filesystem and changes for different filesystems are independent from each other. If at some point in 2.5 .generic_ip is the only member of union - fine, we just do %s/u.generic_ip/fs_inode/g
or something like that. Moreover, if maintainer of filesystem foo is OK with change it _can_ be done in 2.4 - it doesn't affect anything outside of foofs.

Guys, doing all these patches is ~20 man-hours. And that's bloody generous estimate. Looking through the results and doing necessary tweaking (as in "hmm... we keep passing pointer to inode through the long chain of functions and all of them need only fs-specific part", etc.) - about the same. Verifiying that thing wasn't fucked up - maybe an hour or two of audit per filesystem (split the patch into encapsulation part - trivial to verify - and the rest - pretty small). Grrr...

Oh, well... Initial HFS patch follows:

fs/hfs/inode.c +5 -5

diff -urN S4-pre6/fs/hfs/inode.c S4-pre6-hfs/fs/hfs/inode.c
@@ -231,7 +231,7 @@
 static int hfs_prepare_write(struct file *file, struct page *page, unsigned from, unsigned to)
 {
 	return cont_prepare_write(page,from,to,hfs_get_block,
-		&page->mapping->host->u.hfs_i.mmu_private);
+		&HFS_I(page->mapping->host)->mmu_private);
 }
 static int hfs_bmap(struct address_space *mapping, long block)
 {

@@ -309,7 +309,7 @@
 	        return NULL;
 	}
 
-	if (inode->i_dev != sb->s_dev) {
+	if (inode->i_dev != sb->s_dev || !HFS_I(inode)) {
 	        iput(inode); /* automatically does an hfs_cat_put */
 		inode = NULL;
 	} else if (!inode->i_mode || (*sys_entry == NULL)) {

@@ -373,7 +373,7 @@
 		inode->i_op = &hfs_file_inode_operations;
 		inode->i_fop = &hfs_file_operations;
 		inode->i_mapping->a_ops = &hfs_aops;
-		inode->u.hfs_i.mmu_private = inode->i_size;
+		HFS_I(inode)->mmu_private = inode->i_size;
 	} else { /* Directory */
 		struct hfs_dir *hdir = &entry->u.dir;

@@ -433,7 +433,7 @@
 		inode->i_op = &hfs_file_inode_operations;
 		inode->i_fop = &hfs_file_operations;
 		inode->i_mapping->a_ops = &hfs_aops;
-		inode->u.hfs_i.mmu_private = inode->i_size;
+		HFS_I(inode)->mmu_private = inode->i_size;
 	} else { /* Directory */
 		struct hfs_dir *hdir = &entry->u.dir;

@@ -479,7 +479,7 @@
 		inode->i_op = &hfs_file_inode_operations;
 		inode->i_fop = &hfs_file_operations;
 		inode->i_mapping->a_ops = &hfs_aops;
-		inode->u.hfs_i.mmu_private = inode->i_size;
+		HFS_I(inode)->mmu_private = inode->i_size;
 	} else { /* Directory */
 		struct hfs_dir *hdir = &entry->u.dir;

fs/hfs/super.c +20 -0

diff -urN S4-pre6/fs/hfs/super.c S4-pre6-hfs/fs/hfs/super.c
@@ -35,6 +35,7 @@
 /*================ Forward declarations ================*/
 
 static void hfs_read_inode(struct inode *);
+static void hfs_clear_inode(struct inode *);
 static void hfs_put_super(struct super_block *);
 static int hfs_statfs(struct super_block *, struct statfs *);
 static void hfs_write_super(struct super_block *);

@@ -43,6 +44,7 @@
 
 static struct super_operations hfs_super_operations = { 
 	read_inode:	hfs_read_inode,
+	clear_inode:	hfs_clear_inode,
 	put_inode:	hfs_put_inode,
 	put_super:	hfs_put_super,
 	write_super:	hfs_write_super,

@@ -52,6 +54,7 @@
 /*================ File-local variables ================*/
 
 static DECLARE_FSTYPE_DEV(hfs_fs, "hfs", hfs_read_super);
+static kmem_cache_t *hfs_cachep;
 
 /*================ File-local functions ================*/

@@ -64,6 +67,15 @@
 static void hfs_read_inode(struct inode *inode)
 {
   inode->i_mode = 0;
+  inode->u.generic_ip = kmem_cache_alloc(hfs_cachep, SLAB_KERNEL);
+}
+
+static void hfs_clear_inode(struct inode *inode)
+{
+  struct hfs_inode_info *hfsi = HFS_I(inode);
+  inode->u.generic_ip = NULL;
+  if (hfsi)
+	kmem_cache_free(hfs_cachep, hfsi);
 }
 
 /*

@@ -475,12 +487,20 @@
 static int __init init_hfs_fs(void)
 {
         hfs_cat_init();
+	hfs_cachep = kmem_cache_create("hfs_inodes",
+				     sizeof(struct hfs_inode_info),
+				     0, SLAB_HWCACHE_ALIGN,
+				     NULL, NULL);
+	if (hfs_cachep == NULL)
+		return -ENOMEM;
 	return register_filesystem(&hfs_fs);
 }
 
 static void __exit exit_hfs_fs(void) {
 	hfs_cat_free();
 	unregister_filesystem(&hfs_fs);
+	if (kmem_cache_destroy(hfs_cachep))
+		printk(KERN_INFO "hfs_inodes: not all structures were freed\n");
 }
 
 module_init(init_hfs_fs)

include/linux/fs.h +0 -2

diff -urN S4-pre6/include/linux/fs.h S4-pre6-hfs/include/linux/fs.h
@@ -293,7 +293,6 @@
 #include <linux/romfs_fs_i.h>
 #include <linux/shmem_fs.h>
 #include <linux/smb_fs_i.h>
-#include <linux/hfs_fs_i.h>
 #include <linux/adfs_fs_i.h>
 #include <linux/qnx4_fs_i.h>
 #include <linux/reiserfs_fs_i.h>

@@ -457,7 +456,6 @@
 		struct shmem_inode_info		shmem_i;
 		struct coda_inode_info		coda_i;
 		struct smb_inode_info		smbfs_i;
-		struct hfs_inode_info		hfs_i;
 		struct adfs_inode_info		adfs_i;
 		struct qnx4_inode_info		qnx4_i;
 		struct reiserfs_inode_info	reiserfs_i;

include/linux/hfs_fs.h +5 -1

diff -urN S4-pre6/include/linux/hfs_fs.h S4-pre6-hfs/include/linux/hfs_fs.h
@@ -317,7 +317,11 @@
 extern int hfs_mac2triv(char *, const struct hfs_name *);
 extern void hfs_tolower(unsigned char *, int);
 
-#define	HFS_I(X)	(&((X)->u.hfs_i))
+static inline struct hfs_inode_info *HFS_I(struct inode *inode)
+{
+	return (struct hfs_inode_info *)inode->u.generic_ip;
+}
+
 #define	HFS_SB(X)	(&((X)->u.hfs_sb))
 
 static inline void hfs_nameout(struct inode *dir, struct hfs_name *out,

Andreas Dilger

2001-04-26 21:49 UTC #3 lore raw

Quoted 2 lines · Al

It's not a fscking rocket science - encapsulate accesses to ->u.foofs_i into inlined function, find ->read_inode, find places that do get_empty_inode

OK, I was doing this for the ext3 port I'm working on for 2.4, and ran into a snag. In the ext3_inode_info, there is a list_head. However, if this is moved into a separate slab struct, it is now impossible to locate the inode from the offset in the slab struct. When I was checking the size of each inode_info struct, I noticed several others that had list_heads in them. One solution is that we store list_heads in the inode proper, after generic_ip.

Cheers, Andreas

Signature · 3 lines

Andreas Dilger                               TurboLabs filesystem development
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Alexander Viro

2001-04-25 18:47 UTC #4 lore raw

Quoted 10 lines · Andreas Dilger

Quoted 2 lines · Al
It's not a fscking rocket science - encapsulate accesses to ->u.foofs_i into inlined function, find ->read_inode, find places that do get_empty_inode
OK, I was doing this for the ext3 port I'm working on for 2.4, and ran into a snag. In the ext3_inode_info, there is a list_head. However, if this is moved into a separate slab struct, it is now impossible to locate the inode from the offset in the slab struct. When I was checking the size of each inode_info struct, I noticed several others that had list_heads in them. One solution is that we store list_heads in the inode proper, after generic_ip.

If you need to go from ext3_inode_info to inode - put the pointer into the thing and be done with that. No need to bump ->i_count - fs-private part dies before inode itself.

Erik Mouw

2001-04-24 13:28 UTC #5 lore raw

In reply to Andreas Dilger

Quoted 2 lines · Alexander Viro

For fsck sake! HFS patch. Time: 14 minutes, including checking that sucker builds (it had most of the accesses to ->u.hfs_i already encapsulated).

Al is right, it is no rocket science. Here is a patch against 2.4.4-pre6 for procfs and isofs. It took me an hour to do because I'm not familiar with the fs code. It compiles, and the procfs code even runs (sorry, no CDROM player availeble on my embedded StrongARM system), though it is possible that there are some bugs in it.

Erik

Index: fs/isofs/inode.c
===================================================================
RCS file: /home/erik/cvsroot/elinux/fs/isofs/inode.c,v retrieving revision 1.1.1.24

isofs/inode.c +35 ~~-10~~

diff -d -u -r1.1.1.24 inode.c
@@ -15,6 +15,7 @@
 #include <linux/stat.h>
 #include <linux/sched.h>
 #include <linux/iso_fs.h>
+#include <linux/iso_fs_i.h>
 #include <linux/kernel.h>
 #include <linux/major.h>
 #include <linux/mm.h>

@@ -44,6 +45,8 @@
 static int check_bread = 0;
 #endif
 
+static kmem_cache_t *isofs_cachep;
+
 static int isofs_hashi(struct dentry *parent, struct qstr *qstr);
 static int isofs_hash(struct dentry *parent, struct qstr *qstr);
 static int isofs_dentry_cmpi(struct dentry *dentry, struct qstr *a, struct qstr *b);

@@ -74,10 +77,12 @@
 }
 
 static void isofs_read_inode(struct inode *);
+static void isofs_clear_inode(struct inode *);
 static int isofs_statfs (struct super_block *, struct statfs *);
 
 static struct super_operations isofs_sops = {
 	read_inode:	isofs_read_inode,
+	clear_inode:    isofs_clear_inode,
 	put_super:	isofs_put_super,
 	statfs:		isofs_statfs,
 };

@@ -908,9 +913,9 @@
 		goto abort_beyond_end;
 
 	offset    = 0;
-	firstext  = inode->u.isofs_i.i_first_extent;
-	sect_size = inode->u.isofs_i.i_section_size >> ISOFS_BUFFER_BITS(inode);
-	nextino   = inode->u.isofs_i.i_next_section_ino;
+	firstext  = ISOFS_I(inode)->i_first_extent;
+	sect_size = ISOFS_I(inode)->i_section_size >> ISOFS_BUFFER_BITS(inode);
+	nextino   = ISOFS_I(inode)->i_next_section_ino;
 
 	i = 0;
 	if (nextino) {

@@ -923,9 +928,9 @@
 			ninode = iget(inode->i_sb, nextino);
 			if (!ninode)
 				goto abort;
-			firstext  = ninode->u.isofs_i.i_first_extent;
-			sect_size = ninode->u.isofs_i.i_section_size;
-			nextino   = ninode->u.isofs_i.i_next_section_ino;
+			firstext  = ISOFS_I(ninode)->i_first_extent;
+			sect_size = ISOFS_I(ninode)->i_section_size;
+			nextino   = ISOFS_I(ninode)->i_next_section_ino;
 			iput(ninode);
 
 			if (++i > 100)

@@ -1025,7 +1030,7 @@
 	struct iso_directory_record * tmpde = NULL;
 
 	inode->i_size = 0;
-	inode->u.isofs_i.i_next_section_ino = 0;
+	ISOFS_I(inode)->i_next_section_ino = 0;
 
 	block = f_pos >> ISOFS_BUFFER_BITS(inode);
 	offset = f_pos & (bufsize-1);

@@ -1077,7 +1082,7 @@
 
 		inode->i_size += isonum_733(de->size);
 		if (i == 1)
-			inode->u.isofs_i.i_next_section_ino = f_pos;
+			ISOFS_I(inode)->i_next_section_ino = f_pos;
 
 		more_entries = de->flags[-high_sierra] & 0x80;

@@ -1174,9 +1179,10 @@
 	inode->i_uid = inode->i_sb->u.isofs_sb.s_uid;
 	inode->i_gid = inode->i_sb->u.isofs_sb.s_gid;
 	inode->i_blocks = inode->i_blksize = 0;
+	inode->u.generic_ip = kmem_cache_alloc(isofs_cachep, SLAB_KERNEL);
 
 
-	inode->u.isofs_i.i_section_size = isonum_733 (de->size);
+	ISOFS_I(inode)->i_section_size = isonum_733 (de->size);
 	if(de->flags[-high_sierra] & 0x80) {
 		if(isofs_read_level3_size(inode)) goto fail;
 	} else {

@@ -1230,7 +1236,7 @@
 	inode->i_mtime = inode->i_atime = inode->i_ctime =
 		iso_date(de->date, high_sierra);
 
-	inode->u.isofs_i.i_first_extent = (isonum_733 (de->extent) +
+	ISOFS_I(inode)->i_first_extent = (isonum_733 (de->extent) +
 					   isonum_711 (de->ext_attr_length));
 
 	/*

@@ -1298,6 +1304,16 @@
 	goto out;
 }
 
+
+static void isofs_clear_inode(struct inode *inode)
+{
+	struct iso_inode_info *isofsi = ISOFS_I(inode);
+	inode->u.generic_ip = NULL;
+	if(isofsi)
+		kmem_cache_free(isofs_cachep, isofsi);
+}
+
+
 #ifdef LEAK_CHECK
 #undef malloc
 #undef free_s

@@ -1332,12 +1348,21 @@
 
 static int __init init_iso9660_fs(void)
 {
+	isofs_cachep = kmem_cache_create("isofs_inodes",
+					 sizeof(struct iso_inode_info),
+					 0, SLAB_HWCACHE_ALIGN,
+					 NULL, NULL);
+	if(isofs_cachep == NULL)
+		return -ENOMEM;
+
         return register_filesystem(&iso9660_fs_type);
 }
 
 static void __exit exit_iso9660_fs(void)
 {
         unregister_filesystem(&iso9660_fs_type);
+	if(kmem_cache_destroy(isofs_cachep))
+		printk(KERN_INFO "isofs_inodes: not all structures were freed\n");
 }
 
 EXPORT_NO_SYMBOLS;

Index: fs/isofs/namei.c
===================================================================
RCS file: /home/erik/cvsroot/elinux/fs/isofs/namei.c,v retrieving revision 1.1.1.10

isofs/namei.c +2 -1

diff -d -u -r1.1.1.10 namei.c
@@ -8,6 +8,7 @@
 
 #include <linux/sched.h>
 #include <linux/iso_fs.h>
+#include <linux/iso_fs_i.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/stat.h>

@@ -65,7 +66,7 @@
 	unsigned int block, f_pos, offset;
 	struct buffer_head * bh = NULL;
 
-	if (!dir->u.isofs_i.i_first_extent)
+	if (!ISOFS_I(dir)->i_first_extent)
 		return 0;
   
 	f_pos = 0;

Index: fs/isofs/rock.c
===================================================================
RCS file: /home/erik/cvsroot/elinux/fs/isofs/rock.c,v retrieving revision 1.1.1.53

isofs/rock.c +3 -2

diff -d -u -r1.1.1.53 rock.c
@@ -9,6 +9,7 @@
 #include <linux/stat.h>
 #include <linux/sched.h>
 #include <linux/iso_fs.h>
+#include <linux/iso_fs_i.h>
 #include <linux/string.h>
 #include <linux/mm.h>
 #include <linux/slab.h>

@@ -349,9 +350,9 @@
 	printk(KERN_WARNING "Attempt to read inode for relocated directory\n");
 	goto out;
       case SIG('C','L'):
-	inode->u.isofs_i.i_first_extent = isonum_733(rr->u.CL.location);
+	ISOFS_I(inode)->i_first_extent = isonum_733(rr->u.CL.location);
 	reloc = iget(inode->i_sb,
-		     (inode->u.isofs_i.i_first_extent <<
+		     (ISOFS_I(inode)->i_first_extent <<
 		      inode -> i_sb -> u.isofs_sb.s_log_zone_size));
 	if (!reloc)
 		goto out;

Index: fs/proc/base.c
===================================================================
RCS file: /home/erik/cvsroot/elinux/fs/proc/base.c,v retrieving revision 1.1.1.7

proc/base.c +46 ~~-42~~

264 lines not on this page · raw message

Index: fs/proc/generic.c
===================================================================
RCS file: /home/erik/cvsroot/elinux/fs/proc/generic.c,v retrieving revision 1.1.1.24

proc/generic.c +3 -0

diff -d -u -r1.1.1.24 generic.c
@@ -445,6 +445,9 @@
 	const char *fn = name;
 	int len;
 
+	if (! (S_ISCHR(mode) || S_ISBLK(mode)))
+		BUG();
+
 	if (!parent && xlate_proc_name(name, &parent, &fn) != 0)
 		goto out;
 	len = strlen(fn);

Index: fs/proc/inode.c
===================================================================
RCS file: /home/erik/cvsroot/elinux/fs/proc/inode.c,v retrieving revision 1.1.1.7

proc/inode.c +9 -0

diff -d -u -r1.1.1.7 inode.c
@@ -80,6 +80,14 @@
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 }
 
+static void proc_clear_inode(struct inode *inode)
+{
+	struct proc_inode_info *proci = PROCFS_I(inode);
+	inode->u.generic_ip = NULL;
+	if (proci)
+		kmem_cache_free(proc_cachep, proci);
+}
+
 static int proc_statfs(struct super_block *sb, struct statfs *buf)
 {
 	buf->f_type = PROC_SUPER_MAGIC;

@@ -93,6 +101,7 @@
 
 static struct super_operations proc_sops = { 
 	read_inode:	proc_read_inode,
+	clear_inode:    proc_clear_inode,
 	put_inode:	force_delete,
 	delete_inode:	proc_delete_inode,
 	statfs:		proc_statfs,

Index: fs/proc/procfs_syms.c
===================================================================
RCS file: /home/erik/cvsroot/elinux/fs/proc/procfs_syms.c,v retrieving revision 1.1.1.12

proc/procfs_syms.c +30 -8

diff -d -u -r1.1.1.12 procfs_syms.c
@@ -2,6 +2,7 @@
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/proc_fs.h>
+#include <linux/proc_fs_i.h>
 #include <linux/init.h>
 
 extern struct proc_dir_entry *proc_sys_root;

@@ -21,18 +22,37 @@
 EXPORT_SYMBOL(proc_root_driver);
 
 static DECLARE_FSTYPE(proc_fs_type, "proc", proc_read_super, FS_SINGLE);
+kmem_cache_t *proc_cachep;
 
 static int __init init_proc_fs(void)
 {
-	int err = register_filesystem(&proc_fs_type);
-	if (!err) {
-		proc_mnt = kern_mount(&proc_fs_type);
-		err = PTR_ERR(proc_mnt);
-		if (IS_ERR(proc_mnt))
-			unregister_filesystem(&proc_fs_type);
-		else
-			err = 0;
+	int err;
+
+	proc_cachep = kmem_cache_create("proc_inodes",
+					sizeof(struct proc_inode_info),
+					0, SLAB_HWCACHE_ALIGN,
+					NULL, NULL);
+
+	if (proc_cachep == NULL)
+		return -ENOMEM;
+
+	err = register_filesystem(&proc_fs_type);
+	if(err)
+		goto badproc;
+		
+	proc_mnt = kern_mount(&proc_fs_type);
+	err = PTR_ERR(proc_mnt);
+	if (IS_ERR(proc_mnt)) {
+		unregister_filesystem(&proc_fs_type);
+		goto badproc;
 	}
+
+	return 0;
+
+badproc:
+	if (kmem_cache_destroy(proc_cachep))
+		printk(KERN_INFO "proc_inodes: not all structures were freed\n");
+
 	return err;
 }

@@ -40,6 +60,8 @@
 {
 	unregister_filesystem(&proc_fs_type);
 	kern_umount(proc_mnt);
+	if (kmem_cache_destroy(proc_cachep))
+		printk(KERN_INFO "proc_inodes: not all structures were freed\n");
 }
 
 module_init(init_proc_fs)

Index: include/linux/fs.h
===================================================================
RCS file: /home/erik/cvsroot/elinux/include/linux/fs.h,v retrieving revision 1.1.1.44

linux/fs.h +0 -4

diff -d -u -r1.1.1.44 fs.h
@@ -283,7 +283,6 @@
 #include <linux/ntfs_fs_i.h>
 #include <linux/msdos_fs_i.h>
 #include <linux/umsdos_fs_i.h>
-#include <linux/iso_fs_i.h>
 #include <linux/nfs_fs_i.h>
 #include <linux/sysv_fs_i.h>
 #include <linux/affs_fs_i.h>

@@ -300,7 +299,6 @@
 #include <linux/bfs_fs_i.h>
 #include <linux/udf_fs_i.h>
 #include <linux/ncp_fs_i.h>
-#include <linux/proc_fs_i.h>
 #include <linux/usbdev_fs_i.h>
 
 /*

@@ -447,7 +445,6 @@
 		struct ntfs_inode_info		ntfs_i;
 		struct msdos_inode_info		msdos_i;
 		struct umsdos_inode_info	umsdos_i;
-		struct iso_inode_info		isofs_i;
 		struct nfs_inode_info		nfs_i;
 		struct sysv_inode_info		sysv_i;
 		struct affs_inode_info		affs_i;

@@ -464,7 +461,6 @@
 		struct bfs_inode_info		bfs_i;
 		struct udf_inode_info		udf_i;
 		struct ncp_inode_info		ncpfs_i;
-		struct proc_inode_info		proc_i;
 		struct socket			socket_i;
 		struct usbdev_inode_info        usbdev_i;
 		void				*generic_ip;

Index: include/linux/iso_fs.h
===================================================================
RCS file: /home/erik/cvsroot/elinux/include/linux/iso_fs.h,v retrieving revision 1.1.1.9

linux/iso_fs.h +5 -0

diff -d -u -r1.1.1.9 iso_fs.h
@@ -203,6 +203,11 @@
 extern void leak_check_brelse(struct buffer_head * bh);
 #endif /* LEAK_CHECK */
 
+static inline struct iso_inode_info *ISOFS_I(struct inode *inode)
+{
+	return (struct iso_inode_info *)inode->u.generic_ip;
+}
+
 #endif /* __KERNEL__ */
 
 #endif

Index: include/linux/proc_fs.h
===================================================================
RCS file: /home/erik/cvsroot/elinux/include/linux/proc_fs.h,v retrieving revision 1.1.1.15

linux/proc_fs.h +7 -0

diff -d -u -r1.1.1.15 proc_fs.h
@@ -83,6 +83,8 @@
 extern struct proc_dir_entry *proc_root_driver;
 extern struct proc_dir_entry *proc_root_kcore;
 
+extern kmem_cache_t *proc_cachep;
+
 extern void proc_root_init(void);
 extern void proc_misc_init(void);

@@ -162,6 +164,11 @@
 static inline void proc_net_remove(const char *name)
 {
 	remove_proc_entry(name,proc_net);
+}
+
+static inline struct proc_inode_info *PROCFS_I(struct inode * inode)
+{
+	return (struct proc_inode_info *)inode->u.generic_ip;
 }
 
 #else

Signature · 5 lines

J.A.K. (Erik) Mouw, Information and Communication Theory Group, Department
of Electrical Engineering, Faculty of Information Technology and Systems,
Delft University of Technology, PO BOX 5031,  2600 GA Delft, The Netherlands
Phone: +31-15-2783635  Fax: +31-15-2781843  Email: [email protected]
WWW: http://www-ict.its.tudelft.nl/~erik/

Andreas Dilger

2001-04-24 18:49 UTC #6 lore raw

Quoted 5 lines · Eric Mouw

Al is right, it is no rocket science. Here is a patch against 2.4.4-pre6 for procfs and isofs. It took me an hour to do because I'm not familiar with the fs code. It compiles, and the procfs code even runs (sorry, no CDROM player availeble on my embedded StrongARM system), though it is possible that there are some bugs in it.

While I applaud your initiative, you made an unfortunate choice of filesystems to convert. The iso_inode_info is only 4*__u32, as is proc_inode_info. Given that we still need to keep a pointer to the external info structs, and the overhead of the slab cache itself (both CPU usage and memory overhead, however small), I don't think it is worthwhile to have isofs and procfs in separate slabs.

On the other hand, sockets and shmem are both relatively large... Watch out that the *_inode_info structs have all of the fields initialized, because the union field is zeroed for us, but slab is not.

Cheers, Andreas

Signature · 3 lines

Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Alexander Viro

2001-04-24 18:53 UTC #7 lore raw

Quoted 10 lines · Andreas Dilger

While I applaud your initiative, you made an unfortunate choice of filesystems to convert. The iso_inode_info is only 4*__u32, as is proc_inode_info. Given that we still need to keep a pointer to the external info structs, and the overhead of the slab cache itself (both CPU usage and memory overhead, however small), I don't think it is worthwhile to have isofs and procfs in separate slabs.
On the other hand, sockets and shmem are both relatively large... Watch out that the *_inode_info structs have all of the fields initialized, because the union field is zeroed for us, but slab is not.

Frankly, I'd rather start with encapsulation part. It's easy to verify, it can go in right now and it makes separate allocation part uncluttered. Besides, it simply makes code cleaner, so it makes sense even if don't want to go for separate allocation for that particular fs.

Christoph Rohland

2001-04-25 07:29 UTC #8 lore raw

In reply to Andreas Dilger

Hi Andreas,

Quoted 1 line · Andreas Dilger

On the other hand, sockets and shmem are both relatively large...

shmem is only large because the union is large. I introduced the direct swap array of size SHMEM_NR_DIRECT simply to take advantage of the union. We can decrease SHMEM_NR_DIRECT very easily. I am thinking about 1 or 5 which would mean that we allocate an indirect block for files bigger than 4k or 20k respectively.

The shmem_inode_info would then be 8 or 12 words.

Greetings
Christoph

Erik Mouw

2001-04-24 20:46 UTC #9 lore raw

In reply to Erik Mouw

Quoted 6 lines · Andreas Dilger

While I applaud your initiative, you made an unfortunate choice of filesystems to convert. The iso_inode_info is only 4*__u32, as is proc_inode_info. Given that we still need to keep a pointer to the external info structs, and the overhead of the slab cache itself (both CPU usage and memory overhead, however small), I don't think it is worthwhile to have isofs and procfs in separate slabs.

Well, I know a little bit about procfs because I'm currently documenting it, so that's why I picked it first. After I got the idea, isofs was quite easy.

In retrospect it would have been more effective to pick a filesystem with a larger *_inode_info field, but then again: Al is right. Struct inode is cluttered with *_inode_info fields, while we use anonymous data entries in other parts of the kernel (like the data pointer in struct proc_dir_entry, or the priv pointer in struct net_device).

There is another advantage: suppose you're hacking on a filesystem and change it's *_fs_i.h header. With Al's proposal you only have to recompile the filesystem you're hacking on, while you have to recompile the complete kernel in the current situation.

Erik

Signature · 5 lines

J.A.K. (Erik) Mouw, Information and Communication Theory Group, Department
of Electrical Engineering, Faculty of Information Technology and Systems,
Delft University of Technology, PO BOX 5031,  2600 GA Delft, The Netherlands
Phone: +31-15-2783635  Fax: +31-15-2781843  Email: [email protected]
WWW: http://www-ict.its.tudelft.nl/~erik/