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

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

[Erez Zadok]

The following is a series of patches related to Unionfs.  The main changes
here are bug fixes and improved cache-coherency methods.

These patches were tested (where appropriate) on Linus's 2.6.24 latest code
(as of v2.6.24-rc2-330-g325d22d), for the first time on MM
(mmotm-2007-11-10-19-05), as well as the backports to
2.6.{23,22,21,20,19,18,9} on ext2/3/4, xfs, reiserfs, nfs2/3/4, 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

to receive the following:

Erez Zadok (9):
      Unionfs: flush and release updates
      Unionfs: use i_size wrappers
      Unionfs: update cache-coherency detection heuristics
      Unionfs: writepage updates
      Unionfs: clear partial read in readpage
      Unionfs: debugging updates
      Unionfs: remove unnecessary lower atime updates
      Unionfs: fold do_readpage into unionfs_readpage
      Unionfs: move debugging checks inside locks

 Documentation/filesystems/unionfs/concepts.txt |   18 ++++++
 fs/unionfs/commonfops.c                        |   47 +++++++-----------
 fs/unionfs/debug.c                             |    2 
 fs/unionfs/dentry.c                            |   31 +++++++----
 fs/unionfs/file.c                              |   18 +-----
 fs/unionfs/inode.c                             |   29 +++++------
 fs/unionfs/mmap.c                              |   65 ++++++++-----------------
 fs/unionfs/rdstate.c                           |    4 -
 fs/unionfs/rename.c                            |    4 -
 fs/unionfs/super.c                             |    6 +-
 fs/unionfs/union.h                             |    5 +
 fs/unionfs/xattr.c                             |    8 +--
 12 files changed, 115 insertions(+), 122 deletions(-)

---
Erez Zadok
ezk@cs.sunysb.edu

[PATCH 1/9] Unionfs: flush and release updates

[Erez Zadok]

Remove the totalopens counter which was intended to reduce unnecessary
processing of d_deleted dentries.  Move that processing from file_release to
flush.

Cc: Hugh Dickins <hugh@veritas.com>

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/unionfs/commonfops.c |   30 +++++++++++-------------------
 fs/unionfs/union.h      |    2 --
 2 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index 50e5775..ba84529 100644
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -551,9 +551,6 @@ int unionfs_open(struct inode *inode, struct file *file)
 	bstart = fbstart(file) = dbstart(dentry);
 	bend = fbend(file) = dbend(dentry);
 
-	/* increment, so that we can flush appropriately */
-	atomic_inc(&UNIONFS_I(dentry->d_inode)->totalopens);
-
 	/*
 	 * open all directories and make the unionfs file struct point to
 	 * these lower file structs
@@ -565,7 +562,6 @@ int unionfs_open(struct inode *inode, struct file *file)
 
 	/* freeing the allocated resources, and fput the opened files */
 	if (err) {
-		atomic_dec(&UNIONFS_I(dentry->d_inode)->totalopens);
 		for (bindex = bstart; bindex <= bend; bindex++) {
 			lower_file = unionfs_lower_file_idx(file, bindex);
 			if (!lower_file)
@@ -606,6 +602,7 @@ int unionfs_file_release(struct inode *inode, struct file *file)
 	struct unionfs_file_info *fileinfo;
 	struct unionfs_inode_info *inodeinfo;
 	struct super_block *sb = inode->i_sb;
+	struct dentry *dentry = file->f_path.dentry;
 	int bindex, bstart, bend;
 	int fgen, err = 0;
 
@@ -628,6 +625,7 @@ int unionfs_file_release(struct inode *inode, struct file *file)
 	bstart = fbstart(file);
 	bend = fbend(file);
 
+	unionfs_lock_dentry(dentry);
 	for (bindex = bstart; bindex <= bend; bindex++) {
 		lower_file = unionfs_lower_file_idx(file, bindex);
 
@@ -635,7 +633,15 @@ int unionfs_file_release(struct inode *inode, struct file *file)
 			fput(lower_file);
 			branchput(sb, bindex);
 		}
+
+		/* if there are no more refs to the dentry, dput it */
+		if (d_deleted(dentry)) {
+			dput(unionfs_lower_dentry_idx(dentry, bindex));
+			unionfs_set_lower_dentry_idx(dentry, bindex, NULL);
+		}
 	}
+	unionfs_unlock_dentry(dentry);
+
 	kfree(fileinfo->lower_files);
 	kfree(fileinfo->saved_branch_ids);
 
@@ -799,11 +805,6 @@ int unionfs_flush(struct file *file, fl_owner_t id)
 		goto out;
 	unionfs_check_file(file);
 
-	if (!atomic_dec_and_test(&UNIONFS_I(dentry->d_inode)->totalopens))
-		goto out;
-
-	unionfs_lock_dentry(dentry);
-
 	bstart = fbstart(file);
 	bend = fbend(file);
 	for (bindex = bstart; bindex <= bend; bindex++) {
@@ -813,14 +814,7 @@ int unionfs_flush(struct file *file, fl_owner_t id)
 		    lower_file->f_op->flush) {
 			err = lower_file->f_op->flush(lower_file, id);
 			if (err)
-				goto out_lock;
-
-			/* if there are no more refs to the dentry, dput it */
-			if (d_deleted(dentry)) {
-				dput(unionfs_lower_dentry_idx(dentry, bindex));
-				unionfs_set_lower_dentry_idx(dentry, bindex,
-							     NULL);
-			}
+				goto out;
 		}
 
 	}
