LinuxLists.cc - [RFC] [PATCH] Clean up fs.h union for ext2

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

Em Wed, Dec 26, 2001 at 10:28:09PM -0500, Legacy Fishtank escreveu:
> On Thu, Dec 27, 2001 at 04:21:42AM +0100, Daniel Phillips wrote:
> > --- ../2.4.17.clean/include/linux/fs.h Fri Dec 21 12:42:03 2001
> > +++ ./include/linux/fs.h Wed Dec 26 23:30:55 2001
> > @@ -478,7 +478,7 @@
> > __u32 i_generation;
> > union {
> > struct minix_inode_info minix_i;
> > - struct ext2_inode_info ext2_i;
> > + struct ext2_inode_info ext2_inode_info;
> > struct ext3_inode_info ext3_i;
> > struct hpfs_inode_info hpfs_i;
> > struct ntfs_inode_info ntfs_i;
>
> Change in principle looks good except IMHO you should go ahead and
> remove the ext2 stuff from the union... (with the additional changes
> that implies)

Jeff, he's trying to go incremental, the idea is to make the union
something like ->i_fs_private (void pointer) or something like that, or do
like I did for the struct sock case, using a per fs slabcache, to avoid the
extra allocation for the private area, Al thinks that this is not needed,
but Daniel thinks it can be worth it, doing it incrementally we can test
both approaches and avoid getting to big a patch. But yes, the idea is to
kill the union completely and remove all the fs specific includes in fs.h.

After the access to the union is abstracted, we can do the next step, which
is to remove the union, touching then only the function/macro that
abstracts the access to the fs specific data.

So, yes, the ext2 stuff will be removed, as all the other fs specific stuff
8)

- Arnaldo

2001-12-27 03:49:26

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

On December 27, 2001 04:28 am, Legacy Fishtank wrote:
> On Thu, Dec 27, 2001 at 04:21:42AM +0100, Daniel Phillips wrote:
> > --- ../2.4.17.clean/include/linux/fs.h Fri Dec 21 12:42:03 2001
> > +++ ./include/linux/fs.h Wed Dec 26 23:30:55 2001
> > @@ -478,7 +478,7 @@
> > __u32 i_generation;
> > union {
> > struct minix_inode_info minix_i;
> > - struct ext2_inode_info ext2_i;
> > + struct ext2_inode_info ext2_inode_info;
> > struct ext3_inode_info ext3_i;
> > struct hpfs_inode_info hpfs_i;
> > struct ntfs_inode_info ntfs_i;
>
> Change in principle looks good except IMHO you should go ahead and
> remove the ext2 stuff from the union... (with the additional changes
> that implies)

Hi Jeff,

Thanks for your confidence, but that would be a considerably bigger patch.
It's not just a matter of removing the includes - other bits and pieces have
to be put in place, such as per-filesystem inode slab. The support for this
goes outside ext2.

My idea is to just let people have a look and test this minimally intrusive
change. Getting rid of the includes for ext2 inodes will be a two-patch
change:

1) Abstract away the ext2 .u's (done)
2) Per-fs inode slab, initially only for ext2 (partly done)

Removing the includes for ext2 superblocks will need another two patches. By
the time all filesystems are done, it would be thousands of lines if it was
all in one patch. I think it's better to keep it broken up, and do it
incrementally.

--
Daniel

2001-12-27 18:17:39

by Andreas Dilger

[permalink] [raw]

Subject: Re: [Ext2-devel] [RFC] [PATCH] Clean up fs.h union for ext2

