Re: [PATCH 7/7] ovl: stop using lookup_one() in ovl_iterate().

[NeilBrown]

On Wed, 30 Sep 2026, Amir Goldstein wrote:
> On Tue, Sep 29, 2026 at 5:43 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
> >
> > From: NeilBrown <neil@xxxxxxxxxx>
> >
> > lookup_one() is expected to be removed as it does not fit well with
> > proposed changes to directory locking.
> > Specifically d_alloc_parallel() will be ordered outside of i_rwsem
> > and as iterate_shared() is called with i_rwsem held it is not safe
> > to call d_alloc_parallel().
> >
> > We can instead call d_alloc_trylock() and then call the ->lookup, but
> > that can fail if there is a lookup attempt concurrent with the
> > readdir().
> >
> > ovl cannot afford for the lookup to fail as that could produce incorrect
> > results, and it cannot safely drop i_rwsem temporarily as that could
> > introduce races with handling of the directory cache.
> >
> > Instead we rely on the fact that ovl_iterate() has an exclusive lock on
> > the directory, so any concurrent lookup will wait for the ovl_iterate()
> > call to complete.  We allocate a separate dentry and if the lookup is
> > successful, it is hashed with the result.
> 
> Please document this assumption with:
> 
> rwsem_assert_held_write(&dir->d_inode->i_rwsem);
> 
> and comment in the code. Having it in the git commit message is not
> enough for future code reviewers.

Yes, I can do that.

> 
> Other than that, I have no further comments, but I would like
> to have a review from Miklos before this patch lands, because
> I don't remember him reviewing any of the revisions and this is subtle.
> I am a bit concerned that we have no test coverage for this subtle code.
> 
> >
> > When the concurrent lookup gets i_rwsem it mustn't do its own lookup -
> > it must use the existing dentry.  This is found, if it exists, using
> > try_lookup_noperm().
> >
> > Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> > ---
> >  fs/overlayfs/namei.c   | 12 ++++++++++++
> >  fs/overlayfs/readdir.c | 26 ++++++++++++++++++++++++--
> >  2 files changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index ca899fdfaafd..f56e33fd4d6b 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -1385,6 +1385,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >         struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> >         struct ovl_entry *poe = OVL_E(dentry->d_parent);
> >         bool check_redirect = (ovl_redirect_follow(ofs) || ofs->numdatalayer);
> > +       struct dentry *alias;
> >         int err;
> >         struct ovl_lookup_ctx ctx = {
> >                 .dentry = dentry,
> > @@ -1399,6 +1400,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >         if (dentry->d_name.len > ofs->namelen)
> >                 return ERR_PTR(-ENAMETOOLONG);
> >
> > +       /*
> > +        * The existence of this in-lookup dentry might have forced
> > +        * readdir to do the lookup with a new dentry.  If so we must
> > +        * return that one.
> > +        */
> > +       alias = try_lookup_noperm(&QSTR_LEN(dentry->d_name.name,
> > +                                           dentry->d_name.len),
> > +                                 dentry->d_parent);
> > +       if (alias && !IS_ERR(alias))
> > +               return alias;
> > +
> >         with_ovl_creds(dentry->d_sb)
> >                 err = ovl_lookup_layers(&ctx, &d);
> >
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index e7fe29cb6028..bc41da4bace8 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -574,8 +574,30 @@ static int ovl_cache_update(const struct path *path, struct ovl_cache_entry *p,
> >                 }
> >         }
> >         /* This checks also for xwhiteouts */
> > -       this = lookup_one(mnt_idmap(path->mnt), &QSTR_LEN(p->name, p->len), dir);
> > -       if (IS_ERR_OR_NULL(this) || !this->d_inode) {
> > +       this = d_alloc_trylock(dir, &QSTR_LEN(p->name, p->len));
> > +       if (this == ERR_PTR(-EWOULDBLOCK)) {
> > +               /*
> > +                * Some other thread is looking up this name and will
> > +                * block on i_rwsem before it can complete the lookup.
> > +                * We will do the lookup in a new dentry and when that
> > +                * lookup gets a turn it will find and return this
> > +                * dentry.
> > +                */
> > +               this = d_alloc_name(dir, p->name);
> > +               if (!this)
> > +                       this = ERR_PTR(-ENOMEM);
> > +       }
> > +       if (!IS_ERR(this) && d_unhashed(this)) {
> > +               /* Either we got an in-lookup or we made our own unhashed */
> > +               struct dentry *alias = ovl_lookup(dir->d_inode, this, 0);
> > +
> > +               d_lookup_done(this);
> > +               if (alias) {
> > +                       dput(this);
> > +                       this = alias;
> > +               }
> > +       }
> > +       if (IS_ERR(this) || !this->d_inode) {
> >                 /* Mark a stale entry */
> >                 p->is_whiteout = true;
> >                 if (IS_ERR(this)) {
> 
> How do you test your patch set? Do you have tests that exercise
> parallel create/lookup/readdir?
> 
> It would be irresponsible to merge code without showing that the `alias`
> branches have been exercised. Do you have an idea how to do that?

I think that to trigger the 'alias' code we would need to instrument the
code to block and wait.
e.g.  when readdir sees a particular name it could signal the test
harness somehow and then wait.  The test harness would trigger a lookup
of the name which would block on the parent i_rwsem.
The readdir then continues somehow and gets -EWOULDBLOCK and installs
the alias dentry.  When it completes the lookup continues and picks up
the alias.

Is there some way to set a tracepoint to block?

Maybe it would be enough to have something like:
   if (strcmp(name, "MAGIC_NAME") == 0) {
	printk("waiting for MAGIC_NAME\n");
	ssleep(15);
   }

and have the test harness watch dmesg for MAGIC_NAME then trigger the
lookup.   Then adds some tracing to ovl_lookup() to ensure it picks up
the right thing...

NeilBrown

> 
> Thanks,
> Amir.
>