From: Oleg Drokin <green@linuxhacker.ru>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Mailing List <linux-kernel@vger.kernel.org>,
"<linux-fsdevel@vger.kernel.org>" <linux-fsdevel@vger.kernel.org>
Subject: Re: More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes.
Date: Sat, 25 Jun 2016 12:38:40 -0400 [thread overview]
Message-ID: <FCD75A55-A1BE-48FD-8E90-E9DFFD4DFD99@linuxhacker.ru> (raw)
In-Reply-To: <20160617042914.GD14480@ZenIV.linux.org.uk>
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.
next prev parent reply other threads:[~2016-06-25 16:38 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-17 4:09 More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes Oleg Drokin
2016-06-17 4:29 ` Al Viro
2016-06-25 16:38 ` Oleg Drokin [this message]
2016-07-03 6:29 ` Al Viro
2016-07-04 0:08 ` Al Viro
2016-07-04 0:37 ` Oleg Drokin
2016-07-04 3:08 ` Al Viro
2016-07-04 3:55 ` Oleg Drokin
2016-07-05 2:25 ` Al Viro
2016-07-10 17:01 ` Oleg Drokin
2016-07-10 18:14 ` James Simmons
2016-07-11 1:01 ` Al Viro
2016-07-11 1:03 ` Al Viro
2016-07-11 22:54 ` lustre sendmsg stuff Oleg Drokin
2016-07-11 17:15 ` More parallel atomic_open/d_splice_alias fun with NFS and possibly more FSes James Simmons
2016-07-05 2:28 ` Oleg Drokin
2016-07-05 2:32 ` Oleg Drokin
2016-07-05 4:43 ` Oleg Drokin
2016-07-05 6:22 ` Oleg Drokin
2016-07-05 12:31 ` Al Viro
2016-07-05 13:51 ` Al Viro
2016-07-05 15:21 ` Oleg Drokin
2016-07-05 17:42 ` Al Viro
2016-07-05 18:12 ` Oleg Drokin
2016-07-05 16:33 ` Oleg Drokin
2016-07-05 18:08 ` Al Viro
2016-07-05 19:12 ` Oleg Drokin
2016-07-05 20:08 ` Al Viro
2016-07-05 20:21 ` Oleg Drokin
2016-07-06 0:29 ` Oleg Drokin
2016-07-06 3:20 ` Al Viro
2016-07-06 3:25 ` Oleg Drokin
2016-07-06 4:35 ` Oleg Drokin
2016-07-06 16:24 ` Oleg Drokin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=FCD75A55-A1BE-48FD-8E90-E9DFFD4DFD99@linuxhacker.ru \
--to=green@linuxhacker.ru \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=viro@ZenIV.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).