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(). The race with ovl_instantiate() you pointed out is interesting, but easy to solve by checking for non-NULL lower. 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. 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
