On Thu, Oct 19, 2017 at 5:58 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> On Thu, Oct 19, 2017 at 04:21:46PM +0300, Amir Goldstein wrote:
...
>>
>> Process 2 will get lower dentry on open for read at 8AM
>> Process 1 will copy up file at 9AM (on CPU1)
>> Process 2 will open same file for read at 9AM (on CPU2)
>> Does it matter if process 2 gets lower or upper dentry? No.
>> It only matter that IF process 2 gets an upper dentry, that
>> this dentry is consistent, so it only matters that IF __upperdentry
>> is visible to CPU2 AND OVL_UPPER_DATA flag is visible to
>> CPU2 then dentry and its inode are consistent.
>
> That's a good point. So if OVL_UPPER_DATA update is not visible on CPU2
> yet, then CPU1 will use lower dentry. And this is equivalent to as if file
> copy up has not taken place yet.
>
> And if CPU1 needed to do use upper dentry only, then it will do flags=WRITE
> and that will take oi->lock and make sure OVL_UPPER_DATA is set.
>
> So only *additional* smp_rmb()/smp_wmb() we require for the case when
> data is copied up later and we need to make sure OVL_UPPER_DATA is
> visible only after the full data copy up is done and stable.
>
>
Right. forgot about that wmb.
>>
>> So IMO you may only need to add smp_rmb() before
>> ovl_test_flag(OVL_UPPER_DATA in ovl_d_real() and the smp_wmb()
>> in ovl_inode_update() should be sufficient.
>> Change the comment in ovl_inode_update() to mention that wmb also
>> matches rmb in ovl_d_real() w.r.t OVL_UPPER_DATA flag.
>
> Hmm..., I agree that we require smp_rmb() here but it will pair with
> smp_wmb() in ovl_copy_meta_data_inode() and not the one in
> ovl_inode_update(), right? Something like.
Right. my bad.
>
> ovl_d_real() {
> bool has_upper_data;
>
> has_upper_data = ovl_test_flag(OVL_UPPER_DATA, d_inode(dentry));
> /* Pairs with smp_wmb() in ovl_copy_up_meta_inode_data() */
> smp_rmb();
> if (!has_upper_data)
> goto lower;
Just put smp_rmb() here. no need for the bool variable.
rmb does matter if you goto lower...
>
> ...
> ...
> return real;
> }
>
> Note that now smp_rmb() will be placed after loading OVL_UPPER_DATA and
> not before it. Because we are ensuring ordering w.r.t smp_wmb() in
> ovl_copy_up_meta_inode_data().
>
> In fact I think my current patches are buggy because they should have had
> this smp_rmb() to begin with.
>
--
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