@@ -830,8 +824,6 @@ int unionfs_flush(struct file *file, fl_owner_t id)
 	/* parent time could have changed too (async) */
 	unionfs_copy_attr_times(dentry->d_parent->d_inode);
 
-out_lock:
-	unionfs_unlock_dentry(dentry);
 out:
 	unionfs_read_unlock(dentry->d_sb);
 	unionfs_check_file(file);
diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h
index 0476f97..09b9ac9 100644
--- a/fs/unionfs/union.h
+++ b/fs/unionfs/union.h
@@ -102,8 +102,6 @@ struct unionfs_inode_info {
 
 	/* The lower inodes */
 	struct inode **lower_inodes;
-	/* to keep track of reads/writes for unlinks before closes */
-	atomic_t totalopens;
 
 	struct inode vfs_inode;
 };
-- 
1.5.2.2

[PATCH 2/9] Unionfs: use i_size wrappers

[Erez Zadok]

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/unionfs/commonfops.c |   10 +++++-----
 fs/unionfs/inode.c      |    4 ++--
 fs/unionfs/rdstate.c    |    4 ++--
 fs/unionfs/rename.c     |    4 ++--
 fs/unionfs/super.c      |    2 +-
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index ba84529..624f920 100644
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -72,7 +72,8 @@ retry:
 	dput(tmp_dentry);
 
 	err = copyup_named_file(dentry->d_parent->d_inode, file, name, bstart,
-				bindex, file->f_path.dentry->d_inode->i_size);
+				bindex,
+				i_size_read(file->f_path.dentry->d_inode));
 	if (err) {
 		if (unlikely(err == -EEXIST))
 			goto retry;
@@ -199,7 +200,6 @@ static int open_highest_file(struct file *file, bool willwrite)
 	struct dentry *dentry = file->f_path.dentry;
 	struct inode *parent_inode = dentry->d_parent->d_inode;
 	struct super_block *sb = dentry->d_sb;
-	size_t inode_size = dentry->d_inode->i_size;
 
 	bstart = dbstart(dentry);
 	bend = dbend(dentry);
@@ -208,7 +208,7 @@ static int open_highest_file(struct file *file, bool willwrite)
 	if (willwrite && IS_WRITE_FLAG(file->f_flags) && is_robranch(dentry)) {
 		for (bindex = bstart - 1; bindex >= 0; bindex--) {
 			err = copyup_file(parent_inode, file, bstart, bindex,
-					  inode_size);
+					  i_size_read(dentry->d_inode));
 			if (!err)
 				break;
 		}
@@ -243,7 +243,6 @@ static int do_delayed_copyup(struct file *file)
 	int bindex, bstart, bend, err = 0;
 	struct dentry *dentry = file->f_path.dentry;
 	struct inode *parent_inode = dentry->d_parent->d_inode;
-	loff_t inode_size = dentry->d_inode->i_size;
 
 	bstart = fbstart(file);
 	bend = fbend(file);
@@ -255,7 +254,8 @@ static int do_delayed_copyup(struct file *file)
 	for (bindex = bstart - 1; bindex >= 0; bindex--) {
 		if (!d_deleted(dentry))
 			err = copyup_file(parent_inode, file, bstart,
-					  bindex, inode_size);
+					  bindex,
+					  i_size_read(dentry->d_inode));
 		else
 			err = copyup_deleted_file(file, dentry, bstart,
 						  bindex);
diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c
index bcefe45..223a64a 100644
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -312,7 +312,7 @@ docopyup:
 					    old_dentry, old_bstart,
 					    bindex, old_dentry->d_name.name,
 					    old_dentry->d_name.len, NULL,
-					    old_dentry->d_inode->i_size);
+					    i_size_read(old_dentry->d_inode));
 			if (!err) {
 				lower_new_dentry =
 					create_parents(dir, new_dentry,
@@ -1043,7 +1043,7 @@ static int unionfs_setattr(struct dentry *dentry, struct iattr *ia)
 				continue;
 			/* Only if its the leftmost file, copyup the file */
 			for (i = bstart - 1; i >= 0; i--) {
-				loff_t size = dentry->d_inode->i_size;
+				loff_t size = i_size_read(dentry->d_inode);
 				if (ia->ia_valid & ATTR_SIZE)
 					size = ia->ia_size;
 				err = copyup_dentry(dentry->d_parent->d_inode,
diff --git a/fs/unionfs/rdstate.c b/fs/unionfs/rdstate.c
index 0df5f52..bdf335d 100644
--- a/fs/unionfs/rdstate.c
+++ b/fs/unionfs/rdstate.c
@@ -76,10 +76,10 @@ static int guesstimate_hash_size(struct inode *inode)
 		if (!lower_inode)
 			continue;
 
-		if (lower_inode->i_size == DENTPAGE)
+		if (i_size_read(lower_inode) == DENTPAGE)
 			hashsize += DENTPERONEPAGE;
 		else
-			hashsize += (lower_inode->i_size / DENTPAGE) *
+			hashsize += (i_size_read(lower_inode) / DENTPAGE) *
 				DENTPERPAGE;
 	}
 
diff --git a/fs/unionfs/rename.c b/fs/unionfs/rename.c
index 4a35b5e..452d1e7 100644
--- a/fs/unionfs/rename.c
+++ b/fs/unionfs/rename.c
@@ -234,8 +234,8 @@ static int do_unionfs_rename(struct inode *old_dir,
 			err = copyup_dentry(old_dentry->d_parent->d_inode,
 					    old_dentry, old_bstart, bindex,
 					    old_dentry->d_name.name,
-					    old_dentry->d_name.len,
-					    NULL, old_dentry->d_inode->i_size);
+					    old_dentry->d_name.len, NULL,
+					    i_size_read(old_dentry->d_inode));
 			/* if copyup failed, try next branch to the left */
 			if (err)
 				continue;
diff --git a/fs/unionfs/super.c b/fs/unionfs/super.c
index 7d28045..70487bf 100644
--- a/fs/unionfs/super.c
+++ b/fs/unionfs/super.c
@@ -69,7 +69,7 @@ static void unionfs_read_inode(struct inode *inode)
  */
 static void unionfs_delete_inode(struct inode *inode)
 {
-	inode->i_size = 0;	/* every f/s seems to do that */
+	i_size_write(inode, 0);	/* every f/s seems to do that */
 
 	if (inode->i_data.nrpages)
 		truncate_inode_pages(&inode->i_data, 0);
-- 
1.5.2.2

[PATCH 3/9] Unionfs: update cache-coherency detection heuristics

[Erez Zadok]

Use a small delay to reduce the number of times unionfs has to detect
changed mtime's/ctime's, and also reduce the potential for false positives.
See Documentation/filesystems/unionfs/concepts.txt for a detailed
discussion.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 Documentation/filesystems/unionfs/concepts.txt |   18 ++++++++++++++
 fs/unionfs/dentry.c                            |   29 ++++++++++++++---------
 fs/unionfs/union.h                             |    3 ++
 3 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/Documentation/filesystems/unionfs/concepts.txt b/Documentation/filesystems/unionfs/concepts.txt
index 37a62d8..7654ccc 100644
--- a/Documentation/filesystems/unionfs/concepts.txt
+++ b/Documentation/filesystems/unionfs/concepts.txt
@@ -177,5 +177,23 @@ would be really nice if the VFS would export an optional file system hook
 ->file_revalidate (similarly to dentry->d_revalidate) that will be called
 before each VFS op that has a "struct file" in it.
 
+Certain file systems have micro-second granularity (or better) for inode
+times, and asynchronous actions could cause those times to change with some
+small delay.  In such cases, Unionfs may see a changed inode time that only
+differs by a tiny fraction of a second: such a change may be a false
+positive indication that the lower object has changed, whereas if unionfs
+waits a little longer, that false indication will not be seen.  (These false
+positives are harmless, because they would at most cause unionfs to
+re-validate an object that may need no revalidation, and print a debugging
+message that clutters the console/logs.)  Therefore, to minimize the chances
+of these situations, we delay the detection of changed times by a small
+factor of a few seconds, called UNIONFS_MIN_CC_TIME (which defaults to 3
+seconds, as does NFS).  This means that we will detect the change, only a
+couple of seconds later, if indeed the time change persists in the lower
+file object.  This delayed detection has an added performance benefit: we
+reduce the number of times that unionfs has to revalidate objects, in case
+there's a lot of concurrent activity on both the upper and lower objects,
+for the same file(s).  Lastly, this delayed time attribute detection is
+similar to how NFS clients operate (e.g., acregmin).
 
 For more information, see <http://unionfs.filesystems.org/>.
diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index a3d7b6e..aed4d39 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -192,6 +192,16 @@ out:
 /*
  * Determine if the lower inode objects have changed from below the unionfs
  * inode.  Return true if changed, false otherwise.
+ *
+ * We check if the mtime or ctime have changed.  However, the inode times
+ * can be changed by anyone without much protection, including
+ * asynchronously.  This can sometimes cause unionfs to find that the lower
+ * file system doesn't change its inode times quick enough, resulting in a
+ * false positive indication (which is harmless, it just makes unionfs do
+ * extra work in re-validating the objects).  To minimize the chances of
+ * these situations, we delay the detection of changed times by
+ * UNIONFS_MIN_CC_TIME (which defaults to 3 seconds, as with NFS's
+ * acregmin).
  */
 bool is_newer_lower(const struct dentry *dentry)
 {
@@ -211,26 +221,23 @@ bool is_newer_lower(const struct dentry *dentry)
 		lower_inode = unionfs_lower_inode_idx(inode, bindex);
 		if (!lower_inode)
 			continue;
-		/*
-		 * We may want to apply other tests to determine if the
-		 * lower inode's data has changed, but checking for changed
-		 * ctime and mtime on the lower inode should be enough.
-		 */
-		if (unlikely(timespec_compare(&inode->i_mtime,
-					      &lower_inode->i_mtime) < 0)) {
+
+		/* check if mtime/ctime have changed */
+		if (unlikely((lower_inode->i_mtime.tv_sec -
+			      inode->i_mtime.tv_sec) > UNIONFS_MIN_CC_TIME)) {
 			pr_info("unionfs: new lower inode mtime "
 				"(bindex=%d, name=%s)\n", bindex,
 				dentry->d_name.name);
 			show_dinode_times(dentry);
-			return true; /* mtime changed! */
+			return true;
 		}
-		if (unlikely(timespec_compare(&inode->i_ctime,
-					      &lower_inode->i_ctime) < 0)) {
+		if (unlikely((lower_inode->i_ctime.tv_sec -
+			      inode->i_ctime.tv_sec) > UNIONFS_MIN_CC_TIME)) {
 			pr_info("unionfs: new lower inode ctime "
 				"(bindex=%d, name=%s)\n", bindex,
 				dentry->d_name.name);
 			show_dinode_times(dentry);
-			return true; /* ctime changed! */
+			return true;
 		}
 	}
 	return false;		/* default: lower is not newer */
diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h
index 09b9ac9..f61e32d 100644
--- a/fs/unionfs/union.h
+++ b/fs/unionfs/union.h
@@ -63,6 +63,9 @@
 /* maximum number of branches we support, to avoid memory blowup */
 #define UNIONFS_MAX_BRANCHES	128
 
+/* minimum time (seconds) required for time-based cache-coherency */
+#define UNIONFS_MIN_CC_TIME	3
+
 /* Operations vectors defined in specific files. */
 extern struct file_operations unionfs_main_fops;
 extern struct file_operations unionfs_dir_fops;
-- 
1.5.2.2

[PATCH 4/9] Unionfs: writepage updates

[Erez Zadok]

Don't set/reset the PageUptodate flag on our page.  Call flush_dcache_page
on the lower page after copy_highpage, and set it uptodate.  Call
set_page_dirty right before clear_page_dirty_for_io.

CC: Hugh Dickins <hugh@veritas.com>
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/unionfs/mmap.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/unionfs/mmap.c b/fs/unionfs/mmap.c
index 4b00b98..468dc61 100644
--- a/fs/unionfs/mmap.c
+++ b/fs/unionfs/mmap.c
@@ -28,6 +28,7 @@ static int unionfs_writepage(struct page *page, struct writeback_control *wbc)
 	struct address_space *lower_mapping; /* lower inode mapping */
 	gfp_t mask;
 
+	BUG_ON(!PageUptodate(page));
 	inode = page->mapping->host;
 	lower_inode = unionfs_lower_inode(inode);
 	lower_mapping = lower_inode->i_mapping;
@@ -53,6 +54,8 @@ static int unionfs_writepage(struct page *page, struct writeback_control *wbc)
 
 	/* copy page data from our upper page to the lower page */
 	copy_highpage(lower_page, page);
+	flush_dcache_page(lower_page);
+	SetPageUptodate(lower_page);
 
 	/*
 	 * Call lower writepage (expects locked page).  However, if we are
@@ -68,12 +71,11 @@ static int unionfs_writepage(struct page *page, struct writeback_control *wbc)
 		goto out_release;
 	}
 	BUG_ON(!lower_mapping->a_ops->writepage);
+	set_page_dirty(lower_page);
 	clear_page_dirty_for_io(lower_page); /* emulate VFS behavior */
 	err = lower_mapping->a_ops->writepage(lower_page, wbc);
-	if (err < 0) {
-		ClearPageUptodate(page);
+	if (err < 0)
 		goto out_release;
-	}
 
 	/*
 	 * Lower file systems such as ramfs and tmpfs, may return
@@ -97,7 +99,7 @@ static int unionfs_writepage(struct page *page, struct writeback_control *wbc)
 	}
 
 	/* all is well */
-	SetPageUptodate(page);
+
 	/* lower mtimes have changed: update ours */
 	unionfs_copy_attr_times(inode);
 
-- 
1.5.2.2

[PATCH 5/9] Unionfs: clear partial read in readpage

[Erez Zadok]

Signed-off-by: Hugh Dickins <hugh@veritas.com>
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/unionfs/mmap.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/unionfs/mmap.c b/fs/unionfs/mmap.c
index 468dc61..bb00fd5 100644
--- a/fs/unionfs/mmap.c
+++ b/fs/unionfs/mmap.c
@@ -178,7 +178,8 @@ static int unionfs_do_readpage(struct file *file, struct page *page)
 	err = vfs_read(lower_file, page_data, PAGE_CACHE_SIZE,
 		       &lower_file->f_pos);
 	set_fs(old_fs);
-
+	if (err >= 0 && err < PAGE_CACHE_SIZE)
+		memset(page_data + err, 0, PAGE_CACHE_SIZE - err);
 	kunmap(page);
 
 	if (err < 0)
-- 
1.5.2.2

[PATCH 6/9] Unionfs: debugging updates

[Erez Zadok]

Don't perform dentry+inode checks unless both are valid.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/unionfs/debug.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/unionfs/debug.c b/fs/unionfs/debug.c
index 0066ccd..8464fbb 100644
--- a/fs/unionfs/debug.c
+++ b/fs/unionfs/debug.c
@@ -299,7 +299,7 @@ check_inode:
 	 * be NULL.  But, check that all three are NULL: lower dentry, mnt,
 	 * and inode.
 	 */
-	if (S_ISDIR(inode->i_mode))
+	if (dstart >= 0 && dend >= 0 && S_ISDIR(inode->i_mode))
 		for (bindex = dstart+1; bindex < dend; bindex++) {
 			lower_inode = unionfs_lower_inode_idx(inode, bindex);
 			lower_dentry = unionfs_lower_dentry_idx(dentry,
-- 
1.5.2.2

[PATCH 7/9] Unionfs: remove unnecessary lower atime updates

[Erez Zadok]

No need for this because our readpage calls vfs_read on the lower objects,
which would update the atime as/if needed.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/unionfs/file.c |    8 --------
 fs/unionfs/mmap.c |    6 ------
 2 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/fs/unionfs/file.c b/fs/unionfs/file.c
index 126df5e..809e0f1 100644
--- a/fs/unionfs/file.c
+++ b/fs/unionfs/file.c
@@ -31,10 +31,6 @@ static ssize_t unionfs_read(struct file *file, char __user *buf,
 
 	err = do_sync_read(file, buf, count, ppos);
 
-	if (err >= 0)
-		touch_atime(unionfs_lower_mnt(file->f_path.dentry),
-			    unionfs_lower_dentry(file->f_path.dentry));
-
 out:
 	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
@@ -58,10 +54,6 @@ static ssize_t unionfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
 	if (err == -EIOCBQUEUED)
 		err = wait_on_sync_kiocb(iocb);
 
-	if (err >= 0)
-		touch_atime(unionfs_lower_mnt(file->f_path.dentry),
-			    unionfs_lower_dentry(file->f_path.dentry));
-
 out:
 	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
diff --git a/fs/unionfs/mmap.c b/fs/unionfs/mmap.c
index bb00fd5..34fd8aa 100644
--- a/fs/unionfs/mmap.c
+++ b/fs/unionfs/mmap.c
@@ -212,12 +212,6 @@ static int unionfs_readpage(struct file *file, struct page *page)
 
 	err = unionfs_do_readpage(file, page);
 
-	if (!err) {
-		touch_atime(unionfs_lower_mnt(file->f_path.dentry),
-			    unionfs_lower_dentry(file->f_path.dentry));
-		unionfs_copy_attr_times(file->f_path.dentry->d_inode);
-	}
-
 	/*
 	 * we have to unlock our page, b/c we _might_ have gotten a locked
 	 * page.  but we no longer have to wakeup on our page here, b/c
-- 
1.5.2.2

[PATCH 8/9] Unionfs: fold do_readpage into unionfs_readpage

[Erez Zadok]

Simplify the code and reduce stack pressure a bit.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/unionfs/mmap.c |   44 ++++++++++++++------------------------------
 1 files changed, 14 insertions(+), 30 deletions(-)

diff --git a/fs/unionfs/mmap.c b/fs/unionfs/mmap.c
index 34fd8aa..ef8822f 100644
--- a/fs/unionfs/mmap.c
+++ b/fs/unionfs/mmap.c
@@ -136,22 +136,22 @@ out:
 	return err;
 }
 
-/*
- * readpage is called from generic_page_read and the fault handler.
- * If your file system uses generic_page_read for the read op, it
- * must implement readpage.
- *
- * Readpage expects a locked page, and must unlock it.
- */
-static int unionfs_do_readpage(struct file *file, struct page *page)
+/* Readpage expects a locked page, and must unlock it */
+static int unionfs_readpage(struct file *file, struct page *page)
 {
-	int err = -EIO;
+	int err;
 	struct file *lower_file;
 	struct inode *inode;
 	mm_segment_t old_fs;
 	char *page_data = NULL;
 	loff_t offset;
 
+	unionfs_read_lock(file->f_path.dentry->d_sb);
+	err = unionfs_file_revalidate(file, false);
+	if (unlikely(err))
+		goto out;
+	unionfs_check_file(file);
+
 	if (!UNIONFS_F(file)) {
 		err = -ENOENT;
 		goto out;
@@ -191,33 +191,17 @@ static int unionfs_do_readpage(struct file *file, struct page *page)
 
 	flush_dcache_page(page);
 
-out:
-	if (err == 0)
-		SetPageUptodate(page);
-	else
-		ClearPageUptodate(page);
-
-	return err;
-}
-
-static int unionfs_readpage(struct file *file, struct page *page)
-{
-	int err;
-
-	unionfs_read_lock(file->f_path.dentry->d_sb);
-	err = unionfs_file_revalidate(file, false);
-	if (unlikely(err))
-		goto out;
-	unionfs_check_file(file);
-
-	err = unionfs_do_readpage(file, page);
-
 	/*
 	 * we have to unlock our page, b/c we _might_ have gotten a locked
 	 * page.  but we no longer have to wakeup on our page here, b/c
 	 * UnlockPage does it
 	 */
 out:
+	if (err == 0)
+		SetPageUptodate(page);
+	else
+		ClearPageUptodate(page);
+
 	unlock_page(page);
 	unionfs_check_file(file);
 	unionfs_read_unlock(file->f_path.dentry->d_sb);
-- 
1.5.2.2

[PATCH 9/9] Unionfs: move debugging checks inside locks

[Erez Zadok]

This is to ensure that the objects we want to check aren't being destroyed
or changed by another thread.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/unionfs/commonfops.c |    7 ++++---
 fs/unionfs/dentry.c     |    2 +-
 fs/unionfs/file.c       |   10 +++++-----
 fs/unionfs/inode.c      |   25 +++++++++++++------------
 fs/unionfs/mmap.c       |    2 +-
 fs/unionfs/super.c      |    4 ++--
 fs/unionfs/xattr.c      |    8 ++++----
 7 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index 624f920..b33917e 100644
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -573,6 +573,7 @@ int unionfs_open(struct inode *inode, struct file *file)
 		}
 	}
 
+	/* XXX: should this unlock be moved to the function bottom? */
 	unionfs_unlock_dentry(dentry);
 
 out:
@@ -582,12 +583,12 @@ out:
 		kfree(UNIONFS_F(file));
 	}
 out_nofree:
-	unionfs_read_unlock(inode->i_sb);
 	unionfs_check_inode(inode);
 	if (!err) {
 		unionfs_check_file(file);
 		unionfs_check_dentry(file->f_path.dentry->d_parent);
 	}
+	unionfs_read_unlock(inode->i_sb);
 	return err;
 }
 
@@ -786,8 +787,8 @@ long unionfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	}
 
 out:
-	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	return err;
 }
 
@@ -825,7 +826,7 @@ int unionfs_flush(struct file *file, fl_owner_t id)
 	unionfs_copy_attr_times(dentry->d_parent->d_inode);
 
 out:
-	unionfs_read_unlock(dentry->d_sb);
 	unionfs_check_file(file);
+	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index aed4d39..edea1a4 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -433,11 +433,11 @@ 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);
 	if (likely(err > 0)) { /* true==1: dentry is valid */
 		unionfs_check_dentry(dentry);
 		unionfs_check_nd(nd);
 	}
+	unionfs_unlock_dentry(dentry);
 
 	unionfs_read_unlock(dentry->d_sb);
 
diff --git a/fs/unionfs/file.c b/fs/unionfs/file.c
index 809e0f1..cfbfb8e 100644
--- a/fs/unionfs/file.c
+++ b/fs/unionfs/file.c
@@ -32,8 +32,8 @@ static ssize_t unionfs_read(struct file *file, char __user *buf,
 	err = do_sync_read(file, buf, count, ppos);
 
 out:
-	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	return err;
 }
 
@@ -55,8 +55,8 @@ static ssize_t unionfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
 		err = wait_on_sync_kiocb(iocb);
 
 out:
-	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	return err;
 }
 
@@ -127,13 +127,13 @@ static int unionfs_mmap(struct file *file, struct vm_area_struct *vma)
 	}
 
 out:
-	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	if (!err) {
 		/* copyup could cause parent dir times to change */
 		unionfs_copy_attr_times(file->f_path.dentry->d_parent->d_inode);
 		unionfs_check_file(file);
 		unionfs_check_dentry(file->f_path.dentry->d_parent);
 	}
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	return err;
 }
 
@@ -180,8 +180,8 @@ int unionfs_fsync(struct file *file, struct dentry *dentry, int datasync)
 	unionfs_copy_attr_times(inode);
 
 out:
-	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	return err;
 }
 
