Re: [PATCH 02/12] ovl: Call ovl_create_temp() and ovl_create_index() without lock held.

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Jun 25, 2025 at 5:44 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@brown.name> wrote:
> >
> > ovl currently locks a directory or two and then performs multiple actions
> > in one or both directories.  This is incompatible with proposed changes
> > which will lock just the dentry of objects being acted on.
> >
> > This patch moves calls to ovl_create_temp() and ovl_create_index() out
> > of the locked region and has them take and release the relevant lock
> > themselves.
> >
> > The lock that was taken before these functions are called is now taken
> > after.  This means that any code between where the lock was taken and
> > these calls is now unlocked.  This necessitates the creation of
> > _unlocked() versions of ovl_cleanup() and ovl_lookup_upper().  These
> > will be used more widely in future patches.
> >
> > ovl_cleanup_unlocked() takes a dentry for the directory rather than an
> > inode as this simplifies calling slightly.
> >
> > Note that when we move a lookup or create out of a locked region in
> > which the dentry is acted on, we need to ensure after taking the lock
> > that the dentry is still in the directory we expect it to be in.  It is
> > conceivable that it was moved.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/overlayfs/copy_up.c   | 37 +++++++++++-------
> >  fs/overlayfs/dir.c       | 84 +++++++++++++++++++++++++---------------
> >  fs/overlayfs/overlayfs.h | 10 +++++
> >  fs/overlayfs/super.c     |  7 ++--
> >  4 files changed, 88 insertions(+), 50 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 8a3c0d18ec2e..7a21ad1f2b6e 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -517,15 +517,12 @@ static int ovl_set_upper_fh(struct ovl_fs *ofs, struct dentry *upper,
> >
> >  /*
> >   * Create and install index entry.
> > - *
> > - * Caller must hold i_mutex on indexdir.
> >   */
> >  static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> >                             struct dentry *upper)
> >  {
> >         struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> >         struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
> > -       struct inode *dir = d_inode(indexdir);
> >         struct dentry *index = NULL;
> >         struct dentry *temp = NULL;
> >         struct qstr name = { };
> > @@ -558,17 +555,21 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> >         err = ovl_set_upper_fh(ofs, upper, temp);
> >         if (err)
> >                 goto out;
> > -
> > +       lock_rename(indexdir, indexdir);
>
> This is a really strange hack.
> I assume your next patch set is going to remove this call, but I do not wish
> to merge this hack as is for an unknown period of time.
>
> Please introduce helpers {un,}lock_parent()
>
> >         index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
> >         if (IS_ERR(index)) {
> >                 err = PTR_ERR(index);
> > +       } else if (temp->d_parent != indexdir) {
> > +               err = -EINVAL;
> > +               dput(index);
>
> This can be inside lock_parent(parent, child)
> where child is an optional arg.
>
> err = lock_parent(indexdir, temp);
> if (err)
>        goto out;
>
> Because we should be checking this right after lock and
> not after ovl_lookup_upper().
>
> >         } else {
> >                 err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
> >                 dput(index);
> >         }
> > +       unlock_rename(indexdir, indexdir);
> >  out:
> >         if (err)
> > -               ovl_cleanup(ofs, dir, temp);
> > +               ovl_cleanup_unlocked(ofs, indexdir, temp);
> >         dput(temp);
> >  free_name:
> >         kfree(name.name);
> > @@ -779,9 +780,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >                 return err;
> >
> >         ovl_start_write(c->dentry);
> > -       inode_lock(wdir);
> >         temp = ovl_create_temp(ofs, c->workdir, &cattr);
> > -       inode_unlock(wdir);
> >         ovl_end_write(c->dentry);
> >         ovl_revert_cu_creds(&cc);
> >
> > @@ -794,6 +793,8 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >          */
> >         path.dentry = temp;
> >         err = ovl_copy_up_data(c, &path);
> > +       if (err)
> > +               goto cleanup_write_unlocked;
> >         /*
> >          * We cannot hold lock_rename() throughout this helper, because of
> >          * lock ordering with sb_writers, which shouldn't be held when calling
> > @@ -801,6 +802,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >          * temp wasn't moved before copy up completion or cleanup.
> >          */
> >         ovl_start_write(c->dentry);
> > +
> > +       if (S_ISDIR(c->stat.mode) && c->indexed) {
> > +               err = ovl_create_index(c->dentry, c->origin_fh, temp);
> > +               if (err)
> > +                       goto cleanup_unlocked;
> > +       }
> > +
> >         trap = lock_rename(c->workdir, c->destdir);
> >         if (trap || temp->d_parent != c->workdir) {
> >                 /* temp or workdir moved underneath us? abort without cleanup */
> > @@ -809,20 +817,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >                 if (IS_ERR(trap))
> >                         goto out;
> >                 goto unlock;
> > -       } else if (err) {
> > -               goto cleanup;
> >         }
> >
> >         err = ovl_copy_up_metadata(c, temp);
> >         if (err)
> >                 goto cleanup;
> >
> > -       if (S_ISDIR(c->stat.mode) && c->indexed) {
> > -               err = ovl_create_index(c->dentry, c->origin_fh, temp);
> > -               if (err)
> > -                       goto cleanup;
> > -       }
> > -
> >         upper = ovl_lookup_upper(ofs, c->destname.name, c->destdir,
> >                                  c->destname.len);
> >         err = PTR_ERR(upper);
> > @@ -857,6 +857,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >         ovl_cleanup(ofs, wdir, temp);
> >         dput(temp);
> >         goto unlock;
> > +
> > +cleanup_write_unlocked:
> > +       ovl_start_write(c->dentry);
> > +cleanup_unlocked:
> > +       ovl_cleanup_unlocked(ofs, c->workdir, temp);
> > +       dput(temp);
> > +       goto out;
> >  }
>
> Wow these various cleanup flows are quite hard to follow.
> This is a massive patch set which is very hard for me to review
> and it will also be hard for me to maintain the code as it is.
> We need to figure out a way to simplify the code flow
> either more re-factoring or using some scoped cleanup hooks.
> I am open to suggestions.
>
> Thanks,
> Amir.
>
> >
> >  /* Copyup using O_TMPFILE which does not require cross dir locking */
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 4fc221ea6480..a51a3dc02bf5 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -43,6 +43,21 @@ int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
> >         return err;
> >  }
> >
> > +int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
> > +                        struct dentry *wdentry)
> > +{
> > +       int err;
> > +
> > +       inode_lock_nested(workdir->d_inode, I_MUTEX_PARENT);
> > +       if (wdentry->d_parent == workdir)
> > +               ovl_cleanup(ofs, workdir->d_inode, wdentry);
> > +       else
> > +               err = -EINVAL;
> > +       inode_unlock(workdir->d_inode);
> > +
> > +       return err;
> > +}
> > +

Just to illustrate what I meant and how the flow of ovl_cleanup_unlocked()
and later on ovl_cleanup() would look simpler:

int lock_parent(struct dentry *parent, struct dentry *child)
{
       int err;

       inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
       if (!child || child->d_parent == parent)
              return 0;

       inode_unlock(parent->d_inode);
       return -EINVAL;
}

int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
                        struct dentry *wdentry)
{
       int err;

       err = parent_lock(workdir, wdentry);
       if (err)
               return err;

       ovl_cleanup(ofs, workdir->d_inode, wdentry);
       parent_unlock(workdir);
       return 0;
}

Thanks,
Amir.