Unionfs: update cache-coherency detection heuristics

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>

diff --git a/Documentation/filesystems/unionfs/concepts.txt b/Documentation/filesystems/unionfs/concepts.txt
index 37a62d858f52f5fca7260909f360ea896dcd6b28..7654ccca1343628fce1fd0d57b8c5435bd79a575 100644 (file)
--- 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 355e92d9da4c8c6de3efd8c0fd1df55587eacd12..889d5a081e351383e482d11f2f34c8e6d33183b6 100644 (file)
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -185,6 +185,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)
 {
@@ -204,26 +214,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 aad38c52d7dcdbb7fe1bde71ed7f684cd9ad5e3c..a7dcdff3b1fe6f16582384a51fe367dab6d56322 100644 (file)
--- 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;