On Tue, Jul 05, 2016 at 03:12:44PM -0400, Oleg Drokin wrote:
>
> When d_in_lookup was introduced, it was described as:
> New primitives: d_in_lookup() (a predicate checking if dentry is in
> the in-lookup state) and d_lookup_done() (tells the system that
> we are done with lookup and if it's still marked as in-lookup, it
> should cease to be such).
>
> I don't see where it mentions anything about exclusive vs parallel lookup
> that probably led to some confusion.
In the same commit:
#define DCACHE_PAR_LOOKUP 0x10000000 /* being looked up (with parent locked shared) */
static inline int d_in_lookup(struct dentry *dentry)
{
return dentry->d_flags & DCACHE_PAR_LOOKUP;
}
Sure, we could use d_alloc_parallel() for all lookups, but it wouldn't buy
us anything in terms of exclusion (parent locked exclusive => no other
lookup attempts on that parent/name pair anyway) and it would cost extra
searches both in the primary and in-lookup hashes, as well as insertions and
removals from the latter.
Hell knows - perhaps teaching d_alloc_parallel() that NULL wq => just
allocate and mark in-lookup, without touching either primary or in-lookup
hash (and scream bloody murder if the parent isn't locked exclusive) would
be a good idea. A few places in fs/dcache.c would need to check for
->d_wait being non-NULL (__d_lookup_done(), __d_add() an __d_move());
We could use that for lookups when parent is locked exclusive; then
d_in_lookup() would be true for *all* dentries passed to ->lookup().
I'll look into that, but it's obviously the next cycle fodder.
> So with Lustre the dentry can be in three states, really:
>
> 1. hashed dentry that's all A-ok to reuse.
> 2. hashed dentry that's NOT valid (dlm lock went away) - this is distinguished in d_compare by looking at a bit in the fs_data
> 3. unhashed dentry ( I guess could be both valid and invalid lustre-wise).
>
> So the logic in ll_lookup_it_finish() (part of regular lookup) is this:
>
> If the dentry we have is not hashed - this is a new lookup, so we need to
> call into ll_splice_alias() to see if there's a better dentry we need to
> reuse that was already rejected by VFS before since we did not have necessary locks,
> but we do have them now.
> The comment at the top of ll_dcompare() explains why we don't just unhash the
> dentry on lock-loss - that apparently leads to a loop around real_lookup for
> real-contended dentries.
> This is also why we cannot use d_splice_alias here - such cases are possible
> for regular files and directories.
>
> Anyway, I guess additional point of confusion here is then why does
> ll_lookup_it_finish() need to check for hashedness of the dentry since it's in
> lookup, so we should be unhashed here.
> I checked the commit history and this check was added along with atomic_open
> support, so I imagine we can just move it up into ll_atomic_open and then your
> change starts to make sense along with a few other things.
So basically this
} else if (!it_disposition(it, DISP_LOOKUP_NEG) &&
!it_disposition(it, DISP_OPEN_CREATE)) {
/* With DISP_OPEN_CREATE dentry will be
* instantiated in ll_create_it.
*/
LASSERT(!d_inode(*de));
d_instantiate(*de, inode);
}
is something we should do only for negative hashed fed to it by
->atomic_open(), and that - only if we have no O_CREAT in flags?
Then, since 3/3 eliminates that case completely, we could just rip that
else-if, along with the check for d_unhashed itself, making the call of
ll_splice_alias() unconditional there. Or am I misreading what you said?
Do you see any problems with what's in #for-linus now (head at 11f0083)?
--
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
