[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@xxxxxxxxxxxxx>
---
 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/