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.-- 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
