Re: [PATCH v13 27/28] ovl: Verify a data dentry has been found for metacopy inode

[Vivek Goyal <vgoyal@redhat.com>]

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@redhat.com> 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