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

[Vivek Goyal <vgoyal@redhat.com>]

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?


[..]
> >> > +static inline bool open_for_write(int flags)
> >>
> >> Need ovl_ namespace prefix
> >> and the name sounds like an action (i.e. a request to open for write)
> >
> > ovl_opened_for_write_or_trunc()?
> 
> OK. maybe ovl_open_flags_need_copy_up() ?

Ok, will use ovl_open_flags_need_copy_up().

Vivek