@@ -226,8 +226,8 @@ int unionfs_fasync(int fd, struct file *file, int flag)
 	unionfs_copy_attr_times(inode);
 
 out:
-	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	return err;
 }
 
diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c
index 223a64a..1708f40 100644
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -158,8 +158,6 @@ out:
 
 	if (!err)
 		unionfs_postcopyup_setmnt(dentry);
-	unionfs_unlock_dentry(dentry);
-	unionfs_read_unlock(dentry->d_sb);
 
 	unionfs_check_inode(parent);
 	if (!err) {
@@ -167,6 +165,8 @@ out:
 		unionfs_check_nd(nd);
 	}
 	unionfs_check_dentry(dentry);
+	unionfs_unlock_dentry(dentry);
+	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
 
@@ -206,13 +206,14 @@ 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);
 	unionfs_check_dentry(dentry);
 	unionfs_check_dentry(dentry->d_parent);
 	unionfs_check_nd(nd);
+	if (!IS_ERR(ret))
+		unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 
 	return ret;
@@ -356,12 +357,12 @@ out:
 	if (!err)
 		unionfs_postcopyup_setmnt(new_dentry);
 
-	unionfs_unlock_dentry(new_dentry);
-	unionfs_unlock_dentry(old_dentry);
-
 	unionfs_check_inode(dir);
 	unionfs_check_dentry(new_dentry);
 	unionfs_check_dentry(old_dentry);
