Re: [PATCH v7 7/8] ovl: update cache version of impure parent on rename

[Miklos Szeredi <miklos@szeredi.hu>]

On Mon, Nov 6, 2017 at 11:06 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Nov 6, 2017 at 11:17 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Thu, Nov 2, 2017 at 9:38 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> ovl_rename() updates dir cache version for impure old parent if an entry
>>> with copy up origin is moved into old parent, but it did not update
>>> cache version if the entry moved out of old parent has a copy up origin.
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>> ---
>>>  fs/overlayfs/dir.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>>> index ef533198be45..26aef3b5f007 100644
>>> --- a/fs/overlayfs/dir.c
>>> +++ b/fs/overlayfs/dir.c
>>> @@ -1075,9 +1075,10 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>>>                         drop_nlink(d_inode(new));
>>>         }
>>>
>>> -       ovl_dentry_version_inc(old->d_parent,
>>> -                              !overwrite && ovl_type_origin(new));
>>> +       ovl_dentry_version_inc(old->d_parent, ovl_type_origin(old));
>>>         ovl_dentry_version_inc(new->d_parent, ovl_type_origin(old));
>>> +       if (!overwrite)
>>> +               ovl_dentry_version_inc(old->d_parent, ovl_type_origin(new));
>>
>> How about the opposite (i.e. newdir losing impurity)?  That can happen two ways:
>>
>>   - overwriting copied up file (new) with pure one (old)
>>   - exchange copied up file (new) with pure one (old)
>>
>> Also I'd merge the two version increments into one, although that
>> doesn't really matter:
>>
>> -    ovl_dentry_version_inc(old->d_parent,
>> -                   !overwrite && ovl_type_origin(new));
>> -    ovl_dentry_version_inc(new->d_parent, ovl_type_origin(old));
>> +    ovl_dentry_version_inc(old->d_parent, ovl_type_origin(old) ||
>> +                   (!overwrite && ovl_type_origin(new)));
>> +    ovl_dentry_version_inc(new->d_parent,
>> +                   ovl_type_origin(old) || ovl_type_origin(new));
>>
>> Fixed up the patch, but tell me if I'm missing something here.
>>
>
> I think you are.
> Both my version and your version are wrong because the argument
> 'impurity' should really be 'impurity_change'
>
> so if we have:
> oldimpurity = ovl_type_origin(old);
> newimpurity = d_inode(new) ? ovl_type_origin(new) : false;
>
> version increment should really be something like this:
> ovl_dentry_version_inc(old->d_parent, oldimpurity ^ (overwrite ?
> newimpurity : false));
> ovl_dentry_version_inc(new->d_parent, oldimpurity ^ newimpurity);
>
>
> I think...

We are not only interested in change of state, but rather if the cache
is valid or not.  If an impurity is removed and another added, then
that needs to be noted as well.

Note: strictly speaking we don't need to invalidate the cache on entry
removal (that's why things worked fine before this patch).  And in
fact recalculating the cache on every removal of an impurity might
well cause worse performance than not doing that.  So this could be
further optimized to differentiate between removal and addition of
impurity and only "garbage collect" old caches occasionally.  But lets
not get lost in such details for now.

Thanks,
Miklos