On Fri, Oct 27, 2017 at 07:35:00AM +0300, Amir Goldstein wrote: > On Thu, Oct 26, 2017 at 8:54 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > > On Thu, Oct 26, 2017 at 09:34:15AM +0300, Amir Goldstein wrote: > >> On Wed, Oct 25, 2017 at 10:09 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > >> > If a file is copied up metadata only and later when same file is opened > >> > for WRITE, then data copy up takes place. We copy up data, remove METACOPY > >> > xattr and then set the UPPERDATA flag in ovl_entry->flags. While all > >> > these operations happen with oi->lock held, read side of oi->flags is > >> > lockless. That is another thread on another cpu can check if UPPERDATA > >> > flag is set or not. > >> > > >> > So this gives us an ordering requirement w.r.t UPPERDATA flag. That is, if > >> > another cpu sees UPPERDATA flag set, then it should be guaranteed that > >> > effects of data copy up and remove xattr operations are also visible. > >> > > >> > For example. > >> > > >> > CPU1 CPU2 > >> > ovl_copy_up_flags() acquire(oi->lock) > >> > ovl_dentry_needs_data_copy_up() ovl_copy_up_data() > >> > ovl_test_flag(OVL_UPPERDATA) vfs_removexattr() > >> > ovl_set_flag(OVL_UPPERDATA) > >> > release(oi->lock) > >> > > >> > Say CPU2 is copying up data and in the end sets UPPERDATA flag. But if > >> > CPU1 perceives the effects of setting UPPERDATA flag but not effects of > >> > preceeding operations, that would be a problem. > >> > >> Why would that be a problem? > >> What can go wrong? > > > > That's a good question. I really don't have a concrete example where I can > > say this this can go wrong. Can you think of something. > > > >> If you try to answer the question instead of referring to a vague "problem" > >> you will see that only the ovl_d_real() code path can be a problem. > > > > Right. And ovl_copy_up_flags() will be called from ovl_d_real(). Will > > update it to show cover more of parent chain. > > > > That is not what I meant. > I don't see any negative outcome that could happen from false view > of OVL_UPPERDATA flag in ovl_copy_up_flags(). Sure, but barrier needs to be near to variable being protected by barrier. My point is that if you move them out in ovl_d_real() instead, then it is hard to understand what's being protected by barrier. > The race with ovl_instantiate() you pointed out is interesting, but easy to > solve by checking for non-NULL lower. Yep it is. But it does not give me a good feeling because if barriers are protecting something, it should protect in all cases. Now we have a special case where barrier is not protecting a field fully and that means we will rely on some other field. And that makes understanding locking such a difficult task. > > The path of ovl_d_real() that exposes upper inode to early is the only > path I see that can cause problems, as well as perhaps ovl_getattr(), > which may expose wrong blocks. Right. At this point of time ovl_d_real() and ovl_getattr() seem to be only two paths which access OVL_UPPERDATA in lockless manner. Vivek -- 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
