Re: [PATCH 2/2] reftable/stack: fix race in up-to-date check

From: Jeff King <peff@peff.net>
Date: Jan 23, 2024, 00:32 UTC
Message-ID: <20240123003244.GB827890@coredump.intra.peff.net>
In-Reply-To: <Za5ELqZps0DLbb5q@tanuki>

On Mon, Jan 22, 2024 at 11:32:14AM +0100, Patrick Steinhardt wrote:

Show 7 quoted lines

> > I didn't think too hard about the details, but does this mean that
> > every user of stat_validity_check() has the same issue? The other big
> > one is packed-refs (for which the code was originally written). Should
> > this fix go into that API?
> 
> In theory, the issue is the same for the `packed-refs` file. But in
> practice it's much less likely to be an issue:

Thanks for laying this all out. It does concern me a little that there's still a possible race, because they can be so hard to catch and debug in practice. But I think you make a compelling argument that it's probably not happening a lot in practice, and especially...

Show 8 quoted lines

> Also, applying the same fix for the packed-refs would essentially mean
> that the caching mechanism is now ineffective on Windows systems where
> we do not have proper `st_dev` and `st_ino` values available. I think
> this is a no-go in the context of packed-refs because we don't have a
> second-level caching mechanism like we do in the reftable backend. It's
> not great that we have to reread the "tables.list" file on Windows
> systems for now, but at least it's better than having to reread the
> complete "packed-refs" file like we'd have to do with the files backend.

...here that the performance profile is so different. If the "fix" means re-reading the whole packed-refs file constantly, that's going to be quite noticeable.

Given that this race has been here forever-ish, I agree with you that we should leave it be.

-Peff