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? -- 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
