Unionfs: Rewrite unionfs_d_revalidate

Rewrite unionfs_d_revalidate code to avoid stack-unfriendly recursion: split
into a call to revalidate just one dentry, and an interative driver function
to revalidate an entire dentry-parent chain.

Fix vfsmount ref leaks which prevented lower f/s from being unmounted after
generation increment, esp. during heavy loads.

Fix one deadlock between revalidation code and VFS.

Better documentation of what the code does.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
[jsipek: compile & whitespace fixes]
Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>

diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index c20dd6f73035448271170efeb600d1f08901d780..9b6128ccad27b3ee136232dd27ad28bebf117de4 100644 (file)
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -266,7 +266,11 @@ static int do_delayed_copyup(struct file *file, struct dentry *dentry)
        return err;
 }
 
-/* revalidate the stuct file */
+/*
+ * Revalidate the struct file
+ * @file: file to revalidate
+ * @willwrite: 1 if caller may cause changes to the file; 0 otherwise.
+ */
 int unionfs_file_revalidate(struct file *file, int willwrite)
 {
        struct super_block *sb;
@@ -280,8 +284,9 @@ int unionfs_file_revalidate(struct file *file, int willwrite)
        dentry = file->f_dentry;
        unionfs_lock_dentry(dentry);
        sb = dentry->d_sb;
-       unionfs_read_lock(sb);
-       if (!__unionfs_d_revalidate(dentry, NULL) && !d_deleted(dentry)) {
+
+       /* first revalidate the dentry inside struct file */
+       if (!__unionfs_d_revalidate_chain(dentry, NULL) && !d_deleted(dentry)) {
                err = -ESTALE;
                goto out_nofree;
        }
@@ -351,7 +356,6 @@ out:
        }
 out_nofree:
        unionfs_unlock_dentry(dentry);
-       unionfs_read_unlock(dentry->d_sb);
        return err;
 }
 

diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index 5ee14517213e1333a5d435562f5a03d0ec8e8509..c841f08a61abfda1dc3dfda9a92bae46bacf7a9a 100644 (file)
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -18,10 +18,15 @@
 
 #include "union.h"
 
+
 /*
- * returns 1 if valid, 0 otherwise.
+ * Revalidate a single dentry.
+ * Assume that dentry's info node is locked.
+ * Assume that parent(s) are all valid already, but
+ * the child may not yet be valid.
+ * Returns 1 if valid, 0 otherwise.
  */
