Re: [PATCH 13/18] fs: split locking of inode writeback and LRU lists

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux