| Message ID | 20160603042648.GN14480@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 03, 2016 at 05:26:48AM +0100, Al Viro wrote: > Looks like the right thing to do would be to do d_drop() at no_open:, > just before we call nfs_lookup(). If not earlier, actually... How > about the following? A bit of rationale: dentry in question is negative and attempt to open it has just failed; in case it's really negative we did that d_drop() anyway (and then unconditionally rehashed it). In case when we proceed to nfs_lookup() and it does not fail, we'll have it rehashed there (with the right inode). What do we lose from doing d_drop() on *any* error here? It's negative, with dubious validity. In the worst case, we had open and lookup fail, but it's just us - the sucker really is negative and somebody else would be able to revalidate it. If we drop it here (and not rehash later), that somebody else will have to allocate an in-core dentry before doing lookup or atomic_open. Which is negligible extra burden. > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index aaf7bd0..6e3a6f4 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -1536,9 +1536,9 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry, > err = PTR_ERR(inode); > trace_nfs_atomic_open_exit(dir, ctx, open_flags, err); > put_nfs_open_context(ctx); > + d_drop(dentry); > switch (err) { > case -ENOENT: > - d_drop(dentry); > d_add(dentry, NULL); > nfs_set_verifier(dentry, nfs_save_change_attribute(dir)); > break; -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 03, 2016 at 05:42:53AM +0100, Al Viro wrote: > On Fri, Jun 03, 2016 at 05:26:48AM +0100, Al Viro wrote: > > > Looks like the right thing to do would be to do d_drop() at no_open:, > > just before we call nfs_lookup(). If not earlier, actually... How > > about the following? > > A bit of rationale: dentry in question is negative and attempt to open > it has just failed; in case it's really negative we did that d_drop() > anyway (and then unconditionally rehashed it). In case when we proceed to > nfs_lookup() and it does not fail, we'll have it rehashed there (with the > right inode). What do we lose from doing d_drop() on *any* error here? > It's negative, with dubious validity. In the worst case, we had open > and lookup fail, but it's just us - the sucker really is negative and > somebody else would be able to revalidate it. If we drop it here (and > not rehash later), that somebody else will have to allocate an in-core > dentry before doing lookup or atomic_open. Which is negligible extra > burden. I suspect that it got broken in commit 275bb3078 (NFSv4: Move dentry instantiation into the NFSv4-specific atomic open code). Prior to that commit, d_drop() was there (error or no error). Looks like 3.10+, indeed. I agree that we shouldn't drop it on successful open, but it needs to be done on errors. All of them, not just ENOENT, as in that commit. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jun 3, 2016, at 12:26 AM, Al Viro wrote: > On Thu, Jun 02, 2016 at 11:43:59PM -0400, Oleg Drokin wrote: > >>> Which of the call sites had that been and how does one reproduce that fun? >>> If you feel that posting a reproducer in the open is a bad idea, just send >>> it off-list... >> >> This is fs/nfs/dir.c::nfs_lookup() right after no_entry label. > > Bloody hell... Was that sucker hashed on the entry into nfs_lookup()? > If we get that with call as a method, we are really fucked. > > <greps> > > Ho-hum... One of the goto no_open; in nfs_atomic_open()? That would > mean a stale negative dentry in dcache that has escaped revalidation... > Wait a minute, didn't nfs ->d_revalidate() skip everything in some > cases, leaving it to ->atomic_open()? > > Looks like the right thing to do would be to do d_drop() at no_open:, > just before we call nfs_lookup(). If not earlier, actually... How > about the following? This one cures the insta-crash I was having, and I see no other ill-effects so far. > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index aaf7bd0..6e3a6f4 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -1536,9 +1536,9 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry, > err = PTR_ERR(inode); > trace_nfs_atomic_open_exit(dir, ctx, open_flags, err); > put_nfs_open_context(ctx); > + d_drop(dentry); > switch (err) { > case -ENOENT: > - d_drop(dentry); > d_add(dentry, NULL); > nfs_set_verifier(dentry, nfs_save_change_attribute(dir)); > break; -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index aaf7bd0..6e3a6f4 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1536,9 +1536,9 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry, err = PTR_ERR(inode); trace_nfs_atomic_open_exit(dir, ctx, open_flags, err); put_nfs_open_context(ctx); + d_drop(dentry); switch (err) { case -ENOENT: - d_drop(dentry); d_add(dentry, NULL); nfs_set_verifier(dentry, nfs_save_change_attribute(dir)); break;