| Message ID | 8d69ba362261690e58b3879c33ac01c8888dc473.1573196960.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsmonitor: skip sanity check if the index is split | expand |
On Fri, Nov 08, 2019 at 07:09:20AM +0000, Utsav Shah via GitGitGadget wrote: > The checks added in 3444ec2eb2 ("fsmonitor: don't fill bitmap with > entries to be removed", 2019-10-11), to ensure that the > fsmonitor_dirty bitmap does not have more bits than the index > do not play well with the split index. > > git update-index --fsmonitor --split-index calls write_locked_index > which calls write_shared_index as well as write_split_index. > The first call fills up the fsmonitor_dirty bitmap, > and the second modifies the index such that istate->cache_nr is zero and > this assert is hit. Just to make sure that we are on the same page, is this the one? BUG: fsmonitor.c:88: fsmonitor_dirty has more entries than the index (102 > 1) > The test written does reproduce the error, but only flakily. There is > limited difference with GIT_TEST_FSMONITOR=fsmonitor-all or > GIT_TEST_FSMONITOR=fsmonitor-watchman, so the flakiness might come from > somewhere else, which I haven't tracked down. > > The test also requires checkout of a new branch, and checking out back > to master. It doesn't; see below. > It's clear that the index gets into some poor state through > these operations, and there is a deeper bug somewhere. > > At the very least, this patch mitigates an over-eager check for split > index users while maintaining good invariants for the standard case. > Also, I haven't been able to reproduce this with "standard" user > commands, like status/checkout/stash, so the blast radius seems limited. > > Helped-by: Kevin Willford <kewillf@microsoft.com> > Helped-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Utsav Shah <utsav@dropbox.com> > --- > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh > index d8df990972..b5029eff3e 100755 > --- a/t/t7519-status-fsmonitor.sh > +++ b/t/t7519-status-fsmonitor.sh > @@ -371,4 +371,27 @@ test_expect_success 'status succeeds after staging/unstaging ' ' > ) > ' > > +# Git will only split indices if we have a bunch of files created, > +# so that prep work of creating a few hundred files is required. 'git update-index --split-index' splits the index no matter what; it even splits an empty index. > +# Note that this test doesn't fail determinstically without > +# its corresponding bugfix. > +test_expect_success 'update-index succeeds after staging with split index' ' > + test_create_repo fsmonitor-stage-split && > + ( > + cd fsmonitor-stage-split && > + test_commit initial && > + files=$(test_seq 1 100) && > + echo "hello world" > file && > + touch $files && > + git add -A && > + git commit -m "next" && > + git config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-watchman" && > + echo "hello world" > file && > + git checkout -b new-branch && > + git checkout master && > + echo hello >> file && > + git update-index --split-index --untracked-cache --fsmonitor > + ) > +' I could reproduce the failure with '-r 30 --stress' relatively easily [1], but with those options I could shave off a lot from this test: test_create_repo fsmonitor-stage-split && ( cd fsmonitor-stage-split && >tracked && git add tracked && git config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-watchman" && >untracked && git update-index --split-index --untracked-cache --fsmonitor ) and could still trigger the failure: + git update-index --split-index --untracked-cache --fsmonitor open2: exec of watchman -j failed at /home/szeder/src/git/t/t7519/fsmonitor-watchman line 47. BUG: fsmonitor.c:88: fsmonitor_dirty has more entries than the index (1 > 0) [1] There is a quite expensive lazy prereq evaluation outside of 'test_expect_*' that I disabled it with the following for speedier stress testing: diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index 997d5fb349..103520415d 100755 --- a/t/t7519-status-fsmonitor.sh +++ b/t/t7519-status-fsmonitor.sh @@ -50,8 +50,7 @@ write_integration_script () { } test_lazy_prereq UNTRACKED_CACHE ' - { git update-index --test-untracked-cache; ret=$?; } && - test $ret -ne 1 + true ' test_expect_success 'setup' '
On Tue, Nov 12, 2019 at 3:18 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Fri, Nov 08, 2019 at 07:09:20AM +0000, Utsav Shah via GitGitGadget wrote: > > The checks added in 3444ec2eb2 ("fsmonitor: don't fill bitmap with > > entries to be removed", 2019-10-11), to ensure that the > > fsmonitor_dirty bitmap does not have more bits than the index > > do not play well with the split index. > > > > git update-index --fsmonitor --split-index calls write_locked_index > > which calls write_shared_index as well as write_split_index. > > The first call fills up the fsmonitor_dirty bitmap, > > and the second modifies the index such that istate->cache_nr is zero and > > this assert is hit. > > Just to make sure that we are on the same page, is this the one? > > BUG: fsmonitor.c:88: fsmonitor_dirty has more entries than the index (102 > 1) > Yes, that's the one. > > The test written does reproduce the error, but only flakily. There is > > limited difference with GIT_TEST_FSMONITOR=fsmonitor-all or > > GIT_TEST_FSMONITOR=fsmonitor-watchman, so the flakiness might come from > > somewhere else, which I haven't tracked down. > > > > The test also requires checkout of a new branch, and checking out back > > to master. > > It doesn't; see below. > > > It's clear that the index gets into some poor state through > > these operations, and there is a deeper bug somewhere. > > > > At the very least, this patch mitigates an over-eager check for split > > index users while maintaining good invariants for the standard case. > > Also, I haven't been able to reproduce this with "standard" user > > commands, like status/checkout/stash, so the blast radius seems limited. > > > > Helped-by: Kevin Willford <kewillf@microsoft.com> > > Helped-by: Junio C Hamano <gitster@pobox.com> > > Signed-off-by: Utsav Shah <utsav@dropbox.com> > > --- > > > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh > > index d8df990972..b5029eff3e 100755 > > --- a/t/t7519-status-fsmonitor.sh > > +++ b/t/t7519-status-fsmonitor.sh > > @@ -371,4 +371,27 @@ test_expect_success 'status succeeds after staging/unstaging ' ' > > ) > > ' > > > > +# Git will only split indices if we have a bunch of files created, > > +# so that prep work of creating a few hundred files is required. > > 'git update-index --split-index' splits the index no matter what; it > even splits an empty index. > > > +# Note that this test doesn't fail determinstically without > > +# its corresponding bugfix. > > +test_expect_success 'update-index succeeds after staging with split index' ' > > + test_create_repo fsmonitor-stage-split && > > + ( > > + cd fsmonitor-stage-split && > > + test_commit initial && > > + files=$(test_seq 1 100) && > > + echo "hello world" > file && > > + touch $files && > > + git add -A && > > + git commit -m "next" && > > + git config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-watchman" && > > + echo "hello world" > file && > > + git checkout -b new-branch && > > + git checkout master && > > + echo hello >> file && > > + git update-index --split-index --untracked-cache --fsmonitor > > + ) > > +' > > I could reproduce the failure with '-r 30 --stress' relatively easily > [1], but with those options I could shave off a lot from this test: > > test_create_repo fsmonitor-stage-split && > ( > cd fsmonitor-stage-split && > >tracked && > git add tracked && > git config core.fsmonitor > "$TEST_DIRECTORY/t7519/fsmonitor-watchman" && > >untracked && > git update-index --split-index --untracked-cache --fsmonitor > ) > > and could still trigger the failure: > > + git update-index --split-index --untracked-cache --fsmonitor > open2: exec of watchman -j failed at /home/szeder/src/git/t/t7519/fsmonitor-watchman line 47. > BUG: fsmonitor.c:88: fsmonitor_dirty has more entries than the index (1 > 0) > > > [1] There is a quite expensive lazy prereq evaluation outside of > 'test_expect_*' that I disabled it with the following for > speedier stress testing: > > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh > index 997d5fb349..103520415d 100755 > --- a/t/t7519-status-fsmonitor.sh > +++ b/t/t7519-status-fsmonitor.sh > @@ -50,8 +50,7 @@ write_integration_script () { > } > > test_lazy_prereq UNTRACKED_CACHE ' > - { git update-index --test-untracked-cache; ret=$?; } && > - test $ret -ne 1 > + true > ' > > test_expect_success 'setup' '
diff --git a/fsmonitor.c b/fsmonitor.c index 1f4aa1b150..01cba22b38 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -16,7 +16,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is) struct index_state *istate = (struct index_state *)is; struct cache_entry *ce; - if (pos >= istate->cache_nr) + if (!istate->split_index && pos >= istate->cache_nr) BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)", (uintmax_t)pos, istate->cache_nr); @@ -55,7 +55,7 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data, } istate->fsmonitor_dirty = fsmonitor_dirty; - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); @@ -83,7 +83,7 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) uint32_t ewah_size = 0; int fixup = 0; - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); @@ -252,7 +252,7 @@ void tweak_fsmonitor(struct index_state *istate) } /* Mark all previously saved entries as dirty */ - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr) + if (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr) BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)", (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr); ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index d8df990972..b5029eff3e 100755 --- a/t/t7519-status-fsmonitor.sh +++ b/t/t7519-status-fsmonitor.sh @@ -371,4 +371,27 @@ test_expect_success 'status succeeds after staging/unstaging ' ' ) ' +# Git will only split indices if we have a bunch of files created, +# so that prep work of creating a few hundred files is required. +# Note that this test doesn't fail determinstically without +# its corresponding bugfix. +test_expect_success 'update-index succeeds after staging with split index' ' + test_create_repo fsmonitor-stage-split && + ( + cd fsmonitor-stage-split && + test_commit initial && + files=$(test_seq 1 100) && + echo "hello world" > file && + touch $files && + git add -A && + git commit -m "next" && + git config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-watchman" && + echo "hello world" > file && + git checkout -b new-branch && + git checkout master && + echo hello >> file && + git update-index --split-index --untracked-cache --fsmonitor + ) +' + test_done