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

Amir Goldstein
2017-07-29T10:33:38Z

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.
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.
The reason is that my original patch did "call vfs_getattr() on the
overlay entries",
which meant constant d_ino IFF constant st_ino.
Your modifications turned the code to do constant d_ino, even for
non samefs, which is not that bad, but I don't think you meant for it.

Amir.