On Fri, Oct 08, 2010 at 01:10:52PM +0100, Al Viro wrote: > On Fri, Oct 08, 2010 at 09:52:49PM +1100, Dave Chinner wrote: > > On Fri, Oct 08, 2010 at 11:18:19AM +0100, Al Viro wrote: > > > On Fri, Oct 08, 2010 at 04:21:32PM +1100, Dave Chinner wrote: > > > > > > > + spin_unlock(&sb->s_inodes_lock); > > > > > > > > - spin_lock(&inode_lru_lock); > > > > - list_move(&inode->i_lru, dispose); > > > > - spin_unlock(&inode_lru_lock); > > > > + dispose_one_inode(inode); > > > > > > > > - percpu_counter_dec(&nr_inodes_unused); > > > > + spin_lock(&sb->s_inodes_lock); > > > > > > And now you've unlocked the list and even blocked. What's going to > > > keep next valid through that fun? > > > > See the comment at the start of the loop in invalidate_list(): > > > > /* > > * We can reschedule here without worrying about the list's > > * consistency because the per-sb list of inodes must not > > * change during umount anymore, and because iprune_sem keeps > > * shrink_icache_memory() away. > > */ > > cond_resched_lock(&sb->s_inodes_lock); > > > > Hence I've assumed it's ok to add another point that drops locks and blocks > > inside the loop and next will still be valid. > > I'm not convinced, TBH; IOW, the original might have been broken by that. > The trouble is, this function is called not only on umount(). Block device > invalidation paths also can lead to it. Yeah, I see that now. Thanks for pointing it out. > Moreover, even for umount-only > side of things, remember that there's fsnotify as well. I thought that the fsnotify_unmount_inodes() cleaned everything up before we called invalidate_list(). > Original code > did _everything_ except the actual dropping inodes without releasing > inode_lock. I'm not saying that change is broken (or, in case of > non-umount paths, makes breakage worse), but I'd like to see more analysis > of that area. I think I'll avoid the whole issue right now by not making this change to invalidate_list().... 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
