On Sat, Nov 11, 2023 at 10:05 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Sat, Nov 11, 2023 at 8:50 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Sat, Nov 11, 2023 at 08:31:11PM +0200, Amir Goldstein wrote:
> > > > in ovl_lookup(), and in case we have d_splice_alias() return a non-NULL
> > > > dentry we can simply copy it there. Sure, somebody might race with
> > > > us, pick dentry from hash and call ->d_revalidate() before we notice that
> > > > DCACHE_OP_REVALIDATE could be cleaned. So what? That call of ->d_revalidate()
> > > > will find nothing to do and return 1. Which is the effect of having
> > > > DCACHE_OP_REVALIDATE cleared, except for pointless method call. Anyone
> > > > who finds that dentry after the flag is cleared will skip the call.
> > > > IOW, that race is harmless.
> > >
> > > Just a minute.
> > > Do you know that ovl_obtain_alias() is *only* used to obtain a disconnected
> > > non-dir overlayfs dentry?
> >
> > D'oh...
> >
> > > I think that makes all the analysis regarding race with d_splice_alias()
> > > moot. Right?
> >
> > Right you are.
> >
> > > Do DCACHE_OP_*REVALIDATE even matter for a disconnected
> > > non-dir dentry?
> >
> > As long as nothing picks it via d_find_any_alias() and moves it somewhere
> > manually. The former might happen, the latter, AFAICS, doesn't - nothing
> > like d_move() anywhere in sight...
> >
> > > You are missing that the OVL_E_UPPER_ALIAS flag is a property of
> > > the overlay dentry, not a property of the inode.
> > >
> > > N lower hardlinks, the first copy up created an upper inode
> > > all the rest of the N upper aliases to that upper inode are
> > > created lazily.
> > >
> > > However, for obvious reasons, OVL_E_UPPER_ALIAS is not
> > > well defined for a disconnected overlay dentry.
> > > There should not be any code (I hope) that cares about
> > > OVL_E_UPPER_ALIAS for a disconnected overlay dentry,
> > > so I *think* ovl_dentry_set_upper_alias() in this code is moot.
> > >
> > > I need to look closer to verify, but please confirm my assumption
> > > regarding the irrelevance of DCACHE_OP_*REVALIDATE for a
> > > disconnected non-dir dentry.
> >
> > Correct; we only care if it gets reconnected to the main tree.
> > The fact that it's only for non-directories simplifies life a lot
> > there. Sorry, got confused by the work you do with ->d_flags
> > and hadn't stopped to ask whether it's needed in the first place
> > in there.
> >
> > OK, so... are there any reasons why simply calling d_obtain_alias()
> > wouldn't do the right thing these days?
>
> None that I can think of.
>
> I will try it out and run the tests to see if I have missed something.
>
Tested the patch below.
If you want to apply it as part of dcache cleanup, it's fine by me.
Otherwise, I will queue it for the next overlayfs update.
Thanks,
Amir.
>From 3415a62597161b03b2db48ca195af34d25afc5d5 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Sun, 12 Nov 2023 08:55:57 +0200
Subject: [PATCH] ovl: stop using d_alloc_anon()/d_instantiate_anon()
Commit f9c34674bc60 ("vfs: factor out helpers d_instantiate_anon() and
d_alloc_anon()") was introduced so overlayfs could initialize a non-dir
disconnected overlay dentry before overlay inode is attached to it.
Since commit ("0af950f57fef ovl: move ovl_entry into ovl_inode"), all
ovl_obtain_alias() can do is set DCACHE_OP_*REVALIDATE flags in ->d_flags
and OVL_E_UPPER_ALIAS flag in ->d_fsdata.
The DCACHE_OP_*REVALIDATE flags and OVL_E_UPPER_ALIAS flag are irrelevant
for a disconnected non-dir dentry, so it is better to use d_obtain_alias()
instead of open coding it.
Since it has no more users, remove d_instantiate_anon() and do not export
d_alloc_anon().
Suggested-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
fs/dcache.c | 7 -------
fs/overlayfs/export.c | 22 +---------------------
include/linux/dcache.h | 1 -
3 files changed, 1 insertion(+), 29 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index c82ae731df9a..8afa9d2b636f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1866,7 +1866,6 @@ struct dentry *d_alloc_anon(struct super_block *sb)
{
return __d_alloc(sb, NULL);
}
-EXPORT_SYMBOL(d_alloc_anon);
struct dentry *d_alloc_cursor(struct dentry * parent)
{
@@ -2115,12 +2114,6 @@ static struct dentry
*__d_instantiate_anon(struct dentry *dentry,
return res;
}
-struct dentry *d_instantiate_anon(struct dentry *dentry, struct inode *inode)
-{
- return __d_instantiate_anon(dentry, inode, true);
-}
-EXPORT_SYMBOL(d_instantiate_anon);
-
static struct dentry *__d_obtain_alias(struct inode *inode, bool disconnected)
{
struct dentry *tmp;
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 7e16bbcad95e..f2f41d4fb5d4 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -320,27 +320,7 @@ static struct dentry *ovl_obtain_alias(struct
super_block *sb,
if (upper)
ovl_set_flag(OVL_UPPERDATA, inode);
- dentry = d_find_any_alias(inode);
- if (dentry)
- goto out_iput;
-
- dentry = d_alloc_anon(inode->i_sb);
- if (unlikely(!dentry))
- goto nomem;
-
- if (upper_alias)
- ovl_dentry_set_upper_alias(dentry);
-
- ovl_dentry_init_reval(dentry, upper, OVL_I_E(inode));
-
- return d_instantiate_anon(dentry, inode);
-
-nomem:
- dput(dentry);
- dentry = ERR_PTR(-ENOMEM);
-out_iput:
- iput(inode);
- return dentry;
+ return d_obtain_alias(inode);
}
/* Get the upper or lower dentry in stack whose on layer @idx */
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 3da2f0545d5d..c760553fb88a 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -221,7 +221,6 @@ extern seqlock_t rename_lock;
extern void d_instantiate(struct dentry *, struct inode *);
extern void d_instantiate_new(struct dentry *, struct inode *);
extern struct dentry * d_instantiate_unique(struct dentry *, struct inode *);
-extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *);
extern void __d_drop(struct dentry *dentry);
extern void d_drop(struct dentry *dentry);
extern void d_delete(struct dentry *);
--
2.34.1
