Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote on 09/07/2009 05:01:14 PM: > This is a series of eight trivial patches that I'd like people to take a > look at, because I am hoping to eventually do multiple path component > lookups in one go without taking the per-dentry lock or incrementing (and > then decrementing) the per-dentry atomic count for each component. > > The aim would be to try to avoid getting that annoying cacheline ping-pong > on the common top-level dentries that everybody looks up (ie root and home > directories, /usr, /usr/bin etc). > > Right now I have some simple (but real) loads that show the contention on > dentry->d_lock to be a roughly 3% performance hit on a single-socket > nehalem, and I assume it can be much worse on multi-socket machines. > > And the thing is, it should be entirely possible to do everything but the > last component lookup with just a single read_seqbegin()/read_seqretry() > around the whole lookup. Yes, the last component is special and absolutely > needs locking and counting - but the last component also doesn't tend to > be shared, so locking it is fine. > > Now, I may never actually get there, but when looking at it, the biggest > problem is actually not so much the path lookup itself, as the security > tests that are done for each path component. And it should be noted that > in order for a lockless seq-lock only lookup make sense, any such > operations would have to be totally lock-free too. They certainly can't > take mutexes etc, but right now they do. > > Those security tests fall into two categories: > > - actual security layer callouts: ima_path_check(). > > This one looks totally pointless. Path component lookup is a horribly > timing-critical path, and we will only do a successful lookup on a > directory (inode needs to have a ->lookup operation), yet in the middle > of that is a call to "ima_path_check()". > > Now, it looks like ima_path_check() is very much designed to only check > the _final_ path anyway, and was never meant to be used to check the > directories we hit on the way. In fact, the whole function starts with > > if (!ima_initialized || !S_ISREG(inode->i_mode)) > return 0; > > so it's totally pointless to do that thing on a directory where > that !S_ISREG() test will trigger. > > So just remove it. IMA should never have put that check in there to > begin with, it's just way too performance-sensitive. You're right. We don't need to call ima_path_check() here, as IMA only measures the integrity of the file itself, and not directories. 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
