[GIT PULL -mm] 0/6 Unionfs updates/cleanups/fixes

[GIT PULL -mm] 0/6 Unionfs updates/cleanups/fixes

[Erez Zadok]

The following is a series of patches related to Unionfs.  They represent a
few code cleanups (suggested or inspired by comments on the previous set of
patches), and a few more bug fixes (esp. to cache coherency).  These fixes
were tested on our 2.6.23-rc6 latest code, as well as the backports to
2.6.{22,21,20,19,18,9} on ext2/3/4, xfs, reiserfs, nfs, jffs2, ramfs, tmpfs,
cramfs, and squashfs (where available).  See http://unionfs.filesystems.org/
to download backported unionfs code.

Please pull from the 'master' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/ezk/unionfs.git
                                              ^^^
(Note: the git URL to pull from had changed from "jsipek" to "ezk")

to receive the following:

Erez_Zadok (6):
      Unionfs: use bool type in dentry and file revalidation code
      Unionfs: remove unnecessary comment
      Unionfs: add missing newlines to printks
      Unionfs: check integrity only if validated dentry successfully
      Unionfs: unionfs_lookup locking consistency
      Unionfs: cache coherency after lower objects are removed

 commonfops.c |   12 ++++++------
 dentry.c     |    9 +++++----
 dirfops.c    |    4 ++--
 fanout.h     |    1 -
 file.c       |   12 ++++++------
 inode.c      |    6 +++++-
 lookup.c     |   52 +++++++++++++++++++++++++++++++---------------------
 mmap.c       |    6 +++---
 union.h      |    2 +-
 9 files changed, 59 insertions(+), 45 deletions(-)

Erez Zadok
ezk@cs.sunysb.edu

[PATCH 1/6] Unionfs: use bool type in dentry and file revalidation code

[Erez Zadok]

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
Acked-by: Josef Sipek <jsipek@fsl.cs.sunysb.edu>
---
 fs/unionfs/commonfops.c |   12 ++++++------
 fs/unionfs/dentry.c     |    2 +-
 fs/unionfs/dirfops.c    |    4 ++--
 fs/unionfs/file.c       |   12 ++++++------
 fs/unionfs/mmap.c       |    6 +++---
 fs/unionfs/union.h      |    2 +-
 6 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index 724128d..87cbb09 100644
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -190,7 +190,7 @@ out:
 }
 
 /* open the highest priority file for a given upper file */
