Re: [PATCH 0/5] hybrid union filesystem prototype

[Neil Brown <neilb@suse.de>]

On Mon, 30 Aug 2010 21:40:27 +1000
Neil Brown <neilb@suse.de> wrote:

> > > > I think it's best to leave that stuff until someone actually cares.
> > > > The "people might find it useful" argument is not strong enough to put
> > > > nontrivial effort into thinking about all the weird corner cases.
> > > 
> > > My thought on this is that in order to describe the behaviour of the
> > > filesystem accurately you need to think about all the weird corner cases.
> > > 
> > > Then it becomes a question of: is it easier to document the weird behaviour,
> > > or change the code so the documentation will be easier to understand?
> > > Some cases will go one way, some the other.
> > > 
> > > But as you suggest this isn't urgent.
> > 
> > You didn't mention one possibility: add limitations that prevent the
> > weird corner cases arising.  I believe this is the simplest option.
> 
> Val has been following that approach and asking if it is possible to make an
> NFS filesystem really-truly read-only. i.e. no changes.
> I don't believe it is.
> 
> But I won't pursue this further without patches.
> 

And here is a patch.  It isn't really complete, but I have done enough for
today.  It at least shows what I am trying to do.

To see a clear indication of the effect, try the following script before and
after the patch..

-----------------------
#!/bin/sh

# L = lower U = upper O = overlay

umount /tmp/O
rm -rf /tmp/[LUO]

mkdir /tmp/{L,U,O}

for i in 1 2 3; do > /tmp/L/$i ; done

mount -t union -o upperdir=/tmp/U,lowerdir=/tmp/L pointlessword /tmp/O

ls -l /tmp/O/foo
ls -l /tmp/O
> /tmp/L/foo
ls -l /tmp/O
-------------------------


The last 'ls -l' report a non-existent 'foo' without the patch.  With the
patch it more correctly doesn't report anything at all about foo.


No patch change-log yet because I'm not suggesting it be included, just
reviewed.

NeilBrown

diff --git a/fs/union/union.c b/fs/union/union.c
index e19fe62..434d152 100644
--- a/fs/union/union.c
+++ b/fs/union/union.c
@@ -392,9 +392,160 @@ static void union_dentry_iput(struct dentry *dentry, struct inode *inode)
 	iput(inode);
 }
 