On Dec 27, 2001 04:21 +0100, Daniel Phillips wrote:
> The strategy is to abstract all references to the struct inode union through
> an inline function:
>
> static inline struct ext2_inode_info *ext2_i (struct inode *inode)
> {
> return &(inode->u.ext2_inode_info);
> }
>
> There is some grist here for the mills of language lawyers here. Note the
> compilation warning:
>
> ialloc.c:336: warning: passing arg 1 of `ext2_i' discards qualifiers from
> pointer target type

Why not just declare ext2_i like the following? It _should_ work:

static inline struct ext2_inode_info *ext2_i(const struct inode *inode)
{
return &(inode->u.ext2_inode_info);
}

Minor nit: this is already done for the ext3 code, but it looks like:

#define EXT3_I (&((inode)->u.ext3_i))

We already have the EXT3_SB, so I thought I would be consistent with it:

#define EXT3_SB (&((sb)->u.ext3_sb))

Do people like the inline version better? Either way, I would like to make
the ext2 and ext3 codes more similar, rather than less.

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

2001-12-28 01:52:32

[permalink] [raw]

Subject: Re: [Ext2-devel] [RFC] [PATCH] Clean up fs.h union for ext2

Hi Andreas

On December 27, 2001 07:14 pm, Andreas Dilger wrote:
> On Dec 27, 2001 04:21 +0100, Daniel Phillips wrote:
> > The strategy is to abstract all references to the struct inode union through
> > an inline function:
> >
> > static inline struct ext2_inode_info *ext2_i (struct inode *inode)
> > {
> > return &(inode->u.ext2_inode_info);
> > }
> >
> > There is some grist here for the mills of language lawyers here. Note the
> > compilation warning:
> >
> > ialloc.c:336: warning: passing arg 1 of `ext2_i' discards qualifiers from
> > pointer target type
>
> Why not just declare ext2_i like the following? It _should_ work:
>
> static inline struct ext2_inode_info *ext2_i(const struct inode *inode)
> {
> return &(inode->u.ext2_inode_info);
> }

If that's all we do, then we get 'warning: return discards qualifiers from
pointer target type'. Adding an explicit cast gets rid of that:

static inline struct ext2_inode_info *ext2_i (const struct inode *inode)
{
return (struct ext2_inode_info *) &(inode->u.ext2_inode_info);
}

However, then we're lying to the compiler. I wonder how safe that is.

> Minor nit: this is already done for the ext3 code, but it looks like:
>
> #define EXT3_I (&((inode)->u.ext3_i))
>
> We already have the EXT3_SB, so I thought I would be consistent with it:
>
> #define EXT3_SB (&((sb)->u.ext3_sb))
>
> Do people like the inline version better? Either way, I would like to make
> the ext2 and ext3 codes more similar, rather than less.

An upcoming version will use an explicit cast:

return (struct ext2_inode_info *) (inode + 1);

In other words, it doesn't rely on the union, which we're trying to get rid of.
In this case, an inline function provides type saftey and a macro wouldn't. I
also prefer to pass the inode/sb explicitly, rather than having a macro pick it
up from context.

In the superblock patch, the inline will be 'ext2_s'. Al seems comfortable with
this terminology, so...

--
Daniel

2001-12-29 16:05:17

by Oliver Xymoron

[permalink] [raw]

Subject: Re: [Ext2-devel] [RFC] [PATCH] Clean up fs.h union for ext2

On Thu, 27 Dec 2001, Andreas Dilger wrote:

