Re: [PATCH 16/53] ovl: drop dir lock for lookups in impure readdir

On Sat, 21 Mar 2026, Amir Goldstein wrote:
> On Wed, Mar 18, 2026 at 10:10 PM NeilBrown <[email protected]> wrote:
> >
> > [[ CC list trimmed ]]
> >
> > On Mon, 16 Mar 2026, Amir Goldstein wrote:
> > > On Thu, Mar 12, 2026 at 10:49 PM NeilBrown <[email protected]> wrote:
> > > >
> > > > From: NeilBrown <[email protected]>
> > > >
> > > > When performing an "impure" readdir, ovl needs to perform a lookup on some
> > > > of the names that it found.
> > > > With proposed locking changes it will not be possible to perform this
> > > > lookup (in particular, not safe to wait for d_alloc_parallel()) while
> > > > holding a lock on the directory.
> > > >
> > > > ovl doesn't really need the lock at this point.
> > >
> > > Not exactly. see below.
> > >
> > > > It has already iterated
> > > > the directory and has cached a list of the contents.  It now needs to
> > > > gather extra information about some contents.  It can do this without
> > > > the lock.
> > > >
> > > > After gathering that info it needs to retake the lock for API
> > > > correctness.  After doing this it must check IS_DEADDIR() again to
> > > > ensure readdir always returns -ENOENT on a removed directory.
> > > >
> > > > Note that while ->iterate_shared is called with a shared lock, ovl uses
> > > > WRAP_DIR_ITER() so an exclusive lock is held and so we drop and retake
> > > > that exclusive lock.
> > > >
> > > > As the directory is no longer locked in ovl_cache_update() we need
> > > > dget_parent() to get a reference to the parent.
> > > >
> > > > Signed-off-by: NeilBrown <[email protected]>
> > > > ---
> > > >  fs/overlayfs/readdir.c | 19 ++++++++++++-------
> > > >  1 file changed, 12 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > > > index 1dcc75b3a90f..d5123b37921c 100644
> > > > --- a/fs/overlayfs/readdir.c
> > > > +++ b/fs/overlayfs/readdir.c
> > > > @@ -568,13 +568,12 @@ static int ovl_cache_update(const struct path *path, struct ovl_cache_entry *p,
> > > >                         goto get;
> > > >                 }
> > > >                 if (p->len == 2) {
> > > > -                       /* we shall not be moved */
> > > > -                       this = dget(dir->d_parent);
> > > > +                       this = dget_parent(dir);
> > > >                         goto get;
> > > >                 }
> > > >         }
> > > >         /* This checks also for xwhiteouts */
> > > > -       this = lookup_one(mnt_idmap(path->mnt), &QSTR_LEN(p->name, p->len), dir);
> > > > +       this = lookup_one_unlocked(mnt_idmap(path->mnt), &QSTR_LEN(p->name, p->len), dir);
> > >
> > > ovl_cache_update() is also called from ovl_iterate_merged() where inode
> > > is locked.
> > >
> > > >         if (IS_ERR_OR_NULL(this) || !this->d_inode) {
> > > >                 /* Mark a stale entry */
> > > >                 p->is_whiteout = true;
> > > > @@ -666,11 +665,12 @@ static int ovl_dir_read_impure(const struct path *path,  struct list_head *list,
> > > >         if (err)
> > > >                 return err;
> > > >
> > > > +       inode_unlock(path->dentry->d_inode);
> > > >         list_for_each_entry_safe(p, n, list, l_node) {
> > > >                 if (!name_is_dot_dotdot(p->name, p->len)) {
> > > >                         err = ovl_cache_update(path, p, true);
> > > >                         if (err)
> > > > -                               return err;
> > > > +                               break;
> > > >                 }
> > > >                 if (p->ino == p->real_ino) {
> > > >                         list_del(&p->l_node);
> > > > @@ -680,14 +680,19 @@ static int ovl_dir_read_impure(const struct path *path,  struct list_head *list,
> > > >                         struct rb_node *parent = NULL;
> > > >
> > > >                         if (WARN_ON(ovl_cache_entry_find_link(p->name, p->len,
> > > > -                                                             &newp, &parent)))
> > > > -                               return -EIO;
> > > > +                                                             &newp, &parent))) {
> > > > +                               err = -EIO;
> > > > +                               break;
> > > > +                       }
> > > >
> > > >                         rb_link_node(&p->node, parent, newp);
> > > >                         rb_insert_color(&p->node, root);
> > > >                 }
> > > >         }
> > > > -       return 0;
> > > > +       inode_lock(path->dentry->d_inode);
> > > > +       if (IS_DEADDIR(path->dentry->d_inode))
> > > > +               err = -ENOENT;
> > > > +       return err;
> > > >  }
> > > >
> > > >  static struct ovl_dir_cache *ovl_cache_get_impure(const struct path *path)
> > > > --
> > >
> > > You missed the fact that overlayfs uses the dir inode lock
> > > to protect the readdir inode cache, so your patch introduces
> > > a risk for storing a stale readdir cache when dir modify operations
> > > invalidate the readdir cache version while lock is dropped
> > > and also introduces memory leak when cache is stomped
> > > without freeing cache created by a competing thread.
> > > I think something like the untested patch below should fix this.
> >
> > Yes, I did miss that - thanks. I think I missed a few other details too.
> > I no longer think it can be safe to drop the lock without substantial
> > rewrites - and even then maybe not.
> >
> > So I'm considering a different approach.
> > This patch demonstrates what I'm thinking, though it still needs work I
> > think.
> 
> I like this direction.
> 
> I always thought that we need to get rid of this vfs lookup
> inside readdir but I thought it would be a lot of work.
> 
> Your suggestion walks around this in an elegant way.
> 

Thanks :-)


> >
> > Thanks,
> > NeilBrown
> >
> > From: NeilBrown <[email protected]>
> > Subject: [PATCH] ovl: stop using lookup_one() in iterate_shared() handling.
> >
> > 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_noblock() 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 and 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.
> >
> > When the concurrent lookup gets i_rwsem it mustn't do its own lookup -
> > it must use the existing dentry.  This is done using
> > try_lookup_noperm().  To manage overheads we keep a counter of the
> > number of "Stray dentries" there might be on each directory and only
> > check for one when this count is non zero.
> >
> > If a 'stray dentry' were discarded for any reason before the concurrent
> > lookup completed, the count would never reach zero.  That might be a problem.
> 
> Can we deal with the discarded dentries using OVL_E_FLAGS() for
> a stray ovl dentry implement the relevant ovl_dentry_operations to decrement
> the stray counter?

Maybe ... though I'd really rather get rid of the stray counter.
We could drop it completely and always do the try_lookup_noperm() in
ovl_lookup(), but that might have a performance cost.
I'd really like to be able to mark the in-lookup dentry which caused
d_alloc_noblock() to fail so that when the lookup on that dentry happens
we can know to do the try_lookup_noperm().  That avoids both the counter
and the cost.

I had previously suggested a d_alloc_locked() which could return an
in-lookup dentry providing the filesystem provided some interlock so the
caller of d_alloc_locked() and ->lookup wouldn't both instantiate the
dentry.  Al wasn't keen on that.  Maybe I could try again.  I'd like to
get some review from him on the current patches first though...

Thanks,
NeilBrown