On Sat, 12 Jul 2025, Amir Goldstein wrote:
> On Fri, Jul 11, 2025 at 1:21 AM NeilBrown <neil@xxxxxxxxxx> wrote:
> >
> > ovl_whiteout() relies on the workdir i_rwsem to provide exclusive access
> > to ofs->whiteout which it manipulates. Rather than depending on this,
> > add a new mutex, "whiteout_lock" to explicitly provide the required
> > locking. Use guard(mutex) for this so that we can return without
> > needing to explicitly unlock.
> >
> > Then take the lock on workdir only when needed - to lookup the temp name
> > and to do the whiteout or link.
> >
> > Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> > ---
> > fs/overlayfs/dir.c | 49 +++++++++++++++++++++-------------------
> > fs/overlayfs/ovl_entry.h | 1 +
> > fs/overlayfs/params.c | 2 ++
> > 3 files changed, 29 insertions(+), 23 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 086719129be3..fd89c25775bd 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -84,41 +84,44 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> > struct dentry *workdir = ofs->workdir;
> > struct inode *wdir = workdir->d_inode;
> >
> > - inode_lock_nested(wdir, I_MUTEX_PARENT);
> > + guard(mutex)(&ofs->whiteout_lock);
> > +
> > if (!ofs->whiteout) {
> > + inode_lock_nested(wdir, I_MUTEX_PARENT);
> > whiteout = ovl_lookup_temp(ofs, workdir);
> > - if (IS_ERR(whiteout))
> > - goto out;
> > -
> > - err = ovl_do_whiteout(ofs, wdir, whiteout);
> > - if (err) {
> > - dput(whiteout);
> > - whiteout = ERR_PTR(err);
> > - goto out;
> > + if (!IS_ERR(whiteout)) {
> > + err = ovl_do_whiteout(ofs, wdir, whiteout);
> > + if (err) {
> > + dput(whiteout);
> > + whiteout = ERR_PTR(err);
> > + }
> > }
> > + inode_unlock(wdir);
> > + if (IS_ERR(whiteout))
> > + return whiteout;
> > ofs->whiteout = whiteout;
> > }
> >
> > if (!ofs->no_shared_whiteout) {
> > + inode_lock_nested(wdir, I_MUTEX_PARENT);
> > whiteout = ovl_lookup_temp(ofs, workdir);
> > - if (IS_ERR(whiteout))
> > - goto out;
> > -
> > - err = ovl_do_link(ofs, ofs->whiteout, wdir, whiteout);
> > - if (!err)
> > - goto out;
> > -
> > - if (err != -EMLINK) {
> > - pr_warn("Failed to link whiteout - disabling whiteout inode sharing(nlink=%u, err=%i)\n",
> > - ofs->whiteout->d_inode->i_nlink, err);
> > - ofs->no_shared_whiteout = true;
> > + if (!IS_ERR(whiteout)) {
> > + err = ovl_do_link(ofs, ofs->whiteout, wdir, whiteout);
> > + if (err) {
> > + dput(whiteout);
> > + whiteout = ERR_PTR(err);
> > + }
> > }
> > - dput(whiteout);
> > + inode_unlock(wdir);
> > + if (!IS_ERR(whiteout) || PTR_ERR(whiteout) != -EMLINK)
> > + return whiteout;
>
> + if (!IS_ERR(whiteout))
> + return whiteout;
>
> > +
> > + pr_warn("Failed to link whiteout - disabling whiteout inode sharing(nlink=%u, err=%i)\n",
> > + ofs->whiteout->d_inode->i_nlink, err);
> > + ofs->no_shared_whiteout = true;
>
> Logic was changed.
> The above pr_warn and no_shared_whiteout = true and for the case of
> PTR_ERR(whiteout) != -EMLINK
>
> > }
> > whiteout = ofs->whiteout;
> > ofs->whiteout = NULL;
>
> The outcome is the same with all errors - we return and reset
> ofs->whiteout, but with EMLINK this is expected and not a warning
> with other errors unexpected and warning and we do not try again
> to hardlink to singleton whiteout.
I see that now - thanks. I've fix up the code.
Thanks,
NeilBrown
