On Mar 8, 2016, at 11:05 AM, Al Viro wrote:
> Suppose the last dentry refering to an inode is being evicted.
> It went through __dentry_kill(), into dentry_iput() and called ->d_iput().
> At that stage it is unhashed, out of the list of children, out of the list
> of aliases and does not refer to inode anymore. No locks are held and
> it's about to call iput().
ok.
> In the meanwhile, two lookups have found links to that inode. The
The links are hardlinks, right? (because otherwise they would not be
parallel due to parent dir i_mutex held).
> same in-core inode has already been picked by ll_iget() by both. Their
> dentries are unhashed, so ll_lookup_it_finish() hits
> alias = ll_splice_alias(inode, *de);
> in both. inode is non-NULL, so
> new = ll_find_alias(inode, de);
> is done.
> static struct dentry *ll_find_alias(struct inode *inode, struct dentry *dentry)
> {
> struct dentry *alias, *discon_alias, *invalid_alias;
>
> if (hlist_empty(&inode->i_dentry))
> return NULL;
>
> discon_alias = invalid_alias = NULL;
>
> ll_lock_dcache(inode);
>
> ends up returning NULL for both (ll_lock_dcache == grab ->i_lock). So we
> get NULL from ll_find_alias(), proceed to allocate ->d_fsdata for both
> dentries, then do d_add(de, inode). See the problem?
>
> We do not hold ->i_lock between ll_find_alias() and d_add(), so there's
> nothing to guarantee that negative result will stay valid. Even had we kept
> it (and with block allocation it would require some work), decision that
> there's no aliases at all is made _outside_ of ->i_lock, so we have a race
> there as well.
Hm, The race is a "safe" one, since if the alias have appeared, it does not break
anything other than using up some RAM, I think?
In fact what's to stop one appearing after we released the locking leading to the
same situation?
> Moreover, ll_find_alias() itself is rather fishy - it accepts any alias that
> happens to have DCACHE_DISCONNECTED and anything unhashed with exact match
> to parent/name/hash. The former part is trouble - DCACHE_DISCONNECTED is
> cleared *after* a detached subtree had been glued in. It's _not_ equivalent
> to "it's a root of detached subtree".
yes, this indeed seems troublesome.
> IOW, that thing can return dentries that do have parent other than the one
> we'd been looking for. Which means that d_move() done by ll_splice_alias()
> after having found one is short on locking.
>
> Another oddity: you have ll_d_init() called on the result of ll_find_alias(),
> but that alias must have either been through ll_splice_alias() before or
> through d_instantiate() in ll_lookup_it_finish(), or it wouldn't have been
> positive. And the second variant required that it had been hashed at the
> time, which means that it had been through ll_splice_alias() at some earlier
> point. Once ->d_fsdata had been allocated and set, it's never cleared until
> we free that dentry, so how the hell is it possible for that call of
> ll_d_init() on result of ll_find_alias() to actually do anything at all?
I traced a bit through the history and apparently it used to be a call
that set dentry->d_op before, which is no longer necessary after 2.6.38
> Am I right assuming that it's basically trying to do d_splice_alias() with
> a couple of twists - we are likely to find an exact unhashed alias (due to
> promiscuous ->d_revalidate()?) we want to try and reuse even for
> non-directories and it's likely enough to make unconditional allocation of
> ->d_fsdata a bad idea since dentry is likely to be replaced by alias?
Yes, pretty much.
Lustre has "locks" that guard metadata and when locks are revoked (due to
conflicting accesses) the dentries are "invalidated", but since users
will likely come for them again we do not want to use new dentry every time
and prefer to find old ones if possible.
> If so, how about adding d_hash_exact_alias(dentry) that would try to find
> and hash an exact unhashed alias, so that this thing would
> * call d_hash_exact_alias(dentry), if found - just return it
> * ll_d_init(dentry)
> * return d_splice_alias() ?: dentry
> Do you see any problems with that? Parent directory is locked, so no new
> unhashed exact aliases should have a chance to appear and d_splice_alias()
> would take care of everything else (correctness and detached ones).
This sounds fine on the surface. I think I remember there were some other
complications with d_splice_alias.
Andreas, do ou remember?
I'll try to research it a bit more and follow up soon.
Thanks for looking into this.
Bye,
Oleg
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
