On 09/08/2017 08:52 AM, Jeff King wrote:

On Tue, Aug 29, 2017 at 10:20:25AM +0200, Michael Haggerty wrote:

quoted

The old code incremented the packed ref cache reference count when
acquiring the packed-refs lock, and decremented the count when
releasing the lock. This is unnecessary because a locked packed-refs
file cannot be changed, so there is no reason for the cache to become
stale.

Hmm, I thought that after your last series, we might hold the lock but
update the packed-refs from a separate tempfile. I.e., 42dfa7ecef
(commit_packed_refs(): use a staging file separate from the lockfile,
2017-06-23).

quoted

Moreover, the extra reference count causes a problem if we
intentionally clear the packed refs cache, as we sometimes need to do
if we change the cache in anticipation of writing a change to disk,
but then the write to disk fails. In that case, `packed_refs_unlock()`
would have no easy way to find the cache whose reference count it
needs to decrement.

This whole issue will soon become moot due to upcoming changes that
avoid changing the in-memory cache as part of updating the packed-refs
on disk, but this change makes that transition easier.

All of this makes sense, and I'm happy this complexity is going away in
the long run. But I guess what I'm wondering is in the meantime if we
can have a sequence like:

  1. We hold packed-refs.lock

  2. We update the file without releasing the lock, via 42dfa7ecef.

  3. Still holding the lock, we try to look at packed-refs. The
     stat_validity code says no, we're not up to date.

  4. We discard the old packed_ref_cache and reload it. Because its
     reference count was not incremented during step 1, we actually
     free() it.

  5. We try to look at at the old freed pointer.

There are several steps in there that might be implausible. So I'm
mostly playing devil's advocate here.

I'm wondering if the "don't validate while we hold the lock" logic in
get_packed_refs_cache() means that step 3 is impossible.

That's one of the reasons your scenario can't happen, but that just begs
the question of whether the "don't validate while we hold the lock" code
is wrongheaded.

In fact it's OK. The method by which the packed-refs file on disk is
modified at this point in the patch series is by modifying the packed
ref-cache and then writing the data from the ref-cache to disk. So the
packed ref-cache remains fresh because any changes that we plan to make
to the file are made in the cache first anyway.

I'll explain that a bit better in the log message.

The next question is whether this change interacts badly with changes
later in the patch series, when we cease to modify the ref-cache before
writing the new packed-refs file. Here we're OK, too, because
immediately after `rename_tempfile()` is used to rename the new
packed-refs file into place, we clear the packed ref-cache, so no
subsequent callers of `get_packed_refs_cache()` should see the stale cache.

Which in turn is partly because your step 5 is also implausible: code
shouldn't be holding a pointer to the packed ref-cache across operations
that might change the file. (Code that calls `get_packed_refs_cache()`
is OK because that function would see that `refs->cache` is NULL and
reload it regardless of whether we hold the lock.)

So I think everything is OK, but thanks for making me think through and
explain it better :-)

quoted

@@ -560,9 +559,7 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
 	 */
 	validate_packed_ref_cache(refs);
 
-	packed_ref_cache = get_packed_ref_cache(refs);
-	/* Increment the reference count to prevent it from being freed: */
-	acquire_packed_ref_cache(packed_ref_cache);
+	get_packed_ref_cache(refs);

It seems a bit funny to call a "get" function and throw away the return
value. Presumably we care about its side effect of updating refs->cache.
That might be worth a comment (though if this is all going away soon, I
care a lot less about niceties like that).

I'm rerolling anyway, so I'll add a comment. Thanks.

Michael