On Thu, May 27, 2010 at 05:01:20AM +0100, Al Viro wrote: > On Thu, May 27, 2010 at 11:53:35AM +1000, Dave Chinner wrote: > > On Thu, May 27, 2010 at 09:12:14AM +1000, Dave Chinner wrote: > > > On Thu, May 27, 2010 at 02:41:16AM +1000, Nick Piggin wrote: > > .... > > > > Nitpick but I prefer just the restart label wher it is previously. This > > > > is moving setup for the next iteration into the "error" case. > > > > > > Ok, will fix. > > .... > > > > Would you just elaborate on the lock order problem somewhere? (the > > > > comment makes it look like we *could* take the mutex if we wanted > > > > to). > > > > > > The shrinker is unregistered in deactivate_locked_super() which is > > > just before ->kill_sb is called. The sb->s_umount lock is held at > > > this point. hence is the shrinker is operating, we will deadlock if > > > we try to lock it like this: > > > > > > unmount: shrinker: > > > down_read(&shrinker_lock); > > > down_write(&sb->s_umount) > > > unregister_shrinker() > > > down_write(&shrinker_lock) > > > prune_super() > > > down_read(&sb->s_umount); > > > (deadlock) > > > > > > hence if we can't get the sb->s_umount lock in prune_super(), then > > > the superblock must be being unmounted and the shrinker should abort > > > as the ->kill_sb method will clean up everything after the shrinker > > > is unregistered. Hence the down_read_trylock(). > > 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). >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. 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
