From: Erez Zadok <ezk@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
[jsipek: compile & whitespace fixes]
Signed-off-by: Josef 'Jeff' Sipek <jsipek@xxxxxxxxxxxxx>
---
fs/unionfs/commonfops.c | 12 +++-
fs/unionfs/dentry.c | 153 +++++++++++++++++++++++++++++++++++++++--------
fs/unionfs/union.h | 1 +
3 files changed, 136 insertions(+), 30 deletions(-)
diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index c20dd6f..9b6128c 100644
--- 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 5ee1451..c841f08 100644
--- 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 53c7c2c..66ffe88 100644
--- 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
--
1.5.0.3.268.g3dda
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
