Groups > linux.kernel > #1431167 > unrolled thread

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

Started by	Oleg Drokin <green@linuxhacker.ru>
First post	2016-06-25 18:40 +0200
Last post	2016-07-06 18:30 +0200
Articles	20 on this page of 32 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Page 1 of 2 [1] 2 Next page →

#1431167 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	Oleg Drokin <green@linuxhacker.ru>
Date	2016-06-25 18:40 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rNYMF-7KV-7@gated-at.bofh.it>

Hello!

On Jun 17, 2016, at 12:29 AM, Al Viro wrote:

> On Fri, Jun 17, 2016 at 12:09:19AM -0400, Oleg Drokin wrote:
> 
>>    So they both do d_drop(), the dentry is now unhashed, and they both
>>    dive into nfs_lookup().
>>    There eventually they both call
>> 
>> 	res = d_splice_alias(inode, dentry);
>> 
>>   And so the first lucky one continues on it's merry way with a hashed dentry,
>>   but the other less lucky one ends up calling into d_splice_alias() with
>>   dentry that's already hashed and hits the very familiar assertion.
>> 
>>   I took a brief look into ceph and it looks like a very similar thing
>>   might happen there with handle_reply() for two parallel replies calling into
>>   ceph_fill_trace() and then splice_alias()->d_splice_alias(), since the
>>   unhashed check it does is not under any locks, it's unsafe, so the problem
>>   might be more generic than just NFS too.
>> 
>>   So I wonder how to best fix this? Holding some sort of dentry lock across a call
>>   into atomic_open in VFS? We cannot just make d_splice_alias() callers call with
>>   inode->i_lock held because dentry might be negative.
> 
> Oh, lovely...  So basically the problem is that we violate the "no lookups on
> the same name in parallel" rule on those fallbacks from foo_atomic_open() to
> foo_lookup().  The thing is, a lot of ->atomic_open() instances have such
> fallbacks and I wonder if that's a sign that we need to lift some of that
> to fs/namei.c...
> 
> Hell knows; alternative is to have that d_drop() followed by d_alloc_parallel()
> and feeding that dentry to lookup.  I'll play with that a bit and see what's
> better; hopefully I'll have something by tomorrow.

Sorry to nag you about this, but did any of those pan out?

d_alloc_parallel() sounds like a bit too heavy there, esp. considering we came in with
a dentry already (though a potentially shared one, I understand).
Would not it be better to try and establish some dentry locking rule for calling into
d_splice_alias() instead? At least then the callers can make sure the dentry does
not change under them?
Though I guess if there's dentry locking like that, we might as well do all the
checking in d_splice_alias(), but that means the unhashed dentries would no
longer be disallowed which is a change of semantic from now.

[toc] | [next] | [standalone]

#1435919 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	Al Viro <viro@ZenIV.linux.org.uk>
Date	2016-07-03 08:40 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rQJep-5my-5@gated-at.bofh.it>
In reply to	#1431167

On Sat, Jun 25, 2016 at 12:38:40PM -0400, Oleg Drokin wrote:

> Sorry to nag you about this, but did any of those pan out?
> 
> d_alloc_parallel() sounds like a bit too heavy there, esp. considering we came in with
> a dentry already (though a potentially shared one, I understand).
> Would not it be better to try and establish some dentry locking rule for calling into
> d_splice_alias() instead? At least then the callers can make sure the dentry does
> not change under them?
> Though I guess if there's dentry locking like that, we might as well do all the
> checking in d_splice_alias(), but that means the unhashed dentries would no
> longer be disallowed which is a change of semantic from now.--

FWIW, the only interesting case here is this:
	* no O_CREAT in flags (otherwise the parent is held exclusive).
	* dentry is found in hash
	* dentry is negative
	* dentry has passed ->d_revalidate() (i.e. in case of
NFS it had nfs_neg_need_reval() return false).

Only two instances are non-trivial in that respect - NFS and Lustre.
Everything else will simply fail open() with ENOENT in that case.

And at least for NFS we could bloody well do d_drop + d_alloc_parallel +
finish_no_open and bugger off in case it's not in_lookup, otherwise do
pretty much what we do in case we'd got in_lookup from the very beginning.
Some adjustments are needed for that case (basically, we need to make
sure we hit d_lookup_done() matching that d_alloc_parallel() and deal
with refcounting correctly).

Tentative NFS patch follows; I don't understand Lustre well enough, but it
looks like a plausible strategy there as well.

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index d8015a03..5474e39 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1485,11 +1485,13 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 		    struct file *file, unsigned open_flags,
 		    umode_t mode, int *opened)
 {
+	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
 	struct nfs_open_context *ctx;
 	struct dentry *res;
 	struct iattr attr = { .ia_valid = ATTR_OPEN };
 	struct inode *inode;
 	unsigned int lookup_flags = 0;
+	bool switched = false;
 	int err;
 
 	/* Expect a negative dentry */
@@ -1528,6 +1530,17 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 		attr.ia_size = 0;
 	}
 
+	if (!(open_flags & O_CREAT) && !d_unhashed(dentry)) {
+		d_drop(dentry);
+		switched = true;
+		dentry = d_alloc_parallel(dentry->d_parent,
+					  &dentry->d_name, &wq);
+		if (IS_ERR(dentry))
+			return PTR_ERR(dentry);
+		if (unlikely(!d_in_lookup(dentry)))
+			return finish_no_open(file, dentry);
+	}
+
 	ctx = create_nfs_open_context(dentry, open_flags);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
@@ -1563,14 +1576,23 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 	trace_nfs_atomic_open_exit(dir, ctx, open_flags, err);
 	put_nfs_open_context(ctx);
 out:
+	if (unlikely(switched)) {
+		d_lookup_done(dentry);
+		dput(dentry);
+	}
 	return err;
 
 no_open:
 	res = nfs_lookup(dir, dentry, lookup_flags);
-	err = PTR_ERR(res);
+	if (switched) {
+		d_lookup_done(dentry);
+		if (!res)
+			res = dentry;
+		else
+			dput(dentry);
+	}
 	if (IS_ERR(res))
-		goto out;
-
+		return PTR_ERR(res);
 	return finish_no_open(file, res);
 }
 EXPORT_SYMBOL_GPL(nfs_atomic_open);

[toc] | [prev] | [next] | [standalone]

#1436086 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	Al Viro <viro@ZenIV.linux.org.uk>
Date	2016-07-04 02:10 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rQZCy-6Z7-11@gated-at.bofh.it>
In reply to	#1435919

On Sun, Jul 03, 2016 at 07:29:46AM +0100, Al Viro wrote:

> Tentative NFS patch follows; I don't understand Lustre well enough, but it
> looks like a plausible strategy there as well.

