linux-kernel - Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160705200826.GP14480@ZenIV.linux.org.uk>
Date:	Tue, 5 Jul 2016 21:08:26 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Oleg Drokin <green@...uxhacker.ru>
Cc:	Mailing List <linux-kernel@...r.kernel.org>,
	"<linux-fsdevel@...r.kernel.org>" <linux-fsdevel@...r.kernel.org>
Subject: Re: More parallel atomic_open/d_splice_alias fun with NFS and
 possibly more FSes.

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)?