> On Dec 27, 2001 04:21 +0100, Daniel Phillips wrote:
> > The strategy is to abstract all references to the struct inode union through
> > an inline function:
> >
> > static inline struct ext2_inode_info *ext2_i (struct inode *inode)
> > {
> > return &(inode->u.ext2_inode_info);
> > }
> >
> > There is some grist here for the mills of language lawyers here. Note the
> > compilation warning:
> >
> > ialloc.c:336: warning: passing arg 1 of `ext2_i' discards qualifiers from
> > pointer target type
>
> Why not just declare ext2_i like the following? It _should_ work:
>
> static inline struct ext2_inode_info *ext2_i(const struct inode *inode)
> {
> return &(inode->u.ext2_inode_info);
> }

This will get rid of the warning but it doesn't solve the underlying
problem of discarding the potentially useful const information. In C++,
you'd define a pair of functions to deal with this, one with and one
without const.

> Minor nit: this is already done for the ext3 code, but it looks like:
>
> #define EXT3_I (&((inode)->u.ext3_i))
>
> We already have the EXT3_SB, so I thought I would be consistent with it:
>
> #define EXT3_SB (&((sb)->u.ext3_sb))
>
> Do people like the inline version better? Either way, I would like to make
> the ext2 and ext3 codes more similar, rather than less.

The ext3 macros are rather revolting, simply because they assume the
variable name. A parameterized macro might be the best compromise:

#define EXT2_I(i) (&(i->u.ext2_inode_info))

--
"Love the dolphins," she advised him. "Write by W.A.S.T.E.."

2001-12-29 21:02:15

by Andreas Dilger

[permalink] [raw]

Subject: Re: [Ext2-devel] [RFC] [PATCH] Clean up fs.h union for ext2

On Dec 29, 2001 10:04 -0600, Oliver Xymoron wrote:
> On Thu, 27 Dec 2001, Andreas Dilger wrote:
> > Minor nit: this is already done for the ext3 code, but it looks like:
> >
> > #define EXT3_I (&((inode)->u.ext3_i))
> >
> > We already have the EXT3_SB, so I thought I would be consistent with it:
> >
> > #define EXT3_SB (&((sb)->u.ext3_sb))
> >
> > Do people like the inline version better? Either way, I would like to make
> > the ext2 and ext3 codes more similar, rather than less.
>
> The ext3 macros are rather revolting, simply because they assume the
> variable name. A parameterized macro might be the best compromise:
>
> #define EXT2_I(i) (&(i->u.ext2_inode_info))

My mistake, the Ext3 macros _do_ take an inode/sb parameter. It's not that
I'm a huge fan of macros over inline functions, it's just that I would like
to have a consensus about how it should be done so that it is consistent
between ext2 and ext3.

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

2001-12-29 21:12:16

by Andrew Morton

[permalink] [raw]

Subject: Re: [Ext2-devel] [RFC] [PATCH] Clean up fs.h union for ext2

Oliver Xymoron wrote:
>
> > Minor nit: this is already done for the ext3 code, but it looks like:
> >
> > #define EXT3_I (&((inode)->u.ext3_i))
> >
> > We already have the EXT3_SB, so I thought I would be consistent with it:
> >
> > #define EXT3_SB (&((sb)->u.ext3_sb))
> >
> > Do people like the inline version better? Either way, I would like to make
> > the ext2 and ext3 codes more similar, rather than less.
>
> The ext3 macros are rather revolting, simply because they assume the
> variable name. A parameterized macro might be the best compromise:
>
> #define EXT2_I(i) (&(i->u.ext2_inode_info))
>

They _would_ be revolting, except Andreas mistyped :) We have:

#define EXT3_SB(sb) (&((sb)->u.ext3_sb))
#define EXT3_I(inode) (&((inode)->u.ext3_i))

(A number of the mm macros accidentally only work correctly if their
argument is called "page". Dunno if this is stil the case though).

-

2001-12-29 21:30:39

by Oliver Xymoron

[permalink] [raw]

Subject: Re: [Ext2-devel] [RFC] [PATCH] Clean up fs.h union for ext2

On Sat, 29 Dec 2001, Andreas Dilger wrote:

> > The ext3 macros are rather revolting, simply because they assume the
> > variable name. A parameterized macro might be the best compromise:
> >
> > #define EXT2_I(i) (&(i->u.ext2_inode_info))
>
> My mistake, the Ext3 macros _do_ take an inode/sb parameter. It's not that
> I'm a huge fan of macros over inline functions, it's just that I would like
> to have a consensus about how it should be done so that it is consistent
> between ext2 and ext3.

The inline route is the way to go. The const guarantee on *inode doesn't
get propagated down to the objects it points to by the compiler anyway so
when the unions go away being const-correct gains us nothing.

--
"Love the dolphins," she advised him. "Write by W.A.S.T.E.."

2002-01-03 09:57:55

by Pavel Machek

[permalink] [raw]

Subject: Re: [Ext2-devel] [RFC] [PATCH] Clean up fs.h union for ext2

Hi!

> Why not just declare ext2_i like the following? It _should_ work:
>
> static inline struct ext2_inode_info *ext2_i(const struct inode *inode)
> {
> return &(inode->u.ext2_inode_info);
> }
>
>
> Minor nit: this is already done for the ext3 code, but it looks like:
>
> #define EXT3_I (&((inode)->u.ext3_i))
>
> We already have the EXT3_SB, so I thought I would be consistent with it:
>
> #define EXT3_SB (&((sb)->u.ext3_sb))
>
> Do people like the inline version better? Either way, I would like to make
> the ext2 and ext3 codes more similar, rather than less.

Maybe 2.5 is time to merge ext2 and ext3?
Pavel

--
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

2002-01-05 14:29:22

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

At 03:52 27/12/01, Daniel Phillips wrote:
>On December 27, 2001 04:28 am, Legacy Fishtank wrote:
> > Change in principle looks good except IMHO you should go ahead and
> > remove the ext2 stuff from the union... (with the additional changes
> > that implies)
>
>Thanks for your confidence, but that would be a considerably bigger patch.
>It's not just a matter of removing the includes - other bits and pieces have
>to be put in place, such as per-filesystem inode slab. The support for this
>goes outside ext2.
>
>My idea is to just let people have a look and test this minimally intrusive
>change. Getting rid of the includes for ext2 inodes will be a two-patch
>change:
>
> 1) Abstract away the ext2 .u's (done)
> 2) Per-fs inode slab, initially only for ext2 (partly done)
>
>Removing the includes for ext2 superblocks will need another two patches. By
>the time all filesystems are done, it would be thousands of lines if it was
>all in one patch. I think it's better to keep it broken up, and do it
>incrementally.

If anyone wants a look NTFS TNG already has gone all the way (for a while
now in fact). Both fs inode and super block are fs internal slab caches and
both use static inline NTFS_I / NTFS_SB functions and the ntfs includes
from linux/fs.h are removed altogether. Code is in sourceforge cvs. For
instructions how to download the code or to browse it online, see:

http://sourceforge.net/cvs/?group_id=13956

The module of interest is ntfs-driver-tng.

If anyone is doing patches for all kernel file systems, don't bother with
ntfs in 2.5 as NTFS TNG will replace the old ntfs driver soon (certainly in
2.5 time frame).

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/

2002-01-05 14:44:12

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

On January 5, 2002 03:29 pm, Anton Altaparmakov wrote:
> If anyone wants a look NTFS TNG already has gone all the way (for a while
> now in fact). Both fs inode and super block are fs internal slab caches and
> both use static inline NTFS_I / NTFS_SB functions and the ntfs includes
> from linux/fs.h are removed altogether. Code is in sourceforge cvs. For
> instructions how to download the code or to browse it online, see:

Nice, did you use the generic_ip fields?

--
Daniel

2002-01-05 14:56:33

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

At 14:47 05/01/02, Daniel Phillips wrote:
>On January 5, 2002 03:29 pm, Anton Altaparmakov wrote:
> > If anyone wants a look NTFS TNG already has gone all the way (for a while
> > now in fact). Both fs inode and super block are fs internal slab caches
> and
> > both use static inline NTFS_I / NTFS_SB functions and the ntfs includes
> > from linux/fs.h are removed altogether. Code is in sourceforge cvs. For
> > instructions how to download the code or to browse it online, see:
>
>Nice, did you use the generic_ip fields?

Yes. From ntfs-driver-tng/linux/fs/ntfs/fs.h:

/**
* NTFS_SB - return the ntfs volume given a vfs super block
* @sb: VFS super block
*
* NTFS_SB() returns the ntfs volume associated with the VFS super block @sb.
*/
static inline ntfs_volume *NTFS_SB(struct super_block *sb)
{
return sb->u.generic_sbp;
}

/**
* NTFS_I - return the ntfs inode given a vfs inode
* @inode: VFS inode
*
* NTFS_I() returns the ntfs inode associated with the VFS @inode.
*/
static inline ntfs_inode *NTFS_I(struct inode *inode)
{
return inode->u.generic_ip;
}

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/

2002-01-06 03:29:22

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

On January 5, 2002 03:56 pm, Anton Altaparmakov wrote:
> At 14:47 05/01/02, Daniel Phillips wrote:
> >On January 5, 2002 03:29 pm, Anton Altaparmakov wrote:
> > > If anyone wants a look NTFS TNG already has gone all the way (for a
> > > while now in fact). Both fs inode and super block are fs internal slab
> > > caches and both use static inline NTFS_I / NTFS_SB functions and the
> > > ntfs includes from linux/fs.h are removed altogether. Code is in
> > > sourceforge cvs. For instructions how to download the code or to browse
> > > it online, see:
> >
> >Nice, did you use the generic_ip fields?
>
> Yes. From ntfs-driver-tng/linux/fs/ntfs/fs.h:
>
> [...]
>
> static inline ntfs_inode *NTFS_I(struct inode *inode)
> {
> return inode->u.generic_ip;
> }

OK, so are doing two kmem_cache_allocs for every new_inode. With the
unbork.fs patch you could save 50% of the kmem_cache_allocs by
rewriting as follows:

static inline ntfs_inode *NTFS_I(struct inode *inode)
{
/* should bug-check to be sure it's really one of ours */
return (ntfs_inode *) &(inode->u);
}

And you just fill in the inode_size field of the file_system_type
declaration. The vfs will then handle all the details of allocating/freeing
inodes and the inode slab cache. (Note that Al seems to think this is the
wrong way of doing it, but hasn't said why he thinks that yet.)

For superblocks - are you sure you want a dedicated slab cache for those? It
seems to me that kmalloc is perfectly appropriate for this, and saves the
code needed to set up, keep track of, and tear down the slab cache.

--
Daniel

2002-01-06 04:04:53

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

At 03:32 06/01/2002, Daniel Phillips wrote:
>On January 5, 2002 03:56 pm, Anton Altaparmakov wrote:
> > At 14:47 05/01/02, Daniel Phillips wrote:
> > >On January 5, 2002 03:29 pm, Anton Altaparmakov wrote:
> > > > If anyone wants a look NTFS TNG already has gone all the way (for a
> > > > while now in fact). Both fs inode and super block are fs internal slab
> > > > caches and both use static inline NTFS_I / NTFS_SB functions and the
> > > > ntfs includes from linux/fs.h are removed altogether. Code is in
> > > > sourceforge cvs. For instructions how to download the code or to
> browse
> > > > it online, see:
> > >
> > >Nice, did you use the generic_ip fields?
> >
> > Yes. From ntfs-driver-tng/linux/fs/ntfs/fs.h:
> >
> > [...]
> >
> > static inline ntfs_inode *NTFS_I(struct inode *inode)
> > {
> > return inode->u.generic_ip;
> > }
>
>OK, so are doing two kmem_cache_allocs for every new_inode. With the
>unbork.fs patch you could save 50% of the kmem_cache_allocs by
>rewriting as follows:
>
> static inline ntfs_inode *NTFS_I(struct inode *inode)
> {
> /* should bug-check to be sure it's really one of ours */
> return (ntfs_inode *) &(inode->u);
> }
>
>And you just fill in the inode_size field of the file_system_type
>declaration. The vfs will then handle all the details of allocating/freeing
>inodes and the inode slab cache. (Note that Al seems to think this is the
>wrong way of doing it, but hasn't said why he thinks that yet.)

I will hold back any changes until all the details have been ironed out
first and Al has given his seal of approval... To be honest I fail to see
how one additional slab allocation will make any difference. Certainly for
NTFS where we attach dynamically allocated data to the fs specific part of
the inode (sometimes going via the slow vmalloc() at that) during the read
inode call, I doubt very much that there will be any visible performance
difference. But if it is decided that this is the Right Way(TM) to do it, I
will of course go with it.

>For superblocks - are you sure you want a dedicated slab cache for those? It
>seems to me that kmalloc is perfectly appropriate for this, and saves the
>code needed to set up, keep track of, and tear down the slab cache.

No, I was doing both at the same time and got slightly carried away. (-; I
will convert the super block allocations to simple kmalloc()s when I get bored.

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/

2002-01-06 22:39:40

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

> To be honest I fail to see how one additional slab allocation will make
> any difference. /
/
You could say the same about any aspect of Linux: and, relaxing your /
standards in such a way, you would inevitably end up with a dog. A /
good fast system emerges from its many small perfections. Half of /
the number of cache entries for inodes qualifies as one of those. /
_________________________________________________________________/
----------------------------------------------------------------
Daniel

2002-01-07 00:30:51

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

At 22:42 06/01/02, Daniel Phillips wrote:
>I wrote:
> > To be honest I fail to see how one additional slab allocation will make
> > any difference. /
> /
>You could say the same about any aspect of Linux: and, relaxing your /
>standards in such a way, you would inevitably end up with a dog. A /
>good fast system emerges from its many small perfections. Half of /
>the number of cache entries for inodes qualifies as one of those. /

Big words but mere rhetoric IMHO... You would first have to prove that
combining the two structures (vfs and fs inodes) is an actual "perfection"
compared to the case where they are individual, which is what I am not
convinced about.

Due to the nature of the content in the vfs vs. fs inode I would expect
that one is used independent of the other in many, if not in the majority
of cases. If this is correct, then it might well be an actual benefit to
have the two separate and to benefit from the hwcache line alignment in the
fs specific part. Also considering that allocation happens once in
read_inode but the structure is then accessed many times.

Please note, I am not saying you are wrong, most likely you are quite right
in fact, I am just raising a caution flag that perhaps benchmarks of both
implementations for the same fs might be a Good Idea(TM)...

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/

2002-01-07 01:28:45

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

Em Mon, Jan 07, 2002 at 12:30:58AM +0000, Anton Altaparmakov escreveu:
> At 22:42 06/01/02, Daniel Phillips wrote:
> >I wrote:
> >> To be honest I fail to see how one additional slab allocation will make
> >> any difference. /
> > /
> >You could say the same about any aspect of Linux: and, relaxing your /
> >standards in such a way, you would inevitably end up with a dog. A /
> >good fast system emerges from its many small perfections. Half of /
> >the number of cache entries for inodes qualifies as one of those. /
>
> Due to the nature of the content in the vfs vs. fs inode I would expect
> that one is used independent of the other in many, if not in the majority
> of cases. If this is correct, then it might well be an actual benefit to
> have the two separate and to benefit from the hwcache line alignment in the
> fs specific part. Also considering that allocation happens once in
> read_inode but the structure is then accessed many times.
>
> Please note, I am not saying you are wrong, most likely you are quite right
> in fact, I am just raising a caution flag that perhaps benchmarks of both
> implementations for the same fs might be a Good Idea(TM)...

Yes, having benchmarks done is a good idea and can clear any doubts about
the validity of such approach on a performace point of view.

When I did similar work for the network protocols, cleaning up
include/net/fs.h DaveM asked for benchmarks to see if the new approach,
i.e., using per network family slabcaches would lead to a performance drop,
I did it and we realized that it lead to performance _gains_, that in turn
made DaveM ask for a per network protocol slabcache, which made furter
memory savings and lead to further performance gains.

Yes, the usage pattern for sockets and inodes is different, thats why having
Daniel patches benchmarked against the current scheme can clear up the
matter about the validity of having the slabcaches.

Please note that we can have both approaches by leaving the
generic_ip/generic_sbp. In the struct sock case I left protinfo as a void
pointer, like generic_ip and some protocols use the slabcache approach
while others use the private area allocated separately and its pointer
stored in struct sock->protinfo.

- Arnaldo

2002-01-07 02:09:31

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

On January 7, 2002 02:27 am, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 07, 2002 at 12:30:58AM +0000, Anton Altaparmakov escreveu:
> > At 22:42 06/01/02, Daniel Phillips wrote:
> > >I wrote:
> > >> To be honest I fail to see how one additional slab allocation will make
> > >> any difference. /
> > > /
> > >You could say the same about any aspect of Linux: and, relaxing your /
> > >standards in such a way, you would inevitably end up with a dog. A /
> > >good fast system emerges from its many small perfections. Half of /
> > >the number of cache entries for inodes qualifies as one of those. /
> >
> > Due to the nature of the content in the vfs vs. fs inode I would expect
> > that one is used independent of the other in many, if not in the majority
> > of cases. If this is correct, then it might well be an actual benefit to
> > have the two separate and to benefit from the hwcache line alignment in
> > the fs specific part. Also considering that allocation happens once in
> > read_inode but the structure is then accessed many times.
> >
> > Please note, I am not saying you are wrong, most likely you are quite
> > right in fact, I am just raising a caution flag that perhaps benchmarks
> > of both implementations for the same fs might be a Good Idea(TM)...
>
> Yes, having benchmarks done is a good idea and can clear any doubts about
> the validity of such approach on a performace point of view.
>
> When I did similar work for the network protocols, cleaning up
> include/net/fs.h DaveM asked for benchmarks to see if the new approach,
> i.e., using per network family slabcaches would lead to a performance drop,
> I did it and we realized that it lead to performance _gains_, that in turn
> made DaveM ask for a per network protocol slabcache, which made furter
> memory savings and lead to further performance gains.

Oh, so that's why you were too busy to do the fs.h patch ;-)

> Yes, the usage pattern for sockets and inodes is different, thats why having
> Daniel patches benchmarked against the current scheme can clear up the
> matter about the validity of having the slabcaches.

Yes, I can test against the good old version. Actually, my htree benchmarks
will do nicely here. They process a lot of inodes quickly and they put a lot
of pressure on the icache, which needs to be tested.

> Please note that we can have both approaches by leaving the
> generic_ip/generic_sbp. In the struct sock case I left protinfo as a void
> pointer, like generic_ip and some protocols use the slabcache approach
> while others use the private area allocated separately and its pointer
> stored in struct sock->protinfo.

Even if we leave the generic_ip in the common inode, we will for sure remove
the union at some point, meaning that even filesystems that use the
generic_ip now will have to do a big edit to clean up the fallout. Which
isn't such a bad thing I suppose.

If we wanted to be lazy, we could just leave the union there, with one
element, the generic_ip. How ugly would that be?

--
Daniel

2002-01-07 02:18:51

[permalink] [raw]

Subject: Re: [RFC] [PATCH] Clean up fs.h union for ext2

Em Mon, Jan 07, 2002 at 03:12:06AM +0100, Daniel Phillips escreveu:
> On January 7, 2002 02:27 am, Arnaldo Carvalho de Melo wrote:
> > When I did similar work for the network protocols, cleaning up
> > include/net/fs.h DaveM asked for benchmarks to see if the new approach,
> > i.e., using per network family slabcaches would lead to a performance drop,
> > I did it and we realized that it lead to performance _gains_, that in turn
> > made DaveM ask for a per network protocol slabcache, which made furter
> > memory savings and lead to further performance gains.
>
> Oh, so that's why you were too busy to do the fs.h patch ;-)

*grin* And I still have to break the ipv6_sk_cachep into tcp6_sk_cachep,
udp6_sk_cachep and raw6_sk_cachep and test if moving the IPv4 identity
members in struct sock (sport, dport, saddr, rcv_saddr, daddr, etc) to
struct inet_opt is ok performance wise, doing that would remove the last
remnants of protocol specific stuff from include/net/sock.h 8)

<SNIP>

> Even if we leave the generic_ip in the common inode, we will for sure remove
> the union at some point, meaning that even filesystems that use the
> generic_ip now will have to do a big edit to clean up the fallout. Which
> isn't such a bad thing I suppose.

yes, in the sock.h cleanup the protinfo big union turned into just a void
pointer.

> If we wanted to be lazy, we could just leave the union there, with one
> element, the generic_ip. How ugly would that be?

Well, it can be left for later, first step would be to abstract the access
to the private areas in all the filesystems, but hey, don't worry as as
from the quick look I had some of the filesystems already use abstractions
for such access, like MSDOS_I, ext3 already does, NTFS NG also, etc 8)

- Arnaldo

2002-01-07 02:22:51