+static int union_still_valid(struct dentry *dentry,
+			     struct dentry *parent, struct dentry *child)
+{
+	/* dentry is in the union filesystem
+	 * child is the corresponding dentry in the upper to lower layer
+	 * parent is the corresponding dentry of dentry's parent in the same layer
+	 *
+	 * If child is NULL, the parent must be NULL or negative.
+	 * Otherwise child must be hashed, have parent as the d_parent, and
+	 * have the same name as dentry
+	 *
+	 */
+	struct qstr *qstr;
+	int rv;
+
+	if (child == NULL)
+		return (parent == NULL || parent->d_inode == NULL);
+
+	if (child->d_parent != parent)
+		return 0;
+	if (d_unhashed(child))
+		return 0;
+
+	/* Unfortunately we need d_lock to compare names */
+	spin_lock(&dentry->d_lock);
+	spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
+	qstr = &child->d_name;
+	if (parent->d_op && parent->d_op->d_compare)
+		rv = ! (parent->d_op->d_compare(parent, qstr, &dentry->d_name));
+	else
+		rv =  (qstr->len == dentry->d_name.len &&
+		       memcmp(qstr->name, dentry->d_name.name, qstr->len) == 0);
+
+	spin_unlock(&child->d_lock);
+	spin_unlock(&dentry->d_lock);
+	return rv;
+}
+
+static struct dentry *union_lookup_real(struct dentry *dir, struct qstr *name);
+static struct inode *union_new_inode(struct super_block *sb, umode_t mode);
+
+static struct union_entry do_union_lookup(struct inode *dir, struct dentry *dentry,
+					   struct nameidata *nd)
+{
+	struct union_entry *pue = dentry->d_parent->d_fsdata;
+	struct union_entry ue;
+	struct dentry *upperdir = pue->upperpath.dentry;
+	struct dentry *upperdentry = NULL;
+	struct dentry *lowerdir = pue->lowerpath.dentry;
+	struct dentry *lowerdentry = NULL;
+	int err;
+
+	memset(&ue, 0, sizeof(ue));
+
+	if (upperdir) {
+		upperdentry = union_lookup_real(upperdir, &dentry->d_name);
+		err = PTR_ERR(upperdentry);
+		if (IS_ERR(upperdentry))
+			goto out;
+
+		if (upperdentry) {
+			if (union_is_opaquedir(upperdentry))
+				ue.opaque = true;
+			else if (union_is_whiteout(upperdentry)) {
+				dput(upperdentry);
+				upperdentry = NULL;
+				ue.opaque = true;
+			}
+		}
+	}
+	if (lowerdir && !ue.opaque) {
+		lowerdentry = union_lookup_real(lowerdir, &dentry->d_name);
+		if (IS_ERR(lowerdentry)) {
+			err = PTR_ERR(lowerdentry);
+			dput(upperdentry);
+			goto out;
+		}
+	}
+
+	if (lowerdentry && upperdentry &&
+	    (!S_ISDIR(upperdentry->d_inode->i_mode) ||
+	     !S_ISDIR(lowerdentry->d_inode->i_mode))) {
+		dput(lowerdentry);
+		lowerdentry = NULL;
+		ue.opaque = true;
+	}
+
+	if (upperdentry) {
+		ue.upperpath.mnt = pue->upperpath.mnt;
+		ue.upperpath.dentry = upperdentry;
+		path_get(&ue.upperpath);
+		dput(upperdentry);
+	}
+	if (lowerdentry) {
+		ue.lowerpath.mnt = pue->lowerpath.mnt;
+		ue.lowerpath.dentry = lowerdentry;
+		path_get(&ue.lowerpath);
+		dput(lowerdentry);
+	}
+
+	return ue;
+
+out:
+	ue.upperpath.dentry = ERR_PTR(err);
+	return ue;
+}
+
+static int union_dentry_revalidate(struct dentry *dentry, struct nameidata *nd)
+{
+	/* We need to revalidate this dentry if the upper parent might have
+	 * changed.
+	 */
+	struct dentry *parent = dget_parent(dentry);
+	struct union_entry *ue = dentry->d_fsdata;
+	struct union_entry *pue = parent->d_fsdata;
+	struct union_entry nue;
+	int rv = 1;
+
+	if (union_still_valid(dentry, pue->upperpath.dentry,  ue->upperpath.dentry) &&
+	    union_still_valid(dentry, pue->lowerpath.dentry,  ue->lowerpath.dentry))
+		goto out;
+
+	/* If dentry is not a directory, it is safe to fail */
+	rv = 0;
+	if (dentry->d_inode == NULL || !S_ISDIR(dentry->d_inode->i_mode))
+		goto out;
+
+	/* If this dentry or a child is in use by someone else, it cannot be
+	 * invalidated so the best thing to do is re-lookup the names
+	 */
+	mutex_lock(&parent->d_inode->i_mutex);
+	if (dentry->d_parent != parent)
+		goto out_unlock;
+	rv = 1;
+	nue = do_union_lookup(parent->d_inode, dentry, nd);
+	if (IS_ERR(nue.upperpath.dentry))
+		/* Leave the dentry unchanged, but return the error */
+		rv = PTR_ERR(nue.upperpath.dentry);
+	else {
+		path_put(&ue->upperpath);
+		path_put(&ue->lowerpath);
+		*ue = nue;
+	}
+out_unlock:
+	mutex_unlock(&parent->d_inode->i_mutex);
+out:
+	dput(parent);
+	return rv;
+}
+
 static const struct dentry_operations union_dentry_operations = {
 	.d_release = union_dentry_release,
 	.d_iput = union_dentry_iput,
+	.d_revalidate = union_dentry_revalidate,
 };
 
 static struct inode *union_new_inode(struct super_block *sb, umode_t mode)
