From: Michael Haggerty <mhagger@alum.mit.edu>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
Johan Herland <johan@herland.net>,
Ramsay Jones <ramsay@ramsay1.demon.co.uk>,
git@vger.kernel.org
Subject: Re: [PATCH v2 00/12] Fix some reference-related races
Date: Thu, 20 Jun 2013 11:09:49 +0200 [thread overview]
Message-ID: <51C2C6DD.90107@alum.mit.edu> (raw)
In-Reply-To: <20130619185645.GB23647@sigill.intra.peff.net>
On 06/19/2013 08:56 PM, Jeff King wrote:
> On Wed, Jun 19, 2013 at 09:51:21AM +0200, Michael Haggerty wrote:
>
>> Re-roll of mh/ref-races. Thanks to Peff, Junio, and Ramsay for
>> reviewing v1.
>
> Thanks. I just read through them again. Everything looks good to me.
>
> Patches 10 and 11 are missing my signoff, but obviously:
>
> Signed-off-by: Jeff King <peff@peff.net>
>
>> The last patch is still optional--it avoids a little bit of work when
>> rewriting the packed-refs file, but relies on the stat-based freshness
>> check not giving a false negative.
>
> I don't have a real problem with it, but given the cygwin confusions
> that Ramsay mentioned, maybe it is better to hold back on it for now? It
> sounds like the cygwin problems go the other way (false positives
> instead of false negatives).
I just realized that there is another long-standing problem in
loose/packed refs handling:
Suppose there is a loose reference for which an old version is still
present in the packed-refs file. If the reference is deleted:
1. delete_ref() gets a lock on the loose reference file
2. delete_ref() deletes the loose reference file while retaining the lock
3. delete_ref() calls repack_without_ref()
4. repack_without_ref() tries to acquire a lock on the packed-refs file
to delete the packed version of the reference.
If the packed-refs file is already locked by another process (and there
is no reason why that cannot be, and there is only one attempt to
acquire the lock), then repack_without_ref() emits an error and returns
with an error code. delete_ref() passes the error along, but doesn't
restore the loose ref.
The net result is that an old version of the ref (namely the one in the
loose refs file) has been revived. That version might even point at an
object that has already been garbage collected.
This problem is similar to race conditions that have been discussed
earlier, but this problem has nothing to do with the freshness/staleness
of the packed-refs file WRT to the loose reference; it only depends on
the packed-refs file being locked by another process at an inopportune time.
I think this problem would also be fixed by the locking scheme that I
proposed earlier [1]: to acquire and hold the packed-refs lock across
the modification of *both* files, and to rewrite the packed-refs file
*before* deleting the loose-refs file (because rewriting the packed-refs
file without the to-be-deleted reference is a logical NOP).
Michael
[1] http://article.gmane.org/gmane.comp.version-control.git/212131
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
next prev parent reply other threads:[~2013-06-20 9:10 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-19 7:51 [PATCH v2 00/12] Fix some reference-related races Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 01/12] repack_without_ref(): split list curation and entry writing Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 02/12] pack_refs(): split creation of packed refs " Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 03/12] refs: wrap the packed refs cache in a level of indirection Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 04/12] refs: implement simple transactions for the packed-refs file Michael Haggerty
2013-06-19 19:18 ` Junio C Hamano
2013-06-20 7:49 ` Michael Haggerty
2013-06-20 11:55 ` Jeff King
2013-06-20 18:03 ` Michael Haggerty
2013-06-20 19:55 ` Jeff King
2013-06-20 17:11 ` Junio C Hamano
2013-06-20 17:58 ` Michael Haggerty
2013-06-20 18:36 ` Junio C Hamano
2013-06-19 7:51 ` [PATCH v2 05/12] refs: manage lifetime of packed refs cache via reference counting Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 06/12] do_for_each_entry(): increment the packed refs cache refcount Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 07/12] packed_ref_cache: increment refcount when locked Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 08/12] Extract a struct stat_data from cache_entry Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 09/12] add a stat_validity struct Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 10/12] get_packed_ref_cache: reload packed-refs file when it changes Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 11/12] for_each_ref: load all loose refs before packed refs Michael Haggerty
2013-06-19 7:51 ` [PATCH v2 12/12] refs: do not invalidate the packed-refs cache unnecessarily Michael Haggerty
2013-06-19 18:56 ` [PATCH v2 00/12] Fix some reference-related races Jeff King
2013-06-20 9:09 ` Michael Haggerty [this message]
2013-06-20 11:52 ` 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=51C2C6DD.90107@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johan@herland.net \
--cc=peff@peff.net \
--cc=ramsay@ramsay1.demon.co.uk \
/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).