+
+	unionfs_unlock_dentry(new_dentry);
+	unionfs_unlock_dentry(old_dentry);
 	unionfs_read_unlock(old_dentry->d_sb);
 
 	return err;
@@ -522,10 +523,10 @@ out:
 	kfree(name);
 	if (!err)
 		unionfs_postcopyup_setmnt(dentry);
-	unionfs_unlock_dentry(dentry);
 
 	unionfs_check_inode(dir);
 	unionfs_check_dentry(dentry);
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 
 	return err;
@@ -675,9 +676,9 @@ out:
 
 	if (!err)
 		unionfs_copy_attr_times(dentry->d_inode);
-	unionfs_unlock_dentry(dentry);
 	unionfs_check_inode(parent);
 	unionfs_check_dentry(dentry);
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 
 	return err;
@@ -805,10 +806,10 @@ out:
 
 	if (!err)
 		unionfs_postcopyup_setmnt(dentry);
-	unionfs_unlock_dentry(dentry);
 
 	unionfs_check_inode(dir);
 	unionfs_check_dentry(dentry);
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 
 	return err;
@@ -843,8 +844,8 @@ static int unionfs_readlink(struct dentry *dentry, char __user *buf,
 					lower_dentry->d_inode);
 
 out:
-	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 
 	return err;
