Re: [PATCH 17/19] fs: Reduce inode I_FREEING and factor inodedisposal

[Nick Piggin]

On Sun, Oct 17, 2010 at 03:13:13PM +1100, Dave Chinner wrote:
> On Sun, Oct 17, 2010 at 01:49:23PM +1100, Nick Piggin wrote:
> > On Sat, Oct 16, 2010 at 09:30:47PM -0400, Christoph Hellwig wrote:
> > > >   * inode->i_lock is *always* the innermost lock.
> > > >   *
> > > > + * inode->i_lock is *always* the innermost lock.
> > > > + *
> > > 
> > > No need to repeat, we got it..
> > 
> > Except that I didn't see where you fixed all the places where it is
> > *not* the innermost lock. Like for example places that take dcache_lock
> > inside i_lock.
> 
> I can't find any code outside of ceph where the dcache_lock is used
> within 200 lines of code of the inode->i_lock. The ceph code is not
> nesting them, though.

You mustn't have looked very hard? From ceph:

       spin_unlock(&dcache_lock);
       spin_unlock(&inode->i_lock);

(and yes, acquisition side does go in i_lock->dcache_lock order)

If you want to start mandating that i_lock *must* be an inner lock,
please establish the lock order, fix existing code that makes use
of nesting it, and provide a justification to merge the patch.

If the justification is compelling and it gets merged, I can happily
use another lock in the inode for my icache state lock. So please
don't make these off hand insinuations about how that makes your code
better and mine worse. If it is better, let's see a proper
justification and upstream patch, and the issue is really not
central to my locking design.

Thanks,
Nick

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/