Here's my idea for the solution. Each patch in the series has been
tested individually and can be applied individually, as long as all
preceding patches are applied. (ie. to apply and testing patch N,
patches 1 through N-1 must also be applied) The light testing consisted
of unpacking, catting, and removing kernel trees, along with a fillmem
runs to ensure that slab caches are properly purged. An fsck was forced
after each run of tests.
This is the first of seven steps in the Make Fs.h Happy program.
It borrows direction from Daniel and Linus as well as my own.
patch1 (this patch): use accessor function ext2_i to access inode->u.ext2_i
The rest of the patches borrows ideas but no code. This patch
is the only exception: it borrows substantially Daniel's ext2_i
patch.
patch2: use accessor function ext2_sb to access sb->u.ext2_sb
patch3: dynamically allocate sb->u.ext2_sbp
patch4: dynamically allocate inode->u.ext2_ip
patch5: move include/linux/ext2*.h to fs/ext2
at this point we've reached the limits of how far the current
VFS API will go. inode and superblock fs-level private info
is dynamically allocated.
patch6: add sb->s_op->{alloc,destroy}_inode to VFS API
patch7: implement ext2 use of s_op->{alloc,destroy}
at this point we have what Linus described:
struct ext2_inode_info {
...ext2 stuff...
struct inode inode;
};
patch1 follows...
diff -Naur -X /g/g/lib/dontdiff linux-2.5.2-pre9/fs/ext2/dir.c linux-fs1/fs/ext2/dir.c
--- linux-2.5.2-pre9/fs/ext2/dir.c Mon Sep 17 20:16:30 2001
+++ linux-fs1/fs/ext2/dir.c Sun Jan 6 23:08:18 2002
@@ -300,6 +300,7 @@
struct ext2_dir_entry_2 * ext2_find_entry (struct inode * dir,
struct dentry *dentry, struct page ** res_page)
{
+ struct ext2_inode_info *ei = ext2_i(dir);
const char *name = dentry->d_name.name;
int namelen = dentry->d_name.len;
unsigned reclen = EXT2_DIR_REC_LEN(namelen);
@@ -311,7 +312,7 @@
/* OFFSET_CACHE */
*res_page = NULL;
- start = dir->u.ext2_i.i_dir_start_lookup;
+ start = ei->i_dir_start_lookup;
if (start >= npages)
start = 0;
n = start;
@@ -336,7 +337,7 @@
found:
*res_page = page;
- dir->u.ext2_i.i_dir_start_lookup = n;
+ ei->i_dir_start_lookup = n;
return de;
}
diff -Naur -X /g/g/lib/dontdiff linux-2.5.2-pre9/fs/ext2/ialloc.c linux-fs1/fs/ext2/ialloc.c
--- linux-2.5.2-pre9/fs/ext2/ialloc.c Thu Jan 3 22:20:05 2002
+++ linux-fs1/fs/ext2/ialloc.c Mon Jan 7 00:57:57 2002
@@ -313,6 +313,8 @@
struct inode * ext2_new_inode (const struct inode * dir, int mode)
{
+ const struct ext2_inode_info *di = &dir->u.ext2_i;
+ struct ext2_inode_info *ei;
struct super_block * sb;
struct buffer_head * bh;
struct buffer_head * bh2;
@@ -327,14 +329,15 @@
inode = new_inode(sb);
if (!inode)
return ERR_PTR(-ENOMEM);
+ ei = ext2_i(inode);
lock_super (sb);
es = sb->u.ext2_sb.s_es;
repeat:
if (S_ISDIR(mode))
- group = find_group_dir(sb, dir->u.ext2_i.i_block_group);
+ group = find_group_dir(sb, di->i_block_group);
else
- group = find_group_other(sb, dir->u.ext2_i.i_block_group);
+ group = find_group_other(sb, di->i_block_group);
err = -ENOSPC;
if (group == -1)
@@ -385,12 +388,12 @@
inode->i_blksize = PAGE_SIZE; /* This is the optimal IO size (for stat), not the fs block size */
inode->i_blocks = 0;
inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
- inode->u.ext2_i.i_new_inode = 1;
- inode->u.ext2_i.i_flags = dir->u.ext2_i.i_flags;
+ ei->i_new_inode = 1;
+ ei->i_flags = di->i_flags;
if (S_ISLNK(mode))
- inode->u.ext2_i.i_flags &= ~(EXT2_IMMUTABLE_FL|EXT2_APPEND_FL);
- inode->u.ext2_i.i_block_group = group;
- if (inode->u.ext2_i.i_flags & EXT2_SYNC_FL)
+ ei->i_flags &= ~(EXT2_IMMUTABLE_FL|EXT2_APPEND_FL);
+ ei->i_block_group = group;
+ if (ei->i_flags & EXT2_SYNC_FL)
inode->i_flags |= S_SYNC;
insert_inode_hash(inode);
inode->i_generation = sb->u.ext2_sb.s_next_generation++;
diff -Naur -X /g/g/lib/dontdiff linux-2.5.2-pre9/fs/ext2/inode.c linux-fs1/fs/ext2/inode.c
--- linux-2.5.2-pre9/fs/ext2/inode.c Sat Jan 5 04:23:38 2002
+++ linux-fs1/fs/ext2/inode.c Mon Jan 7 09:19:01 2002
@@ -57,7 +57,7 @@
inode->i_ino == EXT2_ACL_IDX_INO ||
inode->i_ino == EXT2_ACL_DATA_INO)
goto no_delete;
- inode->u.ext2_i.i_dtime = CURRENT_TIME;
+ ext2_i(inode)->i_dtime = CURRENT_TIME;
mark_inode_dirty(inode);
ext2_update_inode(inode, IS_SYNC(inode));
inode->i_size = 0;
@@ -75,13 +75,14 @@
void ext2_discard_prealloc (struct inode * inode)
{
#ifdef EXT2_PREALLOCATE
+ struct ext2_inode_info *ei = ext2_i(inode);
lock_kernel();
/* Writer: ->i_prealloc* */
- if (inode->u.ext2_i.i_prealloc_count) {
- unsigned short total = inode->u.ext2_i.i_prealloc_count;
- unsigned long block = inode->u.ext2_i.i_prealloc_block;
- inode->u.ext2_i.i_prealloc_count = 0;
- inode->u.ext2_i.i_prealloc_block = 0;
+ if (ei->i_prealloc_count) {
+ unsigned short total = ei->i_prealloc_count;
+ unsigned long block = ei->i_prealloc_block;
+ ei->i_prealloc_count = 0;
+ ei->i_prealloc_block = 0;
/* Writer: end */
ext2_free_blocks (inode, block, total);
}
@@ -91,6 +92,7 @@
static int ext2_alloc_block (struct inode * inode, unsigned long goal, int *err)
{
+ struct ext2_inode_info *ei = ext2_i(inode);
#ifdef EXT2FS_DEBUG
static unsigned long alloc_hits = 0, alloc_attempts = 0;
#endif
@@ -99,12 +101,12 @@
#ifdef EXT2_PREALLOCATE
/* Writer: ->i_prealloc* */
- if (inode->u.ext2_i.i_prealloc_count &&
- (goal == inode->u.ext2_i.i_prealloc_block ||
- goal + 1 == inode->u.ext2_i.i_prealloc_block))
+ if (ei->i_prealloc_count &&
+ (goal == ei->i_prealloc_block ||
+ goal + 1 == ei->i_prealloc_block))
{
- result = inode->u.ext2_i.i_prealloc_block++;
- inode->u.ext2_i.i_prealloc_count--;
+ result = ei->i_prealloc_block++;
+ ei->i_prealloc_count--;
/* Writer: end */
ext2_debug ("preallocation hit (%lu/%lu).\n",
++alloc_hits, ++alloc_attempts);
@@ -114,8 +116,8 @@
alloc_hits, ++alloc_attempts);
if (S_ISREG(inode->i_mode))
result = ext2_new_block (inode, goal,
- &inode->u.ext2_i.i_prealloc_count,
- &inode->u.ext2_i.i_prealloc_block, err);
+ &ei->i_prealloc_count,
+ &ei->i_prealloc_block, err);
else
result = ext2_new_block (inode, goal, 0, 0, err);
}
@@ -239,13 +241,14 @@
Indirect chain[4],
int *err)
{
+ struct ext2_inode_info *ei = ext2_i(inode);
struct super_block *sb = inode->i_sb;
Indirect *p = chain;
struct buffer_head *bh;
*err = 0;
/* i_data is not going away, no lock needed */
- add_chain (chain, NULL, inode->u.ext2_i.i_data + *offsets);
+ add_chain (chain, NULL, ei->i_data + *offsets);
if (!p->key)
goto no_block;
while (--depth) {
@@ -287,7 +290,8 @@
static inline unsigned long ext2_find_near(struct inode *inode, Indirect *ind)
{
- u32 *start = ind->bh ? (u32*) ind->bh->b_data : inode->u.ext2_i.i_data;
+ struct ext2_inode_info *ei = ext2_i(inode);
+ u32 *start = ind->bh? (u32*) ind->bh->b_data: ei->i_data;
u32 *p;
/* Try to find previous block */
@@ -303,8 +307,7 @@
* It is going to be refered from inode itself? OK, just put it into
* the same cylinder group then.
*/
- return (inode->u.ext2_i.i_block_group *
- EXT2_BLOCKS_PER_GROUP(inode->i_sb)) +
+ return (ei->i_block_group * EXT2_BLOCKS_PER_GROUP(inode->i_sb)) +
le32_to_cpu(inode->i_sb->u.ext2_sb.s_es->s_first_data_block);
}
@@ -327,10 +330,11 @@
Indirect *partial,
unsigned long *goal)
{
+ struct ext2_inode_info *ei = ext2_i(inode);
/* Writer: ->i_next_alloc* */
- if (block == inode->u.ext2_i.i_next_alloc_block + 1) {
- inode->u.ext2_i.i_next_alloc_block++;
- inode->u.ext2_i.i_next_alloc_goal++;
+ if (block == ei->i_next_alloc_block + 1) {
+ ei->i_next_alloc_block++;
+ ei->i_next_alloc_goal++;
}
/* Writer: end */
/* Reader: pointers, ->i_next_alloc* */
@@ -339,8 +343,8 @@
* try the heuristic for sequential allocation,
* failing that at least try to get decent locality.
*/
- if (block == inode->u.ext2_i.i_next_alloc_block)
- *goal = inode->u.ext2_i.i_next_alloc_goal;
+ if (block == ei->i_next_alloc_block)
+ *goal = ei->i_next_alloc_goal;
if (!*goal)
*goal = ext2_find_near(inode, partial);
return 0;
@@ -407,7 +411,7 @@
mark_buffer_uptodate(bh, 1);
unlock_buffer(bh);
mark_buffer_dirty_inode(bh, inode);
- if (IS_SYNC(inode) || inode->u.ext2_i.i_osync) {
+ if (IS_SYNC(inode) || ext2_i(inode)->i_osync) {
ll_rw_block (WRITE, 1, &bh);
wait_on_buffer (bh);
}
@@ -447,6 +451,7 @@
Indirect *where,
int num)
{
+ struct ext2_inode_info *ei = ext2_i(inode);
int i;
/* Verify that place we are splicing to is still there and vacant */
@@ -459,8 +464,8 @@
/* That's it */
*where->p = where->key;
- inode->u.ext2_i.i_next_alloc_block = block;
- inode->u.ext2_i.i_next_alloc_goal = le32_to_cpu(where[num-1].key);
+ ei->i_next_alloc_block = block;
+ ei->i_next_alloc_goal = le32_to_cpu(where[num-1].key);
/* Writer: end */
@@ -471,13 +476,13 @@
/* had we spliced it onto indirect block? */
if (where->bh) {
mark_buffer_dirty_inode(where->bh, inode);
- if (IS_SYNC(inode) || inode->u.ext2_i.i_osync) {
+ if (IS_SYNC(inode) || ei->i_osync) {
ll_rw_block (WRITE, 1, &where->bh);
wait_on_buffer(where->bh);
}
}
- if (IS_SYNC(inode) || inode->u.ext2_i.i_osync)
+ if (IS_SYNC(inode) || ei->i_osync)
ext2_sync_inode (inode);
else
mark_inode_dirty(inode);
@@ -785,7 +790,8 @@
void ext2_truncate (struct inode * inode)
{
- u32 *i_data = inode->u.ext2_i.i_data;
+ struct ext2_inode_info *ei = ext2_i(inode);
+ u32 *i_data = ei->i_data;
int addr_per_block = EXT2_ADDR_PER_BLOCK(inode->i_sb);
int offsets[4];
Indirect chain[4];
@@ -878,6 +884,7 @@
void ext2_read_inode (struct inode * inode)
{
+ struct ext2_inode_info *ei = ext2_i(inode);
struct buffer_head * bh;
struct ext2_inode * raw_inode;
unsigned long block_group;
@@ -939,13 +946,13 @@
inode->i_atime = le32_to_cpu(raw_inode->i_atime);
inode->i_ctime = le32_to_cpu(raw_inode->i_ctime);
inode->i_mtime = le32_to_cpu(raw_inode->i_mtime);
- inode->u.ext2_i.i_dtime = le32_to_cpu(raw_inode->i_dtime);
+ ei->i_dtime = le32_to_cpu(raw_inode->i_dtime);
/* We now have enough fields to check if the inode was active or not.
* This is needed because nfsd might try to access dead inodes
* the test is that same one that e2fsck uses
* NeilBrown 1999oct15
*/
- if (inode->i_nlink == 0 && (inode->i_mode == 0 || inode->u.ext2_i.i_dtime)) {
+ if (inode->i_nlink == 0 && (inode->i_mode == 0 || ei->i_dtime)) {
/* this inode is deleted */
brelse (bh);
goto bad_inode;
@@ -953,25 +960,25 @@
inode->i_blksize = PAGE_SIZE; /* This is the optimal IO size (for stat), not the fs block size */
inode->i_blocks = le32_to_cpu(raw_inode->i_blocks);
inode->i_version = ++event;
- inode->u.ext2_i.i_flags = le32_to_cpu(raw_inode->i_flags);
- inode->u.ext2_i.i_faddr = le32_to_cpu(raw_inode->i_faddr);
- inode->u.ext2_i.i_frag_no = raw_inode->i_frag;
- inode->u.ext2_i.i_frag_size = raw_inode->i_fsize;
- inode->u.ext2_i.i_file_acl = le32_to_cpu(raw_inode->i_file_acl);
+ ei->i_flags = le32_to_cpu(raw_inode->i_flags);
+ ei->i_faddr = le32_to_cpu(raw_inode->i_faddr);
+ ei->i_frag_no = raw_inode->i_frag;
+ ei->i_frag_size = raw_inode->i_fsize;
+ ei->i_file_acl = le32_to_cpu(raw_inode->i_file_acl);
if (S_ISREG(inode->i_mode))
inode->i_size |= ((__u64)le32_to_cpu(raw_inode->i_size_high)) << 32;
else
- inode->u.ext2_i.i_dir_acl = le32_to_cpu(raw_inode->i_dir_acl);
+ ei->i_dir_acl = le32_to_cpu(raw_inode->i_dir_acl);
inode->i_generation = le32_to_cpu(raw_inode->i_generation);
- inode->u.ext2_i.i_prealloc_count = 0;
- inode->u.ext2_i.i_block_group = block_group;
+ ei->i_prealloc_count = 0;
+ ei->i_block_group = block_group;
/*
* NOTE! The in-memory inode i_data array is in little-endian order
* even on big-endian machines: we do NOT byteswap the block numbers!
*/
for (block = 0; block < EXT2_N_BLOCKS; block++)
- inode->u.ext2_i.i_data[block] = raw_inode->i_block[block];
+ ei->i_data[block] = raw_inode->i_block[block];
if (inode->i_ino == EXT2_ACL_IDX_INO ||
inode->i_ino == EXT2_ACL_DATA_INO)
@@ -996,19 +1003,19 @@
le32_to_cpu(raw_inode->i_block[0]));
brelse (bh);
inode->i_attr_flags = 0;
- if (inode->u.ext2_i.i_flags & EXT2_SYNC_FL) {
+ if (ei->i_flags & EXT2_SYNC_FL) {
inode->i_attr_flags |= ATTR_FLAG_SYNCRONOUS;
inode->i_flags |= S_SYNC;
}
- if (inode->u.ext2_i.i_flags & EXT2_APPEND_FL) {
+ if (ei->i_flags & EXT2_APPEND_FL) {
inode->i_attr_flags |= ATTR_FLAG_APPEND;
inode->i_flags |= S_APPEND;
}
- if (inode->u.ext2_i.i_flags & EXT2_IMMUTABLE_FL) {
+ if (ei->i_flags & EXT2_IMMUTABLE_FL) {
inode->i_attr_flags |= ATTR_FLAG_IMMUTABLE;
inode->i_flags |= S_IMMUTABLE;
}
- if (inode->u.ext2_i.i_flags & EXT2_NOATIME_FL) {
+ if (ei->i_flags & EXT2_NOATIME_FL) {
inode->i_attr_flags |= ATTR_FLAG_NOATIME;
inode->i_flags |= S_NOATIME;
}
@@ -1021,6 +1028,7 @@
static int ext2_update_inode(struct inode * inode, int do_sync)
{
+ struct ext2_inode_info *ei = ext2_i(inode);
struct buffer_head * bh;
struct ext2_inode * raw_inode;
unsigned long block_group;
@@ -1077,7 +1085,7 @@
* Fix up interoperability with old kernels. Otherwise, old inodes get
* re-used with the upper 16 bits of the uid/gid intact
*/
- if(!inode->u.ext2_i.i_dtime) {
+ if(!ei->i_dtime) {
raw_inode->i_uid_high = cpu_to_le16(high_16_bits(inode->i_uid));
raw_inode->i_gid_high = cpu_to_le16(high_16_bits(inode->i_gid));
} else {
@@ -1096,14 +1104,14 @@
raw_inode->i_ctime = cpu_to_le32(inode->i_ctime);
raw_inode->i_mtime = cpu_to_le32(inode->i_mtime);
raw_inode->i_blocks = cpu_to_le32(inode->i_blocks);
- raw_inode->i_dtime = cpu_to_le32(inode->u.ext2_i.i_dtime);
- raw_inode->i_flags = cpu_to_le32(inode->u.ext2_i.i_flags);
- raw_inode->i_faddr = cpu_to_le32(inode->u.ext2_i.i_faddr);
- raw_inode->i_frag = inode->u.ext2_i.i_frag_no;
- raw_inode->i_fsize = inode->u.ext2_i.i_frag_size;
- raw_inode->i_file_acl = cpu_to_le32(inode->u.ext2_i.i_file_acl);
+ raw_inode->i_dtime = cpu_to_le32(ei->i_dtime);
+ raw_inode->i_flags = cpu_to_le32(ei->i_flags);
+ raw_inode->i_faddr = cpu_to_le32(ei->i_faddr);
+ raw_inode->i_frag = ei->i_frag_no;
+ raw_inode->i_fsize = ei->i_frag_size;
+ raw_inode->i_file_acl = cpu_to_le32(ei->i_file_acl);
if (S_ISDIR(inode->i_mode))
- raw_inode->i_dir_acl = cpu_to_le32(inode->u.ext2_i.i_dir_acl);
+ raw_inode->i_dir_acl = cpu_to_le32(ei->i_dir_acl);
else {
raw_inode->i_size_high = cpu_to_le32(inode->i_size >> 32);
if (inode->i_size > 0x7fffffffULL) {
@@ -1129,7 +1137,7 @@
if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode))
raw_inode->i_block[0] = cpu_to_le32(kdev_t_to_nr(inode->i_rdev));
else for (block = 0; block < EXT2_N_BLOCKS; block++)
- raw_inode->i_block[block] = inode->u.ext2_i.i_data[block];
+ raw_inode->i_block[block] = ei->i_data[block];
mark_buffer_dirty(bh);
if (do_sync) {
ll_rw_block (WRITE, 1, &bh);
diff -Naur -X /g/g/lib/dontdiff linux-2.5.2-pre9/fs/ext2/ioctl.c linux-fs1/fs/ext2/ioctl.c
--- linux-2.5.2-pre9/fs/ext2/ioctl.c Wed Sep 27 20:41:33 2000
+++ linux-fs1/fs/ext2/ioctl.c Sun Jan 6 23:08:18 2002
@@ -16,13 +16,14 @@
int ext2_ioctl (struct inode * inode, struct file * filp, unsigned int cmd,
unsigned long arg)
{
+ struct ext2_inode_info *ei = ext2_i(inode);
unsigned int flags;
ext2_debug ("cmd = %u, arg = %lu\n", cmd, arg);
switch (cmd) {
case EXT2_IOC_GETFLAGS:
- flags = inode->u.ext2_i.i_flags & EXT2_FL_USER_VISIBLE;
+ flags = ei->i_flags & EXT2_FL_USER_VISIBLE;
return put_user(flags, (int *) arg);
case EXT2_IOC_SETFLAGS: {
unsigned int oldflags;
@@ -36,7 +37,7 @@
if (get_user(flags, (int *) arg))
return -EFAULT;
- oldflags = inode->u.ext2_i.i_flags;
+ oldflags = ei->i_flags;
/*
* The IMMUTABLE and APPEND_ONLY flags can only be changed by
@@ -51,7 +52,7 @@
flags = flags & EXT2_FL_USER_MODIFIABLE;
flags |= oldflags & ~EXT2_FL_USER_MODIFIABLE;
- inode->u.ext2_i.i_flags = flags;
+ ei->i_flags = flags;
if (flags & EXT2_SYNC_FL)
inode->i_flags |= S_SYNC;
diff -Naur -X /g/g/lib/dontdiff linux-2.5.2-pre9/fs/ext2/namei.c linux-fs1/fs/ext2/namei.c
--- linux-2.5.2-pre9/fs/ext2/namei.c Thu Oct 4 05:57:36 2001
+++ linux-fs1/fs/ext2/namei.c Sun Jan 6 23:08:18 2002
@@ -134,7 +134,7 @@
if (IS_ERR(inode))
goto out;
- if (l > sizeof (inode->u.ext2_i.i_data)) {
+ if (l > sizeof (ext2_i(inode)->i_data)) {
/* slow symlink */
inode->i_op = &page_symlink_inode_operations;
inode->i_mapping->a_ops = &ext2_aops;
@@ -144,7 +144,7 @@
} else {
/* fast symlink */
inode->i_op = &ext2_fast_symlink_inode_operations;
- memcpy((char*)&inode->u.ext2_i.i_data,symname,l);
+ memcpy((char *) &ext2_i(inode)->i_data, symname, l);
inode->i_size = l-1;
}
mark_inode_dirty(inode);
diff -Naur -X /g/g/lib/dontdiff linux-2.5.2-pre9/fs/ext2/symlink.c linux-fs1/fs/ext2/symlink.c
--- linux-2.5.2-pre9/fs/ext2/symlink.c Wed Sep 27 20:41:33 2000
+++ linux-fs1/fs/ext2/symlink.c Mon Jan 7 00:58:15 2002
@@ -22,14 +22,14 @@
static int ext2_readlink(struct dentry *dentry, char *buffer, int buflen)
{
- char *s = (char *)dentry->d_inode->u.ext2_i.i_data;
- return vfs_readlink(dentry, buffer, buflen, s);
+ struct ext2_inode_info *ei = ext2_i(dentry->d_inode);
+ return vfs_readlink(dentry, buffer, buflen, (char *) &ei->i_data);
}
static int ext2_follow_link(struct dentry *dentry, struct nameidata *nd)
{
- char *s = (char *)dentry->d_inode->u.ext2_i.i_data;
- return vfs_follow_link(nd, s);
+ struct ext2_inode_info *ei = ext2_i(dentry->d_inode);
+ return vfs_follow_link(nd, (char *) &ei->i_data);
}
struct inode_operations ext2_fast_symlink_inode_operations = {
diff -Naur -X /g/g/lib/dontdiff linux-2.5.2-pre9/include/linux/ext2_fs.h linux-fs1/include/linux/ext2_fs.h
--- linux-2.5.2-pre9/include/linux/ext2_fs.h Sun Dec 16 23:43:50 2001
+++ linux-fs1/include/linux/ext2_fs.h Mon Jan 7 09:24:57 2002
@@ -580,6 +580,14 @@
extern unsigned long ext2_count_free (struct buffer_head *, unsigned);
/* inode.c */
+
+static inline struct ext2_inode_info *ext2_i(struct inode *inode)
+{
+ if (!inode)
+ BUG();
+ return &inode->u.ext2_i;
+}
+
extern void ext2_read_inode (struct inode *);
extern void ext2_write_inode (struct inode *, int);
extern void ext2_put_inode (struct inode *);
Goodie. Now we need benchmarks for all the approaches... (-;
At 13:21 07/01/02, Jeff Garzik wrote:
<snip>
>patch7: implement ext2 use of s_op->{alloc,destroy}
>
> at this point we have what Linus described:
>
> struct ext2_inode_info {
> ...ext2 stuff...
> struct inode inode;
> };
If we were to raise compiler requirements to gcc-2.96 or later this could
be simplified with an annonymous struct (having elements in struct inode
with the same name as elements in ...ext2 stuff... should be a shooting
offence IMO):
struct ext2_inode_info {
...ext2 stuff...
struct inode;
};
Advantage of this would be that as far as the fs is concerned there is only
one inode and each element can just be dereferenced straight away without
need to think was that the generic inode or the fs inode and without need
for keeping two pointers around. This leads to simpler code inside the
filesystems once they adapt.
Of course fs which are not adapted would still just work with the fs_i()
and fs_sb() macros and/or using two separate pointers.
Best regards,
Anton
--
"I've not lost my mind. It's backed up on tape somewhere." - Unknown
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/
On January 7, 2002 02:21 pm, Jeff Garzik wrote:
> Here's my idea for the solution. Each patch in the series has been
> tested individually and can be applied individually, as long as all
> preceding patches are applied. (ie. to apply and testing patch N,
> patches 1 through N-1 must also be applied) The light testing consisted
> of unpacking, catting, and removing kernel trees, along with a fillmem
> runs to ensure that slab caches are properly purged. An fsck was forced
> after each run of tests.
>
> This is the first of seven steps in the Make Fs.h Happy program.
> It borrows direction from Daniel and Linus as well as my own.
>
> patch1 (this patch): use accessor function ext2_i to access inode->u.ext2_i
> The rest of the patches borrows ideas but no code. This patch
> is the only exception: it borrows substantially Daniel's ext2_i
> patch.
> patch2: use accessor function ext2_sb to access sb->u.ext2_sb
> patch3: dynamically allocate sb->u.ext2_sbp
> patch4: dynamically allocate inode->u.ext2_ip
> patch5: move include/linux/ext2*.h to fs/ext2
>
> at this point we've reached the limits of how far the current
> VFS API will go. inode and superblock fs-level private info
> is dynamically allocated.
>
> patch6: add sb->s_op->{alloc,destroy}_inode to VFS API
> patch7: implement ext2 use of s_op->{alloc,destroy}
The two main problems I see with this are:
- If a filesystem doesn't want to use genericp_ip/sbp then fs.h has
to know about it. Why should fs.h know about every filesystem in
the world?
- You are dreferencing a pointer, and have two allocations for every
inode instead of one.
It's not horrible, it's just not optimal.
Moving the ext2 headers from include/linux to fs/ext2 is an interesting
feature of your patch, though it isn't essential to the idea you're
presenting. But is there a good reason why ext2_fs_i.h and ext2_fs_sb.h
should remain separate from ext2_fs.h? It looks like gratuitous
modularity to me.
Minor nit:
if (!inode->u.ext2_ip)
BUG();
You don't have to do this, if the pointer is null you will get a perfectly
fine oops.
--
Daniel
On January 7, 2002 03:13 pm, Anton Altaparmakov wrote:
> Goodie. Now we need benchmarks for all the approaches... (-;
>
> At 13:21 07/01/02, Jeff Garzik wrote:
> <snip>
> >patch7: implement ext2 use of s_op->{alloc,destroy}
> >
> > at this point we have what Linus described:
> >
> > struct ext2_inode_info {
> > ...ext2 stuff...
> > struct inode inode;
> > };
>
> If we were to raise compiler requirements to gcc-2.96 or later this could
> be simplified with an annonymous struct (having elements in struct inode
> with the same name as elements in ...ext2 stuff... should be a shooting
> offence IMO):
>
> struct ext2_inode_info {
> ...ext2 stuff...
> struct inode;
> };
>
> Advantage of this would be that as far as the fs is concerned there is only
> one inode and each element can just be dereferenced straight away without
> need to think was that the generic inode or the fs inode and without need
> for keeping two pointers around. This leads to simpler code inside the
> filesystems once they adapt.
Interesting, it's something I've always wanted to be able to do. But I
suppose the compiler requirement is a stupport.
> Of course fs which are not adapted would still just work with the fs_i()
> and fs_sb() macros and/or using two separate pointers.
Yes, the fs_* macros are the really critical part of all this. I'd like to
get them in early, while we hash out the rest of it. I think Jeff supports
me in this, possibly Al as well.
--
Daniel
At 15:33 07/01/02, Daniel Phillips wrote:
>On January 7, 2002 03:13 pm, Anton Altaparmakov wrote:
> > Goodie. Now we need benchmarks for all the approaches... (-;
> >
> > At 13:21 07/01/02, Jeff Garzik wrote:
> > <snip>
> > >patch7: implement ext2 use of s_op->{alloc,destroy}
> > >
> > > at this point we have what Linus described:
> > >
> > > struct ext2_inode_info {
> > > ...ext2 stuff...
> > > struct inode inode;
> > > };
> >
> > If we were to raise compiler requirements to gcc-2.96 or later this could
> > be simplified with an annonymous struct (having elements in struct inode
> > with the same name as elements in ...ext2 stuff... should be a shooting
> > offence IMO):
> >
> > struct ext2_inode_info {
> > ...ext2 stuff...
> > struct inode;
> > };
> >
> > Advantage of this would be that as far as the fs is concerned there is
> only
> > one inode and each element can just be dereferenced straight away without
> > need to think was that the generic inode or the fs inode and without need
> > for keeping two pointers around. This leads to simpler code inside the
> > filesystems once they adapt.
>
>Interesting, it's something I've always wanted to be able to do. But I
>suppose the compiler requirement is a stupport.
Yes. I wonder if gcc people can be persuaded to backport annonymous
structures and unions into gcc-2.95...?
> > Of course fs which are not adapted would still just work with the fs_i()
> > and fs_sb() macros and/or using two separate pointers.
>
>Yes, the fs_* macros are the really critical part of all this. I'd like to
>get them in early, while we hash out the rest of it. I think Jeff supports
>me in this, possibly Al as well.
Absolutely. The macros (or inline functions) are essential to give us the
ability to play with the internal implementation at will. The sooner they
are merged into the kernels (preferably both 2.4 and 2.5 considering they
don't actually change how things work) the better.
Best regards,
Anton
--
"I've not lost my mind. It's backed up on tape somewhere." - Unknown
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/
On January 7, 2002 04:19 pm, Daniel Phillips wrote:
> - You are dreferencing a pointer, and have two allocations for every
> inode instead of one.
Oh no, you only have one allocator, and you have the filesystem do it, with
per-sb methods. Why is this better than having the VFS do it? Does this
imply you might have different sized inodes with different mounts of the same
filesystem?
The per-fs cost with my variant is: 4-8 bytes per filesystem, period. No
methods needed, and the object management code doesn't get replicated through
all the filesystems.
Also, having the inode point at itself is a little, hmm, 'what's wrong with
this picture', don't you think?
--
Daniel
On January 7, 2002 05:01 pm, Anton Altaparmakov wrote:
> At 15:33 07/01/02, Daniel Phillips wrote:
> >Interesting, it's something I've always wanted to be able to do. But I
> >suppose the compiler requirement is a stupport.
me: "master of the full-word typo" ----^^^^^^^^--> stopper.
--
Daniel
On Mon, Jan 07, 2002 at 04:01:33PM +0000, Anton Altaparmakov wrote:
> >> > at this point we have what Linus described:
> >> >
> >> > struct ext2_inode_info {
> >> > ...ext2 stuff...
> >> > struct inode inode;
> >> > };
> >
> >Interesting, it's something I've always wanted to be able to do. But I
> >suppose the compiler requirement is a stupport.
>
> Yes. I wonder if gcc people can be persuaded to backport annonymous
> structures and unions into gcc-2.95...?
You can fake it in a semi-messy way, using a #define, like:
#define STRUCT_INODE \
struct list_head i_hash; \
struct list_head i_list; \
struct list_head i_dentry; \
...
struct inode {
STRUCT_INODE;
};
struct ext2_inode_info {
...ext2 stuff...
STRUCT_INODE;
};
--
Mark Zealey
[email protected]
[email protected]
UL++++>$ G!>(GCM/GCS/GS/GM) dpu? s:-@ a16! C++++>$ P++++>+++++$ L+++>+++++$
!E---? W+++>$ N- !o? !w--- O? !M? !V? !PS !PE--@ PGP+? r++ !t---?@ !X---?
!R- b+ !tv b+ DI+ D+? G+++ e>+++++ !h++* r!-- y--
(http://www.geekcode.com)
On Mon, 7 Jan 2002, Daniel Phillips wrote:
> On January 7, 2002 04:19 pm, Daniel Phillips wrote:
> > - You are dreferencing a pointer, and have two allocations for every
> > inode instead of one.
>
> Oh no, you only have one allocator, and you have the filesystem do it, with
> per-sb methods. Why is this better than having the VFS do it? Does this
> imply you might have different sized inodes with different mounts of the same
> filesystem?
Because exposing sizes of private objects is Wrong.
Now, the problems I see with Jeff's variant:
a) if you make struct inode a part of ext2_inode - WTF bother with pointer?
b) ->destroy_inode() / ->clear_inode(). Merge them - that way it's one
method.
c) get_empty_inode() must die. Make it new_inode() and be done with that.
And have socket.c explicitly set ->i_dev to NODEV afterwards.
d) ext2/balloc.c cleanup probably should be merged before.
I can live with "maintain refcounts in common part and leave allocation/freeing
to filesystem". It's definitely better than allocating/freeing opaque objects
in VFS using numeric fields in fs_type.
We will need to set very strict rules on passing around/storing pointers to
ext2_inode and its ilk, though. There will be bugs when somebody just decides
that keeping such pointers might be a good idea and forgets to be nice with
->i_count. Or decrement it manually instead of calling iput(), etc.
It _MUST_ be explicitly documented - preferably beaten into skulls.
Daniel Phillips wrote:
> The two main problems I see with this are:
>
> - If a filesystem doesn't want to use genericp_ip/sbp then fs.h has
> to know about it. Why should fs.h know about every filesystem in
> the world?
We keep type information through this method. There is no ugly casting.
> - You are dreferencing a pointer, and have two allocations for every
> inode instead of one.
<blink> re-read.... With patch6/7 there is only one allocation.
> Moving the ext2 headers from include/linux to fs/ext2 is an interesting
> feature of your patch, though it isn't essential to the idea you're
> presenting. But is there a good reason why ext2_fs_i.h and ext2_fs_sb.h
> should remain separate from ext2_fs.h? It looks like gratuitous
> modularity to me.
I agree this is a better end goal; my patches simply did not take things
that far.
> Minor nit:
>
> if (!inode->u.ext2_ip)
> BUG();
>
> You don't have to do this, if the pointer is null you will get a perfectly
> fine oops.
BUG oops are a little bit more perfectly fine :), since they print out
filename and line number when CONFIG_DEBUG_BUGVERBOSE is enabled..
Jeff
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
Daniel Phillips wrote:
>
> On January 7, 2002 03:13 pm, Anton Altaparmakov wrote:
> > Goodie. Now we need benchmarks for all the approaches... (-;
> >
> > At 13:21 07/01/02, Jeff Garzik wrote:
> > <snip>
> > >patch7: implement ext2 use of s_op->{alloc,destroy}
> > >
> > > at this point we have what Linus described:
> > >
> > > struct ext2_inode_info {
> > > ...ext2 stuff...
> > > struct inode inode;
> > > };
> >
> > If we were to raise compiler requirements to gcc-2.96 or later this could
> > be simplified with an annonymous struct (having elements in struct inode
> > with the same name as elements in ...ext2 stuff... should be a shooting
> > offence IMO):
> >
> > struct ext2_inode_info {
> > ...ext2 stuff...
> > struct inode;
> > };
> >
> > Advantage of this would be that as far as the fs is concerned there is only
> > one inode and each element can just be dereferenced straight away without
> > need to think was that the generic inode or the fs inode and without need
> > for keeping two pointers around. This leads to simpler code inside the
> > filesystems once they adapt.
>
> Interesting, it's something I've always wanted to be able to do. But I
> suppose the compiler requirement is a stupport.
I am not a fan of anon unions/structs, so I must disagree with Anton
here...
> > Of course fs which are not adapted would still just work with the fs_i()
> > and fs_sb() macros and/or using two separate pointers.
>
> Yes, the fs_* macros are the really critical part of all this. I'd like to
> get them in early, while we hash out the rest of it. I think Jeff supports
> me in this, possibly Al as well.
agreed, from my side
Jeff
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
Daniel Phillips wrote:
>
> On January 7, 2002 04:19 pm, Daniel Phillips wrote:
> > - You are dreferencing a pointer, and have two allocations for every
> > inode instead of one.
>
> Oh no, you only have one allocator, and you have the filesystem do it, with
> per-sb methods. Why is this better than having the VFS do it? Does this
> imply you might have different sized inodes with different mounts of the same
> filesystem?
>
> The per-fs cost with my variant is: 4-8 bytes per filesystem, period. No
> methods needed, and the object management code doesn't get replicated through
> all the filesystems.
Having the VFS allocate objects by passing in object sizes in structs is
ugly to the extreme. So I have similar objections as Al here.
The API change as I have implemented things is more flexible. Having
the fs perform the kmem cache allocation for its inodes is much more
clean than your version IMHO, and one of the big reasons why I did
things this way. If you dislike the size of ext2_alloc_inode some of
that code can probably go into a helper macro/function.
> Also, having the inode point at itself is a little, hmm, 'what's wrong with
> this picture', don't you think?
I am very much interested in a better solution... I could not figure
out how to get a private pointer from a struct inode*, without using a
nasty OFFSET_OF macro or a pointer to self as I implemented.
Jeff
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
Alexander Viro wrote:
> Now, the problems I see with Jeff's variant:
>
> a) if you make struct inode a part of ext2_inode - WTF bother with pointer?
You mean the typed pointer inside struct inode's union? Because I
needed a way to go from struct inode to struct ext2_inode_info,
-without- a nasty cast. inode->u.ext2_ip maintains the type information
without resorting to a nastier solution like an OFFSET_OF macro.
Suggestions for improvement welcome.
> b) ->destroy_inode() / ->clear_inode(). Merge them - that way it's one
> method.
Agreed. That would be [not yet written] patch8 in my plan.
> c) get_empty_inode() must die. Make it new_inode() and be done with that.
> And have socket.c explicitly set ->i_dev to NODEV afterwards.
In my patch get_empty_inode and new_inode are completely identical.
This is easy.
> d) ext2/balloc.c cleanup probably should be merged before.
I don't have an opinion on this one way or the other...
> I can live with "maintain refcounts in common part and leave allocation/freeing
> to filesystem". It's definitely better than allocating/freeing opaque objects
> in VFS using numeric fields in fs_type.
Yes... the opacity factor in the other patch bothers me.
> We will need to set very strict rules on passing around/storing pointers to
> ext2_inode and its ilk, though. There will be bugs when somebody just decides
> that keeping such pointers might be a good idea and forgets to be nice with
> ->i_count. Or decrement it manually instead of calling iput(), etc.
Not doing so now is a shoot-on-sight offense, I thought...
Jeff
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
Em Mon, Jan 07, 2002 at 04:32:40PM -0500, Jeff Garzik escreveu:
> I am very much interested in a better solution... I could not figure
> out how to get a private pointer from a struct inode*, without using a
> nasty OFFSET_OF macro or a pointer to self as I implemented.
Why nasty, don't you like the list_head macros? 8) BTW, thats how Linus
suggested it to be done in private conversation. Look at list_entry.
- Arnaldo
Arnaldo Carvalho de Melo wrote:
>
> Em Mon, Jan 07, 2002 at 04:32:40PM -0500, Jeff Garzik escreveu:
> > I am very much interested in a better solution... I could not figure
> > out how to get a private pointer from a struct inode*, without using a
> > nasty OFFSET_OF macro or a pointer to self as I implemented.
>
> Why nasty, don't you like the list_head macros? 8) BTW, thats how Linus
> suggested it to be done in private conversation. Look at list_entry.
Yep, sorry Linus :) I just much prefer inserting the back-ref at object
creation time to pointer arithmetic... that way there is no type
information lost, and it gives the compiler a better opportunity to see
the relationship between the two structs.
Jeff
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
>>>>> "daniel" == Daniel Phillips <[email protected]> writes:
Hi
daniel> Minor nit:
daniel> if (!inode->u.ext2_ip)
daniel> BUG();
daniel> You don't have to do this, if the pointer is null you will get a perfectly
daniel> fine oops.
Without nice line information :(
Later, Juan.
--
In theory, practice and theory are the same, but in practice they
are different -- Larry McVoy
diff -Naur -X /g/g/lib/dontdiff linux-fs6/fs/ext2/dir.c linux-fs7/fs/ext2/dir.c
--- linux-fs6/fs/ext2/dir.c Mon Jan 7 07:32:28 2002
+++ linux-fs7/fs/ext2/dir.c Mon Jan 7 22:48:42 2002
@@ -24,7 +24,6 @@
#include <linux/fs.h>
#include "ext2_fs.h"
#include "ext2_fs_sb.h"
-#include "ext2_fs_i.h"
#include <linux/pagemap.h>
typedef struct ext2_dir_entry_2 ext2_dirent;
diff -Naur -X /g/g/lib/dontdiff linux-fs6/fs/ext2/ext2_fs.h linux-fs7/fs/ext2/ext2_fs.h
--- linux-fs6/fs/ext2/ext2_fs.h Mon Jan 7 07:54:33 2002
+++ linux-fs7/fs/ext2/ext2_fs.h Mon Jan 7 22:49:48 2002
@@ -17,7 +17,9 @@
#define _LINUX_EXT2_FS_H
#include <linux/types.h>
+#include <linux/list.h>
#include <linux/slab.h>
+#include "ext2_fs_i.h"
/*
* The second extended filesystem constants/structures
@@ -581,12 +583,15 @@
extern unsigned long ext2_count_free (struct buffer_head *, unsigned);
/* inode.c */
+#define ext2_inode_entry(inode) \
+ list_entry((inode), struct ext2_inode_info, i_inode_data)
static inline struct ext2_inode_info *ext2_i(struct inode *inode)
{
- if (!inode->u.ext2_ip)
+ struct ext2_inode_info *ei = ext2_inode_entry(inode);
+ if (!ei)
BUG();
- return inode->u.ext2_ip;
+ return ei;
}
extern void ext2_read_inode (struct inode *);
@@ -596,7 +601,8 @@
extern int ext2_sync_inode (struct inode *);
extern void ext2_discard_prealloc (struct inode *);
extern void ext2_truncate (struct inode *);
-extern void ext2_clear_inode (struct inode *inode);
+extern struct inode *ext2_alloc_inode (struct super_block *sb);
+extern void ext2_destroy_inode (struct inode *inode);
/* ioctl.c */
extern int ext2_ioctl (struct inode *, struct file *, unsigned int,
diff -Naur -X /g/g/lib/dontdiff linux-fs6/fs/ext2/ext2_fs_i.h linux-fs7/fs/ext2/ext2_fs_i.h
--- linux-fs6/fs/ext2/ext2_fs_i.h Mon Sep 17 20:16:30 2001
+++ linux-fs7/fs/ext2/ext2_fs_i.h Mon Jan 7 05:08:38 2002
@@ -36,6 +36,10 @@
__u32 i_prealloc_count;
__u32 i_dir_start_lookup;
int i_new_inode:1; /* Is a freshly allocated inode */
+
+#ifdef __KERNEL__
+ struct inode i_inode_data;
+#endif
};
#endif /* _LINUX_EXT2_FS_I */
diff -Naur -X /g/g/lib/dontdiff linux-fs6/fs/ext2/ialloc.c linux-fs7/fs/ext2/ialloc.c
--- linux-fs6/fs/ext2/ialloc.c Mon Jan 7 08:58:19 2002
+++ linux-fs7/fs/ext2/ialloc.c Mon Jan 7 22:48:50 2002
@@ -16,7 +16,6 @@
#include <linux/fs.h>
#include "ext2_fs.h"
#include "ext2_fs_sb.h"
-#include "ext2_fs_i.h"
#include <linux/locks.h>
#include <linux/quotaops.h>
@@ -317,7 +316,8 @@
struct inode * ext2_new_inode (const struct inode * dir, int mode)
{
- const struct ext2_inode_info *di = dir->u.ext2_ip;
+ const struct ext2_inode_info *di =
+ (const struct ext2_inode_info *) ext2_inode_entry(dir);
struct ext2_inode_info *ei;
struct super_block * sb;
struct buffer_head * bh;
@@ -335,18 +335,7 @@
inode = new_inode(sb);
if (!inode)
return ERR_PTR(-ENOMEM);
-
- /* allocate private per-inode info. note that for
- * the error cases below, iput and s_op->clear_inode
- * will handle freeing the private area.
- */
- inode->u.ext2_ip = kmem_cache_alloc(ext2_ino_cache, SLAB_NOFS);
- if (!inode->u.ext2_ip) {
- err = -ENOMEM;
- goto no_priv;
- }
- ei = inode->u.ext2_ip;
- memset(ei, 0, sizeof(*ei));
+ ei = ext2_i(inode);
lock_super (sb);
@@ -439,7 +428,6 @@
mark_buffer_dirty(bh2);
fail:
unlock_super(sb);
-no_priv:
make_bad_inode(inode);
iput(inode);
return ERR_PTR(err);
diff -Naur -X /g/g/lib/dontdiff linux-fs6/fs/ext2/inode.c linux-fs7/fs/ext2/inode.c
--- linux-fs6/fs/ext2/inode.c Mon Jan 7 09:20:19 2002
+++ linux-fs7/fs/ext2/inode.c Mon Jan 7 22:48:55 2002
@@ -25,7 +25,6 @@
#include <linux/fs.h>
#include "ext2_fs.h"
#include "ext2_fs_sb.h"
-#include "ext2_fs_i.h"
#include <linux/locks.h>
#include <linux/smp_lock.h>
#include <linux/sched.h>
@@ -37,7 +36,7 @@
MODULE_DESCRIPTION("Second Extended Filesystem");
MODULE_LICENSE("GPL");
-
+extern void inode_init_once(void *, kmem_cache_t *, unsigned long);
static int ext2_update_inode(struct inode * inode, int do_sync);
/*
@@ -890,7 +889,7 @@
{
struct super_block *sb = inode->i_sb;
struct ext2_sb_info *esb = ext2_sb(sb);
- struct ext2_inode_info *ei;
+ struct ext2_inode_info *ei = ext2_i(inode);
struct buffer_head * bh;
struct ext2_inode * raw_inode;
unsigned long block_group;
@@ -900,12 +899,6 @@
unsigned long offset;
struct ext2_group_desc * gdp;
- inode->u.ext2_ip = kmem_cache_alloc(ext2_ino_cache, SLAB_NOFS);
- if (!inode->u.ext2_ip)
- goto bad_inode;
- ei = inode->u.ext2_ip;
- memset(ei, 0, sizeof(*ei));
-
if ((inode->i_ino != EXT2_ROOT_INO && inode->i_ino != EXT2_ACL_IDX_INO &&
inode->i_ino != EXT2_ACL_DATA_INO &&
inode->i_ino < EXT2_FIRST_INO(sb)) ||
@@ -1034,10 +1027,6 @@
return;
bad_inode:
- if (inode->u.ext2_ip) {
- kmem_cache_free(ext2_ino_cache, inode->u.ext2_ip);
- inode->u.ext2_ip = NULL;
- }
make_bad_inode(inode);
return;
}
@@ -1181,12 +1170,24 @@
return ext2_update_inode (inode, 1);
}
-void ext2_clear_inode (struct inode *inode)
+struct inode *ext2_alloc_inode (struct super_block *sb)
{
- struct ext2_inode_info *ei = inode->u.ext2_ip;
+ struct ext2_inode_info *ei;
+ struct inode *inode;
- if (ei) {
- kmem_cache_free(ext2_ino_cache, ei);
- inode->u.ext2_ip = NULL;
- }
+ ei = kmem_cache_alloc(ext2_ino_cache, SLAB_NOFS);
+ if (!ei)
+ return NULL;
+ inode = &ei->i_inode_data;
+
+ memset(ei, 0, sizeof(*ei));
+ inode_init_once(inode, ext2_ino_cache, SLAB_CTOR_CONSTRUCTOR);
+ return inode;
+}
+
+void ext2_destroy_inode (struct inode *inode)
+{
+ struct ext2_inode_info *ei = ext2_i(inode);
+ kmem_cache_free(ext2_ino_cache, ei);
}
+
diff -Naur -X /g/g/lib/dontdiff linux-fs6/fs/ext2/ioctl.c linux-fs7/fs/ext2/ioctl.c
--- linux-fs6/fs/ext2/ioctl.c Mon Jan 7 07:32:43 2002
+++ linux-fs7/fs/ext2/ioctl.c Mon Jan 7 22:48:58 2002
@@ -9,7 +9,6 @@
#include <linux/fs.h>
#include "ext2_fs.h"
-#include "ext2_fs_i.h"
#include <linux/sched.h>
#include <asm/uaccess.h>
diff -Naur -X /g/g/lib/dontdiff linux-fs6/fs/ext2/namei.c linux-fs7/fs/ext2/namei.c
--- linux-fs6/fs/ext2/namei.c Mon Jan 7 07:32:47 2002
+++ linux-fs7/fs/ext2/namei.c Mon Jan 7 22:49:02 2002
@@ -31,7 +31,6 @@
#include <linux/fs.h>
#include "ext2_fs.h"
-#include "ext2_fs_i.h"
#include <linux/pagemap.h>
/*
diff -Naur -X /g/g/lib/dontdiff linux-fs6/fs/ext2/super.c linux-fs7/fs/ext2/super.c
--- linux-fs6/fs/ext2/super.c Mon Jan 7 07:32:50 2002
+++ linux-fs7/fs/ext2/super.c Mon Jan 7 22:49:05 2002
@@ -22,7 +22,6 @@
#include <linux/fs.h>
#include "ext2_fs.h"
#include "ext2_fs_sb.h"
-#include "ext2_fs_i.h"
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/locks.h>
@@ -160,7 +159,9 @@
write_inode: ext2_write_inode,
put_inode: ext2_put_inode,
delete_inode: ext2_delete_inode,
- clear_inode: ext2_clear_inode,
+ alloc_inode: ext2_alloc_inode,
+ destroy_inode: ext2_destroy_inode,
+
put_super: ext2_put_super,
write_super: ext2_write_super,
statfs: ext2_statfs,
diff -Naur -X /g/g/lib/dontdiff linux-fs6/fs/ext2/symlink.c linux-fs7/fs/ext2/symlink.c
--- linux-fs6/fs/ext2/symlink.c Mon Jan 7 07:32:54 2002
+++ linux-fs7/fs/ext2/symlink.c Mon Jan 7 22:49:08 2002
@@ -19,7 +19,6 @@
#include <linux/fs.h>
#include "ext2_fs.h"
-#include "ext2_fs_i.h"
static int ext2_readlink(struct dentry *dentry, char *buffer, int buflen)
{
diff -Naur -X /g/g/lib/dontdiff linux-fs6/include/linux/fs.h linux-fs7/include/linux/fs.h
--- linux-fs6/include/linux/fs.h Mon Jan 7 22:19:16 2002
+++ linux-fs7/include/linux/fs.h Mon Jan 7 22:40:37 2002
@@ -313,8 +313,6 @@
#include <linux/jffs2_fs_i.h>
#include <linux/cramfs_fs_sb.h>
-struct ext2_inode_info;
-
/*
* Attribute flags. These should be or-ed together to figure out what
* has been changed!
@@ -502,8 +500,6 @@
struct proc_inode_info proc_i;
struct socket socket_i;
struct jffs2_inode_info jffs2_i;
-
- struct ext2_inode_info *ext2_ip;
void *generic_ip;
} u;
Daniel Phillips wrote:
>
> On January 7, 2002 04:19 pm, Daniel Phillips wrote:
> > - You are dreferencing a pointer, and have two allocations for every
> > inode instead of one.
>
> Oh no, you only have one allocator, and you have the filesystem do it, with
> per-sb methods. Why is this better than having the VFS do it? Does this
> imply you might have different sized inodes with different mounts of the same
> filesystem?
>
> The per-fs cost with my variant is: 4-8 bytes per filesystem, period. No
> methods needed, and the object management code doesn't get replicated through
> all the filesystems.
I greatly prefer function pointers to [possibly] generic obj management
code, to storing object sizes. Some filesystem is inevitably going to
want to do something even more clever with inode allocation. My method
gives developers the freedom to experiement with inode alloc to their
heart's desires, without affecting any other filesystem.
> Also, having the inode point at itself is a little, hmm, 'what's wrong with
> this picture', don't you think?
gone in the updated patch :)
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
Daniel Phillips wrote:
>
> On January 7, 2002 02:21 pm, Jeff Garzik wrote:
> > Here's my idea for the solution. Each patch in the series has been
> > tested individually and can be applied individually, as long as all
> > preceding patches are applied. (ie. to apply and testing patch N,
> > patches 1 through N-1 must also be applied) The light testing consisted
> > of unpacking, catting, and removing kernel trees, along with a fillmem
> > runs to ensure that slab caches are properly purged. An fsck was forced
> > after each run of tests.
> >
> > This is the first of seven steps in the Make Fs.h Happy program.
> > It borrows direction from Daniel and Linus as well as my own.
> >
> > patch1 (this patch): use accessor function ext2_i to access inode->u.ext2_i
> > The rest of the patches borrows ideas but no code. This patch
> > is the only exception: it borrows substantially Daniel's ext2_i
> > patch.
> > patch2: use accessor function ext2_sb to access sb->u.ext2_sb
> > patch3: dynamically allocate sb->u.ext2_sbp
> > patch4: dynamically allocate inode->u.ext2_ip
> > patch5: move include/linux/ext2*.h to fs/ext2
> >
> > at this point we've reached the limits of how far the current
> > VFS API will go. inode and superblock fs-level private info
> > is dynamically allocated.
> >
> > patch6: add sb->s_op->{alloc,destroy}_inode to VFS API
> > patch7: implement ext2 use of s_op->{alloc,destroy}
>
> The two main problems I see with this are:
>
> - If a filesystem doesn't want to use genericp_ip/sbp then fs.h has
> to know about it. Why should fs.h know about every filesystem in
> the world?
>
> - You are dreferencing a pointer, and have two allocations for every
> inode instead of one.
>
> It's not horrible, it's just not optimal.
new patch fixes both of these objections
> Moving the ext2 headers from include/linux to fs/ext2 is an interesting
> feature of your patch, though it isn't essential to the idea you're
> presenting. But is there a good reason why ext2_fs_i.h and ext2_fs_sb.h
> should remain separate from ext2_fs.h? It looks like gratuitous
> modularity to me.
apparently userspace includes them, which is the reason for the strange
types. good reason to continue to keep them separate. That's also why
my patch7 adds an ifdef __KERNEL__.
Jeff
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
On Jan 07, 2002 18:28 -0500, Jeff Garzik wrote:
> --- linux-fs6/fs/ext2/ext2_fs_i.h Mon Sep 17 20:16:30 2001
> +++ linux-fs7/fs/ext2/ext2_fs_i.h Mon Jan 7 05:08:38 2002
> @@ -36,6 +36,10 @@
> __u32 i_prealloc_count;
> __u32 i_dir_start_lookup;
> int i_new_inode:1; /* Is a freshly allocated inode */
> +
> +#ifdef __KERNEL__
> + struct inode i_inode_data;
> +#endif
> };
Since ext2_fs_i.h only describes the in-memory data for ext2 inodes, there
is no reason to #ifdef __KERNEL__ any changes therein. I have seen several
other people worry about changes to this file in the past also, and I was
going to suggest adding a comment to the file, but I see it already says
"inode data in memory" so I don't know what else to add...
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/
Andreas Dilger wrote:
>
> On Jan 07, 2002 18:28 -0500, Jeff Garzik wrote:
> > --- linux-fs6/fs/ext2/ext2_fs_i.h Mon Sep 17 20:16:30 2001
> > +++ linux-fs7/fs/ext2/ext2_fs_i.h Mon Jan 7 05:08:38 2002
> > @@ -36,6 +36,10 @@
> > __u32 i_prealloc_count;
> > __u32 i_dir_start_lookup;
> > int i_new_inode:1; /* Is a freshly allocated inode */
> > +
> > +#ifdef __KERNEL__
> > + struct inode i_inode_data;
> > +#endif
> > };
>
> Since ext2_fs_i.h only describes the in-memory data for ext2 inodes, there
> is no reason to #ifdef __KERNEL__ any changes therein. I have seen several
> other people worry about changes to this file in the past also, and I was
> going to suggest adding a comment to the file, but I see it already says
> "inode data in memory" so I don't know what else to add...
If that is ok with everyone else, it's ok with me. I just noticed that
ext2_fs_sb.h already uses kernel-specific types, adding weight to the
argument.
Jeff
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
On Jan 07, 2002 18:48 -0500, Jeff Garzik wrote:
> Daniel Phillips wrote:
> > Moving the ext2 headers from include/linux to fs/ext2 is an interesting
> > feature of your patch, though it isn't essential to the idea you're
> > presenting. But is there a good reason why ext2_fs_i.h and ext2_fs_sb.h
> > should remain separate from ext2_fs.h? It looks like gratuitous
> > modularity to me.
>
> apparently userspace includes them, which is the reason for the strange
> types. good reason to continue to keep them separate. That's also why
> my patch7 adds an ifdef __KERNEL__.
Could you be more specific? AFAIK, the ext2_fs.h file is also used by
e2fsprogs (which actually has its own, _more_ up-to-date version of this
file), but the _i.h and _sb.h files are for kernel use only. They do not
have any relation to on-disk ext2 structs, so there would not really be
any point in referencing them from userspace.
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/
Andreas Dilger wrote:
> Could you be more specific? AFAIK, the ext2_fs.h file is also used by
> e2fsprogs (which actually has its own, _more_ up-to-date version of this
> file), but the _i.h and _sb.h files are for kernel use only. They do not
> have any relation to on-disk ext2 structs, so there would not really be
> any point in referencing them from userspace.
Just a based on the types and history of the file. As noted in the
other e-mail, ext2_fs_sb.h has no such userspace restriction, I was
basically just being cautious :)
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
On Jan 07, 2002 18:46 -0500, Jeff Garzik wrote:
> I greatly prefer function pointers to [possibly] generic obj management
> code, to storing object sizes. Some filesystem is inevitably going to
> want to do something even more clever with inode allocation. My method
> gives developers the freedom to experiement with inode alloc to their
> heart's desires, without affecting any other filesystem.
Oh, I totally agree. I think stacking filesystems will generally want to
be able to do their own inode-private allocation.
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/
On January 7, 2002 10:25 pm, Jeff Garzik wrote:
> Daniel Phillips wrote:
> > The two main problems I see with this are:
> >
> > - If a filesystem doesn't want to use genericp_ip/sbp then fs.h has
> > to know about it. Why should fs.h know about every filesystem in
> > the world?
>
> We keep type information through this method. There is no ugly casting.
There is a far uglier 1) tying of fs.h to every filesystem in the world 2) a
gratuitous extra pointer dereference and 3) a pointer field wasted in every
inode.
In the ugly contest, you win, hands down.
--
Daniel
On January 8, 2002 12:48 am, Jeff Garzik wrote:
> > Moving the ext2 headers from include/linux to fs/ext2 is an interesting
> > feature of your patch, though it isn't essential to the idea you're
> > presenting. But is there a good reason why ext2_fs_i.h and ext2_fs_sb.h
> > should remain separate from ext2_fs.h? It looks like gratuitous
> > modularity to me.
>
> apparently userspace includes them, which is the reason for the strange
> types. good reason to continue to keep them separate. That's also why
> my patch7 adds an ifdef __KERNEL__.
It's unnecessary for userspace to include those headers, they are
kernel-private.
--
Daniel
On January 7, 2002 09:54 pm, Juan Quintela wrote:
> >>>>> "daniel" == Daniel Phillips <[email protected]> writes:
>
> Hi
>
> daniel> Minor nit:
>
> daniel> if (!inode->u.ext2_ip)
> daniel> BUG();
>
> daniel> You don't have to do this, if the pointer is null you will get a perfectly
daniel> fine oops.
>
> Without nice line information :(
>
> Later, Juan.
Well that's a problem with the oopser, don't you think? It's not right to put in
cpu-eating BUG checks for null pointer dereference all over the place just to work
around this deficiency.
--
Daniel
On January 7, 2002 10:27 pm, Jeff Garzik wrote:
> Daniel Phillips wrote:
> > On January 7, 2002 03:13 pm, Anton Altaparmakov wrote:
> > > Of course fs which are not adapted would still just work with the fs_i()
> > > and fs_sb() macros and/or using two separate pointers.
> >
> > Yes, the fs_* macros are the really critical part of all this. I'd like to
> > get them in early, while we hash out the rest of it. I think Jeff supports
> > me in this, possibly Al as well.
>
> agreed, from my side
OK, are we agreed that:
- We're waiting for Al to merge ext/*alloc.c changes
- When that's done we will apply what would be my unbork patches (2: ext2_i) and
(3: ext2_sb) to both 2.4.current and 2.5.current? Subject to getting these two
patches into the form everybody likes, of course.
- We have some time in the interim to figure out how best to unbork fs.h, but we
all agree it needs to be done soon.
--
Daniel
Daniel Phillips wrote:
> - When that's done we will apply what would be my unbork patches (2: ext2_i) and
> (3: ext2_sb) to both 2.4.current and 2.5.current? Subject to getting these two
> patches into the form everybody likes, of course.
predictably I would rather my patches got applied for this :)
--
Jeff Garzik | Alternate titles for LOTR:
Building 1024 | Fast Times at Uruk-Hai
MandrakeSoft | The Took, the Elf, His Daughter and Her Lover
| Samwise Gamgee: International Hobbit of Mystery
On January 8, 2002 07:31 am, Jeff Garzik wrote:
> Daniel Phillips wrote:
> > - When that's done we will apply what would be my unbork patches (2: ext2_i) and
> > (3: ext2_sb) to both 2.4.current and 2.5.current? Subject to getting these two
> > patches into the form everybody likes, of course.
>
> predictably I would rather my patches got applied for this :)
Naturally ;)
--
Daniel