Speaking of Lustre: WTF is
                        /* Open dentry. */
                        if (S_ISFIFO(d_inode(dentry)->i_mode)) {
                                /* We cannot call open here as it would
                                 * deadlock.
                                 */
                                if (it_disposition(it, DISP_ENQ_OPEN_REF))
                                        ptlrpc_req_finished(
                                                       (struct ptlrpc_request *)
                                                          it->d.lustre.it_data);
                                rc = finish_no_open(file, de);
                        } else {
about and why do we only do that to FIFOs?  What about symlinks or device
nodes?  Directories, for that matter...  Shouldn't that be if (!S_ISREG(...))
instead?

[toc] | [prev] | [next] | [standalone]

#1436091

From	Oleg Drokin <green@linuxhacker.ru>
Date	2016-07-04 02:40 +0200
Message-ID	<rR05z-78B-5@gated-at.bofh.it>
In reply to	#1436086

On Jul 3, 2016, at 8:08 PM, Al Viro wrote:

> On Sun, Jul 03, 2016 at 07:29:46AM +0100, Al Viro wrote:
> 
>> Tentative NFS patch follows; I don't understand Lustre well enough, but it
>> looks like a plausible strategy there as well.
> 
> Speaking of Lustre: WTF is
>                        /* Open dentry. */
>                        if (S_ISFIFO(d_inode(dentry)->i_mode)) {
>                                /* We cannot call open here as it would
>                                 * deadlock.
>                                 */
>                                if (it_disposition(it, DISP_ENQ_OPEN_REF))
>                                        ptlrpc_req_finished(
>                                                       (struct ptlrpc_request *)
>                                                          it->d.lustre.it_data);
>                                rc = finish_no_open(file, de);
>                        } else {
> about and why do we only do that to FIFOs?  What about symlinks or device
> nodes?  Directories, for that matter...  Shouldn't that be if (!S_ISREG(...))
> instead?

Hm… This dates to sometime in 2006 and my memory is a bit hazy here.

I think when we called into the open, it went into fifo open and stuck there
waiting for the other opener. Something like that. And we cannot really be stuck here
because we are holding some locks that need to be released in predictable time.

This code is actually unreachable now because the server never returns an openhandle
for special device nodes anymore (there's a comment about it in current staging tree,
but I guess you are looking at some prior version).

I imagine device nodes might have represented a similar risk too, but it did not
occur to me to test it separately and the testsuite does not do it either.

Directories do not get stuck when you open them so they are ok and we can
atomically open them too, I guess.
Symlinks are handled specially on the server and the open never returns
the actual open handle for those, so this path is also unreachable with those.

[toc] | [prev] | [next] | [standalone]

#1436242 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	Al Viro <viro@ZenIV.linux.org.uk>
Date	2016-07-04 05:10 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rR2qJ-jU-1@gated-at.bofh.it>
In reply to	#1436091

On Sun, Jul 03, 2016 at 08:37:22PM -0400, Oleg Drokin wrote:

> Hm… This dates to sometime in 2006 and my memory is a bit hazy here.
> 
> I think when we called into the open, it went into fifo open and stuck there
> waiting for the other opener. Something like that. And we cannot really be stuck here
> because we are holding some locks that need to be released in predictable time.
> 
> This code is actually unreachable now because the server never returns an openhandle
> for special device nodes anymore (there's a comment about it in current staging tree,
> but I guess you are looking at some prior version).
> 
> I imagine device nodes might have represented a similar risk too, but it did not
> occur to me to test it separately and the testsuite does not do it either.
> 
> Directories do not get stuck when you open them so they are ok and we can
> atomically open them too, I guess.
> Symlinks are handled specially on the server and the open never returns
> the actual open handle for those, so this path is also unreachable with those.

Hmm...  How much does the safety of client depend upon the correctness of
server?

BTW, there's a fun issue in ll_revalidate_dentry(): there's nothing to
promise stability of ->d_parent in there, so uses of d_inode(dentry->d_parent)
are not safe.  That's independent from parallel lookups, and it's hard
to hit, but AFAICS it's not impossible to oops there.

Anyway, for Lustre the analogue of that NFS problem is here:
        } 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);
        }
AFAICS, this (on top of mainline) ought to work:

diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
index 5eba0eb..b8da5b4 100644
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -581,9 +581,11 @@ static int ll_atomic_open(struct inode *dir, struct dentry *dentry,
 			  struct file *file, unsigned open_flags,
 			  umode_t mode, int *opened)
 {
+	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
 	struct lookup_intent *it;
 	struct dentry *de;
 	long long lookup_flags = LOOKUP_OPEN;
+	bool switched = false;
 	int rc = 0;
 
 	CDEBUG(D_VFSTRACE, "VFS Op:name=%pd, dir="DFID"(%p),file %p,open_flags %x,mode %x opened %d\n",
@@ -603,11 +605,28 @@ static int ll_atomic_open(struct inode *dir, struct dentry *dentry,
 	it->it_flags = (open_flags & ~O_ACCMODE) | OPEN_FMODE(open_flags);
 
 	/* Dentry added to dcache tree in ll_lookup_it */
+	if (!(open_flags & O_CREAT) && !d_unhashed(dentry)) {
+		d_drop(dentry);
+		switched = true;
+	        dentry = d_alloc_parallel(dentry->d_parent,
+					  &dentry->d_name, &wq);
+		if (IS_ERR(dentry)) {
+			rc = PTR_ERR(dentry);
+			goto out_release;
+		}
+		if (unlikely(!d_in_lookup(dentry))) {
+			rc = finish_no_open(file, dentry);
+			goto out_release;
+		}
+	}
+
 	de = ll_lookup_it(dir, dentry, it, lookup_flags);
 	if (IS_ERR(de))
 		rc = PTR_ERR(de);
 	else if (de)
 		dentry = de;
+	else if (switched)
+		de = dget(dentry);
 
 	if (!rc) {
 		if (it_disposition(it, DISP_OPEN_CREATE)) {
@@ -648,6 +667,10 @@ static int ll_atomic_open(struct inode *dir, struct dentry *dentry,
 	}
 
 out_release:
+	if (unlikely(switched)) {
+		d_lookup_done(dentry);
+		dput(dentry);
+	}
 	ll_intent_release(it);
 	kfree(it);

[toc] | [prev] | [next] | [standalone]

#1436372

From	Oleg Drokin <green@linuxhacker.ru>
Date	2016-07-04 06:00 +0200
Message-ID	<rR3d7-zy-9@gated-at.bofh.it>
In reply to	#1436242

On Jul 3, 2016, at 11:08 PM, Al Viro wrote:

> On Sun, Jul 03, 2016 at 08:37:22PM -0400, Oleg Drokin wrote:
> 
>> Hm… This dates to sometime in 2006 and my memory is a bit hazy here.
>> 
>> I think when we called into the open, it went into fifo open and stuck there
>> waiting for the other opener. Something like that. And we cannot really be stuck here
>> because we are holding some locks that need to be released in predictable time.
>> 
>> This code is actually unreachable now because the server never returns an openhandle
>> for special device nodes anymore (there's a comment about it in current staging tree,
>> but I guess you are looking at some prior version).
>> 
>> I imagine device nodes might have represented a similar risk too, but it did not
>> occur to me to test it separately and the testsuite does not do it either.
>> 
>> Directories do not get stuck when you open them so they are ok and we can
>> atomically open them too, I guess.
>> Symlinks are handled specially on the server and the open never returns
>> the actual open handle for those, so this path is also unreachable with those.
> 
> Hmm...  How much does the safety of client depend upon the correctness of
> server?

Quite a bit, actually. If you connect to an rogue Lustre server,
currently there are many ways it can crash the client.
I suspect this is true not just of Lustre, if e.g. NFS server starts to
send directory inodes with duplicated inode numbers or some such,
VFS would not be super happy about such "hardlinked" directories either.
This is before we even consider that it can feed you garbage data
to crash your apps (or substitute binaries to do something else).

> BTW, there's a fun issue in ll_revalidate_dentry(): there's nothing to
> promise stability of ->d_parent in there, so uses of d_inode(dentry->d_parent)

Yes, we actually had a discussion about that in March, we were not the only ones
affected, and I think it was decided that dget_parent() was a better solution
to get to the parent (I see ext4 has already converted).
I believe you cannot hit it in Lustre now due to Lustre locking magic, but
I'll create a patch to cover this anyway. Thanks for reminding me about this.

> are not safe.  That's independent from parallel lookups, and it's hard
> to hit, but AFAICS it's not impossible to oops there.
> 
> Anyway, for Lustre the analogue of that NFS problem is here:
>        } 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);
>        }

Hm… Do you mean that when we do come hashed here, with a negative dentry
and positive disposition and hit the assertion about inode not being NULL
(still staying negative, basically)?
This one we cannot hit because negative dentries are protected by a Lustre
dlm lock held by the parent directory. Any create in that parent directory
would invalidate the lock and once that happens, all negative dentries would
be killed.
Hmm… This probably means this is a dead code?
Ah, I guess it's not.
If we do a lookup and find this negative dentry (from 2+ threads) and THEN it gets invalidated and our two threads both race to instantiate it...
It does look like something that is quite hard to hit, but still looks like a race
that could happen.

> AFAICS, this (on top of mainline) ought to work:

Thanks, I'll give this a try.
> 
> diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
> index 5eba0eb..b8da5b4 100644
> --- a/drivers/staging/lustre/lustre/llite/namei.c
> +++ b/drivers/staging/lustre/lustre/llite/namei.c
> @@ -581,9 +581,11 @@ static int ll_atomic_open(struct inode *dir, struct dentry *dentry,
> 			  struct file *file, unsigned open_flags,
> 			  umode_t mode, int *opened)
> {
> +	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
> 	struct lookup_intent *it;
> 	struct dentry *de;
> 	long long lookup_flags = LOOKUP_OPEN;
> +	bool switched = false;
> 	int rc = 0;
> 
> 	CDEBUG(D_VFSTRACE, "VFS Op:name=%pd, dir="DFID"(%p),file %p,open_flags %x,mode %x opened %d\n",
> @@ -603,11 +605,28 @@ static int ll_atomic_open(struct inode *dir, struct dentry *dentry,
> 	it->it_flags = (open_flags & ~O_ACCMODE) | OPEN_FMODE(open_flags);
> 
> 	/* Dentry added to dcache tree in ll_lookup_it */
> +	if (!(open_flags & O_CREAT) && !d_unhashed(dentry)) {
> +		d_drop(dentry);
> +		switched = true;
> +	        dentry = d_alloc_parallel(dentry->d_parent,
> +					  &dentry->d_name, &wq);
> +		if (IS_ERR(dentry)) {
> +			rc = PTR_ERR(dentry);
> +			goto out_release;
> +		}
> +		if (unlikely(!d_in_lookup(dentry))) {
> +			rc = finish_no_open(file, dentry);
> +			goto out_release;
> +		}
> +	}
> +
> 	de = ll_lookup_it(dir, dentry, it, lookup_flags);
> 	if (IS_ERR(de))
> 		rc = PTR_ERR(de);
> 	else if (de)
> 		dentry = de;
> +	else if (switched)
> +		de = dget(dentry);
> 
> 	if (!rc) {
> 		if (it_disposition(it, DISP_OPEN_CREATE)) {
> @@ -648,6 +667,10 @@ static int ll_atomic_open(struct inode *dir, struct dentry *dentry,
> 	}
> 
> out_release:
> +	if (unlikely(switched)) {
> +		d_lookup_done(dentry);
> +		dput(dentry);
> +	}
> 	ll_intent_release(it);
> 	kfree(it);
>

[toc] | [prev] | [next] | [standalone]

#1436729 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	Al Viro <viro@ZenIV.linux.org.uk>
Date	2016-07-05 04:30 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rRohA-5fe-3@gated-at.bofh.it>
In reply to	#1436372

On Sun, Jul 03, 2016 at 11:55:09PM -0400, Oleg Drokin wrote:
> Quite a bit, actually. If you connect to an rogue Lustre server,
> currently there are many ways it can crash the client.
> I suspect this is true not just of Lustre, if e.g. NFS server starts to
> send directory inodes with duplicated inode numbers or some such,
> VFS would not be super happy about such "hardlinked" directories either.
> This is before we even consider that it can feed you garbage data
> to crash your apps (or substitute binaries to do something else).

NFS client is at least supposed to try to be resistant to that.  As in,
"if an 0wn3d NFS server can be escalated to buggered client, it's a bug in
client and we are expected to try and fix it".

[snip]
> Thanks, I'll give this a try.

BTW, could you take a look at
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git#sendmsg.lustre?
It's a bunch of simplifications that became possible once sendmsg()/recvmsg()
switched to iov_iter, stopped mangling the iovecs and went for predictable
behaviour re advancing the iterator.

[toc] | [prev] | [next] | [standalone]

#1440134

From	Oleg Drokin <green@linuxhacker.ru>
Date	2016-07-10 19:10 +0200
Message-ID	<rTqoW-3ub-21@gated-at.bofh.it>
In reply to	#1436729

On Jul 4, 2016, at 10:25 PM, Al Viro wrote:

> BTW, could you take a look at
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git#sendmsg.lustre?
> It's a bunch of simplifications that became possible once sendmsg()/recvmsg()
> switched to iov_iter, stopped mangling the iovecs and went for predictable
> behaviour re advancing the iterator.

Thanks, this looks good to me and passes my testing (on tcp).

+typedef struct bio_vec lnet_kiov_t;

This I guess we'll need to just get rid of all lnet_kiov_t usage, but that's
something we can do ourselves, I guess.

Anyway, your patchset is based on old tree that no longer applies cleanly,
I rebased it to current staging tree to save you time in case
you want to go forward with it.
It's at git@github.com:verygreen/linux.git branch lustre-next-sendmsg

James, can you please give it a try on IB?

Bye,
    Oleg

[toc] | [prev] | [next] | [standalone]

#1440146 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	James Simmons <jsimmons@infradead.org>
Date	2016-07-10 20:20 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rTruG-48u-19@gated-at.bofh.it>
In reply to	#1440134

> On Jul 4, 2016, at 10:25 PM, Al Viro wrote:
> 
> > BTW, could you take a look at
> > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git#sendmsg.lustre?
> > It's a bunch of simplifications that became possible once sendmsg()/recvmsg()
> > switched to iov_iter, stopped mangling the iovecs and went for predictable
> > behaviour re advancing the iterator.
> 
> Thanks, this looks good to me and passes my testing (on tcp).
> 
> +typedef struct bio_vec lnet_kiov_t;
> 
> This I guess we'll need to just get rid of all lnet_kiov_t usage, but that's
> something we can do ourselves, I guess.
> 
> Anyway, your patchset is based on old tree that no longer applies cleanly,
> I rebased it to current staging tree to save you time in case
> you want to go forward with it.
> It's at git@github.com:verygreen/linux.git branch lustre-next-sendmsg
> 
> James, can you please give it a try on IB?

Its broke for the ko2iblnd driver.

[  110.840583] LNet: Using FMR for registration
[  110.991747] LNet: Added LNI 10.37.248.137@o2ib1 [63/2560/0/180]
[  110.998211] ------------[ cut here ]------------
[  111.003012] kernel BUG at lib/iov_iter.c:513!
[  111.007545] invalid opcode: 0000 [#1] SMP
[  111.011731] Modules linked in: ko2iblnd(C) ptlrpc(C+) obdclass(C) 
ksocklnd(C) lnet(C) sha512_generic sha256_generic md5 crc32_generic crc3
2_pclmul libcfs(C) autofs4 ipmi_devintf auth_rpcgss nfsv4 dns_resolver 
8021q iptable_filter ip_tables x_tables ib_ipoib rdma_ucm ib_ucm ib_uv
erbs ib_umad rdma_cm configfs ib_cm iw_cm mlx4_ib ib_core dm_mirror 
dm_region_hash dm_log dm_multipath sg sd_mod joydev pcspkr dm_mod mpt3sas
 raid_class acpi_cpufreq ipmi_ssif ipmi_si ipmi_msghandler isci libsas 
scsi_transport_sas wmi tpm_tis tpm i2c_i801 ahci libahci libata scsi_m
od ehci_pci ehci_hcd button tcp_cubic nfsv3(E) nfs_acl(E) ipv6(E) nfs(E) 
lockd(E) sunrpc(E) grace(E) mlx4_en(E) mlx4_core(E) igb(E) i2c_algo_
bit(E) i2c_core(E) ptp(E) pps_core(E) hwmon(E)
[  111.086669] CPU: 6 PID: 11899 Comm: router_checker Tainted: G         C  
E   4.7.0-rc6+ #1
[  111.095248] Hardware name: Supermicro X9DRT/X9DRT, BIOS 3.0a 02/19/2014
[  111.102040] task: ffff880826d32d80 ti: ffff880811d24000 task.ti: 
ffff880811d24000
[  111.109818] RIP: 0010:[<ffffffff8128daf2>]  [<ffffffff8128daf2>] 
iov_iter_kvec+0x22/0x30
[  111.118302] RSP: 0018:ffff880811d27b28  EFLAGS: 00010246
[  111.123806] RAX: 0000000000000000 RBX: ffff88105e037c00 RCX: 
0000000000000000
[  111.131111] RDX: 0000000000000000 RSI: 0000000000000005 RDI: 
ffff880811d27b78
[  111.138426] RBP: ffff880811d27b28 R08: 0000000000000000 R09: 
0000000000000000
[  111.145751] R10: 0000000000000000 R11: 00000000fffd19f7 R12: 
0000000000000000
[  111.153083] R13: 0000000000000000 R14: 000500010a25ca3b R15: 
ffff880811d27b78
[  111.160407] FS:  0000000000000000(0000) GS:ffff88107fd00000(0000) 
knlGS:0000000000000000
[  111.168797] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  111.174717] CR2: 00007fd075c48945 CR3: 000000105a9ec000 CR4: 
00000000000406e0
[  111.182031] Stack:
[  111.184224]  ffff880811d27be8 ffffffffa04012bd 0000000000000000 
ffff880811d27b40
[  111.192206]  0000000000000000 0000000000000000 0000000000000000 
0000000000000000
[  111.192206]  0000000000000000 0000000000000000 0000000000000000 
0000000000000000
[  111.200196]  0000880800000002 ffff88105e394000 0000000000000000 
0000000000000000
[  111.208179] Call Trace:
[  111.210818]  [<ffffffffa04012bd>] kiblnd_send+0x51d/0x9e0 [ko2iblnd]
[  111.217370]  [<ffffffffa06ec6bd>] lnet_ni_send+0x3d/0xe0 [lnet]
[  111.223487]  [<ffffffffa06ee223>] lnet_send+0x6b3/0xc80 [lnet]
[  111.229501]  [<ffffffffa06eeb58>] LNetGet+0x368/0x650 [lnet]
[  111.235346]  [<ffffffffa0692a50>] ? cfs_percpt_lock+0x50/0x110 [libcfs]
[  111.242139]  [<ffffffffa06f4d8f>] lnet_ping_router_locked+0x20f/0x840 
[lnet]
[  111.249384]  [<ffffffffa06f5909>] lnet_router_checker+0xd9/0x490 [lnet]
[  111.256192]  [<ffffffff8108347d>] ? default_wake_function+0xd/0x10
[  111.262549]  [<ffffffff810923f1>] ? __wake_up_common+0x51/0x80
[  111.268562]  [<ffffffffa06f5830>] ? lnet_prune_rc_data+0x470/0x470 
[lnet]
[  111.275544]  [<ffffffff81508f9b>] ? schedule+0x3b/0xa0
[  111.280871]  [<ffffffffa06f5830>] ? lnet_prune_rc_data+0x470/0x470 
[lnet]
[  111.287849]  [<ffffffff810779d7>] kthread+0xc7/0xe0
[  111.292904]  [<ffffffff8150c3cf>] ret_from_fork+0x1f/0x40
[  111.298475]  [<ffffffff81077910>] ? 
kthread_freezable_should_stop+0x70/0x70
[  111.305631] Code: 2e 0f 1f 84 00 00 00 00 00 55 40 f6 c6 02 48 89 e5 74 
18 89 37 48 89 57 18 48 89 4f 20 48 c7 47 08 00 00 00 00 4c 89 47 10 c9 c3 
<0f> 0b eb fe 66 2e 0f 1f 84 00 00 00 00 00 48 8b 47 10 55 48 89
[  111.329528] RIP  [<ffffffff8128daf2>] iov_iter_kvec+0x22/0x30
[  111.335533]  RSP <ffff880811d27b28>
[  111.339360] ---[ end trace 1ea9288f558e2c8d ]---

[toc] | [prev] | [next] | [standalone]

#1440192 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	Al Viro <viro@ZenIV.linux.org.uk>
Date	2016-07-11 03:10 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rTxTr-5I-1@gated-at.bofh.it>
In reply to	#1440146

On Mon, Jul 11, 2016 at 02:01:13AM +0100, Al Viro wrote:
> On Sun, Jul 10, 2016 at 07:14:18PM +0100, James Simmons wrote:
> 
> > [  111.210818]  [<ffffffffa04012bd>] kiblnd_send+0x51d/0x9e0 [ko2iblnd]
> 
> Mea culpa - in kiblnd_send() this
>         if (payload_kiov)
>                 iov_iter_bvec(&from, ITER_BVEC | WRITE,
>                                 payload_kiov, payload_niov, payload_nob);
>         else
>                 iov_iter_kvec(&from, ITER_BVEC | WRITE,
>                                 payload_iov, payload_niov, payload_nob);
> should have s/BVEC/KVEC/ in the iov_iter_kvec() arguments.  Cut'n'paste
> braindamage...

PS: That was introduced in the last commit in that pile - "lustre: introduce
lnet_copy_{k,}iov2iter(), kill lnet_copy_{k,}iov2{k,}iov()".

[toc] | [prev] | [next] | [standalone]

#1440958 — Re: lustre sendmsg stuff

From	Oleg Drokin <green@linuxhacker.ru>
Date	2016-07-12 01:00 +0200
Subject	Re: lustre sendmsg stuff
Message-ID	<rTSlb-4Sj-1@gated-at.bofh.it>
In reply to	#1440192

On Jul 10, 2016, at 9:03 PM, Al Viro wrote:

> On Mon, Jul 11, 2016 at 02:01:13AM +0100, Al Viro wrote:
>> On Sun, Jul 10, 2016 at 07:14:18PM +0100, James Simmons wrote:
>> 
>>> [  111.210818]  [<ffffffffa04012bd>] kiblnd_send+0x51d/0x9e0 [ko2iblnd]
>> 
>> Mea culpa - in kiblnd_send() this
>>        if (payload_kiov)
>>                iov_iter_bvec(&from, ITER_BVEC | WRITE,
>>                                payload_kiov, payload_niov, payload_nob);
>>        else
>>                iov_iter_kvec(&from, ITER_BVEC | WRITE,
>>                                payload_iov, payload_niov, payload_nob);
>> should have s/BVEC/KVEC/ in the iov_iter_kvec() arguments.  Cut'n'paste
>> braindamage...
> 
> PS: That was introduced in the last commit in that pile - "lustre: introduce
> lnet_copy_{k,}iov2iter(), kill lnet_copy_{k,}iov2{k,}iov()".

Is this something you plan to submit to Linus or should I just submit this to
Greg along with other changes?

[toc] | [prev] | [next] | [standalone]

#1440193 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	Al Viro <viro@ZenIV.linux.org.uk>
Date	2016-07-11 03:10 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rTxTr-5I-3@gated-at.bofh.it>
In reply to	#1440146

On Sun, Jul 10, 2016 at 07:14:18PM +0100, James Simmons wrote:

> [  111.210818]  [<ffffffffa04012bd>] kiblnd_send+0x51d/0x9e0 [ko2iblnd]

Mea culpa - in kiblnd_send() this
        if (payload_kiov)
                iov_iter_bvec(&from, ITER_BVEC | WRITE,
                                payload_kiov, payload_niov, payload_nob);
        else
                iov_iter_kvec(&from, ITER_BVEC | WRITE,
                                payload_iov, payload_niov, payload_nob);
should have s/BVEC/KVEC/ in the iov_iter_kvec() arguments.  Cut'n'paste
braindamage...

[toc] | [prev] | [next] | [standalone]

#1440783 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	James Simmons <jsimmons@infradead.org>
Date	2016-07-11 19:20 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rTN29-1BX-19@gated-at.bofh.it>
In reply to	#1440193

> On Sun, Jul 10, 2016 at 07:14:18PM +0100, James Simmons wrote:
> 
> > [  111.210818]  [<ffffffffa04012bd>] kiblnd_send+0x51d/0x9e0 [ko2iblnd]
> 
> Mea culpa - in kiblnd_send() this
>         if (payload_kiov)
>                 iov_iter_bvec(&from, ITER_BVEC | WRITE,
>                                 payload_kiov, payload_niov, payload_nob);
>         else
>                 iov_iter_kvec(&from, ITER_BVEC | WRITE,
>                                 payload_iov, payload_niov, payload_nob);
> should have s/BVEC/KVEC/ in the iov_iter_kvec() arguments.  Cut'n'paste
> braindamage...

That is the fix. Also I believe payload_nob should be payload_nob + 
payload_offset instead. I will send a patch that against Oleg's tree
that address these issues.

[toc] | [prev] | [next] | [standalone]

#1436728

From	Oleg Drokin <green@linuxhacker.ru>
Date	2016-07-05 04:30 +0200
Message-ID	<rRohz-5fe-1@gated-at.bofh.it>
In reply to	#1435919

On Jul 3, 2016, at 2:29 AM, Al Viro wrote:

> On Sat, Jun 25, 2016 at 12:38:40PM -0400, Oleg Drokin wrote:
> 
>> Sorry to nag you about this, but did any of those pan out?
>> 
>> d_alloc_parallel() sounds like a bit too heavy there, esp. considering we came in with
>> a dentry already (though a potentially shared one, I understand).
>> Would not it be better to try and establish some dentry locking rule for calling into
>> d_splice_alias() instead? At least then the callers can make sure the dentry does
>> not change under them?
>> Though I guess if there's dentry locking like that, we might as well do all the
>> checking in d_splice_alias(), but that means the unhashed dentries would no
>> longer be disallowed which is a change of semantic from now.--
> 
> FWIW, the only interesting case here is this:
> 	* no O_CREAT in flags (otherwise the parent is held exclusive).
> 	* dentry is found in hash
> 	* dentry is negative
> 	* dentry has passed ->d_revalidate() (i.e. in case of
> NFS it had nfs_neg_need_reval() return false).
> 
> Only two instances are non-trivial in that respect - NFS and Lustre.
> Everything else will simply fail open() with ENOENT in that case.
> 
> And at least for NFS we could bloody well do d_drop + d_alloc_parallel +
> finish_no_open and bugger off in case it's not in_lookup, otherwise do
> pretty much what we do in case we'd got in_lookup from the very beginning.
> Some adjustments are needed for that case (basically, we need to make
> sure we hit d_lookup_done() matching that d_alloc_parallel() and deal
> with refcounting correctly).
> 
> Tentative NFS patch follows; I don't understand Lustre well enough, but it
> looks like a plausible strategy there as well.

This patch seems to have brought the other crash back in (or something similar),
the one with a negative dentry being hashed when it's already hashed.
it's not as easy to hit as before, but a lot easier than the race we are hitting here.

Also in all cases it is ls that is crashing now, which seems to highlight it is
taking some of this new paths added.
I have half a dosen crashdumps if you want me to look something up there.
This is on top of 4.7.0-rc6 a99cde438de0c4c0cecc1d1af1a55a75b10bfdef
with just your patch on top.

I can reproduce it with both the complex workload, or with a simplified one (attached),
takes anywhere from 5 to 30 minutes to hit.

> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index d8015a03..5474e39 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1485,11 +1485,13 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
> 		    struct file *file, unsigned open_flags,
> 		    umode_t mode, int *opened)
> {
> +	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
> 	struct nfs_open_context *ctx;
> 	struct dentry *res;
> 	struct iattr attr = { .ia_valid = ATTR_OPEN };
> 	struct inode *inode;
> 	unsigned int lookup_flags = 0;
> +	bool switched = false;
> 	int err;
> 
> 	/* Expect a negative dentry */
> @@ -1528,6 +1530,17 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
> 		attr.ia_size = 0;
> 	}
> 
> +	if (!(open_flags & O_CREAT) && !d_unhashed(dentry)) {
> +		d_drop(dentry);
> +		switched = true;
> +		dentry = d_alloc_parallel(dentry->d_parent,
> +					  &dentry->d_name, &wq);

Hm, d_alloc_parallel can return some preexisting dentry it was able to lookup in
rcu attached to the same parent with the same name it seems?
What's to stop a parallel thread to rehash the dentry after we dropped it here,
and so d_alloc_parallel will happily find it?
I am running a test to verify this theory now.

> +		if (IS_ERR(dentry))
> +			return PTR_ERR(dentry);
> +		if (unlikely(!d_in_lookup(dentry)))
> +			return finish_no_open(file, dentry);
> +	}
> +
> 	ctx = create_nfs_open_context(dentry, open_flags);
> 	err = PTR_ERR(ctx);
> 	if (IS_ERR(ctx))
> @@ -1563,14 +1576,23 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
> 	trace_nfs_atomic_open_exit(dir, ctx, open_flags, err);
> 	put_nfs_open_context(ctx);
> out:
> +	if (unlikely(switched)) {
> +		d_lookup_done(dentry);
> +		dput(dentry);
> +	}
> 	return err;
> 
> no_open:
> 	res = nfs_lookup(dir, dentry, lookup_flags);
> -	err = PTR_ERR(res);
> +	if (switched) {
> +		d_lookup_done(dentry);
> +		if (!res)
> +			res = dentry;
> +		else
> +			dput(dentry);
> +	}
> 	if (IS_ERR(res))
> -		goto out;
> -
> +		return PTR_ERR(res);
> 	return finish_no_open(file, res);
> }
> EXPORT_SYMBOL_GPL(nfs_atomic_open);

[toc] | [prev] | [next] | [standalone]

#1436730

From	Oleg Drokin <green@linuxhacker.ru>
Date	2016-07-05 04:40 +0200
Message-ID	<rRorf-5ik-1@gated-at.bofh.it>
In reply to	#1436728

[Multipart message — attachments visible in raw view] — view raw

ah,
and of course the testcase that I forgot to attach ;)

[toc] | [prev] | [next] | [standalone]

#1436753

From	Oleg Drokin <green@linuxhacker.ru>
Date	2016-07-05 06:50 +0200
Message-ID	<rRqt4-6xp-3@gated-at.bofh.it>
In reply to	#1436728

On Jul 4, 2016, at 10:28 PM, Oleg Drokin wrote:

> 
> On Jul 3, 2016, at 2:29 AM, Al Viro wrote:
> 
>> On Sat, Jun 25, 2016 at 12:38:40PM -0400, Oleg Drokin wrote:
>> 
>>> Sorry to nag you about this, but did any of those pan out?
>>> 
>>> d_alloc_parallel() sounds like a bit too heavy there, esp. considering we came in with
>>> a dentry already (though a potentially shared one, I understand).
>>> Would not it be better to try and establish some dentry locking rule for calling into
>>> d_splice_alias() instead? At least then the callers can make sure the dentry does
>>> not change under them?
>>> Though I guess if there's dentry locking like that, we might as well do all the
>>> checking in d_splice_alias(), but that means the unhashed dentries would no
>>> longer be disallowed which is a change of semantic from now.--
>> 
>> FWIW, the only interesting case here is this:
>> 	* no O_CREAT in flags (otherwise the parent is held exclusive).
>> 	* dentry is found in hash
>> 	* dentry is negative
>> 	* dentry has passed ->d_revalidate() (i.e. in case of
>> NFS it had nfs_neg_need_reval() return false).
>> 
>> Only two instances are non-trivial in that respect - NFS and Lustre.
>> Everything else will simply fail open() with ENOENT in that case.
>> 
>> And at least for NFS we could bloody well do d_drop + d_alloc_parallel +
>> finish_no_open and bugger off in case it's not in_lookup, otherwise do
>> pretty much what we do in case we'd got in_lookup from the very beginning.
>> Some adjustments are needed for that case (basically, we need to make
>> sure we hit d_lookup_done() matching that d_alloc_parallel() and deal
>> with refcounting correctly).
>> 
>> Tentative NFS patch follows; I don't understand Lustre well enough, but it
>> looks like a plausible strategy there as well.
> 
> This patch seems to have brought the other crash back in (or something similar),
> the one with a negative dentry being hashed when it's already hashed.
> it's not as easy to hit as before, but a lot easier than the race we are hitting here.
> 
> Also in all cases it is ls that is crashing now, which seems to highlight it is
> taking some of this new paths added.
> I have half a dosen crashdumps if you want me to look something up there.
> This is on top of 4.7.0-rc6 a99cde438de0c4c0cecc1d1af1a55a75b10bfdef
> with just your patch on top.
> 
> I can reproduce it with both the complex workload, or with a simplified one (attached),
> takes anywhere from 5 to 30 minutes to hit.
> 
>> 
>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>> index d8015a03..5474e39 100644
>> --- a/fs/nfs/dir.c
>> +++ b/fs/nfs/dir.c
>> @@ -1485,11 +1485,13 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>> 		    struct file *file, unsigned open_flags,
>> 		    umode_t mode, int *opened)
>> {
>> +	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
>> 	struct nfs_open_context *ctx;
>> 	struct dentry *res;
>> 	struct iattr attr = { .ia_valid = ATTR_OPEN };
>> 	struct inode *inode;
>> 	unsigned int lookup_flags = 0;
>> +	bool switched = false;
>> 	int err;
>> 
>> 	/* Expect a negative dentry */
>> @@ -1528,6 +1530,17 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>> 		attr.ia_size = 0;
>> 	}
>> 
>> +	if (!(open_flags & O_CREAT) && !d_unhashed(dentry)) {
>> +		d_drop(dentry);
>> +		switched = true;
>> +		dentry = d_alloc_parallel(dentry->d_parent,
>> +					  &dentry->d_name, &wq);
> 
> Hm, d_alloc_parallel can return some preexisting dentry it was able to lookup in
> rcu attached to the same parent with the same name it seems?
> What's to stop a parallel thread to rehash the dentry after we dropped it here,
> and so d_alloc_parallel will happily find it?
> I am running a test to verify this theory now.

Ok, so at least in my case we do not come out of d_alloc_parallel() with a hashed
dentry, though I wonder why are not you trying to catch this case here?
If you rely on the !d_in_lookup(dentry) below, that seems to be racy.
if it was __d_lookup_rcu() that found the dentry, we do not seem to be performing
any additional checks on it and just return it.
But what if it was just added by a parallel caller here that just finished
d_splice_alias (= hashed dentry so it's not rejected by the __d_lookup_rcu) and
at the same time have not progressed all the way to d_lookup_done() call below?

Interesting that in some of the dumps d_hash is all zeroes, which means
it is unhashed, yet the assertion was triggered, so there was some parallel
caller that performed a d_drop on us (but nothing of interest on other CPUs).

In all cases d_flags = 140, so we do not have the DCACHE_PAR_LOOKUP set at the
point of crash.

Also in part of the dumps the dentry is actually positive, not negative.


> 
>> +		if (IS_ERR(dentry))
>> +			return PTR_ERR(dentry);
>> +		if (unlikely(!d_in_lookup(dentry)))
>> +			return finish_no_open(file, dentry);
>> +	}
>> +
>> 	ctx = create_nfs_open_context(dentry, open_flags);
>> 	err = PTR_ERR(ctx);
>> 	if (IS_ERR(ctx))
>> @@ -1563,14 +1576,23 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>> 	trace_nfs_atomic_open_exit(dir, ctx, open_flags, err);
>> 	put_nfs_open_context(ctx);
>> out:
>> +	if (unlikely(switched)) {
>> +		d_lookup_done(dentry);
>> +		dput(dentry);
>> +	}
>> 	return err;
>> 
>> no_open:
>> 	res = nfs_lookup(dir, dentry, lookup_flags);
>> -	err = PTR_ERR(res);
>> +	if (switched) {
>> +		d_lookup_done(dentry);
>> +		if (!res)
>> +			res = dentry;
>> +		else
>> +			dput(dentry);
>> +	}
>> 	if (IS_ERR(res))
>> -		goto out;
>> -
>> +		return PTR_ERR(res);
>> 	return finish_no_open(file, res);
>> }
>> EXPORT_SYMBOL_GPL(nfs_atomic_open);
>

[toc] | [prev] | [next] | [standalone]

#1436792

From	Oleg Drokin <green@linuxhacker.ru>
Date	2016-07-05 08:30 +0200
Message-ID	<rRs1P-7yT-11@gated-at.bofh.it>
In reply to	#1435919

On Jul 3, 2016, at 2:29 AM, Al Viro wrote:

> On Sat, Jun 25, 2016 at 12:38:40PM -0400, Oleg Drokin wrote:
> 
>> Sorry to nag you about this, but did any of those pan out?
>> 
>> d_alloc_parallel() sounds like a bit too heavy there, esp. considering we came in with
>> a dentry already (though a potentially shared one, I understand).
>> Would not it be better to try and establish some dentry locking rule for calling into
>> d_splice_alias() instead? At least then the callers can make sure the dentry does
>> not change under them?
>> Though I guess if there's dentry locking like that, we might as well do all the
>> checking in d_splice_alias(), but that means the unhashed dentries would no
>> longer be disallowed which is a change of semantic from now.--
> 
> FWIW, the only interesting case here is this:
> 	* no O_CREAT in flags (otherwise the parent is held exclusive).
> 	* dentry is found in hash
> 	* dentry is negative
> 	* dentry has passed ->d_revalidate() (i.e. in case of
> NFS it had nfs_neg_need_reval() return false).
> 
> Only two instances are non-trivial in that respect - NFS and Lustre.
> Everything else will simply fail open() with ENOENT in that case.
> 
> And at least for NFS we could bloody well do d_drop + d_alloc_parallel +
> finish_no_open and bugger off in case it's not in_lookup, otherwise do
> pretty much what we do in case we'd got in_lookup from the very beginning.
> Some adjustments are needed for that case (basically, we need to make
> sure we hit d_lookup_done() matching that d_alloc_parallel() and deal
> with refcounting correctly).
> 
> Tentative NFS patch follows; I don't understand Lustre well enough, but it
> looks like a plausible strategy there as well.

I think I know why this does not work or at least part of the reason.

> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index d8015a03..5474e39 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1485,11 +1485,13 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
> 		    struct file *file, unsigned open_flags,
> 		    umode_t mode, int *opened)
> {
> +	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
> 	struct nfs_open_context *ctx;
> 	struct dentry *res;
> 	struct iattr attr = { .ia_valid = ATTR_OPEN };
> 	struct inode *inode;
> 	unsigned int lookup_flags = 0;
> +	bool switched = false;
> 	int err;
> 
> 	/* Expect a negative dentry */
> @@ -1528,6 +1530,17 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
> 		attr.ia_size = 0;
> 	}
> 
> +	if (!(open_flags & O_CREAT) && !d_unhashed(dentry)) {

So we come racing here from multiple threads (say 3 or more - we have seen this
in the older crash reports, so totally possible)

> +		d_drop(dentry);

One lucky one does this first before the others perform the !d_unhashed check above.
This makes the other ones to not enter here.

And we are back to the original problem of multiple threads trying to instantiate
same dentry as before.

Only this time the race is somehow even wider because of some other interactions
I don't really understand, at least removing this patch I feel like I am having
harder time hitting the original issue.


> +		switched = true;
> +		dentry = d_alloc_parallel(dentry->d_parent,
> +					  &dentry->d_name, &wq);
> +		if (IS_ERR(dentry))
> +			return PTR_ERR(dentry);
> +		if (unlikely(!d_in_lookup(dentry)))
> +			return finish_no_open(file, dentry);
> +	}
> +
> 	ctx = create_nfs_open_context(dentry, open_flags);
> 	err = PTR_ERR(ctx);
> 	if (IS_ERR(ctx))
> @@ -1563,14 +1576,23 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
> 	trace_nfs_atomic_open_exit(dir, ctx, open_flags, err);
> 	put_nfs_open_context(ctx);
> out:
> +	if (unlikely(switched)) {
> +		d_lookup_done(dentry);
> +		dput(dentry);
> +	}
> 	return err;
> 
> no_open:
> 	res = nfs_lookup(dir, dentry, lookup_flags);
> -	err = PTR_ERR(res);
> +	if (switched) {
> +		d_lookup_done(dentry);
> +		if (!res)
> +			res = dentry;
> +		else
> +			dput(dentry);
> +	}
> 	if (IS_ERR(res))
> -		goto out;
> -
> +		return PTR_ERR(res);
> 	return finish_no_open(file, res);
> }
> EXPORT_SYMBOL_GPL(nfs_atomic_open);

[toc] | [prev] | [next] | [standalone]

#1436965 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	Al Viro <viro@ZenIV.linux.org.uk>
Date	2016-07-05 14:40 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rRxNU-2Nz-31@gated-at.bofh.it>
In reply to	#1436792

On Tue, Jul 05, 2016 at 02:22:48AM -0400, Oleg Drokin wrote:

> > +	if (!(open_flags & O_CREAT) && !d_unhashed(dentry)) {

s/d_unhashed/d_in_lookup/ in that.

> So we come racing here from multiple threads (say 3 or more - we have seen this
> in the older crash reports, so totally possible)
> 
> > +		d_drop(dentry);
> 
> One lucky one does this first before the others perform the !d_unhashed check above.
> This makes the other ones to not enter here.
> 
> And we are back to the original problem of multiple threads trying to instantiate
> same dentry as before.

Yep.  See above - it should've been using d_in_lookup() in the first place,
through the entire nfs_atomic_open().  Same in the Lustre part of fixes,
obviously.

[toc] | [prev] | [next] | [standalone]

#1437037 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

From	Al Viro <viro@ZenIV.linux.org.uk>
Date	2016-07-05 16:00 +0200
Subject	Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Message-ID	<rRz3k-3vi-19@gated-at.bofh.it>
In reply to	#1436965

On Tue, Jul 05, 2016 at 01:31:10PM +0100, Al Viro wrote:
> On Tue, Jul 05, 2016 at 02:22:48AM -0400, Oleg Drokin wrote:
> 
> > > +	if (!(open_flags & O_CREAT) && !d_unhashed(dentry)) {
> 
> s/d_unhashed/d_in_lookup/ in that.
> 
> > So we come racing here from multiple threads (say 3 or more - we have seen this
> > in the older crash reports, so totally possible)
> > 
> > > +		d_drop(dentry);
> > 
> > One lucky one does this first before the others perform the !d_unhashed check above.
> > This makes the other ones to not enter here.
> > 
> > And we are back to the original problem of multiple threads trying to instantiate
> > same dentry as before.
> 
> Yep.  See above - it should've been using d_in_lookup() in the first place,
> through the entire nfs_atomic_open().  Same in the Lustre part of fixes,
> obviously.

See current #for-linus for hopefully fixed variants (both lustre and nfs)

[toc] | [prev] | [next] | [standalone]

#1437119

From	Oleg Drokin <green@linuxhacker.ru>
Date	2016-07-05 17:30 +0200
Message-ID	<rRAsp-4Ci-15@gated-at.bofh.it>
In reply to	#1437037

On Jul 5, 2016, at 9:51 AM, Al Viro wrote:

> On Tue, Jul 05, 2016 at 01:31:10PM +0100, Al Viro wrote:
>> On Tue, Jul 05, 2016 at 02:22:48AM -0400, Oleg Drokin wrote:
>> 
>>>> +	if (!(open_flags & O_CREAT) && !d_unhashed(dentry)) {
>> 
>> s/d_unhashed/d_in_lookup/ in that.
>> 
>>> So we come racing here from multiple threads (say 3 or more - we have seen this
>>> in the older crash reports, so totally possible)
>>> 
>>>> +		d_drop(dentry);
>>> 
>>> One lucky one does this first before the others perform the !d_unhashed check above.
>>> This makes the other ones to not enter here.
>>> 
>>> And we are back to the original problem of multiple threads trying to instantiate
>>> same dentry as before.
>> 
>> Yep.  See above - it should've been using d_in_lookup() in the first place,
>> through the entire nfs_atomic_open().  Same in the Lustre part of fixes,
>> obviously.
> 
> See current #for-linus for hopefully fixed variants (both lustre and nfs)

The first patch of the series:
> @@ -416,9 +416,9 @@ static int ll_lookup_it_finish(struct ptlrpc_request *request,
> ...
> -       if (d_unhashed(*de)) {
> +       if (d_in_lookup(*de)) {
>                 struct dentry *alias;
>  
>                 alias = ll_splice_alias(inode, *de);

This breaks Lustre because we now might progress further in this function
without calling into ll_splice_alias and that's the only place that we do
ll_d_init() that later code depends on so we violently crash next time
we call e.g. d_lustre_revalidate() further down that code.

Also I still wonder what's to stop d_alloc_parallel() from returning
a hashed dentry with d_in_lookup() still true?
Certainly there's a big gap between hashing the dentry and dropping the PAR
bit in there that I imagine might allow __d_lookup_rcu() to pick it up
in between?

[toc] | [prev] | [next] | [standalone]

Page 1 of 2 [1] 2 Next page →

csiph-web

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

Contents

#1431167 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1435919 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1436086 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1436091

#1436242 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1436372

#1436729 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1440134

#1440146 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1440192 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1440958 — Re: lustre sendmsg stuff

#1440193 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1440783 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1436728

#1436730

#1436753

#1436792

#1436965 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1437037 — Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.

#1437119