On Mon, Feb 22, 2016 at 02:04:35PM +0100, Jan Kara wrote: > On Mon 22-02-16 13:12:22, Peter Zijlstra wrote: > > On Mon, Feb 22, 2016 at 12:54:35PM +0100, Jan Kara wrote: > > > > Also, I think fsnotify_unmount_inodes() (as per mainline) is missing a > > > > final iput(need_iput) at the very end, but I could be mistaken, that > > > > code hurts my brain. > > > > > > I think the code is actually correct since need_iput contains "inode > > > further in the list than the current inode". Thus we will always go though > > > another iteration of the loop which will drop the reference. And inode > > > cannot change state to I_FREEING or I_WILL_FREE because we hold inode > > > reference. But it is subtle as hell so I agree that code needs rewrite. > > > > So while talking to dchinner, he doubted fsnotify will actually remove > > inodes from the sb-list, but wasn't sure and too tired to check now. > > > > (I got lost in the fsnotify code real quick and gave up, for I was > > mostly trying to make a point that we don't need the CPP magic and can > > do with 'readable' code). > > > > If it doesn't, it doesn't need to do this extra special magic dance and > > can use the 'normal' iterator pattern used in all the other functions, > > greatly reducing complexity. > > Yeah, that would be nice. But fsnotify code needs to iterate over all > inodes, drop sb_list_lock and do some fsnotify magic with the inode which > is not substantial for our discussion. Now that fsnotify magic may actually > drop all the remaining inode references so once we drop our reference > pinning the inode, it can just disappear. We don't want to restart the scan > for each inode we have to process so that is the reason why we play ugly > tricks with pinning the next inode in the list. > > But I agree it should be possible to just use list_for_each_entry() instead > of list_for_each_entry_safe() and keep current inode pinned till the next > iteration to make it stick in the sb->s_inodes list. That would make the > iteration more standard. Lightly tested patch attached. That's exactly what I was thinking. Patch looks ok from aquick reading of it, but I haven't I've got anything here to test it at all. Perhaps we need so xfstests coverage of this code.... 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
