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

[Linus Torvalds <torvalds@linux-foundation.org>]

On Mon, Oct 18, 2010 at 6:16 PM, Eric Paris <eparis@redhat.com> wrote:
>
> IMA currently alocated an inode integrity structure for every inode in
> core.  This stucture is about 120 bytes long.  Most files however
> (especially on a system which doesn't make use of IMA) will never need any
> of this space.  The problem is that if IMA is enabled we need to know
> information about the number of readers and the number of writers for every
> inode on the box.  At the moment we collect that information in the per
> inode iint structure and waste the rest of the space.  This patch moves those
> counters into the struct inode so we can eventually stop allocating an IMA
> integrity structure except when absolutely needed.

Hmm. I don't think this is really acceptable as-is.

First off (and most trivially) - the fields are misnamed. Just calling
them "{open,read,write}count" was fine when it was part of an ima
structure, but for all the historical reasons, inode fields are called
'i_xyzzy'.

Secondly, we already maintain a write count (called "i_writecount").
Why is the IMA writecount different, and should it be?

Thirdly, why is it an "unsigned long"? Are the IMA numbers cumulative
or something? How could you ever overflow a 32-bit counter if not?

Finally, why does IMA even care about the read-counts vs open-counts?
Why not just open-counts, and consider any non-write to be an open?

In short, I think this patch would be _much_ more acceptable if it
added just a _single_ 32-bit "i_opencount". And even then I'd ask
"what's the difference between i_opencount and our already existing
i_count?

                                 Linus

IOW, at a glance, I think it might be much more acceptable if we only added
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html