Re: [PATCH v5] fs: add predicts based on nd->depth

[Chris Mason <clm@meta.com>]

On 12/12/25 6:55 AM, Mateusz Guzik wrote:
> On Fri, Dec 12, 2025 at 2:22 AM Chris Mason <clm@meta.com> wrote:
>>
>> On Wed, 19 Nov 2025 15:29:54 +0100 Mateusz Guzik <mjguzik@gmail.com> wrote:
>>
>> Hi everyone,
>>
>> I ran some of the recent commits through the AI review prompts and it
>> flagged this one.  The comments look right to me:
>>
>> [ ... ]
>>
>>> diff --git a/fs/namei.c b/fs/namei.c
>>> index a5bffc2a29f6c..5fbb05fb96b75 100644
>>> --- a/fs/namei.c
>>> +++ b/fs/namei.c
>>
>> [ ... ]
>>
>>> @@ -882,7 +883,7 @@ static bool try_to_unlazy(struct nameidata *nd)
>>>
>>>       BUG_ON(!(nd->flags & LOOKUP_RCU));
>>>
>>> -     if (unlikely(!legitimize_links(nd)))
>>> +     if (unlikely(nd->depth && !legitimize_links(nd)))
>>>               goto out1;
>>
>> When nd->depth is 0, this short-circuits and never calls legitimize_links().
>> However, legitimize_links() contains a LOOKUP_CACHED check that was moved
>> there in commit b93a6e389bdc7 ("fix handling of nd->depth on LOOKUP_CACHED
>> failures in try_to_unlazy*"):
>>
>>     static bool legitimize_links(struct nameidata *nd)
>>     {
>>         if (unlikely(nd->flags & LOOKUP_CACHED)) {
>>             drop_links(nd);
>>             nd->depth = 0;
>>             return false;
>>         }
>>         ...
>>     }
>>
>> Before that commit, the LOOKUP_CACHED check was done directly in
>> try_to_unlazy() before calling legitimize_links(). It was moved into
>> legitimize_links() for consolidation, not because it's semantically
>> related to having links.
>>
>> With LOOKUP_CACHED set (used by io_uring for non-blocking opens) and
>> nd->depth == 0, try_to_unlazy() now proceeds to legitimize_path() and may
>> succeed, when previously it would have failed. This can cause
>> lookup_fast()->try_to_unlazy() to succeed, leading the caller to call
>> lookup_slow() which takes inode_lock_shared() and blocks - violating
>> LOOKUP_CACHED semantics.
>>
>> Should the LOOKUP_CACHED check be moved back to try_to_unlazy() before
>> the depth check, like:
>>
>>     if (unlikely(nd->flags & LOOKUP_CACHED))
>>         goto out1;
>>     if (unlikely(nd->depth && !legitimize_links(nd)))
>>         goto out1;
>>
> 
> Thanks for the report. This is indeed a bug on my end, in my defense
> the current behavior is... interesting -- why would it the routine
> fail when it had nothing to do?

These corners are the best use case for the AI reviews. I never would
have found this bug without a test case and a bisection.

> 
> The commit hash you referenced does not exist in master, I found this
> instead: eacd9aa8cedeb412842c7b339adbaa0477fdd5ad

Sorry about that, the tree I was working on had a bunch of backports to
the fb kernels in different branches.

> 
> That said, the proposed patch does not do the trick as it fails to
> clean up links if nd->depth && nd->flags & LOOKUP_CACHED. The check
> however can be planted *after* if (unlikely(nd->depth &&
> !legitimize_links(nd)))

Oh yeah, I missed that part.  It looks like that's exactly what Al's
eacd9aa8c was fixing.

> 
> This would clean up the bug but retain the weird (for me anyway)
> state. Perhaps this is good enough as a fixup for the release and some
> clean up is -next material

Thanks,
Chris