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

Re: another packed-refs race

From
Jeff King <peff@peff.net>
Date
May 7, 2013, 04:44 UTC
Message-ID
<20130507044456.GA29757@sigill.intra.peff.net>
In-Reply-To
<518883CC.7050609@alum.mit.edu>
On Tue, May 07, 2013 at 06:32:12AM +0200, Michael Haggerty wrote:
Show 15 quoted lines
> Another potential problem caused by the non-atomicity of loose reference
> reading could be to confuse reachability tests if process 1 is reading
> loose references while process 2 is renaming a reference:
> 
> 1. Process 1 looks for refs/heads/aaaaa and finds it missing.
> 
> 2. Process 2 renames zzzzz -> aaaaa
> 
> 3. Process 1 looks for refs/heads/zzzzz and finds it missing.
> 
> Process 2 would think that any objects pointed to by aaaaa (formerly
> zzzzz) are unreachable.  This would be unfortunate if it is doing an
> upload-pack and very bad if it is doing a gc.  I wonder if this could be
> a problem in practice?  (Gee, wouldn't it be nice to keep reflogs for
> deleted refs? :-) )

Ugh. Yeah, that is definitely a possible race, and it could be quite disastrous for prune.

I am really starting to think that we need a pluggable refs architecture, and that busy servers can use something like sqlite to keep the ref storage. That would require bumping repositoryformatversion, of course, but it would be OK for a server accessible only by git protocols.

I also wonder if we can spend extra time to get more reliable results for prune, like checking refs, coming up with a prune list, and then checking again. I have a feeling it's a 2-generals sort of problem where we can always miss a ref that keeps bouncing around, but we could bound the probability. I haven't thought that hard about it. Perhaps this will give us something to talk about on Thursday. :)

Show 5 quoted lines
> > My proposal above gets rid of the need to invalidate the loose refs
> > cache, so we can ignore that complexity.
> 
> The same concern applies to invalidating the packed-ref cache, which you
> still want to do.

True. In theory a call to resolve_ref can invalidate it, so any call from inside a for_each_ref callback would be suspect.

Show 8 quoted lines
> * Preloading the whole tree of loose references before starting an
> iteration.  As I recall, this was a performance *win*.  It was on my
> to-do list of things to pursue when I have some free time (ha, ha).  I
> mostly wanted to check first that there are not callers who abort the
> iteration soon after starting it.  For example, imagine a caller who
> tries to determine "are there any tags at all" by iterating over
> "refs/tags" with a callback that just returns 1; such a caller would
> suffer the cost of reading all of the loose references in "refs/tags".

Well, you can measure my patches, because that's what they do. :) I didn't really consider an early termination from the iteration. Certainly something like:

  git for-each-ref refs/tags | head -1

would take longer. Though if you have that many refs that the latency is a big problem, you should probably consider packing them (it can't possibly bite you with a race condition, right?).

-Peff
Previous: Michael HaggertyNext: Michael Haggerty
Message 13 of 45 in “another packed-refs race”
  1. Jeff KingMay 3, 2013
  2. Johan HerlandMay 3, 2013
  3. Jeff KingMay 3, 2013
  4. Jeff KingMay 3, 2013
  5. Johan HerlandMay 3, 2013
  6. Michael HaggertyMay 6, 2013
  7. Jeff KingMay 6, 2013
  8. Jeff KingMay 3, 2013
  9. Michael HaggertyMay 6, 2013
  10. Jeff KingMay 6, 2013
  11. Jeff KingMay 6, 2013
  12. Michael HaggertyMay 7, 2013
  13. Jeff KingMay 7, 2013
  14. Michael HaggertyMay 7, 2013
  15. 0/4 fix packed-refs racesJeff King, May 7, 2013
  16. 1/4 resolve_ref: close race condition for packed refsJeff King, May 7, 2013
  17. Michael HaggertyMay 12, 2013
  18. Jeff KingMay 16, 2013
  19. Michael HaggertyMay 16, 2013
  20. Michael HaggertyMay 12, 2013
  21. 0/4 Fix a race condition when reading loose refsMichael Haggerty, Jun 11, 2013
  22. 1/4 resolve_ref_unsafe(): extract function handle_missing_loose_ref()Michael Haggerty, Jun 11, 2013
  23. 2/4 resolve_ref_unsafe(): handle the case of an SHA-1 within loopMichael Haggerty, Jun 11, 2013
  24. 3/4 resolve_ref_unsafe(): nest reference-reading code in an infinite loopMichael Haggerty, Jun 11, 2013
  25. 4/4 resolve_ref_unsafe(): close race condition reading loose refsMichael Haggerty, Jun 11, 2013
  26. Jeff KingJun 12, 2013
  27. Thomas RastJun 13, 2013
  28. Michael HaggertyJun 14, 2013
  29. Junio C HamanoJun 11, 2013
  30. 2/4 add a stat_validity structJeff King, May 7, 2013
  31. Michael HaggertyMay 13, 2013
  32. 0/2 Separate stat_data from cache_entryMichael Haggerty, May 13, 2013
  33. 1/2 Extract a struct stat_data from cache_entryMichael Haggerty, May 13, 2013
  34. 2/2 add a stat_validity structMichael Haggerty, May 13, 2013
  35. Junio C HamanoMay 13, 2013
  36. Jeff KingMay 16, 2013
  37. 3/4 get_packed_refs: reload packed-refs file when it changesJeff King, May 7, 2013
  38. 0/2 peel_ref cleanups changesJeff King, May 7, 2013
  39. 1/2 peel_ref: rename "sha1" argument to "peeled"Jeff King, May 7, 2013
  40. 2/2 peel_ref: refactor for safety with simultaneous updateJeff King, May 7, 2013
  41. Eric SunshineMay 9, 2013
  42. Michael HaggertyMay 13, 2013
  43. 4/4 for_each_ref: load all loose refs before packed refsJeff King, May 7, 2013
  44. Junio C HamanoMay 7, 2013
  45. Jeff KingMay 7, 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.