On Sun, Oct 26, 2014 at 11:56:08AM -0700, Linus Torvalds wrote: > On Sat, Oct 25, 2014 at 8:57 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > Your patch looks fine, and I don't think we can livelock - because we > always set 'seq' to 1 if we retry, and that causes us to get the > exclusive lock, so we'd better not then enter the retry loop again. > Although I guess not only renames cause it - looks like we get to that > mis-named "rename_retry" for a deletion too according to the comment. > Although I'm not sure that comment is correct - I don't think we > change d_parent for deletes, do we? The comment is not correct. dentry_kill() won't screw the pointer to parent; it will, however, screw the pointer to next sibling. It used to screw the pointer to parent (which is what the first part of condition was about). After Nick's series back in January 2011 that has stopped being true. However, dentry_kill() does list_del(&dentry->d_u.d_child). Which means that we can't continue past that point if it has happened. Trond has noticed the breakage a bit later and added explicit check for ->d_flags, but the damage was more extensive - Nick had missed the restarts-on-killed logics hidden in the check for changed ->d_parent and assumed that it's all about renames, meaning that once rename_lock has been taken exclusive we won't have restarts at all. With restart-on-killed restored that wasn't true anymore, invalidating the assumption that we only get to rename_retry without rename_lock held exclusive. With deadlocks happening if we ever get there on such pass. We used to have several copies of that logics, before it all got consolidated into d_walk(), which probably contributed to the mess... BTW, the first part of condition (->d_parent hasn't changed since before the "unlock child, lock parent" sequence) is really pointless these days - check for rename_lock will fail if ->d_parent has changed... -- 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