@@ -906,11 +907,11 @@ static void unionfs_put_link(struct dentry *dentry, struct nameidata *nd,
 	if (unlikely(!__unionfs_d_revalidate_chain(dentry, nd, false)))
 		printk(KERN_ERR
 		       "unionfs: put_link failed to revalidate dentry\n");
-	unionfs_unlock_dentry(dentry);
 
 	unionfs_check_dentry(dentry);
 	unionfs_check_nd(nd);
 	kfree(nd_get_link(nd));
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 }
 
@@ -1090,9 +1091,9 @@ static int unionfs_setattr(struct dentry *dentry, struct iattr *ia)
 	/* if setattr succeeded, then parent dir may have changed */
 	unionfs_copy_attr_times(dentry->d_parent->d_inode);
 out:
-	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
 	unionfs_check_dentry(dentry->d_parent);
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 
 	return err;
diff --git a/fs/unionfs/mmap.c b/fs/unionfs/mmap.c
index ef8822f..4918f77 100644
--- a/fs/unionfs/mmap.c
+++ b/fs/unionfs/mmap.c
@@ -296,8 +296,8 @@ out:
 	if (err < 0)
 		ClearPageUptodate(page);
 
-	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	unionfs_check_file(file);
+	unionfs_read_unlock(file->f_path.dentry->d_sb);
 	return err;		/* assume all is ok */
 }
 
