[RFC PATCH 0/5] Put struct inode on a diet

[RFC PATCH 0/5] Put struct inode on a diet

[Theodore Ts'o]

These patches attempt to slim down struct inode to reduce the overhead
of storing cached inodes.  A typical Linux system has hundreds of
thousands of inodes cached, if not more, so reducing the inode size is a
high leverage thing to do.  For example these patches save 24 bytes, or
a little over 4% of an inode (with no debugging enabled) on an x86_64
machine.  But on a system with 160,000 inodes in its inode cache, this
might mean a difference of six and half megabytes of physical memory
(once SLAB rounding issues are taken into account).

Theodore Ts'o (5):
  fs: i_flags and i_state in struct inode only need to be unsigned
    short
  fs: Remove i_cindex from struct inode
  fs: Slim down inode by only using an unsigned int for i_dnotify_mask
  fs: Rearrange inode structure elements to avoid waste due to padding
  Dump the inode structure (for debugging purposes only)

 drivers/ieee1394/dv1394.c        |    5 +-
 drivers/ieee1394/ieee1394_core.h |    5 ++-
 fs/Makefile                      |    1 +
 fs/char_dev.c                    |   14 ++++++-
 fs/inode-struct-dumper.c         |   87 ++++++++++++++++++++++++++++++++++++++
 include/linux/cdev.h             |    2 +
 include/linux/dnotify.h          |    2 +-
 include/linux/fs.h               |    9 ++--
 8 files changed, 115 insertions(+), 10 deletions(-)
 create mode 100644 fs/inode-struct-dumper.c

[PATCH 1/5] fs: i_flags and i_state in struct inode only need to be unsigned short

[Theodore Ts'o]

Currently i_flags and i_state do not need to be an unsigned int and an
unsigned long, respectively.  (We currently use 9 i_flags bits, and 8
i_state bits.)  Changing them to be an unsigned short saves 4 bytes
per inode on an x86 platform, and 8 bytes on an x86_64 platform.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: linux-fsdevel@vger.kernel.org
---
 include/linux/fs.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3b534e5..9b20f13 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -765,10 +765,10 @@ struct inode {
 	struct mutex		inotify_mutex;	/* protects the watches list */
 #endif
 
-	unsigned long		i_state;
 	unsigned long		dirtied_when;	/* jiffies of first dirtying */
 
-	unsigned int		i_flags;
+	unsigned short		i_state;
+	unsigned short		i_flags;
 
 	atomic_t		i_writecount;
 #ifdef CONFIG_SECURITY
-- 
1.6.3.1.1.g75fc.dirty

[PATCH 2/5] fs: Remove i_cindex from struct inode

[Theodore Ts'o]

The only user of the i_cindex element in the inode structure is used
is by the firewire drivers.  As part of an attempt to slim down the
inode structure to save memory --- since a typical Linux system will
have hundreds of thousands if not millions of inodes cached, a
reduction in the size inode has high leverage.

The firewire driver does not need i_cindex in any fast path, so it's
simple enough to calculate when it is needed, instead of wasting space
in the inode structure.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: krh@redhat.com
Cc: stefanr@s5r6.in-berlin.de
Cc: linux-fsdevel@vger.kernel.org
---
 drivers/ieee1394/dv1394.c        |    5 +++--
 drivers/ieee1394/ieee1394_core.h |    5 ++++-
 fs/char_dev.c                    |   14 +++++++++++++-
 include/linux/cdev.h             |    2 ++
 include/linux/fs.h               |    1 -
 5 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/ieee1394/dv1394.c b/drivers/ieee1394/dv1394.c
index 823a629..2cd00b5 100644
--- a/drivers/ieee1394/dv1394.c
+++ b/drivers/ieee1394/dv1394.c
@@ -1789,12 +1789,13 @@ static int dv1394_open(struct inode *inode, struct file *file)
 	} else {
 		/* look up the card by ID */
 		unsigned long flags;
+		int idx = ieee1394_file_to_instance(file);
 
 		spin_lock_irqsave(&dv1394_cards_lock, flags);
 		if (!list_empty(&dv1394_cards)) {
 			struct video_card *p;
 			list_for_each_entry(p, &dv1394_cards, list) {
-				if ((p->id) == ieee1394_file_to_instance(file)) {
+				if ((p->id) == idx) {
 					video = p;
 					break;
 				}
@@ -1803,7 +1804,7 @@ static int dv1394_open(struct inode *inode, struct file *file)
 		spin_unlock_irqrestore(&dv1394_cards_lock, flags);
 
 		if (!video) {
-			debug_printk("dv1394: OHCI card %d not found", ieee1394_file_to_instance(file));
+			debug_printk("dv1394: OHCI card %d not found", idx);
 			return -ENODEV;
 		}
 
diff --git a/drivers/ieee1394/ieee1394_core.h b/drivers/ieee1394/ieee1394_core.h
index 21d50f7..fda0f5e 100644
--- a/drivers/ieee1394/ieee1394_core.h
+++ b/drivers/ieee1394/ieee1394_core.h
@@ -155,7 +155,10 @@ void hpsb_packet_received(struct hpsb_host *host, quadlet_t *data, size_t size,
  */
 static inline unsigned char ieee1394_file_to_instance(struct file *file)
 {
-	return file->f_path.dentry->d_inode->i_cindex;
+	int idx = cdev_index(file->f_path.dentry->d_inode);
+	if (idx < 0)
+		idx = 0;
+	return idx;
 }
 
 extern int hpsb_disable_irm;
diff --git a/fs/char_dev.c b/fs/char_dev.c
index 38f7122..b7c9d51 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -375,7 +375,6 @@ static int chrdev_open(struct inode *inode, struct file *filp)
 		p = inode->i_cdev;
 		if (!p) {
 			inode->i_cdev = p = new;
-			inode->i_cindex = idx;
 			list_add(&inode->i_devices, &p->list);
 			new = NULL;
 		} else if (!cdev_get(p))
@@ -405,6 +404,18 @@ static int chrdev_open(struct inode *inode, struct file *filp)
 	return ret;
 }
 
+int cdev_index(struct inode *inode)
+{
+	int idx;
+	struct kobject *kobj;
+
+	kobj = kobj_lookup(cdev_map, inode->i_rdev, &idx);
+	if (!kobj)
+		return -1;
+	kobject_put(kobj);
+	return idx;
+}
+
 void cd_forget(struct inode *inode)
 {
 	spin_lock(&cdev_lock);
@@ -557,6 +568,7 @@ EXPORT_SYMBOL(cdev_init);
 EXPORT_SYMBOL(cdev_alloc);
 EXPORT_SYMBOL(cdev_del);
 EXPORT_SYMBOL(cdev_add);
+EXPORT_SYMBOL(cdev_index);
 EXPORT_SYMBOL(register_chrdev);
 EXPORT_SYMBOL(unregister_chrdev);
 EXPORT_SYMBOL(directly_mappable_cdev_bdi);
diff --git a/include/linux/cdev.h b/include/linux/cdev.h
index fb45919..f389e31 100644
--- a/include/linux/cdev.h
+++ b/include/linux/cdev.h
@@ -28,6 +28,8 @@ int cdev_add(struct cdev *, dev_t, unsigned);
 
 void cdev_del(struct cdev *);
 
+int cdev_index(struct inode *inode);
+
 void cd_forget(struct inode *);
 
 extern struct backing_dev_info directly_mappable_cdev_bdi;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9b20f13..e3b8334 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -751,7 +751,6 @@ struct inode {
 		struct block_device	*i_bdev;
 		struct cdev		*i_cdev;
 	};
-	int			i_cindex;
 
 	__u32			i_generation;
 
-- 
1.6.3.1.1.g75fc.dirty

[PATCH 3/5] fs: Slim down inode by only using an unsigned int for i_dnotify_mask

[Theodore Ts'o]

The dnotify mask values range from 0x0001 through 0x0020, as well as
0x80000000 (for DN_MULTISHOT).  There is no need for this field to be
an unsigned long (which is 8 bytes on 64-bit systems).  Using an
unsigned int will save 4 bytes for the inode structure on 64-bit
systems.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: sfr@canb.auug.org.au
Cc: linux-fsdevel@vger.kernel.org
---
 include/linux/dnotify.h |    2 +-
 include/linux/fs.h      |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/dnotify.h b/include/linux/dnotify.h
index 102a902..d573542 100644
--- a/include/linux/dnotify.h
+++ b/include/linux/dnotify.h
@@ -10,7 +10,7 @@
 
 struct dnotify_struct {
 	struct dnotify_struct *	dn_next;
-	unsigned long		dn_mask;
+	unsigned int		dn_mask;
 	int			dn_fd;
 	struct file *		dn_filp;
 	fl_owner_t		dn_owner;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e3b8334..7c73c59 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -755,7 +755,7 @@ struct inode {
 	__u32			i_generation;
 
 #ifdef CONFIG_DNOTIFY
-	unsigned long		i_dnotify_mask; /* Directory notify events */
+	unsigned int		i_dnotify_mask; /* Directory notify events */
 	struct dnotify_struct	*i_dnotify; /* for directory notifications */
 #endif
 
-- 
1.6.3.1.1.g75fc.dirty

[PATCH 4/5] fs: Rearrange inode structure elements to avoid waste due to padding

[Theodore Ts'o]

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: linux-fsdevel@vger.kernel.org
---
 include/linux/fs.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7c73c59..88bd16a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -729,8 +729,8 @@ struct inode {
 	struct timespec		i_atime;
 	struct timespec		i_mtime;
 	struct timespec		i_ctime;
-	unsigned int		i_blkbits;
 	blkcnt_t		i_blocks;
+	unsigned int		i_blkbits;
 	unsigned short          i_bytes;
 	umode_t			i_mode;
 	spinlock_t		i_lock;	/* i_blocks, i_bytes, maybe i_size */
-- 
1.6.3.1.1.g75fc.dirty

[PATCH 5/5] Dump the inode structure (for debugging purposes only)

[Theodore Ts'o]

This is not intended for merging; but it's useful for determining how
much varous fields in the struct inode space, and whether there is any
padding leading to waste, especially on a 64-bit platforms.

For example:

Begin dump of struct inode (size 1144)
i_hash    : 0 16 0
i_list    : 16 16 0
i_sb_list : 32 16 0
i_dentry  : 48 16 0
i_ino     : 64 8 0
i_count   : 72 4 0
i_nlink   : 76 4 0
i_uid     : 80 4 0
i_gid     : 84 4 0
i_rdev    : 88 4 0
i_version : 96 8 4
i_size    : 104 8 0
i_atime   : 112 16 0
i_mtime   : 128 16 0
i_ctime   : 144 16 0
i_blocks  : 160 8 0
i_blkbits : 168 4 0
i_bytes   : 172 2 0
i_mode    : 174 2 0
i_lock    : 176 64 0
i_mutex   : 240 152 0
i_alloc_sem: 392 128 0
i_op      : 520 8 0
i_fop     : 528 8 0
i_sb      : 536 8 0
i_flock   : 544 8 0
i_mapping : 552 8 0
i_data    : 560 328 0
i_dquot   : 888 16 0
i_devices : 904 16 0
i_pipe    : 920 8 0
i_bdev    : 920 8 -8
i_cdev    : 920 8 -8
i_generation: 928 4 0
i_dnotify_mask: 932 4 0
i_dnotify : 936 8 0
inotify_watches: 944 16 0
inotify_mutex: 960 152 0
dirtied_when: 1112 8 0
i_state   : 1120 2 0
i_flags   : 1122 2 0
i_writecount: 1124 4 0
i_security: 1128 8 0
i_private : 1136 8 0

This shows us that i_data, an struct address_space data structure,
takes 328 bytes; i_mutex takes 152 bytes, the i_alloc_sem rw_semaphore
takes 128 bytes, and the i_lock spinlock takes 64 bytes.  A lot of
this space is due to varous LOCKDEP and spinlock debugging options.

(With all debugging turned off, the inode structure shrinks from 1144
bytes to 560 bytes, with i_data being 144 bytes, i_mutex being 32
bytes, i_alloc_sem being 24 bytes, and i_lock taking 4 bytes.)
---
 fs/Makefile              |    1 +
 fs/inode-struct-dumper.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 0 deletions(-)
 create mode 100644 fs/inode-struct-dumper.c

diff --git a/fs/Makefile b/fs/Makefile
index af6d047..196f3f5 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_EVENTFD)		+= eventfd.o
 obj-$(CONFIG_AIO)               += aio.o
 obj-$(CONFIG_FILE_LOCKING)      += locks.o
 obj-$(CONFIG_COMPAT)		+= compat.o compat_ioctl.o
+obj-y				+= inode-struct-dumper.o	
 
 nfsd-$(CONFIG_NFSD)		:= nfsctl.o
 obj-y				+= $(nfsd-y) $(nfsd-m)
diff --git a/fs/inode-struct-dumper.c b/fs/inode-struct-dumper.c
new file mode 100644
index 0000000..c85d792
--- /dev/null
+++ b/fs/inode-struct-dumper.c
@@ -0,0 +1,87 @@
+/*
+ * inode struct dumper --- dump out the inode structure
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+
+#include <linux/kernel.h>
+#include <linux/fs.h>
+#include <linux/stddef.h>
+
+#define ELEMENT(xx) { \
+	offset=offsetof(struct inode, xx);		 \
+	size=sizeof(p->xx);					\
+	printk(KERN_NOTICE "%-10s: %d %d %d\n", #xx,		 \
+	       offset, size, offset-(last_offset+last_size));	\
+	last_offset=offset;					\
+	last_size = size;					\
+	}
+
+static int __init dumper_init(void)
+{
+	struct inode *p;
+	int offset, size, last_offset=0, last_size=0;
+
+	printk(KERN_NOTICE "Begin dump of struct inode (size %lu)\n",
+	       sizeof(struct inode));
+	ELEMENT(i_hash);
+	ELEMENT(i_list);
+	ELEMENT(i_sb_list);
+	ELEMENT(i_dentry);
+	ELEMENT(i_ino);
+	ELEMENT(i_count);
+	ELEMENT(i_nlink);
+	ELEMENT(i_uid);
+	ELEMENT(i_gid);
+	ELEMENT(i_rdev);
+	ELEMENT(i_version);
+	ELEMENT(i_size);
+#ifdef __NEED_I_SIZE_ORDERED
+	ELEMENT(i_size_seqcount);
+#endif
+	ELEMENT(i_atime);
+	ELEMENT(i_mtime);
+	ELEMENT(i_ctime);
+	ELEMENT(i_blocks);
+	ELEMENT(i_blkbits);
+	ELEMENT(i_bytes);
+	ELEMENT(i_mode);
+	ELEMENT(i_lock);
+	ELEMENT(i_mutex);
+	ELEMENT(i_alloc_sem);
+	ELEMENT(i_op);
+	ELEMENT(i_fop);
+	ELEMENT(i_sb);
+	ELEMENT(i_flock);
+	ELEMENT(i_mapping);
+	ELEMENT(i_data);
+#ifdef CONFIG_QUOTA
+	ELEMENT(i_dquot);
+#endif
+	ELEMENT(i_devices);
+	ELEMENT(i_pipe);
+	ELEMENT(i_bdev);
+	ELEMENT(i_cdev);
+	ELEMENT(i_generation);
+#ifdef CONFIG_DNOTIFY
+	ELEMENT(i_dnotify_mask);
+	ELEMENT(i_dnotify);
+#endif
+#ifdef CONFIG_INOTIFY
+	ELEMENT(inotify_watches);
+	ELEMENT(inotify_mutex);
+#endif
+	ELEMENT(dirtied_when);
+	ELEMENT(i_state);
+	ELEMENT(i_flags);
+	ELEMENT(i_writecount);
+#ifdef CONFIG_SECURITY
+	ELEMENT(i_security);
+#endif
+	ELEMENT(i_private);
+	return 0;
+}
+
+module_init(dumper_init);
+
-- 
1.6.3.1.1.g75fc.dirty

Re: [PATCH 5/5] Dump the inode structure (for debugging purposes only)

[Matthew Wilcox]

On Thu, May 21, 2009 at 04:01:03PM -0400, Theodore Ts'o wrote:
> This is not intended for merging; but it's useful for determining how
> much varous fields in the struct inode space, and whether there is any
> padding leading to waste, especially on a 64-bit platforms.

Why not just use acme's pahole?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 5/5] Dump the inode structure (for debugging purposes only)

[Theodore Tso]

On Thu, May 21, 2009 at 02:09:42PM -0600, Matthew Wilcox wrote:
> On Thu, May 21, 2009 at 04:01:03PM -0400, Theodore Ts'o wrote:
> > This is not intended for merging; but it's useful for determining how
> > much varous fields in the struct inode space, and whether there is any
> > padding leading to waste, especially on a 64-bit platforms.
> 
> Why not just use acme's pahole?
> 

Because I didn't know about it, and now that I've looked at it, it
requires compiling the kernel with CONFIG_DEBUG_INFO, which means the
kernel takes a lot longer to build.  What I did is a bit more hackish,
to be sure, but it's faster to drop it into the tree and then run it
under kvm and then pull it out of the console logs.  If you were
planning on investigating a large number of structures, acme's
solution probably is a better one; but if you only need one, it might
be faster to whip up a custom kernel module, perhaps built out of the
tree, instead of rebuilding the entire kernel with CONFIG_DEBUG_INFO.

      	      	 	    	       	      - Ted

Re: [PATCH 2/5] fs: Remove i_cindex from struct inode

[Stefan Richter]

Theodore Ts'o wrote:
[...]
> The firewire driver does not need i_cindex in any fast path, so it's
> simple enough to calculate when it is needed, instead of wasting space
> in the inode structure.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Cc: krh@redhat.com
> Cc: stefanr@s5r6.in-berlin.de
> Cc: linux-fsdevel@vger.kernel.org
> ---
>  drivers/ieee1394/dv1394.c        |    5 +++--
>  drivers/ieee1394/ieee1394_core.h |    5 ++++-
>  fs/char_dev.c                    |   14 +++++++++++++-
>  include/linux/cdev.h             |    2 ++
>  include/linux/fs.h               |    1 -
>  5 files changed, 22 insertions(+), 5 deletions(-)
[...]

Acked-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

(No Ack needed from krh, he is concerned with drivers/firewire, not 
really with driver/ieee1394. :-)
-- 
Stefan Richter
-=====-=-=== -=-= -==-=
http://arcgraph.de/sr/