Merge branch 'jc/racy'
authorJunio C Hamano <junkio@cox.net>
Wed, 16 Aug 2006 21:00:34 +0000 (14:00 -0700)
committerJunio C Hamano <junkio@cox.net>
Wed, 16 Aug 2006 21:00:34 +0000 (14:00 -0700)
* jc/racy:
Remove the "delay writing to avoid runtime penalty of racy-git avoidance"
Add check program "git-check-racy"
Documentation/technical/racy-git.txt
avoid nanosleep(2)

Documentation/technical/racy-git.txt [new file with mode: 0644]
Makefile
check-racy.c [new file with mode: 0644]
read-cache.c
diff --git a/Documentation/technical/racy-git.txt b/Documentation/technical/racy-git.txt
new file mode 100644 (file)
index 0000000..7597d04
--- /dev/null
@@ -0,0 +1,193 @@
+Use of index and Racy git problem
+=================================
+
+Background
+----------
+
+The index is one of the most important data structure in git.
+It represents a virtual working tree state by recording list of
+paths and their object names and serves as a staging area to
+write out the next tree object to be committed.  The state is
+"virtual" in the sense that it does not necessarily have to, and
+often does not, match the files in the working tree.
+
+There are cases git needs to examine the differences between the
+virtual working tree state in the index and the files in the
+working tree.  The most obvious case is when the user asks `git
+diff` (or its low level implementation, `git diff-files`) or
+`git-ls-files --modified`.  In addition, git internally checks
+if the files in the working tree is different from what are
+recorded in the index to avoid stomping on local changes in them
+during patch application, switching branches, and merging.
+
+In order to speed up this comparison between the files in the
+working tree and the index entries, the index entries record the
+information obtained from the filesystem via `lstat(2)` system
+call when they were last updated.  When checking if they differ,
+git first runs `lstat(2)` on the files and compare the result
+with this information (this is what was originally done by the
+`ce_match_stat()` function, which the current code does in
+`ce_match_stat_basic()` function).  If some of these "cached
+stat information" fields do not match, git can tell that the
+files are modified without even looking at their contents.
+
+Note: not all members in `struct stat` obtained via `lstat(2)`
+are used for this comparison.  For example, `st_atime` obviously
+is not useful.  Currently, git compares the file type (regular
+files vs symbolic links) and executable bits (only for regular
+files) from `st_mode` member, `st_mtime` and `st_ctime`
+timestamps, `st_uid`, `st_gid`, `st_ino`, and `st_size` members.
+With a `USE_STDEV` compile-time option, `st_dev` is also
+compared, but this is not enabled by default because this member
+is not stable on network filesystems.  With `USE_NSEC`
+compile-time option, `st_mtim.tv_nsec` and `st_ctim.tv_nsec`
+members are also compared, but this is not enabled by default
+because the value of this member becomes meaningless once the
+inode is evicted from the inode cache on filesystems that do not
+store it on disk.
+
+
+Racy git
+--------
+
+There is one slight problem with the optimization based on the
+cached stat information.  Consider this sequence:
+
+  $ git update-index 'foo'
+  : modify 'foo' in-place without changing its size
+
+The first `update-index` computes the object name of the
+contents of file `foo` and updates the index entry for `foo`
+along with the `struct stat` information.  If the modification
+that follows it happens very fast so that the file's `st_mtime`
+timestamp does not change, after this sequence, the cached stat
+information the index entry records still exactly match what you
+can obtain from the filesystem, but the file `foo` is modified.
+This way, git can incorrectly think files in the working tree
+are unmodified even though they actually are.  This is called
+the "racy git" problem (discovered by Pasky), and the entries
+that appear clean when they may not be because of this problem
+are called "racily clean".
+
+To avoid this problem, git does two things:
+
+. When the cached stat information says the file has not been
+  modified, and the `st_mtime` is the same as (or newer than)
+  the timestamp of the index file itself (which is the time `git
+  update-index foo` finished running in the above example), it
+  also compares the contents with the object registered in the
+  index entry to make sure they match.
+
+. When the index file is updated that contains racily clean
+  entries, cached `st_size` information is truncated to zero
+  before writing a new version of the index file.
+
+Because the index file itself is written after collecting all
+the stat information from updated paths, `st_mtime` timestamp of
+it is usually the same as or newer than any of the paths the
+index contains.  And no matter how quick the modification that
+follows `git update-index foo` finishes, the resulting
+`st_mtime` timestamp on `foo` cannot get the timestamp earlier
+than the index file.  Therefore, index entries that can be
+racily clean are limited to the ones that have the same
+timestamp as the index file itself.
+
+The callers that want to check if an index entry matches the
+corresponding file in the working tree continue to call
+`ce_match_stat()`, but with this change, `ce_match_stat()` uses
+`ce_modified_check_fs()` to see if racily clean ones are
+actually clean after comparing the cached stat information using
+`ce_match_stat_basic()`.
+
+The problem the latter solves is this sequence:
+
+  $ git update-index 'foo'
+  : modify 'foo' in-place without changing its size
+  : wait for enough time
+  $ git update-index 'bar'
+
+Without the latter, the timestamp of the index file gets a newer
+value, and falsely clean entry `foo` would not be caught by the
+timestamp comparison check done with the former logic anymore.
+The latter makes sure that the cached stat information for `foo`
+would never match with the file in the working tree, so later
+checks by `ce_match_stat_basic()` would report the index entry
+does not match the file and git does not have to fall back on more
+expensive `ce_modified_check_fs()`.
+
+
+Runtime penalty
+---------------
+
+The runtime penalty of falling back to `ce_modified_check_fs()`
+from `ce_match_stat()` can be very expensive when there are many
+racily clean entries.  An obvious way to artificially create
+this situation is to give the same timestamp to all the files in
+the working tree in a large project, run `git update-index` on
+them, and give the same timestamp to the index file:
+
+  $ date >.datestamp
+  $ git ls-files | xargs touch -r .datestamp
+  $ git ls-files | git update-index --stdin
+  $ touch -r .datestamp .git/index
+
+This will make all index entries racily clean.  The linux-2.6
+project, for example, there are over 20,000 files in the working
+tree.  On my Athron 64X2 3800+, after the above:
+
+  $ /usr/bin/time git diff-files
+  1.68user 0.54system 0:02.22elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
+  0inputs+0outputs (0major+67111minor)pagefaults 0swaps
+  $ git update-index MAINTAINERS
+  $ /usr/bin/time git diff-files
+  0.02user 0.12system 0:00.14elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
+  0inputs+0outputs (0major+935minor)pagefaults 0swaps
+
+Running `git update-index` in the middle checked the racily
+clean entries, and left the cached `st_mtime` for all the paths
+intact because they were actually clean (so this step took about
+the same amount of time as the first `git diff-files`).  After
+that, they are not racily clean anymore but are truly clean, so
+the second invocation of `git diff-files` fully took advantage
+of the cached stat information.
+
+
+Avoiding runtime penalty
+------------------------
+
+In order to avoid the above runtime penalty, the recent "master"
+branch (post 1.4.2) has a code that makes sure the index file
+gets timestamp newer than the youngest files in the index when
+there are many young files with the same timestamp as the
+resulting index file would otherwise would have by waiting
+before finishing writing the index file out.
+
+I suspect that in practice the situation where many paths in the
+index are all racily clean is quite rare.  The only code paths
+that can record recent timestamp for large number of paths I
+know of are:
+
+. Initial `git add .` of a large project.
+
+. `git checkout` of a large project from an empty index into an
+  unpopulated working tree.
+
+Note: switching branches with `git checkout` keeps the cached
+stat information of existing working tree files that are the
+same between the current branch and the new branch, which are
+all older than the resulting index file, and they will not
+become racily clean.  Only the files that are actually checked
+out can become racily clean.
+
+In a large project where raciness avoidance cost really matters,
+however, the initial computation of all object names in the
+index takes more than one second, and the index file is written
+out after all that happens.  Therefore the timestamp of the
+index file will be more than one seconds later than the the
+youngest file in the working tree.  This means that in these
+cases there actually will not be any racily clean entry in
+the resulting index.
+
+So in summary I think we should not worry about avoiding the
+runtime penalty and get rid of the "wait before finishing
+writing" code out.
index 56a915e13f6304bc1a6781eafc8182b5b5b7cde6..23cd8a017ba05986c309c605ae06fba6a98e5c07 100644 (file)
--- a/Makefile
+++ b/Makefile
@@ -195,7 +195,11 @@ PROGRAMS = \
        git-update-server-info$X \
        git-upload-pack$X git-verify-pack$X \
        git-pack-redundant$X git-var$X \
