From: Eric Paris <eparis@redhat.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, hch@infradead.org,
zohar@us.ibm.com, warthog9@kernel.org, david@fromorbit.com,
jmorris@namei.org, kyle@mcmartin.ca, hpa@zytor.com,
akpm@linux-foundation.org, mingo@elte.hu,
viro@zeniv.linux.org.uk
Subject: Re: [PATCH 1/6] IMA: move read/write counters into struct inode
Date: Fri, 22 Oct 2010 23:01:54 -0400 [thread overview]
Message-ID: <1287802914.2734.44.camel@localhost.localdomain> (raw)
In-Reply-To: <AANLkTikgHG+COx7Bps4NhbiLywUvPcwoQWhN1CWU6yUc@mail.gmail.com>
On Tue, 2010-10-19 at 17:25 -0700, Linus Torvalds wrote:
> On Tue, Oct 19, 2010 at 3:58 PM, Eric Paris <eparis@redhat.com> wrote:
> >
> > This patch does the minimum needed to move the location of the data. Further
> > cleanups, especially the location of counter updates, may still be possible.
>
> Hmm. The end result looks fine (adding four bytes to struct inode in
> order to avoid all the complexity seems reasonable), but I do get the
> feeling that this should likely be the last in the series, so that the
> VFS level files would get minimal changes. IOW, do the cleanups inside
> the IMA code first, and then do the switch-over to using counters in
> the inode last.
>
> Well, not last, since I think you need to do this before you can do
> the "only allocate iint when needed" only after you've moved the
> counters. But I think the logical order would be
> - switch to rbtree
> - drop opencount
> - switch counts to 'unsigned int'
> - drop iint->writecount and use i_writecount instead
> - move the remaining readcount to i_readcount
> - only allocate iint when necessary
>
> That way you'd only have _one_ patch that touches <linux/fs.h>, rather
> than four, and the remaining patches would all be to security/ima.
>
> But maybe I missed some reason for this particular ordering.
>
> Oh, and btw, due to alignment reasons it looks like the 4-byte
> i_readcount would take 8 bytes due to bad structure packing. I don't
> know if that is avoidable, but I do think it would make more sense to
> put it next to i_writecount instead of in between two pointers. That
> still doesn't help (we've got 3 32-bit values next to each other), but
> it's at least -closer- to working out.
Believe me, this series has not been forgotten over the week. I know
that IBM research tested my series from yesterday and found that it
didn't break any of their test suite but they haven't reviewed them well
enough to give an ACK.
I probably should spend another couple of hours myself looking over my
series before I ask for a pull from anyone but I'm willing to show my
latest work.
http://git.infradead.org/users/eparis/ima.git
(these patches are still being changed so don't trust this tree)
Main changes from last series:
1) did away with rcu altogether
2) added a new inode->i_flags, S_IMA which gets set when an ima
integrity structure is allocated so common case on inode free is
lockless.
3) shrunk the integrity structure more. Now even with all of lock
debugging turned on it's 232 bytes (most of that is a struct mutex i'm
going to look at doing away with down the line)
-Eric
next prev parent reply other threads:[~2010-10-23 3:01 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-19 22:58 [PATCH 1/6] IMA: move read/write counters into struct inode Eric Paris
2010-10-19 22:58 ` [PATCH 2/6] IMA: drop the inode opencount since it isn't needed for operation Eric Paris
2010-10-19 22:58 ` [PATCH 3/6] IMA: use unsigned int instead of long for counters Eric Paris
2010-10-19 22:58 ` [PATCH 4/6] IMA: only allocate iint when needed Eric Paris
2010-10-20 3:53 ` Al Viro
2010-10-19 22:58 ` [PATCH 5/6] IMA: use rbtree instead of radix tree for inode information cache Eric Paris
2010-10-19 23:17 ` Dave Chinner
2010-10-20 11:31 ` Peter Zijlstra
2010-10-20 22:05 ` Dave Chinner
2010-10-20 22:22 ` Linus Torvalds
2010-10-20 22:47 ` Trond Myklebust
2010-10-21 0:58 ` Linus Torvalds
2010-10-21 2:17 ` Dave Chinner
2010-10-19 22:58 ` [PATCH 6/6] IMA: use i_writecount rather than a private counter Eric Paris
2010-10-20 0:25 ` [PATCH 1/6] IMA: move read/write counters into struct inode Linus Torvalds
2010-10-23 3:01 ` Eric Paris [this message]
2010-10-24 6:52 ` Mimi Zohar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1287802914.2734.44.camel@localhost.localdomain \
--to=eparis@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=david@fromorbit.com \
--cc=hch@infradead.org \
--cc=hpa@zytor.com \
--cc=jmorris@namei.org \
--cc=kyle@mcmartin.ca \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=torvalds@linux-foundation.org \
--cc=viro@zeniv.linux.org.uk \
--cc=warthog9@kernel.org \
--cc=zohar@us.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).