-int __unionfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
+static int __unionfs_d_revalidate_one(struct dentry *dentry, struct nameidata *nd)
 {
        int valid = 1;          /* default is valid (1); invalid is 0. */
        struct dentry *hidden_dentry;
@@ -29,7 +34,6 @@ int __unionfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
        int sbgen, dgen;
        int positive = 0;
        int locked = 0;
-       int restart = 0;
        int interpose_flag;
 
        struct nameidata lowernd; /* TODO: be gentler to the stack */
@@ -39,7 +43,6 @@ int __unionfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
        else
                memset(&lowernd, 0, sizeof(struct nameidata));
 
-restart:
        verify_locked(dentry);
 
        /* if the dentry is unhashed, do NOT revalidate */
@@ -62,28 +65,12 @@ restart:
                struct dentry *result;
                int pdgen;
 
-               unionfs_read_lock(dentry->d_sb);
-               locked = 1;
-
                /* The root entry should always be valid */
                BUG_ON(IS_ROOT(dentry));
 
                /* We can't work correctly if our parent isn't valid. */
                pdgen = atomic_read(&UNIONFS_D(dentry->d_parent)->generation);
-               if (!restart && (pdgen != sbgen)) {
-                       unionfs_read_unlock(dentry->d_sb);
-                       locked = 0;
-                       /* We must be locked before our parent. */
-                       if (!
-                           (dentry->d_parent->d_op->
-                            d_revalidate(dentry->d_parent, nd))) {
-                               valid = 0;
-                               goto out;
-                       }
-                       restart = 1;
-                       goto restart;
-               }
-               BUG_ON(pdgen != sbgen);
+               BUG_ON(pdgen != sbgen); /* should never happen here */
 
                /* Free the pointers for our inodes and this dentry. */
                bstart = dbstart(dentry);
@@ -102,7 +89,17 @@ restart:
                interpose_flag = INTERPOSE_REVAL_NEG;
                if (positive) {
                        interpose_flag = INTERPOSE_REVAL;
-                       mutex_lock(&dentry->d_inode->i_mutex);
+                       /*
+                        * During BRM, the VFS could already hold a lock on
+                        * a file being read, so don't lock it again
+                        * (deadlock), but if you lock it in this function,
+                        * then release it here too.
+                        */
+                       if (!mutex_is_locked(&dentry->d_inode->i_mutex)) {
+                               mutex_lock(&dentry->d_inode->i_mutex);
+                               locked = 1;
+                       }
+
                        bstart = ibstart(dentry->d_inode);
                        bend = ibend(dentry->d_inode);
                        if (bstart >= 0) {
@@ -118,7 +115,8 @@ restart:
                        UNIONFS_I(dentry->d_inode)->lower_inodes = NULL;
                        ibstart(dentry->d_inode) = -1;
                        ibend(dentry->d_inode) = -1;
-                       mutex_unlock(&dentry->d_inode->i_mutex);
+                       if (locked)
+                               mutex_unlock(&dentry->d_inode->i_mutex);
                }
 
                result = unionfs_lookup_backend(dentry, &lowernd, interpose_flag);
@@ -169,8 +167,111 @@ restart:
        }
 
 out:
-       if (locked)
-               unionfs_read_unlock(dentry->d_sb);
+       return valid;
+}
+
+/*
+ * Revalidate a parent chain of dentries, then the actual node.
+ * Assumes that dentry is locked, but will lock all parents if/when needed.
+ */
+int __unionfs_d_revalidate_chain(struct dentry *dentry, struct nameidata *nd)
+{
+       int valid = 0;          /* default is invalid (0); valid is 1. */
+       struct dentry **chain = NULL; /* chain of dentries to reval */
+       int chain_len = 0;
+       struct dentry *dtmp;
+       int sbgen, dgen, i;
+       int saved_bstart, saved_bend, bindex;
+
+       /* find length of chain needed to revalidate */
+       /* XXX: should I grab some global (dcache?) lock? */
+       chain_len = 0;
+       sbgen = atomic_read(&UNIONFS_SB(dentry->d_sb)->generation);
+       dtmp = dentry->d_parent;
+       dgen = atomic_read(&UNIONFS_D(dtmp)->generation);
+       while (sbgen != dgen) {
+               /* The root entry should always be valid */
+               BUG_ON(IS_ROOT(dtmp));
+               chain_len++;
+               dtmp = dtmp->d_parent;
+               dgen = atomic_read(&UNIONFS_D(dtmp)->generation);
+       }
+       if (chain_len == 0) {
+               goto out_this;  /* shortcut if parents are OK */
+       }
+
+       /*
+        * Allocate array of dentries to reval.  We could use linked lists,
+        * but the number of entries we need to alloc here is often small,
+        * and short lived, so locality will be better.
+        */
+       chain = kzalloc(chain_len * sizeof(struct dentry *), GFP_KERNEL);
+       if (!chain) {
+               printk("unionfs: no more memory in %s\n", __FUNCTION__);
+               goto out;
+       }
+
+       /*
+        * lock all dentries in chain, in child to parent order.
+        * if failed, then sleep for a little, then retry.
+        */
+       dtmp = dentry->d_parent;
+       for (i=chain_len-1; i>=0; i--) {
+               chain[i] = dget(dtmp);
+               dtmp = dtmp->d_parent;
+       }
+
+       /*
+        * call __unionfs_d_revalidate() on each dentry, but in parent to
+        * child order.
+        */
+       for (i=0; i<chain_len; i++) {
+               unionfs_lock_dentry(chain[i]);
+               saved_bstart = dbstart(chain[i]);
+               saved_bend = dbend(chain[i]);
+               sbgen = atomic_read(&UNIONFS_SB(dentry->d_sb)->generation);
+               dgen = atomic_read(&UNIONFS_D(chain[i])->generation);
+
+               valid = __unionfs_d_revalidate_one(chain[i], nd);
+               /* XXX: is this the correct mntput condition?! */
+               if (valid && chain_len > 0 &&
+                   sbgen != dgen && dentry->d_inode &&
+                   S_ISDIR(dentry->d_inode->i_mode)) {
+                       for (bindex = saved_bstart; bindex <= saved_bend; bindex++)
+                               unionfs_mntput(chain[i], bindex);
+               }
+               unionfs_unlock_dentry(chain[i]);
+
+               if (!valid) {
+                       goto out_free;
+               }
+       }
+
+
+ out_this:
+       /* finally, lock this dentry and revalidate it */
+       verify_locked(dentry);
+       dgen = atomic_read(&UNIONFS_D(dentry)->generation);
+       saved_bstart = dbstart(dentry);
+       saved_bend = dbend(dentry);
+       valid = __unionfs_d_revalidate_one(dentry, nd);
+
+       if (valid && chain_len > 0 &&
+           sbgen != dgen && dentry->d_inode &&
+           S_ISDIR(dentry->d_inode->i_mode)) {
+               for (bindex = saved_bstart; bindex <= saved_bend; bindex++)
+                       unionfs_mntput(dentry, bindex);
+       }
+
+ out_free:
+       /* unlock/dput all dentries in chain and return status */
+       if (chain_len > 0) {
+               for (i=0; i<chain_len; i++) {
+                       dput(chain[i]);
+               }
+               kfree(chain);
+       }
+ out:
        return valid;
 }
 
@@ -179,7 +280,7 @@ static int unionfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
        int err;
 
        unionfs_lock_dentry(dentry);
-       err = __unionfs_d_revalidate(dentry, nd);
+       err = __unionfs_d_revalidate_chain(dentry, nd);
        unionfs_unlock_dentry(dentry);
 
        return err;

diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h
index 53c7c2c4a68d563e315b46aa72e34f27915cbaaa..66ffe88d29674177b4f4d7ba093c8b28cd7cb135 100644 (file)
--- a/fs/unionfs/union.h
+++ b/fs/unionfs/union.h
@@ -302,6 +302,7 @@ int unionfs_unlink(struct inode *dir, struct dentry *dentry);
 int unionfs_rmdir(struct inode *dir, struct dentry *dentry);
 
 int __unionfs_d_revalidate(struct dentry *dentry, struct nameidata *nd);
+int __unionfs_d_revalidate_chain(struct dentry *dentry, struct nameidata *nd);
 
 /* The values for unionfs_interpose's flag. */
 #define INTERPOSE_DEFAULT      0