On Mon, 2010-11-01 at 21:22 -0400, Eric Paris wrote:
> On Mon, 2010-11-01 at 15:45 -0400, Mimi Zohar wrote:
> > Based on the previous posting discussion, i_readcount is now defined as
> > atomic.
> >
> > This patchset separates the incrementing/decrementing of the i_readcount,
> > in the VFS layer, from other IMA functionality, by replacing the current
> > ima_counts_get() call with iget_readcount(). Its unclear whether this
> > call to increment i_readcount should be made earlier, like i_writecount.
> >
> > The patch ordering is a bit redundant in order to leave removing the ifdef
> > around i_readcount until the last patch. The first four patches: redefines
> > i_readcount as atomic, defines iget/iput_readcount(), moves the IMA
> > functionality in ima_counts_get() to ima_file_check(), and removes the IMA
> > imbalance code, simplifying IMA. The last patch moves iput_readcount()
> > to the fs directory and removes the ifdef around i_readcount, making
> > i_readcount into a "first class inode citizen".
> >
> > The generic_setlease code could then take advantage of i_readcount.
>
> Hey Mimi,
>
> couple of comment and questions, can you help me understand what you
> believe the three locks in question are currently protecting? And
> remember I already said I don't think they are quite right before you
> started so try not to use that as your example :)
>
> inode->i_lock
It shouldn't be necessary. As you originally said,
> > My thought was that the IMA read/write checks should happen AFTER
> > the i_writecount and i_readcount counters were updated. Thus even
> > if we raced with another task we can rest assured that the other
> > task would catch the situation we missed....
> inode->i_mutex
As the measurement policy is based on file metdata (eg, permissions,
xattrs), the mutex is taken to prevent the file metadata from changing,
while making the measurement decision.
> iint->mutex
Taken when accessing/modifying the iint structure.
> I also question you finishing in patch 5/5 with:
>
> +void iput_readcount(struct inode *inode)
> +{
> + spin_lock(&inode->i_lock);
> + if (unlikely((atomic_read(&inode->i_readcount) == 0)))
> + printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
> + inode->i_ino);
> + else
> + atomic_dec(&inode->i_readcount);
> + spin_unlock(&inode->i_lock);
> +}
>
> obviously I wonder what the locking is for, but really I question why we
> need this as a conditional at all. If we are really worried it should
> be a WARN_ON() or BUG() but personally I wonder if we need it at all.
> The VFS is by supposed to get stuff right. All of the interesting
> checks around IMA were mostly needed because IMA was an object that hung
> off the side of the VFS and you couldn't be certain that all filesystems
> were adhering to the calling conventions you thought were correct.
> Since we've pretty much moved all of this into the VFS its about time we
> stop wasting time wondering if our assumptions are correct. These are
> pretty hot paths and I'm all for cutting down the IMA overhead on them.
> If we do that this function becomes:
>
> BUG_ON(!atomic_read(&inode->i_readcount))
> atomic_dec(&inode->i_readcont);
yes, nice.
> it also means that we don't need to set the i_readcount to 0 in
> inode_init_always() from patch 3....
True, and init_once() sets the inode structure to 0.
thanks,
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
