On Fri, 2017-02-17 at 17:51 +0000, Al Viro wrote:
> On Fri, Feb 17, 2017 at 09:24:40AM -0800, James Bottomley wrote:
>
> > > What happens when somebody comes along and creates the damn thing
> > > on
> > > the underlying fs? _Not_ via your code, that is - using the
> > > underlying fs mounted elsewhere.
> >
> > Point taken. This, I think fixes the dcache revalidation issue.
>
> No, it doesn't. Consider a local filesystem. Those do not have any
> ->d_revalidate() - the kernel bloody well knows what happens to
> directories. If e.g. a previously absent file gets created, it's
> been done by the kernel itself and dentry has been made positive; if
> a previously existing file has been removed, dentry has either become
> negative or, if it had been pinned (e.g. file was opened at the time,
> or your code had been holding a reference to it, etc.) it will be
> unhashed so that new lookups won't find it, etc. No need to
> revalidate anything.
>
> Now, consider your code. You've done a lookup in the underlying fs.
> It has, at the time, come negative, so you have your (negative)
> dentry pointing to that on the underlying fs. If somebody comes and
> does e.g. mkdir() via your fs, it will call vfs_mkdir() on the
> underlying sucker, hopefully turning it positive and associate a new
> in-core inode with your previously negative dentry. But what happens
> if mkdir is done via underlying fs, or via another instance of yours
> over the same tree?
> Underlying dentry goes positive; yours is still negative. The
> underlying fs either doesn't have ->d_revalidate() or, if there is
> one it says that the underlying dentry is valid, thank you very much,
> no need to invalidate anything.
>
> In other words, your patch does nothing for object getting created.
Right at the moment, the upper layer doesn't cache negative dentries,
but that's only a partial solution. I assume you'd now like me to
cache negative dentries (principle of least surprise) and handle the
underlying negative to positive transition in d_revalidate?
I can do that.
James
---
diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index a4a1f98..5b50447 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -118,9 +118,50 @@ static struct dentry *shiftfs_d_real(struct dentry *dentry,
return real;
}
+static int shiftfs_d_weak_revalidate(struct dentry *dentry, unsigned int flags)
+{
+ struct dentry *real = dentry->d_fsdata;
+
+ if (d_unhashed(real))
+ return 0;
+
+ if (!(real->d_flags & DCACHE_OP_WEAK_REVALIDATE))
+ return 1;
+
+ return real->d_op->d_weak_revalidate(real, flags);
+}
+
+static int shiftfs_d_revalidate(struct dentry *dentry, unsigned int flags)
+{
+ struct dentry *real = dentry->d_fsdata;
+ int ret;
+
+ if (d_unhashed(real))
+ return 0;
+
+ /*
+ * inode state of underlying changed from positive to negative
+ * or vice versa; force a lookup to update our view
+ */
+ if (d_is_negative(real) != d_is_negative(dentry))
+ return 0;
+
+ if (!(real->d_flags & DCACHE_OP_REVALIDATE))
+ return 1;
+
+ ret = real->d_op->d_revalidate(real, flags);
+
+ if (ret == 0 && !(flags & LOOKUP_RCU))
+ d_invalidate(real);
+
+ return ret;
+}
+
static const struct dentry_operations shiftfs_dentry_ops = {
.d_release = shiftfs_d_release,
.d_real = shiftfs_d_real,
+ .d_revalidate = shiftfs_d_revalidate,
+ .d_weak_revalidate = shiftfs_d_weak_revalidate,
};
static int shiftfs_readlink(struct dentry *dentry, char __user *data,
@@ -423,7 +464,7 @@ static struct dentry *shiftfs_lookup(struct inode *dir, struct dentry *dentry,
dentry->d_fsdata = new;
if (!new->d_inode)
- return NULL;
+ goto out;
newi = shiftfs_new_inode(dentry->d_sb, new->d_inode->i_mode, new);
if (!newi) {
@@ -431,9 +472,8 @@ static struct dentry *shiftfs_lookup(struct inode *dir, struct dentry *dentry,
return ERR_PTR(-ENOMEM);
}
- d_splice_alias(newi, dentry);
-
- return NULL;
+ out:
+ return d_splice_alias(newi, dentry);
}
static int shiftfs_permission(struct inode *inode, int mask)
