Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux