On Wed, Oct 11, 2017 at 11:27 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > On Tue, Oct 10, 2017 at 08:12:00PM +0300, Amir Goldstein wrote: >> On Tue, Oct 10, 2017 at 6:32 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: >> > We can access and check metacopy flag outside ovl_inode->lock. IOW, say >> > a file was meta copied up in the past and it has metacopy flag set. Now >> > a data copy up operation in progress. Say another thread reads state of >> > this flag and if flag clearing is visible before file has been fully >> > copied up, that can cause problems. >> > >> > CPU1 CPU2 >> > ovl_copy_up_flags() acquire(oi->lock) >> > ovl_dentry_needs_data_copy_up() ovl_copy_up_data_inode() >> > ovl_test_metacopy_flag() ovl_copy_up_data() >> > ovl_clear_metacopy_flag() >> > release(oi->lock) >> > >> > Say CPU2 is copying up data and in the end clears metacopy flag. But if >> > CPU1 perceives clearing of metacopy before actual data copy up operation >> > being finished, that can become a problem. We want this clearing of flag >> > to be visible only if all previous write operations have finished. >> > >> > Hence this patch introduces smp_wmb() on metacopy flag set/clear operation >> > and smp_rmb() on metacopy flag test operation. >> > >> > May be some other lock or barrier is already covering it. But I am not sure >> > what that is and is it obvious enough that we will not break it in future. >> > >> > So hence trying to be safe here and introducing barriers explicitly for >> > metacopy bit. >> >> Please revisit your changes after addressing my comment on patch 5. > > Assume, we added smp_rmb() in ovl_upperdentry_dereference(), to make > sure by the time upperdentry is visible, OVL_METACOPY is visible too. > >> IIUC the flow you describe above should be addressed by testing >> metacopy flag again under ovl inode lock. >> >> The only subtle part imo is making metacopy flag visible >> Before making upper dentry visible. >> Clearing metacopy flag should not be a problem imo, >> As it comes after fsync, but didn't look closely. > > Say a file has been metadata copied up only. So upper is visible and > OVL_METACOPY flag is set. > > Now, what happens if two paths try to data copy up the file. Say first > path gets the oi->lock and starts doing data copy. Second path enters > ovl_copy_up_flags() and checks if data copy up is required or not. If > OVL_METACOPY is still set, then second process will block on oi->lock > and once first process exits will check OVL_METACOPY again under lock > and bail out. So that is not a problem. > > What about the other case. That is OVL_METACOPY gets cleared before > data copy operation is complete. If I don't introduce smp_wmb()/smp_rmb() > around resetting of OVL_METACOPY, what will make sure that > ovl_copy_up_flags() will see OVL_METACOPY=0 only after data copy up is > complete. Of course we will not like to return to caller of > ovl_copy_up_flags() while data copy up is still going on on a different > cpu. > vfs_removexattr() between data copy up and clearing OVL_METACOPY takes inode lock. IIUC, any lock/unlock has a full memory barrier. 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
