Re: [PATCH v3 5/5] ovl: consistent st_ino/d_ino

Miklos Szeredi
2017-07-31T10:58:07Z

On Sat, Jul 29, 2017 at 12:33 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Jul 28, 2017 at 12:25 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Thu, Jul 27, 2017 at 11:00 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Wed, Jun 21, 2017 at 11:48 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>> On Wed, Jun 21, 2017 at 11:36 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>>> On Wed, Jun 21, 2017 at 12:20 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>>>> On Wed, Jun 21, 2017 at 11:05 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>>>
>>>>>>>
>>>>>>> Following up on your idea:
>>>>>>> - check in ovl_iterate() if version has changed and if dir became impure
>>>>>>> - if it did, populate od->cache, but keep the dir od->is_real
>>>>>>> - iterate upper cache entries and call ovl_cache_update_ino()
>>>>>>> - Then actor of real dir iterator can use the cache to ommit entries or use
>>>>>>> p->ino from cache if p->real_ino match real d_ino, but differs from p->ino.
>>>>>>
>>>>>> For non-merge dirs we can have a simplified cache just containing the
>>>>>> entries with origin, recreated when the version changes or updated in
>>>>>> rename, whichever is simpler. A non-merge dir will never become a
>>>>>> merge one, so we can keep the handling separate.
>>>>>>
>>>>>
>>>>> And use this cache to ommit/fix entries with actor?
>>>>
>>>> Right. I think fixing up is better, because to correctly omit entries
>>>> we'd need separate lists for each open directory. For fixing up we
>>>> can use a common one, just like for the merged dir.
>>>
>>> And back to this, I pushed a branch named "ovl-d_ino" to my vfs tree.
>>>
>>> Please test.
>>>
>>
>> I can confirm that xfstest overlay/017, the only test I have for
>> constant d_ino,
>> passes. Although it does not cover all cases handled by the patch set
>> (e.g. d_ino of merge parent).
>>
>
> Miklos,
>
> Gave the code a quick review pass.
> Overall looking good.

Thanks for the review.

> One thing that stands out in "my" patch ("ovl: constant d_ino across copy up")
> The comments and commit message claim "When all layers are on the same fs",
> but the patch doesn't actually checks that.

I does in ovl_cache_update_ino(). That check would be more logical
in ovl_calc_d_ino(), though. Fixed.

Thanks,
Miklos