Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data

From: Jeff King <peff@peff.net>
Date: Mar 3, 2026, 13:35 UTC
Message-ID: <20260303133540.GA818878@coredump.intra.peff.net>
In-Reply-To: <aaaqgrmOBj-Ly1Vx@pks.im>

On Tue, Mar 03, 2026 at 10:31:46AM +0100, Patrick Steinhardt wrote:

Show 11 quoted lines

> > As far as doing both, I'm not sure if it's worth it. My two concerns
> > are:
> > 
> >   1. It re-opens the question of whether upload-pack might stall waiting
> >      to fill its buffer and fail to produce keepalives correctly.
> 
> I've got a patch for that. The problem can even trigger right now as we
> already do buffer some of the data, and that may cause the keepalives to
> be missed. But this only happens initially in our current
> infrastructure, before we see the "PACK" signature, so it's unlikely to
> be a problem in practice.

I'm not sure what you mean by "this only happens initially" here. If it is: we can only miss keepalives in that time, then I think that is probably a real problem. The time we _most_ need keepalives is before we see the PACK signature, because that is when pack-objects is chewing on the input, looking for deltas, etc, and not producing any output.

It is usually "solved" by pack-objects producing progress over stderr, but for "--quiet" fetches, it could produce nothing for a long time.

But anyway, if you are fixing it either way, then I am happy. :)

Show 10 quoted lines

> We would likely hit this issue if we insist on the buffer being
> completely filled before sending it out. But that's why I adapted the
> logic to say that we send out once we've filled it at least 2/3rds of
> the pktline limit. So in your case above we wouldn't face an issue as
> we'd already send the first 50kB, as it is smaller than 2/3rds of the
> maximum length (~42kB).
> 
> That being said, you'll still be able to construct cases where we have
> weird edge cases. For example if you consistently send one byte less
> than 2/3rds of the capacity.

Right, my numbers were just meant as examples. Whatever the values, it means that whatever is generating the pack data (pack-objects or otherwise) really wants to be in sync with how upload-pack is buffering. Or vice versa. If we just pass back whole chunks of what we read() in upload-pack, then that happens automatically.

-Peff