Re: [PATCH v2 0/5] Fix the racy split index problem
On Mon, Oct 08, 2018 at 04:54:51PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Thanks. I had ~400 runs of the tests I ran before and they were all > >> OK. Now trying also with t1701 (which I hadn't noticed was a new > >> test...). > > > > Ran that overnight with the same conditions as before. 2683 OK runs and > > 0 failures (and counting). So it seems like the combination of the two > > fixed the split index bugs. > > I forgot I ad this running, and got up to 45482 OKs and 0 FAILs before > finally Ctrl+C-ing it now :) Yay! \o/ Thanks for testing, and I feel sorry for your poor machine... I will send an updated version to address the (minor) points raised in [1]... well, most likely not today, but hopefully soon-ish. 1 - https://public-inbox.org/git/20180929091429.GF23446@localhost/
Re: [PATCH v2 0/5] Fix the racy split index problem
On Fri, Sep 28 2018, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Sep 27 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> On Thu, Sep 27 2018, SZEDER Gábor wrote:
>>
>>> On Thu, Sep 27, 2018 at 03:53:24PM +0200, Ævar Arnfjörð Bjarmason wrote:
On Thu, Sep 27 2018, SZEDER Gábor wrote:
> This is the second attempt to fix the racy split index problem, which
> causes occasional failures in several random test scripts when run
> with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches
> 1 and 5 (corresponding to v1's 3 and 5).
Thanks. I'm running the same sorts of tests I noted in
https://public-inbox.org/git/87va7ireuu@evledraar.gmail.com/ on
this. The fix Jeff had that you noted in
https://public-inbox.org/git/20180906151439.GA8016@localhost/ is now in
"master".
I take it your
https://github.com/szeder/git/commits/racy-split-index-fix is the same
as this submission?
>>>
>>> Yes.
>>>
Anyway, I'm testing that cherry-picked on top of the
latest master.
Unfortunate that we couldn't get the isolated test you made in
https://public-inbox.org/git/20180907034942.GA10370@localhost/
>>>
>>> Nah, that's not an isolated test case, that's only a somewhat
>>> narrowed-down, but rather reliable reproduction recipe while I still
>>> had no idea what was going on :)
>>>
>>> The _real_ isolated test is the last test in t1701, that's what it
>>> eventually boiled down to.
>>
>> Thanks. I had ~400 runs of the tests I ran before and they were all
>> OK. Now trying also with t1701 (which I hadn't noticed was a new
>> test...).
>
> Ran that overnight with the same conditions as before. 2683 OK runs and
> 0 failures (and counting). So it seems like the combination of the two
> fixed the split index bugs.
I forgot I ad this running, and got up to 45482 OKs and 0 FAILs before
finally Ctrl+C-ing it now :)
but I
don't see how it could be added without some very liberal
getenv("GIT_TEST_blahblah"), so it's probably best to not add it,
particularly with the C rewrite of git-stash in-flight.
I'll report back when I have enough test data to say how these patches
affect the intermittent test failures under GIT_TEST_SPLIT_INDEX=yes.
Re: [PATCH v2 0/5] Fix the racy split index problem
On Fri, Sep 28, 2018 at 08:57:53AM +0200, Ævar Arnfjörð Bjarmason wrote: > > Thanks. I had ~400 runs of the tests I ran before and they were all > > OK. Now trying also with t1701 (which I hadn't noticed was a new > > test...). > > Ran that overnight with the same conditions as before. 2683 OK runs and > 0 failures (and counting). So it seems like the combination of the two > fixed the split index bugs. Yeah, I thought they would. If you look at the first loop of prepare_to_write_split_index() classifying which cache entries should be included in the new split index, you'll see only two code paths that could leave out an entry from the split index, i.e. where an entry could be left with a non-zero 'ce->index' and without its CE_UPDATE_IN_BASE flag set. Now, with the fix in patch 5/5 both of those code paths have the is_race_timestamp() check.
Re: [PATCH v2 0/5] Fix the racy split index problem
On Thu, Sep 27 2018, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Sep 27 2018, SZEDER Gábor wrote:
>
>> On Thu, Sep 27, 2018 at 03:53:24PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Thu, Sep 27 2018, SZEDER Gábor wrote:
>>>
>>> > This is the second attempt to fix the racy split index problem, which
>>> > causes occasional failures in several random test scripts when run
>>> > with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches
>>> > 1 and 5 (corresponding to v1's 3 and 5).
>>>
>>> Thanks. I'm running the same sorts of tests I noted in
>>> https://public-inbox.org/git/87va7ireuu@evledraar.gmail.com/ on
>>> this. The fix Jeff had that you noted in
>>> https://public-inbox.org/git/20180906151439.GA8016@localhost/ is now in
>>> "master".
>>>
>>> I take it your
>>> https://github.com/szeder/git/commits/racy-split-index-fix is the same
>>> as this submission?
>>
>> Yes.
>>
>>> Anyway, I'm testing that cherry-picked on top of the
>>> latest master.
>>>
>>> Unfortunate that we couldn't get the isolated test you made in
>>> https://public-inbox.org/git/20180907034942.GA10370@localhost/
>>
>> Nah, that's not an isolated test case, that's only a somewhat
>> narrowed-down, but rather reliable reproduction recipe while I still
>> had no idea what was going on :)
>>
>> The _real_ isolated test is the last test in t1701, that's what it
>> eventually boiled down to.
>
> Thanks. I had ~400 runs of the tests I ran before and they were all
> OK. Now trying also with t1701 (which I hadn't noticed was a new
> test...).
Ran that overnight with the same conditions as before. 2683 OK runs and
0 failures (and counting). So it seems like the combination of the two
fixed the split index bugs.
>>> but I
>>> don't see how it could be added without some very liberal
>>> getenv("GIT_TEST_blahblah"), so it's probably best to not add it,
>>> particularly with the C rewrite of git-stash in-flight.
>>>
>>> I'll report back when I have enough test data to say how these patches
>>> affect the intermittent test failures under GIT_TEST_SPLIT_INDEX=yes.
Re: [PATCH v2 0/5] Fix the racy split index problem
On Thu, Sep 27 2018, SZEDER Gábor wrote:
> On Thu, Sep 27, 2018 at 03:53:24PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Thu, Sep 27 2018, SZEDER Gábor wrote:
>>
>> > This is the second attempt to fix the racy split index problem, which
>> > causes occasional failures in several random test scripts when run
>> > with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches
>> > 1 and 5 (corresponding to v1's 3 and 5).
>>
>> Thanks. I'm running the same sorts of tests I noted in
>> https://public-inbox.org/git/87va7ireuu@evledraar.gmail.com/ on
>> this. The fix Jeff had that you noted in
>> https://public-inbox.org/git/20180906151439.GA8016@localhost/ is now in
>> "master".
>>
>> I take it your
>> https://github.com/szeder/git/commits/racy-split-index-fix is the same
>> as this submission?
>
> Yes.
>
>> Anyway, I'm testing that cherry-picked on top of the
>> latest master.
>>
>> Unfortunate that we couldn't get the isolated test you made in
>> https://public-inbox.org/git/20180907034942.GA10370@localhost/
>
> Nah, that's not an isolated test case, that's only a somewhat
> narrowed-down, but rather reliable reproduction recipe while I still
> had no idea what was going on :)
>
> The _real_ isolated test is the last test in t1701, that's what it
> eventually boiled down to.
Thanks. I had ~400 runs of the tests I ran before and they were all
OK. Now trying also with t1701 (which I hadn't noticed was a new
test...).
>> but I
>> don't see how it could be added without some very liberal
>> getenv("GIT_TEST_blahblah"), so it's probably best to not add it,
>> particularly with the C rewrite of git-stash in-flight.
>>
>> I'll report back when I have enough test data to say how these patches
>> affect the intermittent test failures under GIT_TEST_SPLIT_INDEX=yes.
Re: [PATCH v2 0/5] Fix the racy split index problem
On Thu, Sep 27, 2018 at 03:53:24PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Thu, Sep 27 2018, SZEDER Gábor wrote:
>
> > This is the second attempt to fix the racy split index problem, which
> > causes occasional failures in several random test scripts when run
> > with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches
> > 1 and 5 (corresponding to v1's 3 and 5).
>
> Thanks. I'm running the same sorts of tests I noted in
> https://public-inbox.org/git/87va7ireuu@evledraar.gmail.com/ on
> this. The fix Jeff had that you noted in
> https://public-inbox.org/git/20180906151439.GA8016@localhost/ is now in
> "master".
>
> I take it your
> https://github.com/szeder/git/commits/racy-split-index-fix is the same
> as this submission?
Yes.
> Anyway, I'm testing that cherry-picked on top of the
> latest master.
>
> Unfortunate that we couldn't get the isolated test you made in
> https://public-inbox.org/git/20180907034942.GA10370@localhost/
Nah, that's not an isolated test case, that's only a somewhat
narrowed-down, but rather reliable reproduction recipe while I still
had no idea what was going on :)
The _real_ isolated test is the last test in t1701, that's what it
eventually boiled down to.
> but I
> don't see how it could be added without some very liberal
> getenv("GIT_TEST_blahblah"), so it's probably best to not add it,
> particularly with the C rewrite of git-stash in-flight.
>
> I'll report back when I have enough test data to say how these patches
> affect the intermittent test failures under GIT_TEST_SPLIT_INDEX=yes.
Re: [PATCH v2 0/5] Fix the racy split index problem
On Thu, Sep 27 2018, SZEDER Gábor wrote:
> This is the second attempt to fix the racy split index problem, which
> causes occasional failures in several random test scripts when run
> with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches
> 1 and 5 (corresponding to v1's 3 and 5).
Thanks. I'm running the same sorts of tests I noted in
https://public-inbox.org/git/87va7ireuu@evledraar.gmail.com/ on
this. The fix Jeff had that you noted in
https://public-inbox.org/git/20180906151439.GA8016@localhost/ is now in
"master".
I take it your
https://github.com/szeder/git/commits/racy-split-index-fix is the same
as this submission? Anyway, I'm testing that cherry-picked on top of the
latest master.
Unfortunate that we couldn't get the isolated test you made in
https://public-inbox.org/git/20180907034942.GA10370@localhost/ but I
don't see how it could be added without some very liberal
getenv("GIT_TEST_blahblah"), so it's probably best to not add it,
particularly with the C rewrite of git-stash in-flight.
I'll report back when I have enough test data to say how these patches
affect the intermittent test failures under GIT_TEST_SPLIT_INDEX=yes.
[PATCH v2 0/5] Fix the racy split index problem
This is the second attempt to fix the racy split index problem, which
causes occasional failures in several random test scripts when run
with 'GIT_TEST_SPLIT_INDEX=yes'. The important details are in patches
1 and 5 (corresponding to v1's 3 and 5).
Changes since v1:
- Most importanly, patch 5 adds a second is_racy_timestamp()
condition to prepare_to_write_split_index() to deal with those
racily clean cache entries that were copied from the shared index
by unpack_trees() while it created a new index.
This should fix the remainig failures in 't3903-stash.sh' with
split index enabled.
Furthermore, together with patch 4 they add a bunch of comments to
the most (alas, not all) conditions in
prepare_to_write_split_index()'s first loop, trying to explain in
plain English which cache entries are handled by each of those
conditions. I might have gone a bit overboard with the comments,
but I think they would have helped me a lot while working on these
fixes...
- Patch 1 adds one more failing test to demonstrate the raciness
after unpack_trees() created a new index.
Updated some of the comments in those tests, and I also reordered
some tests, because I found that it's more logical to explain them
in this order. (However, this made 'range-diff' think that it's a
total rewrite, which is not really true... but even though I could
convince it otherwise by adjusting the --creation-factor, the
results were utterly unreadable.)
- Patch 4 is new and... interesting? I'd like to hear Duy's
thoughts on this one.
- Patch 3 is new and... I hesitate to call it a bugfix, because,
luckily, there is no real and visible bug, but it's a fix
nonetheless.
- Commit message updates here and there.
- Addressed Ævar's nits in patch 2.
- The first two patches from v1 have already been merged to
'master' (cff90bdc5c (Merge branch 'sg/split-index-test',
2018-09-24)).
SZEDER Gábor (5):
split-index: add tests to demonstrate the racy split index problem
t1700-split-index: date back files to avoid racy situations
split-index: count the number of deleted entries
split-index: don't compare stat data of entries already marked for
split index
split-index: smudge and add racily clean cache entries to split index
cache.h | 2 +
read-cache.c| 2 +-
split-index.c | 121 +---
t/t1700-split-index.sh | 49 +
t/t1701-racy-split-index.sh | 214
5 files changed, 348 insertions(+), 40 deletions(-)
create mode 100755 t/t1701-racy-split-index.sh
Range-diff:
1: 71d973e4c0 < -: -- t1700-split-index: drop unnecessary 'grep'
2: 5d46f58a07 < -: -- t0090: disable GIT_TEST_SPLIT_INDEX for the
test checking split index
3: e60fa2dd9e < -: -- split index: add a test to demonstrate the racy
split index problem
-: -- > 1: a5b46af0ff split-index: add tests to demonstrate the racy
split index problem
4: 799c9c7739 ! 2: ed26c9703e t1700-split-index: date back files to avoid
racy situations
@@ -43,7 +43,7 @@
+# Create a file named as $1 with content read from stdin.
+# Set the file's mtime to a few seconds in the past to avoid racy
situations.
-+create_file () {
++create_non_racy_file () {
+ cat >"$1" &&
+ test-tool chmtime =-5 "$1"
+}
@@ -56,7 +56,7 @@
test_expect_success 'add one file' '
- : >one &&
-+ create_file one &&
++ create_non_racy_file one &&
git update-index --add one &&
git ls-files --stage >ls-files.actual &&
cat >ls-files.expect <<-EOF &&
@@ -65,7 +65,7 @@
test_expect_success 'modify original file, base index untouched' '
- echo modified >one &&
-+ echo modified |create_file one &&
++ echo modified | create_non_racy_file one &&
git update-index one &&
git ls-files --stage >ls-files.actual &&
cat >ls-files.expect <<-EOF &&
@@ -74,7 +74,7 @@
test_expect_success 'add another file, which stays index' '
- : >two &&
-+ create_file two &&
++ create_non_racy_file two &&
git update-index --add two &&
git ls-files --stage >ls-files.actual &&
cat >ls-files.expect <<-EOF &&
@@ -83,7 +83,7 @@
test_expect_success 'add original file back' '
- : >one &&
-+ create_file one &&
++ create_non_racy_file one &&
git update-index --add one &&
git ls-files --stage >ls-files.actual &&
cat >ls-files.expect <<-EOF &&
@@ -92,7 +92,7 @@
test_expect_success 'add new file' '
- : >two &&
-+ create_file two &&
++ create_non_racy_file two &&
git update-index --add two &&
git ls-files --stage >actual &&
cat >expect <<-EOF &&
@@ -101,7 +101,7 @@
