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

[Mateusz Guzik]

On Fri, Dec 12, 2025 at 2:22 AM Chris Mason <clm@xxxxxxxx> wrote:
>
> On Wed, 19 Nov 2025 15:29:54 +0100 Mateusz Guzik <mjguzik@xxxxxxxxx> 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?

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

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)))

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