-       git-describe$X git-merge-tree$X git-blame$X git-imap-send$X
+       git-describe$X git-merge-tree$X git-blame$X git-imap-send$X \
+       $(EXTRA_PROGRAMS)
+
+# Empty...
+EXTRA_PROGRAMS =
 
 BUILT_INS = \
        git-format-patch$X git-show$X git-whatchanged$X \
diff --git a/check-racy.c b/check-racy.c
new file mode 100644 (file)
index 0000000..d6a08b4
--- /dev/null
@@ -0,0 +1,28 @@
+#include "cache.h"
+
+int main(int ac, char **av)
+{
+       int i;
+       int dirty, clean, racy;
+
+       dirty = clean = racy = 0;
+       read_cache();
+       for (i = 0; i < active_nr; i++) {
+               struct cache_entry *ce = active_cache[i];
+               struct stat st;
+
+               if (lstat(ce->name, &st)) {
+                       error("lstat(%s): %s", ce->name, strerror(errno));
+                       continue;
+               }
+
+               if (ce_match_stat(ce, &st, 0))
+                       dirty++;
+               else if (ce_match_stat(ce, &st, 2))
+                       racy++;
+               else
+                       clean++;
+       }
+       printf("dirty %d, clean %d, racy %d\n", dirty, clean, racy);
+       return 0;
+}
index 3228ffb3001fa360dbd4f5a0907fd0b204382423..6bec833eecae934af1dce18683c70522481b002a 100644 (file)
@@ -5,7 +5,6 @@
  */
 #include "cache.h"
 #include "cache-tree.h"