@@ -459,12 +610,7 @@ static struct dentry *union_lookup_real(struct dentry *dir, struct qstr *name)
 static struct dentry *union_lookup(struct inode *dir, struct dentry *dentry,
 				   struct nameidata *nd)
 {
-	struct union_entry *pue = dentry->d_parent->d_fsdata;
 	struct union_entry *ue;
-	struct dentry *upperdir = pue->upperpath.dentry;
-	struct dentry *upperdentry = NULL;
-	struct dentry *lowerdir = pue->lowerpath.dentry;
-	struct dentry *lowerdentry = NULL;
 	struct inode *inode = NULL;
 	int err;
 
@@ -472,71 +618,29 @@ static struct dentry *union_lookup(struct inode *dir, struct dentry *dentry,
 	ue = kzalloc(sizeof(struct union_entry), GFP_KERNEL);
 	if (!ue)
 		goto out;
-
-	if (upperdir) {
-		upperdentry = union_lookup_real(upperdir, &dentry->d_name);
-		err = PTR_ERR(upperdentry);
-		if (IS_ERR(upperdentry))
-			goto out_free;
-
-		if (upperdentry) {
-			if (union_is_opaquedir(upperdentry))
-				ue->opaque = true;
-			else if (union_is_whiteout(upperdentry)) {
-				dput(upperdentry);
-				upperdentry = NULL;
-				ue->opaque = true;
-			}
-		}
-	}
-	if (lowerdir && !ue->opaque) {
-		lowerdentry = union_lookup_real(lowerdir, &dentry->d_name);
-		if (IS_ERR(lowerdentry)) {
-			err = PTR_ERR(lowerdentry);
-			dput(upperdentry);
-			goto out_free;
-		}
+	*ue = do_union_lookup(dir, dentry, nd);
+	if (IS_ERR(ue->upperpath.dentry)) {
+		err = PTR_ERR(ue->upperpath.dentry);
+		goto out_free;
 	}
-
-	if (lowerdentry && upperdentry &&
-	    (!S_ISDIR(upperdentry->d_inode->i_mode) ||
-	     !S_ISDIR(lowerdentry->d_inode->i_mode))) {
-		dput(lowerdentry);
-		lowerdentry = NULL;
-		ue->opaque = true;
-	}
-
-	if (lowerdentry || upperdentry) {
+	if (ue->upperpath.dentry || ue->lowerpath.dentry) {
 		struct dentry *realdentry;
 
-		realdentry = upperdentry ? upperdentry : lowerdentry;
+		realdentry = ue->upperpath.dentry ? : ue->lowerpath.dentry;
 		inode = union_new_inode(dir->i_sb, realdentry->d_inode->i_mode);
-		if (!inode)
-			goto out_dput;
-	}
-
-	if (upperdentry) {
-		ue->upperpath.mnt = pue->upperpath.mnt;
-		ue->upperpath.dentry = upperdentry;
-		path_get(&ue->upperpath);
-		dput(upperdentry);
-	}
-	if (lowerdentry) {
-		ue->lowerpath.mnt = pue->lowerpath.mnt;
-		ue->lowerpath.dentry = lowerdentry;
-		path_get(&ue->lowerpath);
-		dput(lowerdentry);
+		err = -ENOMEM;
+		if (!inode) {
+			path_put(&ue->upperpath);
+			path_put(&ue->lowerpath);
+			goto out_free;
+		}
 	}
-
 	d_add(dentry, inode);
 	dentry->d_fsdata = ue;
 	dentry->d_op = &union_dentry_operations;
 
 	return NULL;
 
-out_dput:
-	dput(upperdentry);
-	dput(lowerdentry);
 out_free:
 	kfree(ue);
 out: