Re: [PATCH v7 09/14] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Nov 20, 2017 at 05:18:48PM +0200, Amir Goldstein wrote:
> On Mon, Nov 20, 2017 at 4:34 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Sat, Nov 18, 2017 at 09:06:41AM +0200, Amir Goldstein wrote:
> >
> > [..]
> >> >> > Note, we now set OVL_UPPERDATA on all kind of dentires and check
> >> >> > OVL_UPPERDATA only where it makes sense. If a file is created on upper,
> >> >> > we set OVL_UPPERDATA. This can be accessed lockless in ovl_d_real()
> >> >> > path again. This time ordering dependency is w.r.t oe->__upperdata. We
> >> >> > need to make surr OVL_UPPERDATA is visible before oe->__upperdata is
> >> >> > visible. Otherwise following code path might try to copy up an upper
> >> >> > only file.
> >> >>
> >> >> Why all this explanations?
> >> >> Just use ovl_set_upperdata() when creating upper and be done with it.
> >> >
> >> > Just using ovl_set_upperdata() will not do away with ordering dependency
> >> > right? I mean, setting OVL_UPPERDATA in file creation path has different
> >> > ordering requirement (same is the case of >has_upper). And I wanted to
> >> > highlight that ordering requirement in changelogs.
> >> >
> >> > I can get rid of it. But this seems such a subtle requirement, I think
> >> > its a good idea to talk about it explicitly.
> >> >
> >>
> >> I am not following. you only need to ovl_set_upperdata() before
> >> ovl_inode_update(), just like in regular copy up. Am I missing something
> >> subtle?
> >
> > Hi Amir,
> >
> > Right. This is ordering dependency between setting of OVL_UPPERDATA and
> > oi->__upperdentry.
> >
> > So as per barrier documentation file, read/write barrier should look
> > as follows.
> >
> >         CPU1                            CPU2
> >      set OVL_UPPERDATA                  smp_rmb()
> >      smp_wmb()                          Load oi->__upperdentry
> >      set oi->__upperdentry
> >
> > So we need to place smp_wmb() after setting OVL_UPPERDATA and place
> > smp_rmb() just before load of oi->__upperdentry.
> >
> > Instead of smp_rmb() we are relying on checking for ovl_dentry_lower().
> > That is even if OVL_UPPERDATA is not visible on CPU2, but
> > oi->__upperdentry is visible, then we don't try to copy up a file which
> > does not have a lower.
> >
> > So ovl_dentry_lower() is preventing the race which *in-theory* can trigger
> > trying to copy up file which does not have lower.
> >
> > Does this make sense?
> >
> 
> Maybe. Brain overflow.
> To put it shortly, if there is a problem with setting OVL_UPPERDATA on new upper
> then there is a problem with setting upper alias as well (as you pointed out).
> I don't think that is really the case, but I am not the one to explain why.
> It just does not make sense that d_instantiate doesn't make sure that inode
> fields are consistent before attaching the inode to dentry.
> I think the barriers hide in raw_write_seqcount_begin()/raw_seqcount_begin()
> for the lockless dentry cache lookup, but I'm not an expert.
> 
> Anyway, I find the commit message text from "If a file is created on upper,..."
> very confusing.
> I suggest that if you are not sure about this leave a "XXX" comment in the
> code path of creating new upper and setting  OVL_UPPERDATA flag or
> "TODO" comment if you thing there is something to be done.
> 
> It is easy to later apply a patch to remove the XXX comment after explaining it
> or remove TODO comment after fixing it.

Ok, for now, I will just put a comment in code and deal with it later if
need be. Once it happens, it will be a NULL pointer dereference as we might
try to copy up a file which does not have a lower. So we will know if
problem happens. :-)

Vivek

> A confusing text remains in git log forever and get git history
> researchers off track.
> 
> Amir.