-static int open_highest_file(struct file *file, int willwrite)
+static int open_highest_file(struct file *file, bool willwrite)
 {
 	int bindex, bstart, bend, err = 0;
 	struct file *lower_file;
@@ -298,9 +298,9 @@ out:
 /*
  * Revalidate the struct file
  * @file: file to revalidate
- * @willwrite: 1 if caller may cause changes to the file; 0 otherwise.
+ * @willwrite: true if caller may cause changes to the file; false otherwise.
  */
-int unionfs_file_revalidate(struct file *file, int willwrite)
+int unionfs_file_revalidate(struct file *file, bool willwrite)
 {
 	struct super_block *sb;
 	struct dentry *dentry;
@@ -612,7 +612,7 @@ int unionfs_file_release(struct inode *inode, struct file *file)
 	 * This is important for open-but-unlinked files, as well as mmap
 	 * support.
 	 */
-	if ((err = unionfs_file_revalidate(file, 1)))
+	if ((err = unionfs_file_revalidate(file, true)))
 		goto out;
 	unionfs_check_file(file);
 	fileinfo = UNIONFS_F(file);
@@ -753,7 +753,7 @@ long unionfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
 
-	if ((err = unionfs_file_revalidate(file, 1)))
+	if ((err = unionfs_file_revalidate(file, true)))
 		goto out;
 
 	/* check if asked for local commands */
@@ -791,7 +791,7 @@ int unionfs_flush(struct file *file, fl_owner_t id)
 
 	unionfs_read_lock(dentry->d_sb);
 
-	if ((err = unionfs_file_revalidate(file, 1)))
+	if ((err = unionfs_file_revalidate(file, true)))
 		goto out;
 	unionfs_check_file(file);
 
diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index e46a53e..f32922e 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -262,7 +262,7 @@ static inline void purge_inode_data(struct inode *inode)
  * Revalidate a parent chain of dentries, then the actual node.
  * Assumes that dentry is locked, but will lock all parents if/when needed.
  *
- * If 'willwrite' is 1, and the lower inode times are not in sync, then
+ * If 'willwrite' is true, and the lower inode times are not in sync, then
  * *don't* purge_inode_data, as it could deadlock if ->write calls us and we
  * try to truncate a locked page.  Besides, if unionfs is about to write
  * data to a file, then there's the data unionfs is about to write is more
diff --git a/fs/unionfs/dirfops.c b/fs/unionfs/dirfops.c
index 980f125..c923e58 100644
--- a/fs/unionfs/dirfops.c
+++ b/fs/unionfs/dirfops.c
@@ -99,7 +99,7 @@ static int unionfs_readdir(struct file *file, void *dirent, filldir_t filldir)
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
 
-	if ((err = unionfs_file_revalidate(file, 0)))
+	if ((err = unionfs_file_revalidate(file, false)))
 		goto out;
 
 	inode = file->f_path.dentry->d_inode;
@@ -201,7 +201,7 @@ static loff_t unionfs_dir_llseek(struct file *file, loff_t offset, int origin)
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
 
-	if ((err = unionfs_file_revalidate(file, 0)))
+	if ((err = unionfs_file_revalidate(file, false)))
 		goto out;
 
 	rdstate = UNIONFS_F(file)->rdstate;
diff --git a/fs/unionfs/file.c b/fs/unionfs/file.c
index e9e63b7..d8eaaa5 100644
--- a/fs/unionfs/file.c
+++ b/fs/unionfs/file.c
@@ -24,7 +24,7 @@ static ssize_t unionfs_read(struct file *file, char __user *buf,
 	int err;
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
-	if ((err = unionfs_file_revalidate(file, 0)))
+	if ((err = unionfs_file_revalidate(file, false)))
 		goto out;
 	unionfs_check_file(file);
 
@@ -47,7 +47,7 @@ static ssize_t unionfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
 	struct file *file = iocb->ki_filp;
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
-	if ((err = unionfs_file_revalidate(file, 0)))
+	if ((err = unionfs_file_revalidate(file, false)))
 		goto out;
 	unionfs_check_file(file);
 
@@ -72,7 +72,7 @@ static ssize_t unionfs_write(struct file *file, const char __user *buf,
 	int err = 0;
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
-	if ((err = unionfs_file_revalidate(file, 1)))
+	if ((err = unionfs_file_revalidate(file, true)))
 		goto out;
 	unionfs_check_file(file);
 
@@ -97,7 +97,7 @@ static int unionfs_file_readdir(struct file *file, void *dirent,
 static int unionfs_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	int err = 0;
-	int willwrite;
+	bool willwrite;
 	struct file *lower_file;
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
@@ -149,7 +149,7 @@ int unionfs_fsync(struct file *file, struct dentry *dentry, int datasync)
 	int err = -EINVAL;
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
-	if ((err = unionfs_file_revalidate(file, 1)))
+	if ((err = unionfs_file_revalidate(file, true)))
 		goto out;
 	unionfs_check_file(file);
 
@@ -196,7 +196,7 @@ int unionfs_fasync(int fd, struct file *file, int flag)
 	int err = 0;
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
-	if ((err = unionfs_file_revalidate(file, 1)))
+	if ((err = unionfs_file_revalidate(file, true)))
 		goto out;
 	unionfs_check_file(file);
 
diff --git a/fs/unionfs/mmap.c b/fs/unionfs/mmap.c
index 6ef19af..88ef6a6 100644
--- a/fs/unionfs/mmap.c
+++ b/fs/unionfs/mmap.c
@@ -211,7 +211,7 @@ static int unionfs_readpage(struct file *file, struct page *page)
 	int err;
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
-	if ((err = unionfs_file_revalidate(file, 0)))
+	if ((err = unionfs_file_revalidate(file, false)))
 		goto out;
 	unionfs_check_file(file);
 
@@ -254,7 +254,7 @@ static int unionfs_prepare_write(struct file *file, struct page *page,
 	 * is here, in ->prepare_write.
 	 */
 	unionfs_copy_attr_times(file->f_path.dentry->d_inode);
-	err = unionfs_file_revalidate(file, 1);
+	err = unionfs_file_revalidate(file, true);
 	unionfs_check_file(file);
 	unionfs_read_unlock(file->f_path.dentry->d_sb);
 
@@ -275,7 +275,7 @@ static int unionfs_commit_write(struct file *file, struct page *page,
 	BUG_ON(file == NULL);
 
 	unionfs_read_lock(file->f_path.dentry->d_sb);
-	if ((err = unionfs_file_revalidate(file, 1)))
+	if ((err = unionfs_file_revalidate(file, true)))
 		goto out;
 	unionfs_check_file(file);
 
diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h
index 97da8f9..1cb2e1d 100644
--- a/fs/unionfs/union.h
+++ b/fs/unionfs/union.h
@@ -311,7 +311,7 @@ extern int unionfs_setlk(struct file *file, int cmd, struct file_lock *fl);
 extern int unionfs_getlk(struct file *file, struct file_lock *fl);
 
 /* Common file operations. */
-extern int unionfs_file_revalidate(struct file *file, int willwrite);
+extern int unionfs_file_revalidate(struct file *file, bool willwrite);
 extern int unionfs_open(struct inode *inode, struct file *file);
 extern int unionfs_file_release(struct inode *inode, struct file *file);
 extern int unionfs_flush(struct file *file, fl_owner_t id);
-- 
1.5.2.2

[PATCH 2/6] Unionfs: remove unnecessary comment

[Erez Zadok]

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
Acked-by: Josef Sipek <jsipek@fsl.cs.sunysb.edu>
---
 fs/unionfs/fanout.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/unionfs/fanout.h b/fs/unionfs/fanout.h
index c5bf454..afeb9f6 100644
--- a/fs/unionfs/fanout.h
+++ b/fs/unionfs/fanout.h
@@ -320,7 +320,6 @@ static inline void unionfs_copy_attr_times(struct inode *upper)
 			upper->i_ctime = lower->i_ctime;
 		if (timespec_compare(&upper->i_atime, &lower->i_atime) < 0)
 			upper->i_atime = lower->i_atime;
-		/* XXX: should we notify_change on our upper inode? */
 	}
 }
 
-- 
1.5.2.2

[PATCH 3/6] Unionfs: add missing newlines to printks

[Erez Zadok]

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
Acked-by: Josef Sipek <jsipek@fsl.cs.sunysb.edu>
---
 fs/unionfs/dentry.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index f32922e..91f9780 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -438,13 +438,13 @@ static void unionfs_d_release(struct dentry *dentry)
 	unionfs_check_dentry(dentry);
 	/* this could be a negative dentry, so check first */
 	if (!UNIONFS_D(dentry)) {
-		printk(KERN_DEBUG "unionfs: dentry without private data: %.*s",
+		printk(KERN_DEBUG "unionfs: dentry without private data: %.*s\n",
 		       dentry->d_name.len, dentry->d_name.name);
 		goto out;
 	} else if (dbstart(dentry) < 0) {
 		/* this is due to a failed lookup */
 		printk(KERN_DEBUG "unionfs: dentry without lower "
-		       "dentries: %.*s",
+		       "dentries: %.*s\n",
 		       dentry->d_name.len, dentry->d_name.name);
 		goto out_free;
 	}
-- 
1.5.2.2

[PATCH 4/6] Unionfs: check integrity only if validated dentry successfully

[Erez Zadok]

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
Acked-by: Josef Sipek <jsipek@fsl.cs.sunysb.edu>
---
 fs/unionfs/dentry.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index 91f9780..9e0742d 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -418,7 +418,8 @@ static int unionfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
 	unionfs_lock_dentry(dentry);
 	err = __unionfs_d_revalidate_chain(dentry, nd, false);
 	unionfs_unlock_dentry(dentry);
-	unionfs_check_dentry(dentry);
+	if (err > 0) /* true==1: dentry is valid */
+		unionfs_check_dentry(dentry);
 
 	unionfs_read_unlock(dentry->d_sb);
 
-- 
1.5.2.2

[PATCH 5/6] Unionfs: unionfs_lookup locking consistency

[Erez Zadok]

Ensure that our lookup locking is consistent and symmetric: if a lock
existed before calling lookup_backend, it should remain so; only if
performing a lookup of a known new dentry, should lookup_backend return a
newly-locked dentry-inode info (and only if there was no error).  Document
this behavior.  This cleanup allowed us to remove two unnecessary int
declarations.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
Acked-by: Josef Sipek <jsipek@fsl.cs.sunysb.edu>
---
 fs/unionfs/inode.c  |    6 +++++-
 fs/unionfs/lookup.c |   38 +++++++++++++++++++-------------------
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c
index 687b9a7..9638b64 100644
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -163,7 +163,10 @@ static struct dentry *unionfs_lookup(struct inode *parent,
 		path_save.mnt = nd->mnt;
 	}
 
-	/* The locking is done by unionfs_lookup_backend. */
+	/*
+	 * unionfs_lookup_backend returns a locked dentry upon success,
+	 * so we'll have to unlock it below.
+	 */
 	ret = unionfs_lookup_backend(dentry, nd, INTERPOSE_LOOKUP);
 
 	/* restore the dentry & vfsmnt in namei */
@@ -176,6 +179,7 @@ static struct dentry *unionfs_lookup(struct inode *parent,
 			dentry = ret;
 		/* parent times may have changed */
 		unionfs_copy_attr_times(dentry->d_parent->d_inode);
+		unionfs_unlock_dentry(dentry);
 	}
 
 	unionfs_check_inode(parent);
diff --git a/fs/unionfs/lookup.c b/fs/unionfs/lookup.c
index 7fa6310..8eb9749 100644
--- a/fs/unionfs/lookup.c
+++ b/fs/unionfs/lookup.c
@@ -77,6 +77,11 @@ out:
  *
  * Returns: NULL (ok), ERR_PTR if an error occurred, or a non-null non-error
  * PTR if d_splice returned a different dentry.
+ *
+ * If lookupmode is INTERPOSE_PARTIAL/REVAL/REVAL_NEG, the passed dentry's
+ * inode info must be locked.  If lookupmode is INTERPOSE_LOOKUP (i.e., a
+ * newly looked-up dentry), then unionfs_lookup_backend will return a locked
+ * dentry's info, which the caller must unlock.
  */
 struct dentry *unionfs_lookup_backend(struct dentry *dentry,
 				      struct nameidata *nd, int lookupmode)
@@ -94,8 +99,6 @@ struct dentry *unionfs_lookup_backend(struct dentry *dentry,
 	struct dentry *first_lower_dentry = NULL;
 	struct vfsmount *first_lower_mnt = NULL;
 	int locked_parent = 0;
-	int locked_child = 0;
-	int allocated_new_info = 0;
 	int opaque;
 	char *whname = NULL;
 	const char *name;
@@ -109,24 +112,21 @@ struct dentry *unionfs_lookup_backend(struct dentry *dentry,
 	if (lookupmode == INTERPOSE_PARTIAL || lookupmode == INTERPOSE_REVAL ||
 	    lookupmode == INTERPOSE_REVAL_NEG)
 		verify_locked(dentry);
-	else {
+	else			/* this could only be INTERPOSE_LOOKUP */
 		BUG_ON(UNIONFS_D(dentry) != NULL);
-		locked_child = 1;
-	}
 
-	switch(lookupmode) {
-		case INTERPOSE_PARTIAL:
-			break;
-		case INTERPOSE_LOOKUP:
-			if ((err = new_dentry_private_data(dentry)))
-				goto out;
-			allocated_new_info = 1;
-			break;
-		default:
-			if ((err = realloc_dentry_private_data(dentry)))
-				goto out;
-			allocated_new_info = 1;
-			break;
+	switch (lookupmode) {
+	case INTERPOSE_PARTIAL:
+		break;
+	case INTERPOSE_LOOKUP:
+		if ((err = new_dentry_private_data(dentry)))
+			goto out;
+		break;
+	default:
+		/* default: can only be INTERPOSE_REVAL/REVAL_NEG */
+		if ((err = realloc_dentry_private_data(dentry)))
+			goto out;
+		break;
 	}
 
 	/* must initialize dentry operations */
@@ -419,7 +419,7 @@ out:
 	if (locked_parent)
 		unionfs_unlock_dentry(parent_dentry);
 	dput(parent_dentry);
-	if (locked_child || (err && allocated_new_info))
+	if (err && (lookupmode == INTERPOSE_LOOKUP))
 		unionfs_unlock_dentry(dentry);
 	if (!err && d_interposed)
 		return d_interposed;
-- 
1.5.2.2

[PATCH 6/6] Unionfs: cache coherency after lower objects are removed

[Erez Zadok]

Prevent an oops if a lower file is deleted and then it is stat'ed from the
upper layer.  Ensure that we return a negative dentry so the user will get
an ENOENT.  Properly dput/mntput so we don't leak references at the lower
file system.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
Acked-by: Josef Sipek <jsipek@fsl.cs.sunysb.edu>
---
 fs/unionfs/lookup.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/unionfs/lookup.c b/fs/unionfs/lookup.c
index 8eb9749..963d622 100644
--- a/fs/unionfs/lookup.c
+++ b/fs/unionfs/lookup.c
@@ -92,7 +92,7 @@ struct dentry *unionfs_lookup_backend(struct dentry *dentry,
 	struct dentry *lower_dir_dentry = NULL;
 	struct dentry *parent_dentry = NULL;
 	struct dentry *d_interposed = NULL;
-	int bindex, bstart, bend, bopaque;
+	int bindex, bstart = -1, bend, bopaque;
 	int dentry_count = 0;	/* Number of positive dentries. */
 	int first_dentry_offset = -1; /* -1 is uninitialized */
 	struct dentry *first_dentry = NULL;
@@ -413,7 +413,14 @@ out:
 	if (!err && UNIONFS_D(dentry)) {
 		BUG_ON(dbend(dentry) > UNIONFS_D(dentry)->bcount);
 		BUG_ON(dbend(dentry) > sbmax(dentry->d_sb));
-		BUG_ON(dbstart(dentry) < 0);
+		if (dbstart(dentry) < 0 &&
+		    dentry->d_inode && bstart >= 0 &&
+		    (!UNIONFS_I(dentry->d_inode) ||
+		     !UNIONFS_I(dentry->d_inode)->lower_inodes)) {
+			unionfs_mntput(dentry->d_sb->s_root, bstart);
+			dput(first_lower_dentry);
+			UNIONFS_I(dentry->d_inode)->stale = 1;
+		}
 	}
 	kfree(whname);
 	if (locked_parent)
@@ -423,6 +430,9 @@ out:
 		unionfs_unlock_dentry(dentry);
 	if (!err && d_interposed)
 		return d_interposed;
+	if (dentry->d_inode && UNIONFS_I(dentry->d_inode)->stale &&
+	    first_dentry_offset >= 0)
+		unionfs_mntput(dentry->d_sb->s_root, first_dentry_offset);
 	return ERR_PTR(err);
 }
 
-- 
1.5.2.2