# Fix double "close()" in ce_compare_data 8 messages from 2006-07-31 to 2006-08-08. Participants: Linus Torvalds, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/5059 ## Linus Torvalds, 2006-07-31 16:55 Subject: Fix double "close()" in ce_compare_data Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0607310945490.4168%40g5.osdl.org ``` Doing an "strace" on "git diff" shows that we close() a file descriptor twice (getting EBADFD on the second one) when we end up in ce_compare_data if the index does not match the checked-out stat information. The "index_fd()" function will already have closed the fd for us, so we should not close it again. Signed-off-by: Linus Torvalds --- The way I found this also showed a potential performance problem: if you do a "git reset --hard" (or similar) after you have changes in your tree, it will write the index file with the same timestamp as the checked out files that it re-wrote. That will also then forever afterwards (well, until the next "git update-index --refresh") cause the "uncommon" timestamp case in ce_match_stat(), where we check the index-file timestamp against the timestamp of the stat data, to trigger. Not very good. The "ce_modified_check_fs()" tests can be quite expensive if you have lots of those files because we end up then calling the "ce_compare_data()" function a lot. And suddenly "git diff" doesn't take a tenth of a second any more. We should really try to have some way to re-generate the index automatically when this case triggers, so that we only need to do it _once_ rather than keep doing it forever while the index is "potentially stale". Any ideas? diff --git a/read-cache.c b/read-cache.c index c0b0313..f92cdaa 100644 --- a/read-cache.c +++ b/read-cache.c @@ -61,7 +61,7 @@ static int ce_compare_data(struct cache_ unsigned char sha1[20]; if (!index_fd(sha1, fd, st, 0, NULL)) match = memcmp(sha1, ce->sha1, 20); - close(fd); + /* index_fd() closed the file descriptor already */ } return match; } ``` ## Junio C Hamano, 2006-07-31 19:05 Subject: Re: Fix double "close()" in ce_compare_data Message-ID: <7vlkq961ea.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vlkq961ea.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds writes: > The way I found this also showed a potential performance problem: if you > do a "git reset --hard" (or similar) after you have changes in your tree, > it will write the index file with the same timestamp as the checked out > files that it re-wrote. >... > We should really try to have some way to re-generate the index > automatically when this case triggers, so that we only need to do it > _once_ rather than keep doing it forever while the index is "potentially > stale". > > Any ideas? Just before writing the index out, we could do a gettimeofday() and wait for 1 second (or two if you are on FAT) if we have many many paths that have the same timestamp. Ugly and would not work on NFS I suspect. We could do gratuitous "update-index --refresh" but that is uglier especially if done at the core level; the caller does not expect the index file to be updated. Or we could remove the check and tell the user not to touch the working tree after calling update-index (I am not seriously suggesting this). ``` ## Junio C Hamano, 2006-08-05 11:20 Subject: [PATCH] Racy git: avoid having to be always too careful Message-ID: <7virl7e8ed.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7virl7e8ed.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Immediately after a bulk checkout, most of the paths in the working tree would have the same timestamp as the index file, and this would force ce_match_stat() to take slow path for all of them. When writing an index file out, if many of the paths have very new (read: the same timestamp as the index file being written out) timestamp, we are better off delaying the return from the command, to make sure that later command to touch the working tree files will leave newer timestamps than recorded in the index, thereby avoiding to take the slow path. Signed-off-by: Junio C Hamano --- Linus Torvalds writes: > Doing an "strace" on "git diff" shows that we close() a file descriptor > twice (getting EBADFD on the second one) when we end up in ce_compare_data > if the index does not match the checked-out stat information. > > The "index_fd()" function will already have closed the fd for us, so we > should not close it again. > > Signed-off-by: Linus Torvalds > --- > > The way I found this also showed a potential performance problem: if you > do a "git reset --hard" (or similar) after you have changes in your tree, > it will write the index file with the same timestamp as the checked out > files that it re-wrote. > > That will also then forever afterwards (well, until the next "git > update-index --refresh") cause the "uncommon" timestamp case in > ce_match_stat(), where we check the index-file timestamp against the > timestamp of the stat data, to trigger. > > Not very good. The "ce_modified_check_fs()" tests can be quite expensive > if you have lots of those files because we end up then calling the > "ce_compare_data()" function a lot. And suddenly "git diff" doesn't take a > tenth of a second any more. > > We should really try to have some way to re-generate the index > automatically when this case triggers, so that we only need to do it > _once_ rather than keep doing it forever while the index is "potentially > stale". > > Any ideas? Here is what I came up with. I am not very happy with its magic, but should be better than nothing. read-cache.c | 44 ++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 40 insertions(+), 4 deletions(-) diff --git a/read-cache.c b/read-cache.c index f92cdaa..ce76c20 100644 --- a/read-cache.c +++ b/read-cache.c @@ -5,6 +5,7 @@ */ #include "cache.h" #include "cache-tree.h" +#include /* Index extensions. * @@ -923,7 +924,7 @@ static void ce_smudge_racily_clean_entry * $ echo filfre >nitfol * $ git-update-index --add nitfol * - * but it does not. Whe the second update-index runs, + * but it does not. When the second update-index runs, * it notices that the entry "frotz" has the same timestamp * as index, and if we were to smudge it by resetting its * size to zero here, then the object name recorded @@ -945,7 +946,9 @@ int write_cache(int newfd, struct cache_ { SHA_CTX c; struct cache_header hdr; - int i, removed; + int i, removed, recent; + struct stat st; + time_t now; for (i = removed = 0; i < entries; i++) if (!cache[i]->ce_mode) @@ -959,15 +962,19 @@ int write_cache(int newfd, struct cache_ if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0) return -1; + now = fstat(newfd, &st) ? 0 : st.st_mtime; + 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 (index_file_timestamp && - index_file_timestamp <= ntohl(ce->ce_mtime.sec)) + if (index_file_timestamp && index_file_timestamp <= entry_time) ce_smudge_racily_clean_entry(ce); if (ce_write(&c, newfd, ce, ce_size(ce)) < 0) return -1; + if (now && now <= entry_time) + recent++; } /* Write extension data here */ @@ -983,5 +990,34 @@ int write_cache(int newfd, struct cache_ 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. + */ + if (20 < recent || entries <= recent * 4) { + now = fstat(newfd, &st) ? 0 : st.st_mtime; + while (now && !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); } -- 1.4.2.rc3.g19a8a ``` ## Johannes Schindelin, 2006-08-08 15:43 Subject: Re: [PATCH] Racy git: avoid having to be always too careful Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0608081659320.1800%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7virl7e8ed.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Sat, 5 Aug 2006, Junio C Hamano wrote: > Immediately after a bulk checkout, most of the paths in the > working tree would have the same timestamp as the index file, > and this would force ce_match_stat() to take slow path for all > of them. When writing an index file out, if many of the paths > have very new (read: the same timestamp as the index file being > written out) timestamp, we are better off delaying the return > from the command, to make sure that later command to touch the > working tree files will leave newer timestamps than recorded in > the index, thereby avoiding to take the slow path. This makes "make test" dog slow. How about making this overrideable? (It is just a guess, but I _think_ that the sleeping is worse than having to check the files with the same time stamp again and again -- a "git status" will help that). "make test" without this patch: real 9m59.314s user 0m36.580s sys 0m39.290s "make test" with this patch: real 1m36.429s user 0m37.410s sys 0m40.460s -- 8< -- [PATCH] read-cache: optionally disable being cautious with racy caches By setting the environment variable GIT_RISK_RACY_CACHE or the config variable core.riskRacyCache, the sleeping (to avoid a racy cache) is disabled. This is also used for "make test", drastically reducing the needed time. Signed-off-by: Johannes Schindelin --- Documentation/config.txt | 4 ++++ Documentation/git.txt | 5 +++++ cache.h | 1 + config.c | 4 ++++ environment.c | 1 + read-cache.c | 3 ++- t/test-lib.sh | 2 ++ 7 files changed, 19 insertions(+), 1 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index d89916b..b61fd91 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -103,6 +103,10 @@ core.legacyheaders:: database directly (where the "http://" and "rsync://" protocols count as direct access). +core.riskRacyCache:: + A boolean which allows to disable the 1-second sleeping to avoid + a racy cache. + alias.*:: Command aliases for the gitlink:git[1] command wrapper - e.g. after defining "alias.last = cat-file commit HEAD", the invocation diff --git a/Documentation/git.txt b/Documentation/git.txt index ba525d3..ff464e8 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -649,6 +649,11 @@ other stderr telling about alias expansion, built-in command execution and external command execution. +'GIT_RISK_RACY_CACHE':: + If this variable is set, gitlink:git-read-cache[1] will never + sleep to avoid a racy cache by making the timestamps of the index + file newer than the files referenced in the index. + Discussion[[Discussion]] ------------------------ include::README[] diff --git a/cache.h b/cache.h index 19fdef6..e8bbe6d 100644 --- a/cache.h +++ b/cache.h @@ -174,6 +174,7 @@ #define REFRESH_UNMERGED 0x0002 /* allow #define REFRESH_QUIET 0x0004 /* be quiet about it */ #define REFRESH_IGNORE_MISSING 0x0008 /* ignore non-existent */ extern int refresh_cache(unsigned int flags); +extern int risk_racy_cache; struct lock_file { struct lock_file *next; diff --git a/config.c b/config.c index c6e6f6a..4ab9329 100644 --- a/config.c +++ b/config.c @@ -291,6 +291,10 @@ int git_default_config(const char *var, return 0; } + if (!strcmp(var, "core.riskracycache")) { + risk_racy_cache = git_config_bool(var, value); + } + if (!strcmp(var, "user.name")) { strlcpy(git_default_name, value, sizeof(git_default_name)); return 0; diff --git a/environment.c b/environment.c index 1ce3411..a481fc4 100644 --- a/environment.c +++ b/environment.c @@ -24,6 +24,7 @@ const char *apply_default_whitespace = N int zlib_compression_level = Z_DEFAULT_COMPRESSION; int pager_in_use; int pager_use_color = 1; +int risk_racy_cache = 0; static int dyn_git_object_dir, dyn_git_index_file, dyn_git_graft_file; static char *git_dir, *git_object_dir, *git_index_file, *git_refs_dir, diff --git a/read-cache.c b/read-cache.c index d64b503..f20dac5 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1021,7 +1021,8 @@ int write_cache(int newfd, struct cache_ * This assumes that nobody is touching the working tree while * we are updating the index. */ - if (20 < recent || entries <= recent * 4) { + if (!risk_racy_cache && !getenv("GIT_RISK_RACY_CACHE") && + (20 < recent || entries <= recent * 4)) { now = fstat(newfd, &st) ? 0 : st.st_mtime; while (now && !fstat(newfd, &st) && st.st_mtime <= now) { struct timespec rq, rm; diff --git a/t/test-lib.sh b/t/test-lib.sh index b6d119a..3b6882d 100755 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -197,6 +197,8 @@ # t/ subdirectory and are run in trash s PATH=$(pwd)/..:$PATH GIT_EXEC_PATH=$(pwd)/.. export PATH GIT_EXEC_PATH +GIT_RISK_RACY_CACHE=1 +export GIT_RISK_RACY_CACHE # Similarly use ../compat/subprocess.py if our python does not # have subprocess.py on its own. -- 1.4.2.rc3.g6b27 ``` ## Junio C Hamano, 2006-08-08 17:25 Subject: Re: [PATCH] Racy git: avoid having to be always too careful Message-ID: <7vr6zrw352.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr6zrw352.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin writes: > This makes "make test" dog slow. How about making this overrideable? > (It is just a guess, but I _think_ that the sleeping is worse than having > to check the files with the same time stamp again and again -- a "git > status" will help that). > > > -- 8< -- > [PATCH] read-cache: optionally disable being cautious with racy caches > > By setting the environment variable GIT_RISK_RACY_CACHE or the config > variable core.riskRacyCache, the sleeping (to avoid a racy cache) is > disabled. "RISK" is a misnomer here -- the original code without patch is being cautious and taking a runtime hit but not risky. The "delay while writing the cache out for the first time" is trying to avoid the runtime hit by taking a hit at cache generation time. In either case you never risk racy cache. Perhaps throwing away the whole "delay" thing might be simpler and more worthwhile. Another possibility is to tweak the heuristics -- currently we say 20 paths or 1/4 of whole paths is too many and would cause too much runtime hit but that was done without any measurement. We could raise the threashold which would solve the case for the testsuite whose trees are all small. ``` ## Johannes Schindelin, 2006-08-08 22:30 Subject: Re: [PATCH] Racy git: avoid having to be always too careful Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0608090030080.1800%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7vr6zrw352.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Tue, 8 Aug 2006, Junio C Hamano wrote: > Another possibility is to tweak the heuristics -- currently we > say 20 paths or 1/4 of whole paths is too many and would cause > too much runtime hit but that was done without any measurement. > We could raise the threashold which would solve the case for the > testsuite whose trees are all small. Okay, how about 100 paths? And make _this_ a config variable? Ciao, Dscho ``` ## Junio C Hamano, 2006-08-08 23:07 Subject: Re: [PATCH] Racy git: avoid having to be always too careful Message-ID: <7vpsfau8qn.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vpsfau8qn.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin writes: > Okay, how about 100 paths? And make _this_ a config variable? Actually, I have a better patch I'll be pushing out in "next" this evening. Hint: ce_write() batches and buffers things, so "now" obtained in the current code is unreliable. ``` ## Johannes Schindelin, 2006-08-08 23:44 Subject: Re: [PATCH] Racy git: avoid having to be always too careful Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0608090143410.1800%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7vpsfau8qn.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Tue, 8 Aug 2006, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Okay, how about 100 paths? And make _this_ a config variable? > > Actually, I have a better patch I'll be pushing out in "next" > this evening. Pity. I just whipped up this patch... -- 8< -- [PATCH] read-cache: introduce core.racyThreshold The expensive sleep is done only when there are more than 100 updated files now. This number is overrideable by the core.racyThreshold variable. Signed-off-by: Johannes Schindelin --- Documentation/config.txt | 5 +++++ cache.h | 1 + config.c | 5 +++++ environment.c | 1 + read-cache.c | 2 +- 5 files changed, 13 insertions(+), 1 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index d89916b..f428fb5 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -103,6 +103,11 @@ core.legacyheaders:: database directly (where the "http://" and "rsync://" protocols count as direct access). +core.racyThreshold:: + The number of files which need to be updated at least, before + read-cache sleeps to make the timestamps of these files and the + timestamp of the index file distinct. + alias.*:: Command aliases for the gitlink:git[1] command wrapper - e.g. after defining "alias.last = cat-file commit HEAD", the invocation diff --git a/cache.h b/cache.h index 19fdef6..8c0a830 100644 --- a/cache.h +++ b/cache.h @@ -193,6 +193,7 @@ extern int warn_ambiguous_refs; extern int shared_repository; extern const char *apply_default_whitespace; extern int zlib_compression_level; +extern int racy_threshold; #define GIT_REPO_VERSION 0 extern int repository_format_version; diff --git a/config.c b/config.c index c6e6f6a..01b8c23 100644 --- a/config.c +++ b/config.c @@ -291,6 +291,11 @@ int git_default_config(const char *var, return 0; } + if (!strcmp(var, "core.racythreshold")) { + racy_threshold = git_config_int(var, value); + return 0; + } + if (!strcmp(var, "user.name")) { strlcpy(git_default_name, value, sizeof(git_default_name)); return 0; diff --git a/environment.c b/environment.c index 1ce3411..3656f36 100644 --- a/environment.c +++ b/environment.c @@ -24,6 +24,7 @@ const char *apply_default_whitespace = N int zlib_compression_level = Z_DEFAULT_COMPRESSION; int pager_in_use; int pager_use_color = 1; +int racy_threshold = 100; static int dyn_git_object_dir, dyn_git_index_file, dyn_git_graft_file; static char *git_dir, *git_object_dir, *git_index_file, *git_refs_dir, diff --git a/read-cache.c b/read-cache.c index d64b503..42a3f78 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1021,7 +1021,7 @@ int write_cache(int newfd, struct cache_ * This assumes that nobody is touching the working tree while * we are updating the index. */ - if (20 < recent || entries <= recent * 4) { + if (racy_threshold < recent) { now = fstat(newfd, &st) ? 0 : st.st_mtime; while (now && !fstat(newfd, &st) && st.st_mtime <= now) { struct timespec rq, rm; -- 1.4.2.rc3.gfa72-dirty ```