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 We will be safer. So if ovl_set_redirect() can happen without taking ovl_inode->lock and completely before underlying fs VFS locks I think that would be better. >> >> So better not add ovl_inode->lock >> Sorry for the noise. > > I will not add ovl_inode->lock if I can explain everything. I have a > feeling that redirect upgrade path will have some funny interactions > with ovl_lookup() path. Let me write the new patches and see if > ovl_inode->lock is still required. > OK. Thanks, Amir. -- 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
