Re: [PATCH 1/3] IMA: move read/write counters into struct inode

[Dave Chinner <david@fromorbit.com>]

On Mon, Oct 18, 2010 at 10:14:03PM -0400, Eric Paris wrote:
> On Mon, 2010-10-18 at 21:30 -0400, Christoph Hellwig wrote:
> > I do not like this at all.   It bloats the inode with three unsigned
> > long values for a feature no sane person would ever use.  And given
> > that distros are sweet-talked by IBM to enable it the world will pay
> > a huge penality for those 0.5% of the userbase that use IMA.
> > 
> > Please reorder your series to have patch to disable IMA unless
> > explicitly enabled on the kernel command line first, and then second
> > use the rbtree from your last patch.
> 
> More cryptic command line options and complexity is not the solution.  I
> have no plans to send (a fixed/"working" version of) Kyle's patch which
> would cause a userspace regression since working machines would
> magically stop working.
> 
> The right solution is to provide features without unreasonably impacting
> those who do not want those features.  At the moment my patch series
> reduces memory usage by a factor of at least 40 and eliminates the
> locking contention and serialization of bringing inodes into and out of
> core.  It does so without introducing ANY additional overhead or
> complexity.
> 
> If there is a general consensus that 24 bytes per inode is too large we
> can move forward from here and drop the 'opencount' and save 8 bytes
> (while eliminating the debugging and verification this code has helped
> to provide in the past)

Eric, just to put that in context - changing the size of an inode
needs to be conidered carefully because we cache so many of them. We
often jump through hoops just to reduce it by 4 or 8 bytes. You are
proposing to increase it by 24 bytes (roughly 5%) and as such that
_should_ be considered a big deal, especially for something that is
currently rarely used.

Personally I that adding a pointer into the struct inode is as much
as I'd want to compromise to. Those that want to use IMA or have the
possibility of turning it on dynamicaly can accept the additional
overhead of another memory allocation during inode allocation as the
cost of using this functionality.  That's the way the security
subsystem works, so I don't see any problems with doing this for IMA
and it turns the overhead problem into one that only affects those
that have it both configured and enabled.  That seems like a
reasonable compromise to me....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com