-#include <time.h>
 
 /* Index extensions.
  *
@@ -170,9 +169,11 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
        return changed;
 }
 
-int ce_match_stat(struct cache_entry *ce, struct stat *st, int ignore_valid)
+int ce_match_stat(struct cache_entry *ce, struct stat *st, int options)
 {
        unsigned int changed;
+       int ignore_valid = options & 01;
+       int assume_racy_is_modified = options & 02;
 
        /*
         * If it's marked as always valid in the index, it's
@@ -201,8 +202,12 @@ int ce_match_stat(struct cache_entry *ce, struct stat *st, int ignore_valid)
         */
        if (!changed &&
            index_file_timestamp &&
-           index_file_timestamp <= ntohl(ce->ce_mtime.sec))
-               changed |= ce_modified_check_fs(ce, st);
+           index_file_timestamp <= ntohl(ce->ce_mtime.sec)) {
+               if (assume_racy_is_modified)
+                       changed |= DATA_CHANGED;
+               else
+                       changed |= ce_modified_check_fs(ce, st);
+       }
 
        return changed;
 }
@@ -954,9 +959,7 @@ int write_cache(int newfd, struct cache_entry **cache, int entries)
 {
        SHA_CTX c;
        struct cache_header hdr;
-       int i, removed, recent;
-       struct stat st;
-       time_t now;
+       int i, removed;
 
        for (i = removed = 0; i < entries; i++)
                if (!cache[i]->ce_mode)
@@ -994,57 +997,5 @@ int write_cache(int newfd, struct cache_entry **cache, int entries)
                        return -1;
                }
        }
-
-       /*
-        * To prevent later ce_match_stat() from always falling into
-        * check_fs(), if we have too many entries that can trigger
-        * racily clean check, we are better off delaying the return.
-        * We arbitrarily say if more than 20 paths or 25% of total
-        * paths are very new, we delay the return until the index
-        * file gets a new timestamp.
-        *
-        * NOTE! NOTE! NOTE!
-        *
-        * This assumes that nobody is touching the working tree while
-        * we are updating the index.
-        */
-
-       /* Make sure that the new index file has st_mtime
-        * that is current enough -- ce_write() batches the data
-        * so it might not have written anything yet.
-        */
-       ce_write_flush(&c, newfd);
-
-       now = fstat(newfd, &st) ? 0 : st.st_mtime;
-       if (now) {
-               recent = 0;
-               for (i = 0; i < entries; i++) {
-                       struct cache_entry *ce = cache[i];
-                       time_t entry_time = (time_t) ntohl(ce->ce_mtime.sec);
-                       if (!ce->ce_mode)
-                               continue;
-                       if (now && now <= entry_time)
-                               recent++;
-               }
-               if (20 < recent && entries <= recent * 4) {
-#if 0
-                       fprintf(stderr, "entries    %d\n", entries);
-                       fprintf(stderr, "recent     %d\n", recent);
-                       fprintf(stderr, "now        %lu\n", now);
-#endif
-                       while (!fstat(newfd, &st) && st.st_mtime <= now) {
-                               struct timespec rq, rm;
-                               off_t where = lseek(newfd, 0, SEEK_CUR);
-                               rq.tv_sec = 0;
-                               rq.tv_nsec = 250000000;
-                               nanosleep(&rq, &rm);
-                               if ((where == (off_t) -1) ||
-                                   (write(newfd, "", 1) != 1) ||
-                                   (lseek(newfd, -1, SEEK_CUR) != where) ||
-                                   ftruncate(newfd, where))
-                                       break;
-                       }
-               }
-       }
        return ce_flush(&c, newfd);
 }