Re: [PATCH 01/10] packed-backend: don't adjust the reference count on lock/unlock

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 8, 2017, 10:02 UTC
Message-ID: <d49a7b75-66d4-7c63-d260-71c7413b3571@alum.mit.edu>
In-Reply-To: <20170908065223.f7f52hjcx5qsjbch@sigill.intra.peff.net>

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

Show 46 quoted lines

> On Tue, Aug 29, 2017 at 10:20:25AM +0200, Michael Haggerty wrote:
> 
>> 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).
> 
>> 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 :-)

Show 13 quoted lines

>> @@ -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