linux-kernel - Re: [PATCH 17/19] fs: Reduce inode I

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101017043514.GA21802@amd>
Date:	Sun, 17 Oct 2010 15:35:14 +1100
From:	Nick Piggin <npiggin@...nel.dk>
To:	Dave Chinner <david@...morbit.com>
Cc:	Nick Piggin <npiggin@...nel.dk>,
	Christoph Hellwig <hch@...radead.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 17/19] fs: Reduce inode I_FREEING and factor inode
 disposal

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/