Re: [PATCH] dir.c: avoid stat() in valid_cached_dir()

From: Duy Nguyen <pclouds@gmail.com>
Date: Jan 2, 2018, 00:02 UTC
Message-ID: <CACsJy8CvYcLm7YGYenea=58EVesLMBiOh3eWMWWSr8LLDsGmPQ@mail.gmail.com>
In-Reply-To: <87h8sar5sl.fsf@evledraar.gmail.com>

On Fri, Dec 29, 2017 at 2:50 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 17 quoted lines

>
> On Thu, Dec 28 2017, Nguyễn Thái Ngọc Duy jotted:
>
>> stat() may follow a symlink and return stat data of the link's target
>> instead of the link itself. We are concerned about the link itself.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  I noticed this while looking at the untracked cache bug [1] but
>>  because it's not directly related to that bug, I'm submitting it
>>  separately here.
>>
>>  [1] https://public-inbox.org/git/CACsJy8AmbKSp0mDLRaDCWn45veeNc03B-Gq8r8PQPrDt9bM_EA@mail.gmail.com/
>
> I'm slowly trying to piece together a re-submission of this whole
> series, if this is a bug and not just an optimziation shouldn't there be
> some test case that demonstrates this bug?

It's kind of hard to demonstrate the bug. I think when path->buf is a symlink, we most likely find that its target's stat data does not match our cached one, which means we ignore the cache and fall back to slow path. This is performance issue, not correctness (though we could still catch it by verying test-dump-untracked-cache, I guess; I could try writing a test case for this if you want). The less unlikely case is, link target stat data matches the cached version and we incorrectly go fast path, ignoring real data on disk. A test for this may involve manipulating stat data, which may be not portable.

-- 
Duy