On Sat, Nov 11, 2023 at 10:04 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> AFAICS, the main reason for exposing those used to be the need
> to store ovl_entry in allocated dentry; we needed to do that before it
> gets attached to inode, so the guts of d_obtain_alias() had to be
> exposed.
>
> These days overlayfs is stashing ovl_entry in the inode, so
> we are left with this:
> 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);
>
> ovl_dentry_init_reval() can bloody well be skipped, AFAICS - all it does
> is potentially clearing DCACHE_OP_{,WEAK_}REVALIDATE. That's also done
> 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?
I think that makes all the analysis regarding race with d_splice_alias()
moot. Right?
Do DCACHE_OP_*REVALIDATE even matter for a disconnected
non-dir dentry?
>
> And as for the ovl_dentry_set_upper_alias()... that information used to
> live in ovl_entry until the need to trim the thing down. These days
> it's in a bit in dentry->d_fsdata.
>
> How painful would it be to switch to storing that in LSB of ovl_entry::__numlower,
> turning ovl_numlower() into
> return oe ? oe->__numlower>>1 : 0
> and ovl_lowerdata() into
> return lowerstack ? &lowerstack[(oe->__numlower>>1) - 1] : NULL
> with obvious adjustment to ovl_alloc_entry().
>
> An entry is coallocated with an array of struct ovl_path, with
> numlower elements. More than 2G layers doesn't seem to be plausible -
> there are fat 64bit boxen, but 32Gb (kmalloc'ed, at that) just in
> the root ovl_entry alone feels somewhat over the top ;-)
>
> So stealing that bit shouldn't be a problem. Is there anything I'm
> missing?
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.
Thanks,
Amir.
