git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 10/12] get_packed_ref_cache: reload packed-refs file when it changes

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Jun 19, 2013, 07:51 UTC
Message-ID
<1371628293-28824-11-git-send-email-mhagger@alum.mit.edu>
In-Reply-To
<1371628293-28824-1-git-send-email-mhagger@alum.mit.edu>
From: Jeff King <peff@peff.net>

Once we read the packed-refs file into memory, we cache it to save work on future ref lookups. However, our cache may be out of date with respect to what is on disk if another process is simultaneously packing the refs. Normally it is acceptable for us to be a little out of date, since there is no guarantee whether we read the file before or after the simultaneous update. However, there is an important special case: our packed-refs file must be up to date with respect to any loose refs we read. Otherwise, we risk the following race condition:

  0. There exists a loose ref refs/heads/master.
  1. Process A starts and looks up the ref "master". It
     first checks $GIT_DIR/master, which does not exist. It
     then loads (and caches) the packed-refs file to see if
     "master" exists in it, which it does not.
  2. Meanwhile, process B runs "pack-refs --all --prune". It
     creates a new packed-refs file which contains
     refs/heads/master, and removes the loose copy at
     $GIT_DIR/refs/heads/master.
  3. Process A continues its lookup, and eventually tries
     $GIT_DIR/refs/heads/master.  It sees that the loose ref
     is missing, and falls back to the packed-refs file. But
     it examines its cached version, which does not have
     refs/heads/master. After trying a few other prefixes,
     it reports master as a non-existent ref.

There are many variants (e.g., step 1 may involve process A looking up another ref entirely, so even a fully qualified refname can fail). One of the most interesting ones is if "refs/heads/master" is already packed. In that case process A will not see it as missing, but rather will report whatever value happened to be in the packed-refs file before process B repacked (which might be an arbitrarily old value).

We can fix this by making sure we reload the packed-refs file from disk after looking at any loose refs. That's unacceptably slow, so we can check its stat()-validity as a proxy, and read it only when it appears to have changed.

Reading the packed-refs file after performing any loose-ref system calls is sufficient because we know the ordering of the pack-refs process: it always makes sure the newly written packed-refs file is installed into place before pruning any loose refs. As long as those operations by B appear in their executed order to process A, by the time A sees the missing loose ref, the new packed-refs file must be in place.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 6d48b42..d53a172 100644
--- a/refs.c
+++ b/refs.c
@@ -825,6 +825,9 @@ struct packed_ref_cache {
 	 * when it is unlocked.
 	 */
 	struct lock_file *lock;
+
+	/* The metadata from when this packed-refs cache was read */
+	struct stat_validity validity;
 };
 
 /*
@@ -859,6 +862,7 @@ static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)
 {
 	if (!--packed_refs->referrers) {
 		free_ref_entry(packed_refs->root);
+		stat_validity_clear(&packed_refs->validity);
 		free(packed_refs);
 		return 1;
 	} else {
@@ -1050,19 +1054,26 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
  */
 static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)
 {
+	const char *packed_refs_file;
+
+	if (*refs->name)
+		packed_refs_file = git_path_submodule(refs->name, "packed-refs");
+	else
+		packed_refs_file = git_path("packed-refs");
+
+	if (refs->packed &&
+	    !stat_validity_check(&refs->packed->validity, packed_refs_file))
+		clear_packed_ref_cache(refs);
+
 	if (!refs->packed) {
-		const char *packed_refs_file;
 		FILE *f;
 
 		refs->packed = xcalloc(1, sizeof(*refs->packed));
 		acquire_packed_ref_cache(refs->packed);
 		refs->packed->root = create_dir_entry(refs, "", 0, 0);
-		if (*refs->name)
-			packed_refs_file = git_path_submodule(refs->name, "packed-refs");
-		else
-			packed_refs_file = git_path("packed-refs");
 		f = fopen(packed_refs_file, "r");
 		if (f) {
+			stat_validity_update(&refs->packed->validity, fileno(f));
 			read_packed_refs(f, get_ref_dir(refs->packed->root));
 			fclose(f);
 		}
-- 
1.8.3.1
Previous: Michael HaggertyNext: Michael Haggerty
Message 19 of 24 in “Fix some reference-related races”
  1. 00/12 Fix some reference-related racesMichael Haggerty, Jun 19, 2013
  2. 01/12 repack_without_ref(): split list curation and entry writingMichael Haggerty, Jun 19, 2013
  3. 02/12 pack_refs(): split creation of packed refs and entry writingMichael Haggerty, Jun 19, 2013
  4. 03/12 refs: wrap the packed refs cache in a level of indirectionMichael Haggerty, Jun 19, 2013
  5. 04/12 refs: implement simple transactions for the packed-refs fileMichael Haggerty, Jun 19, 2013
  6. Junio C HamanoJun 19, 2013
  7. Michael HaggertyJun 20, 2013
  8. Jeff KingJun 20, 2013
  9. Michael HaggertyJun 20, 2013
  10. Jeff KingJun 20, 2013
  11. Junio C HamanoJun 20, 2013
  12. Michael HaggertyJun 20, 2013
  13. Junio C HamanoJun 20, 2013
  14. 05/12 refs: manage lifetime of packed refs cache via reference countingMichael Haggerty, Jun 19, 2013
  15. 06/12 do_for_each_entry(): increment the packed refs cache refcountMichael Haggerty, Jun 19, 2013
  16. 07/12 packed_ref_cache: increment refcount when lockedMichael Haggerty, Jun 19, 2013
  17. 08/12 Extract a struct stat_data from cache_entryMichael Haggerty, Jun 19, 2013
  18. 09/12 add a stat_validity structMichael Haggerty, Jun 19, 2013
  19. 10/12 get_packed_ref_cache: reload packed-refs file when it changesMichael Haggerty, Jun 19, 2013
  20. 11/12 for_each_ref: load all loose refs before packed refsMichael Haggerty, Jun 19, 2013
  21. 12/12 refs: do not invalidate the packed-refs cache unnecessarilyMichael Haggerty, Jun 19, 2013
  22. Jeff KingJun 19, 2013
  23. Michael HaggertyJun 20, 2013
  24. Jeff KingJun 20, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.