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

Amir Goldstein
2023-11-11T20:05:29Z

On Sat, Nov 11, 2023 at 8:50 PM Al Viro <viro@zeniv.linux.org.uk> 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.

Thanks,
Amir.