On Sun, Oct 17, 2010 at 04:19:48AM +1100, Nick Piggin wrote: > On Sat, Oct 16, 2010 at 12:20:21PM -0400, Christoph Hellwig wrote: > > On Sat, Oct 16, 2010 at 06:57:13PM +1100, Nick Piggin wrote: > > > > That needs some documentation both in the changelog and in the code > > > > I think. > > > > > > This is another instance where the irregular i_lock locking is making > > > these little subtleties to the locking. I think that is actually much > > > worse for maintainence/complexity than a few trylocks which can be > > > mostly removed with rcu anyway (which are obvious because of the well > > > documented lock order). > > > > Care to explain why? > > OK. > > > > The I_FREEING and co checks are how we do things > > all over the icache for a long time. > > That's missing my point. My point is that the semantics of icache > concurrency here are changed from the old inode_lock model. With > my design, holding i_lock on an inode is equivalent (stronger, > actually) to holding inode_lock which is an important part of > making small correct steps. That doesn't necesarily make it better, Nick. The existing of I_FREEING checks in the writeback code is an exception rather than the rule - it was the only list traversal where an inode in the I_FREEING state was unacceptable and it special cased that with an undocumented BUG_ON(inode->i_state & I_FREEING). i only found this and understood it as a result of tripping over it while testing this patch. The change I made to allow handling the I_FREEING case in this code in exactly the same way as every other inode list traversal is a significant improvement. it also greatly simplified the i_state locking patches in this area. Any by the end of the series, the behaviour of setting I_FREEING before disposing of the inode is well documented, consistently implemented, and protected by a commented BUG_ON to ensure the rule is always followed in future. IMO, removing an undocumented special case landmine is a much better solution than continuing to hide it and hoping no-one treads on it.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- 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
