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