On Fri, Apr 06, 2018 at 11:10:05PM +0300, Amir Goldstein wrote: > On Fri, Apr 6, 2018 at 8:32 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > [...] > >> >> > >> >> Back to the original question: how should ovl_inode->redirect > >> >> be protected if at all it needs protection? > >> > > >> > At this point I am thinking at max we need to just take ovl_inode->lock > >> > to protect ovl_inode->redirect. It just makes it little safer and > >> > relatively easier to understand. > >> > > >> > >> I agree. I don't think ovl_inode->lock is needed, but I would > >> add a comment explaining why (VFS inode lock). > >> > >> The problem is that we cannot get rid of d_lock for path traversal > >> and I don't see how we can easily take both ovl dentry d_lock with > >> ovl_inode->lock, because the latter is a sleeping lock, but layering > >> wise it is logically below the overlay dentry lock. > > > > Actually, I think ovl_inode->lock is at same layer as inode->i_rwsem. So > > VFS first takes i_rwsem and then d_lock. And we probably should do the > > same thing. In fact we are already taking ovl_inode->lock in > > ovl_nlink_start() first and then calling ovl_set_redirect() which takes > > d_lock. > > > > It's not completely ok that we do that. > > If we can always abide to the rules of locking order: > VFS overlay layer locks -> internal overlay locks -> VFS underlying fs locks Sure, and that's what will happen when we move ovl_set_redirect() above lock_rename(). - When ovl_rename() is called, VFS is holding ovl layer locks. - ovl_nlink_start() and ovl_set_redrect() with take internal overlay lock (ovl_inode->lock) - And then lock_rename() will take VFS underlying fs lock. So ordering of locks will be exactly as you like it. Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
