On Sat, Oct 14, 2017 at 9:05 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > On Fri, Oct 13, 2017 at 9:27 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: >> On Thu, Oct 12, 2017 at 12:08:04AM +0300, Amir Goldstein wrote: >>> 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. >> >> When I read memory-barrier.txt, it seems to suggest that RELEASE can let >> instructions outside critical region sneak into critical region. If that's >> the case, actually clearing of metaflag can happen before xattr actually >> got removed. Though I can't think what will go wrong in that case. > > clear_bit takes a spinlock so I *think* clearing of flag cannot sneak > before removing xattr. > >> >> Also will all this work without other cpu having any matching barrier? >> Again, I am not barrier expert and I am trying to understand these and > > Me neither, trying to understand this along with you. > >> one of the messages seems to be that most of the time they have to be >> paired. So in this case, we are taking inode lock/unlock on one cpu >> but cpu which is checking value of OVL_METACOPY, really does not have >> any matching acquire/release or some other kind of barrier. >> > > Other CPU needs rmb before checking flag to guarantee the order > of setting METACOPY flag before setting upperdentry. > > If other CPU sees positive upperdentry and the METACOPY flag cleared, > then I *think* it should be safe to return the upper inode from d_real(), > because RELEASE on inode lock should have the proper guaranties > for consistency of inode data structures in memory. Nah, smp_wmb before clearing METACOPY is correct, just like smp_wmb in ovl_inode_update(). probably should copy the same comment. But that was already there before this wrappers patch, so I don't see the need for this patch. 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
