On Thu, May 27, 2010 at 04:17:51PM +1000, Dave Chinner wrote: > On Thu, May 27, 2010 at 05:01:20AM +0100, Al Viro wrote: > > Um... Maybe I'm dumb, but what's wrong with doing unregistration from > > deactivate_locked_super(), right after the call of ->kill_sb()? At that > > point ->s_umount is already dropped, so we won't deadlock at all. > > Shrinker rwsem will make sure that all shrinkers-in-progress will run > > to completion, so we won't get a superblock freed under prune_super(). > > I don't particulary mind down_try_read() in prune_super(), but why not > > make life obviously safer? > > > > Am I missing something here? > > I was worried about memory allocation in the ->kill_sb path > deadlocking on the s_umount lock if it enters reclaim. e.g. XFS > inodes can still be dirty even after the VFS has disposed of them, > and writing them back can require page cache allocation for the > backing buffers. If allocation recurses back into the shrinker, we > can deadlock on the s_umount lock. This doesn't seem like an XFS > specific problem, so I used a trylock to avoid that whole class of > problems (same way the current shrinkers do). If GFP_FS is set, we wouldn't touch the locks. It is a concern though, if __GFP_FS allocations were previously permitted under the exclusive lock. > >From there, we can unregister the shrinker before calling ->kill_sb > as per above. That, in turn, means that the unmount > invalidate_inodes() vs shrinker race goes away and the iprune_sem is > not needed in the new prune_icache_sb() function. I'm pretty sure > that I can now remove the iprune_sem, but I haven't written the > patch to do that yet. I do really like that aspect of your patch. It's nice to have the shrinker always only operating against active supers. So I would be in favour of your current scheme. -- 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
