PATCH 2.5.2.9: ext2 unbork fs.h (part 1/7)

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;
                };

Goodie. Now we need benchmarks for all the approaches... (-;

At 13:21 07/01/02, Jeff Garzik wrote:
<snip>

Quoted 8 lines

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.

Quoted 18 lines · Anton Altaparmakov

Goodie. Now we need benchmarks for all the approaches... (-;

At 13:21 07/01/02, Jeff Garzik wrote:
<snip>

Quoted 8 lines

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):

Quoted 6 lines

         struct ext2_inode_info {
                 ...ext2 stuff...
                 struct inode;
         };

Advantage of this would be that as far as the fs is concerned there is

only

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 2 lines · Daniel Phillips

Interesting, it's something I've always wanted to be able to do. But I suppose the compiler requirement is a stupport.

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

        at this point we have what Linus described:

                struct ext2_inode_info {
                        ...ext2 stuff...
                        struct inode inode;
                };

Quoted 2 lines

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...?

Quoted 23 lines · Anton Altaparmakov

Goodie. Now we need benchmarks for all the approaches... (-;

At 13:21 07/01/02, Jeff Garzik wrote:
<snip>

Quoted 8 lines

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;
         };

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 7 lines · Daniel Phillips

Quoted 2 lines · Anton Altaparmakov

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

- 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.

Quoted 3 lines · Daniel Phillips

- 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 :)

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}

- You are dreferencing a pointer, and have two allocations for every inode instead of one.

Quoted 2 lines · Daniel Phillips

- 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?

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.

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.

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.

Quoted 9 lines · Jeff Garzik

fs/ext2/ext2_fs_i.h Mon Jan 7 05:08:38 2002 renamed from fs/ext2/ext2_fs_i.h Mon Sep 17 20:16:30 2001 +3 -0

@@ -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;

fs/ext2/ext2_fs_i.h Mon Jan 7 05:08:38 2002 line 43

Quoted 2 lines

fs/ext2/ext2_fs_i.h Mon Jan 7 05:08:38 2002 renamed from fs/ext2/ext2_fs_i.h Mon Sep 17 20:16:30 2001 +1 -0

+#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...

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.

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.

Quoted 2 lines · Daniel Phillips

- 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?

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.

Em Mon, Jan 07, 2002 at 04:32:40PM -0500, Jeff Garzik escreveu:

Quoted 3 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"daniel" == Daniel Phillips <[email protected]> writes:

Quoted 2 lines · Daniel Phillips

- 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?

Quoted 20 lines · Jeff Garzik

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

Quoted 6 lines

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.

Quoted 5 lines · Daniel Phillips

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.

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.

Quoted 5 lines · Daniel Phillips

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.

Quoted 5 lines

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__.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"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

Without nice line information :(

Later, Juan.