From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
Johan Herland <johan@herland.net>,
git@vger.kernel.org
Subject: Re: [PATCH 00/12] Fix some reference-related races
Date: Sat, 15 Jun 2013 21:13:06 +0100 [thread overview]
Message-ID: <51BCCAD2.9070106@ramsay1.demon.co.uk> (raw)
In-Reply-To: <1370987312-6761-1-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty wrote:
> *This patch series must be built on top of mh/reflife.*
>
[...]
> The other problem was that the for_each_ref() functions will die if
> the ref cache that they are iterating over is freed out from under
> them. This problem is solved by using reference counts to avoid
> freeing the old packed ref cache (even if it is no longer valid) until
> all users are done with it.
Yes, I found exactly this happened to me on cygwin, earlier this week,
with the previous version of this code. After seeing this mail, I had
decided not to describe the failure on the old version, but wait and
test this version instead.
This version is a great improvement, but it still has some failures on
cygwin. So, it may be worth (briefly) describing the old failure anyway!
Note that several tests failed, but I will only mention t3211-peel-ref.sh
tests #7-8.
$ pwd
/home/ramsay/git/t/trash directory.t3211-peel-ref
$
$ ../../bin-wrappers/git show-ref -d
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}
5 [main] git 3540 _cygtls::handle_exceptions: Error while dumping state (p
robably corrupted stack)
Segmentation fault (core dumped)
$
The stack-trace for the faulting code looks something like:
cmd_show_ref()
for_each_ref(show_ref, NULL)
do_for_each_ref(&ref_cache, "", show_ref, 0, 0, NULL)
do_for_each_entry(&ref_cache, "", do_one_ref, &data)
do_for_each_entry_in_dirs(packed_dir, loose_dir, do_one_ref, &data)
*dfeeid() recursive calls*
do_one_ref(entry, &data)
show_ref("refs/outside/foo", sha1, NULL) [2nd match]
peel_ref("refs/outside/foo", sha1)
peel_entry(entry, 0)
peel_object(name, sha1)
deref_tag_noverify(o)
parse_object(sha1 <eb0e854c2...>)
lookup_replace_object(sha1)
do_lookup_replace_object(sha1)
prepare_replace_object()
[un-indent here!]
for_each_replace_ref(register_replace_ref, NULL)
do_for_each_ref(&ref_cache, "refs/replace", fn, 13, 0, NULL)
do_for_each_entry(&ref_cache, "refs/replace", fn, &data)
get_packed_refs(&ref_cache)
clear_packed_ref_cache(&ref_cache) *free_ref_entries etc*
** return to show_ref() [2nd match] above **
** return to recursive dfeeid() call in original iteration
** dir1->entries has been free()-ed and reused => segmentation fault
[dir1->entries == 0x64633263 => dc2c => part of sha1 for refs/outside/foo]
So, the nested "replace-reference-iteration" causes the ref_cache to be
freed out from under the initial show-ref iteration, so this works:
$ GIT_NO_REPLACE_OBJECTS=1 ../../bin-wrappers/git show-ref -d
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/foo^{}
$
You may be wondering why clear_packed_ref_cache() is called? Well, that
is because stat_validity_check() *incorrectly* indicates that the
packed-refs file has changed. Why does it do that? Well, this is one
more example of the problems caused by the cygwin schizophrenic stat()
functions. :( [ARGHHHHHHHHH]
At this point, I tried running 'git show-ref' with core.checkstat set
on the command line; but that didn't work! I had to fix show-ref and
re-build git, and then, this works:
$ ../../bin-wrappers/git -c core.checkstat=minimal -c core.trustctime=f
alse show-ref -d
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/foo^{}
$
Now, turning to the new code, t3211-peel-ref.sh test #7 now works, but
test #8 still fails...
$ ./t3211-peel-ref.sh -i -v
...
ok 7 - refs are peeled outside of refs/tags (old packed)
expecting success:
git pack-refs --all &&
cp .git/packed-refs fully-peeled &&
git branch yadda &&
git pack-refs --all &&
git branch -d yadda &&
test_cmp fully-peeled .git/packed-refs
fatal: internal error: packed-ref cache cleared while locked
not ok 8 - peeled refs survive deletion of packed ref
#
# git pack-refs --all &&
# cp .git/packed-refs fully-peeled &&
# git branch yadda &&
# git pack-refs --all &&
# git branch -d yadda &&
# test_cmp fully-peeled .git/packed-refs
#
$ cd trash\ directory.t3211-peel-ref/
$ ../../bin-wrappers/git pack-refs --all
fatal: internal error: packed-ref cache cleared while locked
$ ls
actual base.t expect
$ ls .git
COMMIT_EDITMSG branches/ description index logs/ packed-refs
HEAD config hooks-disabled/ info/ objects/ refs/
$ ls -l .git/packed-refs
-rw-r--r-- 1 ramsay None 296 Jun 14 20:34 .git/packed-refs
$ cat .git/packed-refs
# pack-refs with: peeled
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e
$
Now, I have a test-stat program which prints the difference between
the two stat implementations used in cygwin git, thus:
$ ../../test-stat .git/packed-refs
stat for '.git/packed-refs':
*dev: -1395862925, 0
*ino: 166044, 0
*mode: 100644 -rw-, 100600 -rw-
nlink: 1, 1
*uid: 1005, 0
*gid: 513, 0
*rdev: -1395862925, 0
size: 296, 296
atime: 1371238550, 1371238550 Fri Jun 14 20:35:50 2013
mtime: 1371238469, 1371238469 Fri Jun 14 20:34:29 2013
ctime: 1371238469, 1371238469 Fri Jun 14 20:34:29 2013
$ ../../bin-wrappers/git -c core.checkstat=minimal pack-refs --all
fatal: internal error: packed-ref cache cleared while locked
$
Hmmm, that should have worked! Wait, fix 'git pack-refs' to support
setting config variables on the command line, rebuild and:
$ ../../bin-wrappers/git -c core.checkstat=minimal pack-refs --all
$ cat .git/packed-refs
# pack-refs with: peeled fully-peeled
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo
^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e
d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base
eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo
^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e
$
I haven't checked the remaining test failures to see if they are
caused by this code (I don't think so, but ...), but this failure
is clearly a cygwin specific issue.
HTH
ATB,
Ramsay Jones
next prev parent reply other threads:[~2013-06-15 20:32 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-11 21:48 [PATCH 00/12] Fix some reference-related races Michael Haggerty
2013-06-11 21:48 ` [PATCH 01/12] repack_without_ref(): split list curation and entry writing Michael Haggerty
2013-06-12 11:38 ` Jeff King
2013-06-12 11:56 ` Michael Haggerty
2013-06-11 21:48 ` [PATCH 02/12] pack_refs(): split creation of packed refs " Michael Haggerty
2013-06-11 21:48 ` [PATCH 03/12] refs: wrap the packed refs cache in a level of indirection Michael Haggerty
2013-06-11 21:48 ` [PATCH 04/12] refs: implement simple transactions for the packed-refs file Michael Haggerty
2013-06-12 12:01 ` Jeff King
2013-06-11 21:48 ` [PATCH 05/12] refs: manage lifetime of packed refs cache via reference counting Michael Haggerty
2013-06-11 21:48 ` [PATCH 06/12] do_for_each_entry(): increment the packed refs cache refcount Michael Haggerty
2013-06-11 21:48 ` [PATCH 07/12] packed_ref_cache: increment refcount when locked Michael Haggerty
2013-06-11 21:48 ` [PATCH 08/12] Extract a struct stat_data from cache_entry Michael Haggerty
2013-06-11 21:48 ` [PATCH 09/12] add a stat_validity struct Michael Haggerty
2013-06-11 21:48 ` [PATCH 10/12] get_packed_ref_cache: reload packed-refs file when it changes Michael Haggerty
2013-06-11 21:48 ` [PATCH 11/12] for_each_ref: load all loose refs before packed refs Michael Haggerty
2013-06-11 21:48 ` [PATCH 12/12] refs: do not invalidate the packed-refs cache unnecessarily Michael Haggerty
2013-06-12 12:39 ` Jeff King
2013-06-12 12:52 ` [PATCH 00/12] Fix some reference-related races Jeff King
2013-06-15 20:13 ` Ramsay Jones [this message]
2013-06-16 5:50 ` Michael Haggerty
2013-06-18 18:13 ` Ramsay Jones
2013-06-19 5:51 ` Michael Haggerty
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=51BCCAD2.9070106@ramsay1.demon.co.uk \
--to=ramsay@ramsay1.demon.co.uk \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johan@herland.net \
--cc=mhagger@alum.mit.edu \
--cc=peff@peff.net \
/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).