On Fri, 2 Aug 2013 11:02:25 +0200
Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> On Thu, Aug 1, 2013 at 8:45 PM, Ric Wheeler <rwheeler@xxxxxxxxxx> wrote:
> >
> > Adding in the NFS list for broader review.
> >
> > I thought that the issue was primarily in FUSE itself, not seen in practice
> > in NFS?
>
> I haven't tested NFS (have but a single notebook here) but AFAICS the
> issue of not fuse specific but is inherent in the fact that NFS does
> d_drop() on a non-empty directory dentry. It does check for submounts
> *before*, but it doesn't do anything about submounts *after* (or
> during) the d_drop(). It's probably not something people complain
> about, because they choose mountpoints to be pretty stable points in
> the tree that don't get removed or moved around.
>
> Thanks,
> Miklos
>
> >>
> >> diff --git a/fs/dcache.c b/fs/dcache.c
> >> index 87bdb53..5c51217 100644
> >> --- a/fs/dcache.c
> >> +++ b/fs/dcache.c
> >> @@ -1103,6 +1103,34 @@ rename_retry:
> >> }
> >> EXPORT_SYMBOL(have_submounts);
> >> +static bool __has_unlinked_ancestor(struct dentry *dentry)
> >> +{
> >> + struct dentry *this;
> >> +
> >> + for (this = dentry; !IS_ROOT(this); this = this->d_parent) {
> >> + if (d_unhashed(this))
> >> + return true;
> >> + }
> >> + return false;
> >> +}
> >> +
> >> +/*
> >> + * Called by mount code to check if the mountpoint is reachable (e.g. NFS
> >> can
> >> + * unhash a directory dentry and then the complete subtree can become
> >> + * unreachable).
> >> + */
> >> +bool has_unlinked_ancestor(struct dentry *dentry)
> >> +{
> >> + bool found;
> >> +
> >> + /* Need exclusion wrt. have_submounts() */
> >> + write_seqlock(&rename_lock);
> >> + found = __has_unlinked_ancestor(dentry);
> >> + write_sequnlock(&rename_lock);
> >> +
> >> + return found;
> >> +}
> >> +
> >> /*
> >> * Search the dentry child list of the specified parent,
> >> * and move any unused dentries to the end of the unused
> >> diff --git a/fs/internal.h b/fs/internal.h
> >> index 7c5f01c..d232355 100644
> >> --- a/fs/internal.h
> >> +++ b/fs/internal.h
> >> @@ -126,6 +126,7 @@ extern int invalidate_inodes(struct super_block *,
> >> bool);
> >> * dcache.c
> >> */
> >> extern struct dentry *__d_alloc(struct super_block *, const struct qstr
> >> *);
> >> +extern bool has_unlinked_ancestor(struct dentry *dentry);
> >> /*
> >> * read_write.c
> >> diff --git a/fs/namespace.c b/fs/namespace.c
> >> index 7b1ca9b..bb92a9c 100644
> >> --- a/fs/namespace.c
> >> +++ b/fs/namespace.c
> >> @@ -634,6 +634,15 @@ static struct mountpoint *new_mountpoint(struct
> >> dentry *dentry)
> >> }
> >> dentry->d_flags |= DCACHE_MOUNTED;
> >> spin_unlock(&dentry->d_lock);
> >> +
> >> + if (has_unlinked_ancestor(dentry)) {
> >> + spin_lock(&dentry->d_lock);
> >> + dentry->d_flags &= ~DCACHE_MOUNTED;
> >> + spin_unlock(&dentry->d_lock);
> >> + kfree(mp);
> >> + return ERR_PTR(-ENOENT);
> >> + }
> >> +
> >> mp->m_dentry = dentry;
> >> mp->m_count = 1;
> >> list_add(&mp->m_hash, chain);
> >
> >
Ok, took me a couple of times to look over the code but I think I
understand the problem now...
IIUC, then this patch should only ever cause this to return -ENOENT in
a situation similar to the one in Anand's reproducer, right? The
mountpoint-to-be was unlinked in another tree, and thus we found it to
be invalid in the tree that we're mounting in. If so, then the dentry
didn't exist at some point during the race window. Returning -ENOENT
seems reasonable to me in that situation.
It might cause some strange setups to regress, but aren't those already
broken anyway? Or does the fact that NFS uses d_materialise_* helpers to
hook these dentries back into the tree help paper over that?
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