diff --git a/fs/unionfs/super.c b/fs/unionfs/super.c
index 70487bf..88f77d7 100644
--- a/fs/unionfs/super.c
+++ b/fs/unionfs/super.c
@@ -153,8 +153,8 @@ static int unionfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	memset(&buf->f_spare, 0, sizeof(buf->f_spare));
 
 out:
-	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(sb);
 	return err;
 }
@@ -797,8 +797,8 @@ out_free:
 	kfree(new_data);
 	kfree(new_lower_inodes);
 out_error:
-	unionfs_write_unlock(sb);
 	unionfs_check_dentry(sb->s_root);
+	unionfs_write_unlock(sb);
 	return err;
 }
 
diff --git a/fs/unionfs/xattr.c b/fs/unionfs/xattr.c
index 602cedf..00c6d0d 100644
--- a/fs/unionfs/xattr.c
+++ b/fs/unionfs/xattr.c
@@ -58,8 +58,8 @@ ssize_t unionfs_getxattr(struct dentry *dentry, const char *name, void *value,
 	err = vfs_getxattr(lower_dentry, (char *) name, value, size);
 
 out:
-	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
@@ -88,8 +88,8 @@ int unionfs_setxattr(struct dentry *dentry, const char *name,
 			   size, flags);
 
 out:
-	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
@@ -116,8 +116,8 @@ int unionfs_removexattr(struct dentry *dentry, const char *name)
 	err = vfs_removexattr(lower_dentry, (char *) name);
 
 out:
-	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
@@ -146,8 +146,8 @@ ssize_t unionfs_listxattr(struct dentry *dentry, char *list, size_t size)
 	err = vfs_listxattr(lower_dentry, encoded_list, size);
 
 out:
-	unionfs_unlock_dentry(dentry);
 	unionfs_check_dentry(dentry);
+	unionfs_unlock_dentry(dentry);
 	unionfs_read_unlock(dentry->d_sb);
 	return err;
 }
-- 
1.5.2.2