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