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 07:21:07PM +0300, Amir Goldstein wrote:
> On Fri, Apr 6, 2018 at 6:37 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Apr 06, 2018 at 12:46:31PM +0300, Amir Goldstein wrote:
> >> On Thu, Apr 5, 2018 at 11:45 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> [...]
> >> >> >
> >> >> > If you don't like this locking ovl_inode->lock model, I guess for now
> >> >> > I could live with not removing REDIRECT after copy up. Once that gets
> >> >> > merged, I could do one patch series just to clean up REDIRECT after copy
> >> >> > up and do proper locking.
> >> >> >
> >> >>
> >> >> I think we are confusing two different things in this discussion.
> >> >> Locking ovl_inode->lock for changing redirect is something that should
> >> >> stay in the patch set IMO and should be simplified by moving
> >> >> set redirect before taking rename locks on two inodes.
> >> >>
> >> >> I was asking about removal of REDIRECT because this patch
> >> >> (ovl_verify_metacopy_data) is a bit tricky for me to review and
> >> >> I still don't feel confident about it.
> >> >>
> >> >> My intuition says we could go other ways as well:
> >> >> - unite METACOPY/REDIRECT xattr (we can call the unite
> >> >> xattr REDIRECT and not METACOPY)
> >> >> - memory barriers between setting/clearing/checking
> >> >> METACOPY/REDIRECT (there is already a barrier for setting
> >> >> upperdata flag, so that's half the work already.
> >> >>
> >> >> We can either have this discussion now or, as you suggested
> >> >> leave it to a following patch set. Rule of thumb - if this is v13
> >> >> with 28 patches, might not be a bad idea to deffer 2 patches
> >> >> and reduce complexity...
> >> >>
> >> >
> >> > Hi Amir,
> >> >
> >> > Ok, let me drop removing REDIRECT from the patchset to reduce
> >> > complexity. Lets first try to get in a basic version in.
> >> >
> >> > Now coming to the question of locking ovl_inode->lock. If REDIRECTS
> >> > are never removed and only upgraded (from relative to absolute), my
> >> > understanding is that current VFS locking is sufficient to prevent
> >> > races. Only rename and link path set redirects and both the paths take
> >> > inode locks and that means two set redirects can't make progress in
> >> > parallel on an inode.
> >> >
> >> > When it comes to ovl_lookup(), we will set ovl_inode->redirect only for
> >> > the case of I_NEW. So no races should be there as well.
> >> >
> >> > So far we had to add locking due to copy up path and now copy up path
> >> > will not touch redirect xattr. I probably will not even clear
> >> > ovl_inode->inode redirect as we are not removing REDIRECT.
> >> >
> >> > Do you see any other path racing and still needed ovl_inode->lock?
> >> >
> >>
> >> Let's see.
> >>
> >> Current code is taking A->d_lock in ovl_set_redirect(A) to protect
> >> against racing with redirect path traversal in ovl_get_redirect(A/B/c).
> >>
> >> For the purpose of protecting ancestors redirect path, d_lock is
> >> sufficient and is needed anyway to access d_name.
> >>
> >> Also any rename in the filesystem is *also* serialized with
> >> ovl_sb->s_vfs_rename_mutex. That would make the change I suggested
> >> to move ovl_set_redirect() before lock_rename() a bit tricky.
> >> You may say that taking d_lock in ovl_set_redirect() is not strictly
> >> needed, but I guess it is better coding (or maybe something I am
> >> missing).
> >>
> >> Now when adding ovl_set_redirect() for non-dir and in ovl_link() the
> >> plot thickens, because:
> >> 1. link is not serialized with ovl_sb->s_vfs_rename_mutex lock,
> >> so path traversal in ovl_get_redirect() in unstable
> >> 2. 'old' dentry in ovl_link() can be disconnected, so there is
> >> no path for ovl_get_redirect() at all.
> >>
> >> The only way to get a disconnected dentry is with nfs export, so for
> >> now, it is enough to WARN_ON and return error in ovl_set_redirect()
> >> for (dentry->d_flags & DCACHE_DISCONNECTED)
> >> with a comment referring to nfs_export+metacopy support.
> >
> > Hi Amir,
> >
> > I can put this warning for DCACHE_DISCONNECTED dentries.
> >
> > Now, most interesting part for me here is that why do we need to
> > stop/synchronize other renames in the system while some set_redirect/get
> > _redirect operation is taking place. That's the part I don't understand.
> > When I am looking at current code, I feel d_lock, seems to be good enough
> > to make sure that ovl_get_redirect() works fine when parallel renames
> > progressing on other cpu.
> 
> Right.
> 
> >
> > So a simple example, is say I am creating a link bar/foo-link.txt to a file
> > foo/foo.txt and that triggers setting absolute redirect on foo.txt
> > and we will call ovl_get_direct() and  traverse the tree up till root.
> > Now say a part of the tree is also being renamed. Say foo/ is being
> > renamed to alpha/. I am wondering is d_lock is not enough to make sure
> > this is not a problem.
> >
> > We always set redirect first and then do rename. That means d_parent
> > should be changed only after redirect has been set on a dentry. And
> > that should guarantee that if ovl_get_redirect() sees new parent,
> > then parent_dentry->redirect has been already set. If it sees dentry
> > before rename, then redirect might be there if not, dentry name would
> > be used and it will also see the old parent and continue traversal
> > up.
> >
> > Is there anything I am missing?
> 
> No. I think you got it right.
> I was confused.
> 
> >
> >>
> >> Maybe the simplest solution w.r.t stabilizing path traversal
> >> is to move ovl_set_redirect() above lock_rename()
> >> as I suggested and use a new lock ofs->ovl_rename_mutex
> >> inside ovl_get_redirect() to protect the !samedir traversal
> >> and also take that lock before lock_rename() in ovl_rename().
> >
> > I already made changes to move ovl_set_redirect() above lock_rename()
> > in my copy. I still can't see the need of ofs->ovl_rename_mutex. Please
> > help me understand the need with an example.
> >
> 
> No need.
> 
> >>
> >> 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.

> 
> 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.

> 
> I hope moving set redirect before lock_rename simplified your
> patches, so this whole exercise served a point.

It definitely does. Thanks for that idea. 

Vivek