Re: [PATCH 32/36] pathspec: allow querying for attributes

Stefan Beller <sbeller@google.com> writes:

I was looking at preload-index.c and its history again, because I wanted to ensure that Lars's process filter stuff introduces no unexpected interactions with getting multi-threaded, similar to the problem we had in the earlier incarnations of this step, which we worked around with <xmqqshwvvaxq.fsf@gitster.mtv.corp.google.com> "pathspec: disable preload-index when attribute pathspec magic is in use".

I think that Lars's series is safe, because the only thing among what preload-index.c::preload_thread() does that can go outside the simple stat data and pattern matching with the pathname is the racy-git check in ie_match_stat(), and the object store access in that call was explicitly disabled by 7c4ea599b0 ("Fix index preloading for racy dirty case", 2008-11-17) long time ago.

By passing CE_MATCH_RACY_IS_DIRTY option to the call, this caller effectively says "A cache entry whose stat data matches may be actually dirty when the timestamp is racy, in which case we usually compare data to determine if it really is clean, but it is OK to err on the safe and lazy side by declaring it dirty and not marking it up-to-date while we are preloading. Do not bother to go to the object store".

The reason why I am bringing this up in this discussion thread on this patch is because I wonder if we would benefit by a similar "let's not do too involved things and be cheap by erring on the safe and lazy side" strategy in the call to ce_path_match() call made in this function to avoid making calls to the attr subsystem.

In other words, would it help the system by either simplifying the processing done or reducing the cycle spent in preload_thread() if we could tell ce_path_match() "A pathspec we are checking may require not just the pattern to match but also attributes given to the path to satisfy the criteria, but for the purpose of preloading, pretend that the attribute satisfies the match criteria" (or "pretend that it does not match"), thereby not having to make any call into the attribute subsystem at all from this codepath?

The strategy this round takes to make it unnecessary to punt preloading (i.e. dropping "pathspec: disable preload-index when attribute pathspec magic is in use" patch the old series had) is to make the attribute subsystem thread-safe. But that thread-safety in the initial round is based on a single Big Attribute Lock, so it may turn out that the end result performs better for this codepath if we did not to make any call into the attribute subsystem.

I am probably being two step ahead of ourselves by saying the above, which is just something to keep in mind as a possible solution if performance in this preload codepath becomes an issue when the pathspec has attributes match specified.