RE: [PATCH 0/1] fsmonitor: skip sanity check if the index is split

From: Utsav Shah <redacted>
Sent: Monday, November 11, 2019 10:26 AM

On Mon, Nov 11, 2019 at 8:55 AM Kevin Willford
[off-list ref] wrote:

quoted

quoted

From: git-owner@vger.kernel.org <redacted> On
Behalf Of Junio C Hamano
Sent: Sunday, November 10, 2019 7:01 PM

Junio C Hamano [off-list ref] writes:

quoted

"Utsav Shah via GitGitGadget" [off-list ref] writes:

quoted

At the very least, this patch mitigates an over-eager check for
split index users while maintaining good invariants for the standard

case.

quoted

quoted

quoted

OK, it sounds more like this "it does not make any sense to
compare the position in the fsmonitor bitmap (which covers the
entire thing) with the position in just a split part of the index
(which covers only the delta over the base index)"?  If that is
the case, it means that the "check" is even worse than being
"over-eager"---it simply is not correct.

Having said all that, I wonder if we are doing the right thing with
or without 3444ec2e ("fsmonitor: don't fill bitmap with entries to
be removed", 2019-10-
11) in the split-index mode in the first place.

The fact that your "loosen the check and allow 'pos' that identifies
a tracked path used by the fsmonitor bitmap to be larger than the
size of the istate-

quoted

cache[]" patch under discussion is needed is that 'pos' may
sometimes be

larger than isate->cache[] no?  Then what happens in this hunk, for

example?

quoted

quoted

diff --git a/fsmonitor.c b/fsmonitor.c index 231e83a94d..1f4aa1b150

100644

--- a/fsmonitor.c
+++ b/fsmonitor.c

@@ -14,8 +14,13 @@ struct trace_key trace_fsmonitor =

TRACE_KEY_INIT(FSMONITOR);  static void
fsmonitor_ewah_callback(size_t pos, void *is)  {
      struct index_state *istate = (struct index_state *)is;
-     struct cache_entry *ce = istate->cache[pos];
+     struct cache_entry *ce;

+     if (pos >= istate->cache_nr)
+             BUG("fsmonitor_dirty has more entries than the index
(%"PRIuMAX" >= %u)",
+                 (uintmax_t)pos, istate->cache_nr);
+
+     ce = istate->cache[pos];
      ce->ce_flags &= ~CE_FSMONITOR_VALID;

The istate->cache[] is a dynamic array whose size is managed via the
usual
ALLOC_GROW() using istate->cache_nr and istate->cache_alloc, whether
the split-index feature is in use.  When your patch makes a
difference, then, doesn't the access to istate->cache[] pick up a
random garbage and then flip the bit?

Puzzled...  In any case, "check is worse than over-eager, it simply
is wrong" I wrote in the message I am responding to is totally
incorrect, it seems.  It smells like lifting the check would just
hide the underlying problem under the rug?

I agree.  The only 2 places that excluding the split-index make sense
are in read_fsmonitor_extension and write_fsmonitor_extension because
the index_state that is being passing into those methods could be the
delta index in which case the number of entries for the fsmonitor
bitmap would almost always be more and cause the BUG to be hit which it

should not be.

quoted

The reason it is not needed and should not be in the other 2 places is
they are ran from tweak_fsmonitor which is ran at post_read_index_from
which is after the base and delta indexes have been loaded into the
indes_state and the index_state will have all the entries and if the
fsmonitor bitmap is bigger than the number of entries then the BUG should

be hit.

Thanks. What exactly is the delta index? Is it the "split" index, vs the shared
indices?

Yes the delta is the same as the split index mentioned here
https://git-scm.com/docs/git-update-index#_split_index.

I was surprised to see cache_nr being zero. My understanding was
that cache and cache_nr would always be the materialized version of the
entire index, which is clearly incorrect.

Most of the time that is correct but if you look in read_index_from, the
index is loaded with the call to

ret = do_read_index(istate, path, 0);

This will read the index extensions so read_fsmonitor_extension will be
called and the cache will only have the entries from the split/delta index.

The base/shared index isn't loaded and in the cache until later when
merge_base_index(istate); is called which is right before the call to
post_read_index_from where tweak_fsmonitor will get called from.