From: Brandon Williams <bmwill@google.com>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"Stefan Beller" <sbeller@google.com>, "Jeff King" <peff@peff.net>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH 00/10] Implement transactions for the packed ref store
Date: Tue, 29 Aug 2017 11:30:08 -0700 [thread overview]
Message-ID: <20170829183008.GC131745@google.com> (raw)
In-Reply-To: <cover.1503993268.git.mhagger@alum.mit.edu>
On 08/29, Michael Haggerty wrote:
> 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
next prev parent reply other threads:[~2017-08-29 18:30 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-29 8:20 [PATCH 00/10] Implement transactions for the packed ref store Michael Haggerty
2017-08-29 8:20 ` [PATCH 01/10] packed-backend: don't adjust the reference count on lock/unlock Michael Haggerty
2017-09-08 6:52 ` Jeff King
2017-09-08 10:02 ` Michael Haggerty
2017-08-29 8:20 ` [PATCH 02/10] struct ref_transaction: add a place for backends to store data Michael Haggerty
2017-09-08 7:02 ` Jeff King
2017-09-08 8:19 ` Michael Haggerty
2017-09-08 8:33 ` Jeff King
2017-08-29 8:20 ` [PATCH 03/10] packed_ref_store: implement reference transactions Michael Haggerty
2017-08-29 8:20 ` [PATCH 04/10] packed_delete_refs(): implement method Michael Haggerty
2017-08-29 18:07 ` Brandon Williams
2017-08-30 3:00 ` Michael Haggerty
2017-08-29 8:20 ` [PATCH 05/10] files_pack_refs(): use a reference transaction to write packed refs Michael Haggerty
2017-08-29 8:20 ` [PATCH 06/10] files_initial_transaction_commit(): use a transaction for " Michael Haggerty
2017-09-08 7:27 ` Jeff King
2017-09-08 10:04 ` Michael Haggerty
2017-08-29 8:20 ` [PATCH 07/10] t1404: demonstrate two problems with reference transactions Michael Haggerty
2017-08-30 17:21 ` Stefan Beller
2017-08-31 3:42 ` Michael Haggerty
2017-09-08 4:44 ` Junio C Hamano
2017-09-08 7:45 ` Jeff King
2017-09-08 10:06 ` Michael Haggerty
2017-08-29 8:20 ` [PATCH 08/10] files_ref_store: use a transaction to update packed refs Michael Haggerty
2017-09-08 7:38 ` Jeff King
2017-09-08 12:44 ` Michael Haggerty
2017-09-08 12:57 ` Jeff King
2017-08-29 8:20 ` [PATCH 09/10] packed-backend: rip out some now-unused code Michael Haggerty
2017-08-29 18:24 ` Brandon Williams
2017-08-29 8:20 ` [PATCH 10/10] files_transaction_finish(): delete reflogs before references Michael Haggerty
2017-08-29 18:30 ` Brandon Williams [this message]
2017-09-08 7:42 ` [PATCH 00/10] Implement transactions for the packed ref store Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170829183008.GC131745@google.com \
--to=bmwill@google.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mhagger@alum.mit.edu \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).