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

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 27, 2026, 18:14 UTC
Message-ID: <aaHfF-CbuEiVJmlS@pks.im>
In-Reply-To: <aaGWRWcxEtLD1OlK@fruit.crustytoothpaste.net>

On Fri, Feb 27, 2026 at 01:04:05PM +0000, brian m. carlson wrote:

Show 28 quoted lines

> On 2026-02-27 at 11:23:01, Patrick Steinhardt wrote:
> > diff --git a/upload-pack.c b/upload-pack.c
> > index c2643c0295..f8ba245616 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -270,6 +270,13 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * Make sure that we buffer some data before sending it to the client.
> > +	 * This significantly reduces the number of write(3p) syscalls.
> > +	 */
> > +	if (readsz && os->used < (LARGE_PACKET_DATA_MAX * 2 / 3))
> > +		return readsz;
> > +
> >  	if (os->used > 1) {
> >  		send_client_data(1, os->buffer, os->used - 1, use_sideband);
> >  		os->buffer[0] = os->buffer[os->used - 1];
> 
> This seems mostly reasonable and well-explained.  The one question I
> have is this: how does this work when packfile generation is actually
> very slow (or when the connection is slow) and we need to send data
> every so often to keep the connection alive?
> 
> I just want to make sure we're not breaking the keepalive sideband case
> when that's necessary, but of course I have no objections to improving
> performance and reducing overhead.

Right. `relay_pack_data()` is handling the logic to soak up data from git-pack-objects(1). The outer loop is in `create_pack_file()`, and there we already have a timeout configured. If data is produced too slow, then we'd eventually land in there and send the keepalive packet.

I guess there is an interesting edge case here: if git-pack-objects(1) creates bytes fast enough to not hit the 1 second timeout, but slow enough to basically never fill the buffer, then we could potentially run into a timeout eventually.

I'm not sure whether this is likely to happen, and whether it's something we want to address. Maybe we should extend the logic so that we also send the keepalive pkt-line in case we have only been buffering data for the last 1 second?

Basically, we'd reset a timestamp every time data was sent. If we see that we received data without writing it out, and if the current time is one second beyond the last time we reset, then we might want to send out the keepalive.

Patrick