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

Re: [PATCH 00/10] Implement transactions for the packed ref store

From
BWBrandon Williams <bmwill@google.com>
Date
Aug 29, 2017, 18:30 UTC
Message-ID
<20170829183008.GC131745@google.com>
In-Reply-To
<cover.1503993268.git.mhagger@alum.mit.edu>
On 08/29, Michael Haggerty wrote:
Show 62 quoted lines
> This should be the second-to-last patch series in the quest for
> mmapped packed-refs.
> 
> Before this patch series, there is still more coupling than necessary
> between `files_ref_store` and `packed_ref_store`:
> 
> * `files_ref_store` adds packed references (e.g., during `git clone`
>   and `git pack-refs`) by inserting them into the `packed_ref_store`'s
>   internal `ref_cache`, then calling `commit_packed_refs()`. But
>   `files_ref_store` shouldn't even *know* that `packed_ref_store` uses
>   a `ref_cache`, let alone muck about in it.
> 
> * `files_ref_store` deletes packed references by calling
>   `repack_without_refs()`.
> 
> Instead, implement reference transactions and `delete_refs()` for
> `packed_ref_store`, and change `files_ref_store` to use these standard
> methods rather than internal `packed_ref_store` methods.
> 
> This patch series finishes up the encapsulation of `packed_ref_store`.
> At the end of the series, the outside world only interacts with it via
> the refs API plus a couple of locking-related functions. That will
> make it easy for the next patch series to change the internal workings
> of `packed_ref_store` without affecting `files_ref_store`.
> 
> Along the way, we fix some longstanding problems with reference
> updates. Quoting from patch [08/10]:
> 
>     First, the old code didn't obtain the `packed-refs` lock until
>     `files_transaction_finish()`. This means that a failure to acquire the
>     `packed-refs` lock (e.g., due to contention with another process)
>     wasn't detected until it was too late (problems like this are supposed
>     to be detected in the "prepare" phase). The new code acquires the
>     `packed-refs` lock in `files_transaction_prepare()`, the same stage of
>     the processing when the loose reference locks are being acquired,
>     removing another reason why the "prepare" phase might succeed and the
>     "finish" phase might nevertheless fail.
> 
>     Second, the old code deleted the loose version of a reference before
>     deleting any packed version of the same reference. This left a moment
>     when another process might think that the packed version of the
>     reference is current, which is incorrect. (Even worse, the packed
>     version of the reference can be arbitrarily old, and might even point
>     at an object that has since been garbage-collected.)
> 
>     Third, if a reference deletion fails to acquire the `packed-refs` lock
>     altogether, then the old code might leave the repository in the
>     incorrect state (possibly corrupt) described in the previous
>     paragraph.
> 
>     Now we activate the new "packed-refs" file (sans any references that
>     are being deleted) *before* deleting the corresponding loose
>     references. But we hold the "packed-refs" lock until after the loose
>     references have been finalized, thus preventing a simultaneous
>     "pack-refs" process from packing the loose version of the reference in
>     the time gap, which would otherwise defeat our attempt to delete it.
> 
> This patch series is also available as branch
> `packed-ref-transactions` in my fork on GitHub [1]. A draft of the
> patch series to change `packed_ref_store` to use mmap and get rid of
> its `ref_cache` is available as branch `mmap-packed-refs` from the
> same source.

Overall the patches look sane to me, though I don't believe I'm qualified in this area to give you a complete thumbs up since I don't understand the refs code super well yet. I do like reading patch from you as you do a great job of laying out what you are doing in code, comments and commit messages, something I'm trying to get better at :)

-- 
Brandon Williams
Previous: Michael HaggertyNext: Jeff King
Message 30 of 31 in “Implement transactions for the packed ref store”
  1. 00/10 Implement transactions for the packed ref storeMichael Haggerty, Aug 29, 2017
  2. 05/10 files_pack_refs(): use a reference transaction to write packed refsMichael Haggerty, Aug 29, 2017
  3. 04/10 packed_delete_refs(): implement methodMichael Haggerty, Aug 29, 2017
  4. Brandon WilliamsAug 29, 2017
  5. Michael HaggertyAug 30, 2017
  6. 06/10 files_initial_transaction_commit(): use a transaction for packed refsMichael Haggerty, Aug 29, 2017
  7. Jeff KingSep 8, 2017
  8. Michael HaggertySep 8, 2017
  9. 07/10 t1404: demonstrate two problems with reference transactionsMichael Haggerty, Aug 29, 2017
  10. Stefan BellerAug 30, 2017
  11. Michael HaggertyAug 31, 2017
  12. Junio C HamanoSep 8, 2017
  13. Jeff KingSep 8, 2017
  14. Michael HaggertySep 8, 2017
  15. 09/10 packed-backend: rip out some now-unused codeMichael Haggerty, Aug 29, 2017
  16. Brandon WilliamsAug 29, 2017
  17. 08/10 files_ref_store: use a transaction to update packed refsMichael Haggerty, Aug 29, 2017
  18. Jeff KingSep 8, 2017
  19. Michael HaggertySep 8, 2017
  20. Jeff KingSep 8, 2017
  21. 10/10 files_transaction_finish(): delete reflogs before referencesMichael Haggerty, Aug 29, 2017
  22. 01/10 packed-backend: don't adjust the reference count on lock/unlockMichael Haggerty, Aug 29, 2017
  23. Jeff KingSep 8, 2017
  24. Michael HaggertySep 8, 2017
  25. 02/10 struct ref_transaction: add a place for backends to store dataMichael Haggerty, Aug 29, 2017
  26. Jeff KingSep 8, 2017
  27. Michael HaggertySep 8, 2017
  28. Jeff KingSep 8, 2017
  29. 03/10 packed_ref_store: implement reference transactionsMichael Haggerty, Aug 29, 2017
  30. Brandon WilliamsAug 29, 2017
  31. Jeff KingSep 8, 2017

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.