From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Duy Nguyen" <pclouds@gmail.com>,
"Thomas Gummerer" <t.gummerer@gmail.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Paul-Sebastian Ungureanu" <ungureanupaulsebastian@gmail.com>,
git@vger.kernel.org, "SZEDER Gábor" <szeder.dev@gmail.com>
Subject: [PATCH v4 0/6] Fix the racy split index problem
Date: Thu, 11 Oct 2018 11:43:03 +0200 [thread overview]
Message-ID: <20181011094309.18626-1-szeder.dev@gmail.com> (raw)
In-Reply-To: <20180928162459.17138-1-szeder.dev@gmail.com>
Fourth and hopefully final round of fixing occasional test failures when
run with 'GIT_TEST_SPLIT_INDEX=yes'. The only code change is the
extraction of a helper function to compare two cache entries' content,
and then a couple of minor log message clarifications. The range-diff
below is rather clear on that.
I will send a 7/6 follow-up patch shortly as well.
SZEDER Gábor (6):
t1700-split-index: document why FSMONITOR is disabled in this test
script
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 cached 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 | 131 +++++++++++++++++++---
t/t1700-split-index.sh | 52 +++++----
t/t1701-racy-split-index.sh | 214 ++++++++++++++++++++++++++++++++++++
5 files changed, 361 insertions(+), 40 deletions(-)
create mode 100755 t/t1701-racy-split-index.sh
Range-diff:
1: ba2b1bdf16 = 1: ba2b1bdf16 t1700-split-index: document why FSMONITOR is disabled in this test script
2: bf1b038f10 ! 2: c7cb9d9115 split-index: add tests to demonstrate the racy split index problem
@@ -136,13 +136,20 @@
git commands will then erroneously consider the file clean.
Note that in the last two 'test_expect_failure' cases I omitted the
- '#' (as in nr. of trial) from the tests' name on purpose for now, as
- it confuses 'prove' into thinking that those tests failed
- unexpectedly.
+ '#' (as in nr. of trial) from the tests' description on purpose for
+ now, as it breakes the TAP output [2]; it will be added at the end of
+ the series, when those two tests will be flipped to
+ 'test_expect_success'.
[1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge
branch 'nd/split-index', 2014-07-16).
+ [2] In the TAP output a '#' should separate the test's description
+ from the TODO directive emitted by 'test_expect_failure'. The
+ additional '#' in "#$trial" interferes with this, the test harness
+ won't recognize the TODO directive, and will report that those
+ tests failed unexpectedly.
+
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh
3: e7f7fb6e2d = 3: ce803d8064 t1700-split-index: date back files to avoid racy situations
4: 6dc0b01ad3 = 4: 1d12d718d1 split-index: count the number of deleted entries
5: 9c420f9c66 ! 5: 0dd448c707 split-index: don't compare stat data of entries already marked for split index
@@ -1,6 +1,6 @@
Author: SZEDER Gábor <szeder.dev@gmail.com>
- split-index: don't compare stat data of entries already marked for split index
+ split-index: don't compare cached data of entries already marked for split index
When unpack_trees() constructs a new index, it copies cache entries
from the original index [1]. prepare_to_write_split_index() has to
@@ -20,7 +20,9 @@
So modify prepare_to_write_split_index() to check the copied cache
entries' CE_UPDATE_IN_BASE flag first, and skip the thorough
- comparison of cached data if the flag is already set.
+ comparison of cached data if the flag is already set. Those couple of
+ lines comparing the cached data would then have too many levels of
+ indentation, so extract them into a helper function.
Note that comparing the cached data in copied and original entries in
the shared index might actually be entirely unnecessary. In theory
@@ -62,6 +64,37 @@
diff --git a/split-index.c b/split-index.c
--- a/split-index.c
+++ b/split-index.c
+@@
+ si->saved_cache_nr = 0;
+ }
+
++/*
++ * Compare most of the fields in two cache entries, i.e. all except the
++ * hashmap_entry and the name.
++ */
++static int compare_ce_content(struct cache_entry *a, struct cache_entry *b)
++{
++ const unsigned int ondisk_flags = CE_STAGEMASK | CE_VALID |
++ CE_EXTENDED_FLAGS;
++ unsigned int ce_flags = a->ce_flags;
++ unsigned int base_flags = b->ce_flags;
++ int ret;
++
++ /* only on-disk flags matter */
++ a->ce_flags &= ondisk_flags;
++ b->ce_flags &= ondisk_flags;
++ ret = memcmp(&a->ce_stat_data, &b->ce_stat_data,
++ offsetof(struct cache_entry, name) -
++ offsetof(struct cache_entry, ce_stat_data));
++ a->ce_flags = ce_flags;
++ b->ce_flags = base_flags;
++
++ return ret;
++}
++
+ void prepare_to_write_split_index(struct index_state *istate)
+ {
+ struct split_index *si = init_split_index(istate);
@@
*/
for (i = 0; i < istate->cache_nr; i++) {
@@ -137,21 +170,7 @@
+ * code paths modifying the cached data do
+ * set CE_UPDATE_IN_BASE as well.
+ */
-+ const unsigned int ondisk_flags =
-+ CE_STAGEMASK | CE_VALID |
-+ CE_EXTENDED_FLAGS;
-+ unsigned int ce_flags, base_flags, ret;
-+ ce_flags = ce->ce_flags;
-+ base_flags = base->ce_flags;
-+ /* only on-disk flags matter */
-+ ce->ce_flags &= ondisk_flags;
-+ base->ce_flags &= ondisk_flags;
-+ ret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,
-+ offsetof(struct cache_entry, name) -
-+ offsetof(struct cache_entry, ce_stat_data));
-+ ce->ce_flags = ce_flags;
-+ base->ce_flags = base_flags;
-+ if (ret)
++ if (compare_ce_content(ce, base))
+ ce->ce_flags |= CE_UPDATE_IN_BASE;
+ }
discard_cache_entry(base);
6: 52c755f210 ! 6: 384b440345 split-index: smudge and add racily clean cache entries to split index
@@ -46,6 +46,11 @@
racily clean cache entries as well, and will then write them with
smudged stat data to the new split index.
+ This change makes all tests in 't1701-racy-split-index.sh' pass, so
+ flip the two 'test_expect_failure' tests to success. Also add the '#'
+ (as in nr. of trial) to those tests' description that were omitted
+ when the tests expected failure.
+
Note that after this change if the index is split when it contains a
racily clean cache entry, then a smudged cache entry will be written
both to the new shared and to the new split indexes. This doesn't
--
2.19.1.465.gaff195083f
next prev parent reply other threads:[~2018-10-11 9:43 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-27 12:44 [PATCH v2 0/5] Fix the racy split index problem SZEDER Gábor
2018-09-27 12:44 ` [PATCH v2 1/5] split-index: add tests to demonstrate " SZEDER Gábor
2018-09-28 0:48 ` SZEDER Gábor
2018-09-28 2:40 ` SZEDER Gábor
2018-09-28 17:30 ` Junio C Hamano
2018-09-27 12:44 ` [PATCH v2 2/5] t1700-split-index: date back files to avoid racy situations SZEDER Gábor
2018-09-27 12:44 ` [PATCH v2 3/5] split-index: count the number of deleted entries SZEDER Gábor
2018-09-27 12:44 ` [PATCH v2 4/5] split-index: don't compare stat data of entries already marked for split index SZEDER Gábor
2018-09-27 13:43 ` SZEDER Gábor
2018-09-27 12:44 ` [PATCH v2 5/5] split-index: smudge and add racily clean cache entries to " SZEDER Gábor
2018-09-27 13:53 ` [PATCH v2 0/5] Fix the racy split index problem Ævar Arnfjörð Bjarmason
2018-09-27 14:23 ` SZEDER Gábor
2018-09-27 15:25 ` Ævar Arnfjörð Bjarmason
2018-09-28 6:57 ` Ævar Arnfjörð Bjarmason
2018-09-28 10:17 ` SZEDER Gábor
2018-10-08 14:54 ` Ævar Arnfjörð Bjarmason
2018-10-08 15:41 ` SZEDER Gábor
2018-09-28 16:24 ` [PATCH v3 0/6] " SZEDER Gábor
2018-09-28 16:24 ` [PATCH v3 1/6] t1700-split-index: document why FSMONITOR is disabled in this test script SZEDER Gábor
2018-09-28 16:24 ` [PATCH v3 2/6] split-index: add tests to demonstrate the racy split index problem SZEDER Gábor
2018-09-28 16:24 ` [PATCH v3 3/6] t1700-split-index: date back files to avoid racy situations SZEDER Gábor
2018-09-28 16:24 ` [PATCH v3 4/6] split-index: count the number of deleted entries SZEDER Gábor
2018-09-28 16:24 ` [PATCH v3 5/6] split-index: don't compare stat data of entries already marked for split index SZEDER Gábor
2018-09-29 5:36 ` Duy Nguyen
2018-09-29 9:14 ` SZEDER Gábor
2018-09-29 10:07 ` SZEDER Gábor
2018-09-28 16:24 ` [PATCH v3 6/6] split-index: smudge and add racily clean cache entries to " SZEDER Gábor
2018-09-29 5:21 ` Duy Nguyen
2018-09-29 7:57 ` SZEDER Gábor
2018-09-30 14:47 ` [PATCH v3 0/6] Fix the racy split index problem SZEDER Gábor
2018-10-05 6:15 ` Junio C Hamano
2018-10-11 9:43 ` SZEDER Gábor [this message]
2018-10-11 9:43 ` [PATCH v4 1/6] t1700-split-index: document why FSMONITOR is disabled in this test script SZEDER Gábor
2018-10-11 9:43 ` [PATCH v4 2/6] split-index: add tests to demonstrate the racy split index problem SZEDER Gábor
2018-10-11 9:43 ` [PATCH v4 3/6] t1700-split-index: date back files to avoid racy situations SZEDER Gábor
2018-10-11 9:43 ` [PATCH v4 4/6] split-index: count the number of deleted entries SZEDER Gábor
2018-10-11 9:43 ` [PATCH v4 5/6] split-index: don't compare cached data of entries already marked for split index SZEDER Gábor
2018-10-11 9:43 ` [PATCH v4 6/6] split-index: smudge and add racily clean cache entries to " SZEDER Gábor
2018-10-11 9:53 ` [PATCH 7/6] split-index: BUG() when cache entry refers to non-existing shared entry SZEDER Gábor
2018-10-11 10:36 ` [PATCH v4 0/6] Fix the racy split index problem Ævar Arnfjörð Bjarmason
2018-10-11 11:38 ` SZEDER Gábor
2018-10-12 3:20 ` Junio C Hamano
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181011094309.18626-1-szeder.dev@gmail.com \
--to=szeder.dev@gmail.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=t.gummerer@gmail.com \
--cc=ungureanupaulsebastian@gmail.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.
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 NNTP newsgroup(s).