On Wed, Oct 11, 2017 at 11:16 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> On Tue, Oct 10, 2017 at 08:03:05PM +0300, Amir Goldstein wrote:
>
> [..]
>> > @@ -511,6 +564,10 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
>> > goto out_cleanup;
>> >
>> > ovl_inode_update(d_inode(c->dentry), newdentry);
>> > + if (c->metadata_only) {
>> > + smp_wmb();
>>
>> This wmb does not address the only problem imo.
>> The problem is you must not allow inode to appear to have
>> An upper dentry before you made sure metacopy flag is visible.
>> So first you need to set metacopy flag before updating upper.
>> Then you also need to test them is correct order with rmb.
>
> Ok, so conceptually it makse sense to me that OVL_METACOPY flag should
> be set before upper dentry is made visible.
>
> So ovl_set_flag(OVL_METACOPY) should move before ovl_inode_update().
> ovl_inode_update() already has a smp_wmb(). So write side is probably
> fine.
>
> But read side, ovl_upperdentry_dereference() only has data dependency
> barrier and not smp_rmb(). (lockless_dereference(oi->__upperdentry)). I
> am not sure if that's sufficient for ovl_inode->flags read or not. May
> be we need to replace data dependency barrier with smp_rmb() in in
> ovl_upperdentry_dereference()?
I think we do need smp_rmb() between ovl_upperdentry_dereference()
and testing OVL_METACOPY, but not inside ovl_upperdentry_dereference()
somewhere in d_real() path maybe in ovl_open_need_copy_up()
>
> Even if we do this, I think we need barriers around setting/resetting
> of OVL_METACOPY for the reset case. I will explain this in other patch.
>
I think the spinlock in atomic bit ops of set_bit/clear_bit is got you covered
w.r.t. memory barriers. Only need to worry about lockless test_bit() IMO.
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
