{"thread":{"id":"65085","subject":"[PATCH 0/2] upload-pack: reduce lock contention when writing packfile data","startedAt":"2026-02-27T11:26:57Z","lastAt":"2026-03-11T06:28:02Z","messageCount":53,"participants":["Patrick Steinhardt","brian m. carlson","Junio C Hamano","Jeff King","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"537281","messageId":"20260227-pks-upload-pack-write-contention-v1-0-7166fe255704@pks.im","threadId":"65085","inReplyTo":null,"subject":"[PATCH 0/2] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-27T11:22:59Z","receivedAt":"2026-02-27T11:26:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series fixes some heavy lock contention when writing\ndata from git-upload-pack(1) into pipes. This lock contention can be\nobserved when having hundreds of git-upload-pack(1) processes active at\nthe same time that write data into pipes at dozens of gigabits per\nsecond.\n\nI have uploaded the flame graph that clearly shows the lock contention\nat [1].\n\nThanks!\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/git/-/work_items/675\n\n---\nPatrick Steinhardt (2):\n upload-pack: fix debug statement when flushing packfile data\n upload-pack: reduce lock contention when writing packfile data\n\n upload-pack.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\n\n---\nbase-commit: 7b2bccb0d58d4f24705bf985de1f4612e4cf06e5\nchange-id: 20260227-pks-upload-pack-write-contention-435ce01f5fe9\n\n"},{"id":"537282","messageId":"20260227-pks-upload-pack-write-contention-v1-1-7166fe255704@pks.im","threadId":"65085","inReplyTo":"20260227-pks-upload-pack-write-contention-v1-0-7166fe255704@pks.im","subject":"[PATCH 1/2] upload-pack: fix debug statement when flushing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-27T11:23:00Z","receivedAt":"2026-02-27T11:26:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When git-upload-pack(1) writes packfile data to the client we have some\nlogic in place that buffers some partial lines. When that buffer still\ncontains data after git-pack-objects(1) has finished we flush the buffer\nso that all remaining bytes are sent out.\n\nCuriously, when we do so we also print the string \"flushed.\" to stderr.\nThis statement has been introduced in b1c71b7281 (upload-pack: avoid\nsending an incomplete pack upon failure, 2006-06-20), so quite a while\nago. What's interesting though is that stderr is typically spliced\nthrough to the client-side, and consequently the client would see this\nmessage. Munging the way how we do the caching indeed confirms this:\n\n $ git clone file:///home/pks/Development/linux/\n Cloning into bare repository 'linux.git'...\n remote: Enumerating objects: 12980346, done.\n remote: Counting objects: 100% (131820/131820), done.\n remote: Compressing objects: 100% (50290/50290), done.\n remote: Total 12980346 (delta 96319), reused 104500 (delta 81217), pack-reused 12848526 (from 1)\n Receiving objects: 100% (12980346/12980346), 3.23 GiB | 57.44 MiB/s, done.\n flushed.\n Resolving deltas: 100% (10676718/10676718), done.\n\nIt's quite clear that this string shouldn't ever be visible to the\nclient, so it rather feels like this is a left-over debug statement. The\nmenitoned commit doesn't mention this line, either.\n\nRemove the debug output to prepare for a change in how we do the\nbuffering in the next commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 2d2b70cbf2..c2643c0295 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -457,11 +457,9 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t}\n \n \t/* flush the data */\n-\tif (output_state->used > 0) {\n+\tif (output_state->used > 0)\n \t\tsend_client_data(1, output_state->buffer, output_state->used,\n \t\t\t\t pack_data->use_sideband);\n-\t\tfprintf(stderr, \"flushed.\\n\");\n-\t}\n \tfree(output_state);\n \tif (pack_data->use_sideband)\n \t\tpacket_flush(1);\n\n-- \n2.53.0.536.g309c995771.dirty\n\n"},{"id":"537283","messageId":"20260227-pks-upload-pack-write-contention-v1-2-7166fe255704@pks.im","threadId":"65085","inReplyTo":"20260227-pks-upload-pack-write-contention-v1-0-7166fe255704@pks.im","subject":"[PATCH 2/2] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-27T11:23:01Z","receivedAt":"2026-02-27T11:27:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In our production systems we have recently observed write contention in\ngit-upload-pack(1). The system in question was consistently streaming\npackfiles at a rate of dozens of gigabits per second, but curiously the\nsystem was neither bottlenecked on CPU, memory or IOPS.\n\nWe eventually discovered that Git was spending 80% of its time in\n`pipe_write()`, out of which almost all of the time was spent in the\n`ep_poll_callback` function in the kernel. Quoting the reporter:\n\n This infrastructure is part of an event notification queue designed to\n allow for multiple producers to emit events, but that concurrency\n safety is guarded by 3 layers of locking. The layer we're hitting\n contention in uses a simple reader/writer lock mode (a.k.a. shared\n versus exclusive mode), where producers need shared-mode (read mode),\n and various other actions use exclusive (write) mode.\n\nThe system in question generates workloads where we have hundreds of\ngit-upload-pack(1) processes active at the same point in time. These\nprocesses end up contending around those locks, and the consequence is\nthat the Git processes stall.\n\nNow git-upload-pack(1) already has the infrastructure in place to buffer\nsome of the data it reads from git-pack-objects(1) before actually\nsending it out. We only use this infrastructure in very limited ways\nthough, so we generally end up matching one read(3p) call with one\nwrite(3p) call. Even worse, when the sideband is enabled we end up\nmatching one read with _two_ writes: one for the pkt-line length, and\none for the packfile data.\n\nExtend our use of the buffering infrastructure so that we soak up bytes\nuntil the buffer is filled up at least 2/3rds of its capacity. The\nchange is relatively simple to implement as we already know to flush the\nbuffer in `create_pack_file()` after git-pack-objects(1) has finished.\n\nThis significantly reduces the number of write(3p) syscalls we need to\ndo. Before this change, cloning the Linux repository resulted in around\n400,000 write(3p) syscalls. With the buffering in place we only do\naround 130,000 syscalls.\n\nNow we could of course go even further and make sure that we always fill\nup the whole buffer. But this might cause an increase in read(3p)\nsyscalls, and some tests show that this only reduces the number of\nwrite(3p) syscalls from 130,000 to 100,000. So overall this doesn't seem\nworth it.\n\nHelped-by: Matt Smiley \nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c2643c0295..f8ba245616 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -270,6 +270,13 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t\t}\n \t}\n \n+\t/*\n+\t * Make sure that we buffer some data before sending it to the client.\n+\t * This significantly reduces the number of write(3p) syscalls.\n+\t */\n+\tif (readsz && os->used < (LARGE_PACKET_DATA_MAX * 2 / 3))\n+\t\treturn readsz;\n+\n \tif (os->used > 1) {\n \t\tsend_client_data(1, os->buffer, os->used - 1, use_sideband);\n \t\tos->buffer[0] = os->buffer[os->used - 1];\n\n-- \n2.53.0.536.g309c995771.dirty\n\n"},{"id":"537287","messageId":"aaGWRWcxEtLD1OlK@fruit.crustytoothpaste.net","threadId":"65085","inReplyTo":"20260227-pks-upload-pack-write-contention-v1-2-7166fe255704@pks.im","subject":"Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-02-27T13:04:05Z","receivedAt":"2026-02-27T13:09:34Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2026-02-27 at 11:23:01, Patrick Steinhardt wrote:\n> diff --git a/upload-pack.c b/upload-pack.c\n> index c2643c0295..f8ba245616 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -270,6 +270,13 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n> \t\t}\n> \t}\n> \n> +\t/*\n> +\t * Make sure that we buffer some data before sending it to the client.\n> +\t * This significantly reduces the number of write(3p) syscalls.\n> +\t */\n> +\tif (readsz && os->used < (LARGE_PACKET_DATA_MAX * 2 / 3))\n> +\t\treturn readsz;\n> +\n> \tif (os->used > 1) {\n> \t\tsend_client_data(1, os->buffer, os->used - 1, use_sideband);\n> \t\tos->buffer[0] = os->buffer[os->used - 1];\n\nThis seems mostly reasonable and well-explained. The one question I\nhave is this: how does this work when packfile generation is actually\nvery slow (or when the connection is slow) and we need to send data\nevery so often to keep the connection alive?\n\nI just want to make sure we're not breaking the keepalive sideband case\nwhen that's necessary, but of course I have no objections to improving\nperformance and reducing overhead.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"537309","messageId":"xmqqpl5qrt7n.fsf@gitster.g","threadId":"65085","inReplyTo":"20260227-pks-upload-pack-write-contention-v1-2-7166fe255704@pks.im","subject":"Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-27T17:29:00Z","receivedAt":"2026-02-27T17:29:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> ...\n> write(3p) call. Even worse, when the sideband is enabled we end up\n> matching one read with _two_ writes: one for the pkt-line length, and\n> one for the packfile data.\n>\n> Extend our use of the buffering infrastructure so that we soak up bytes\n> until the buffer is filled up at least 2/3rds of its capacity. The\n> change is relatively simple to implement as we already know to flush the\n> buffer in `create_pack_file()` after git-pack-objects(1) has finished.\n>\n> This significantly reduces the number of write(3p) syscalls we need to\n> do. Before this change, cloning the Linux repository resulted in around\n> 400,000 write(3p) syscalls. With the buffering in place we only do\n> around 130,000 syscalls.\n>\n> Now we could of course go even further and make sure that we always fill\n> up the whole buffer. But this might cause an increase in read(3p)\n> syscalls, and some tests show that this only reduces the number of\n> write(3p) syscalls from 130,000 to 100,000. So overall this doesn't seem\n> worth it.\n\nVery well reasoned. I love this size ratio between explanation and\ncode change ;-)\n\n>\n> Helped-by: Matt Smiley \n> Signed-off-by: Patrick Steinhardt \n> ---\n> upload-pack.c | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index c2643c0295..f8ba245616 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -270,6 +270,13 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n> \t\t}\n> \t}\n> \n> +\t/*\n> +\t * Make sure that we buffer some data before sending it to the client.\n> +\t * This significantly reduces the number of write(3p) syscalls.\n> +\t */\n> +\tif (readsz && os->used < (LARGE_PACKET_DATA_MAX * 2 / 3))\n> +\t\treturn readsz;\n> +\n> \tif (os->used > 1) {\n> \t\tsend_client_data(1, os->buffer, os->used - 1, use_sideband);\n> \t\tos->buffer[0] = os->buffer[os->used - 1];\n"},{"id":"537313","messageId":"aaHfF-CbuEiVJmlS@pks.im","threadId":"65085","inReplyTo":"aaGWRWcxEtLD1OlK@fruit.crustytoothpaste.net","subject":"Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-27T18:14:47Z","receivedAt":"2026-02-27T18:14:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Feb 27, 2026 at 01:04:05PM +0000, brian m. carlson wrote:\n> On 2026-02-27 at 11:23:01, Patrick Steinhardt wrote:\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index c2643c0295..f8ba245616 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -270,6 +270,13 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n> > \t\t}\n> > \t}\n> > \n> > +\t/*\n> > +\t * Make sure that we buffer some data before sending it to the client.\n> > +\t * This significantly reduces the number of write(3p) syscalls.\n> > +\t */\n> > +\tif (readsz && os->used < (LARGE_PACKET_DATA_MAX * 2 / 3))\n> > +\t\treturn readsz;\n> > +\n> > \tif (os->used > 1) {\n> > \t\tsend_client_data(1, os->buffer, os->used - 1, use_sideband);\n> > \t\tos->buffer[0] = os->buffer[os->used - 1];\n> \n> This seems mostly reasonable and well-explained. The one question I\n> have is this: how does this work when packfile generation is actually\n> very slow (or when the connection is slow) and we need to send data\n> every so often to keep the connection alive?\n> \n> I just want to make sure we're not breaking the keepalive sideband case\n> when that's necessary, but of course I have no objections to improving\n> performance and reducing overhead.\n\nRight. `relay_pack_data()` is handling the logic to soak up data from\ngit-pack-objects(1). The outer loop is in `create_pack_file()`, and\nthere we already have a timeout configured. If data is produced too\nslow, then we'd eventually land in there and send the keepalive packet.\n\nI guess there is an interesting edge case here: if git-pack-objects(1)\ncreates bytes fast enough to not hit the 1 second timeout, but slow\nenough to basically never fill the buffer, then we could potentially run\ninto a timeout eventually.\n\nI'm not sure whether this is likely to happen, and whether it's\nsomething we want to address. Maybe we should extend the logic so that\nwe also send the keepalive pkt-line in case we have only been buffering\ndata for the last 1 second?\n\nBasically, we'd reset a timestamp every time data was sent. If we see\nthat we received data without writing it out, and if the current\ntime is one second beyond the last time we reset, then we might want to\nsend out the keepalive.\n\nPatrick\n"},{"id":"537336","messageId":"20260227193758.GA2931515@coredump.intra.peff.net","threadId":"65085","inReplyTo":"20260227-pks-upload-pack-write-contention-v1-2-7166fe255704@pks.im","subject":"Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-27T19:37:58Z","receivedAt":"2026-02-27T19:38:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 27, 2026 at 12:23:01PM +0100, Patrick Steinhardt wrote:\n\n> Extend our use of the buffering infrastructure so that we soak up bytes\n> until the buffer is filled up at least 2/3rds of its capacity. The\n> change is relatively simple to implement as we already know to flush the\n> buffer in `create_pack_file()` after git-pack-objects(1) has finished.\n\nWe are relaying write() calls from pack-objects here, which is writing\nto us in 8kb chunks (due to csum-file.c buffering). So most of our\nwrites will be 8k.\n\nRather than buffering in upload-pack, would it not be simpler to just\nincrease the write size from pack-objects? Then we do not have to worry\nabout disrupting upload-pack's keepalive timeouts. And as a bonus, if\nyou are worried about the system-wide number of calls, you will likewise\nbe reducing the number of read() and write() calls over the pipe between\npack-objects and upload-pack.\n\nSomething like this:\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 6e21e3cac8..94798fa429 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -206,7 +206,7 @@ struct hashfile *hashfd_throughput(const struct git_hash_algo *algop,\n \t * size so the progress indicators arrive at a more\n \t * frequent rate.\n \t */\n-\treturn hashfd_internal(algop, fd, name, tp, 8 * 1024);\n+\treturn hashfd_internal(algop, fd, name, tp, 32 * 1024);\n }\n \n void hashfile_checkpoint_init(struct hashfile *f,\n\nreduces the number of write calls reported by:\n\n git clone \\\n --upload-pack='perf stat -e syscalls:sys_enter_write git-upload-pack' \\\n --bare --no-local linux.git foo.git\n\nfrom ~420k to ~160k. In theory we expect ~8x reduction in our target\narea, 4x for each of pack-objects and upload-pack, but of course there\nare other writes going on, too, including the extra sideband ones. And\nobviously we could push it further towards LARGE_PACKET_MAX to save even\nmore.\n\n> Now git-upload-pack(1) already has the infrastructure in place to buffer\n> some of the data it reads from git-pack-objects(1) before actually\n> sending it out. We only use this infrastructure in very limited ways\n> though, so we generally end up matching one read(3p) call with one\n> write(3p) call. Even worse, when the sideband is enabled we end up\n> matching one read with _two_ writes: one for the pkt-line length, and\n> one for the packfile data.\n\nUsing writev() would be an easy-ish fix here, modulo portability\nconcerns (though of course it is easy to implement a fallback writev()\nin terms of write()). Doing this:\n\ndiff --git a/sideband.c b/sideband.c\nindex ea7c25211e..b5509fbaa2 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -266,19 +266,25 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n \twhile (sz) {\n \t\tunsigned n;\n \t\tchar hdr[5];\n+\t\tstruct iovec iov[2];\n \n \t\tn = sz;\n \t\tif (packet_max - 5 < n)\n \t\t\tn = packet_max - 5;\n \t\tif (0 <= band) {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 5);\n \t\t\thdr[4] = band;\n-\t\t\twrite_or_die(fd, hdr, 5);\n+\t\t\tiov[0].iov_base = hdr;\n+\t\t\tiov[0].iov_len = 5;\n \t\t} else {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 4);\n-\t\t\twrite_or_die(fd, hdr, 4);\n+\t\t\tiov[0].iov_base = hdr;\n+\t\t\tiov[0].iov_len = 4;\n \t\t}\n-\t\twrite_or_die(fd, p, n);\n+\t\tiov[1].iov_base = p;\n+\t\tiov[1].iov_len = n;\n+\t\t/* obviously needs looping and error detection */\n+\t\twritev(fd, iov, 2);\n \t\tp += n;\n \t\tsz -= n;\n \t}\n\ndrops my 160k write calls down to 82k.\n\nAnother option here is teaching the packet-forming code to reserve a few\nbytes at the front of the packet. There's a little discussion here:\n\n https://lore.kernel.org/git/YBkeYSA5UfQP1m%2Fx@coredump.intra.peff.net/\n\nIn theory it's easy and elegant to do, but I'm not sure what the\nrefactoring fallout would be like.\n\n> This significantly reduces the number of write(3p) syscalls we need to\n> do. Before this change, cloning the Linux repository resulted in around\n> 400,000 write(3p) syscalls. With the buffering in place we only do\n> around 130,000 syscalls.\n\nOut of curiosity, how did you end up measuring? I first tried with\nstrace (without \"-f\") on the upload-pack process, but strace slowed it\nenough that it ended up collecting multiple of pack-object's 8k write()\ncalls in a single read() call. ;) The \"perf stat\" above seemed to work\nOK, though of course it's counting child processes, too.\n\n-Peff\n"},{"id":"537506","messageId":"aaV-l_NyWpkKDDp6@pks.im","threadId":"65085","inReplyTo":"20260227193758.GA2931515@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-02T12:12:07Z","receivedAt":"2026-03-02T12:12:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Feb 27, 2026 at 02:37:58PM -0500, Jeff King wrote:\n> On Fri, Feb 27, 2026 at 12:23:01PM +0100, Patrick Steinhardt wrote:\n> \n> > Extend our use of the buffering infrastructure so that we soak up bytes\n> > until the buffer is filled up at least 2/3rds of its capacity. The\n> > change is relatively simple to implement as we already know to flush the\n> > buffer in `create_pack_file()` after git-pack-objects(1) has finished.\n> \n> We are relaying write() calls from pack-objects here, which is writing\n> to us in 8kb chunks (due to csum-file.c buffering). So most of our\n> writes will be 8k.\n> \n> Rather than buffering in upload-pack, would it not be simpler to just\n> increase the write size from pack-objects? Then we do not have to worry\n> about disrupting upload-pack's keepalive timeouts. And as a bonus, if\n> you are worried about the system-wide number of calls, you will likewise\n> be reducing the number of read() and write() calls over the pipe between\n> pack-objects and upload-pack.\n\nWe can do that. But we also have to keep in mind that downstream in the\npipe may be a process that's not even git-pack-objects(1) in the first\nplace because of \"uploadpack.packObjectsHook\". So maybe we should have a\nlook at doing both.\n\n> > Now git-upload-pack(1) already has the infrastructure in place to buffer\n> > some of the data it reads from git-pack-objects(1) before actually\n> > sending it out. We only use this infrastructure in very limited ways\n> > though, so we generally end up matching one read(3p) call with one\n> > write(3p) call. Even worse, when the sideband is enabled we end up\n> > matching one read with _two_ writes: one for the pkt-line length, and\n> > one for the packfile data.\n> \n> Using writev() would be an easy-ish fix here, modulo portability\n> concerns (though of course it is easy to implement a fallback writev()\n> in terms of write()). Doing this:\n\nRight, I was also wondering about whether we might want to use writev(),\nbut I didn't have the time yet to have a deeper look. I'll have a look\nat whether I can integrate your change in a platform-compatible way.\n\n> Out of curiosity, how did you end up measuring? I first tried with\n> strace (without \"-f\") on the upload-pack process, but strace slowed it\n> enough that it ended up collecting multiple of pack-object's 8k write()\n> calls in a single read() call. ;) The \"perf stat\" above seemed to work\n> OK, though of course it's counting child processes, too.\n\nI used strace for this. I didn't really dig into it too deep as I was\nrather busy handling the havoc that this issue caused :)\n\nI'll send a v2 soonish, thanks!\n\nPatrick\n"},{"id":"537574","messageId":"20260302182023.GG28275@coredump.intra.peff.net","threadId":"65085","inReplyTo":"aaV-l_NyWpkKDDp6@pks.im","subject":"Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-02T18:20:23Z","receivedAt":"2026-03-02T18:20:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 02, 2026 at 01:12:07PM +0100, Patrick Steinhardt wrote:\n\n> > Rather than buffering in upload-pack, would it not be simpler to just\n> > increase the write size from pack-objects? Then we do not have to worry\n> > about disrupting upload-pack's keepalive timeouts. And as a bonus, if\n> > you are worried about the system-wide number of calls, you will likewise\n> > be reducing the number of read() and write() calls over the pipe between\n> > pack-objects and upload-pack.\n> \n> We can do that. But we also have to keep in mind that downstream in the\n> pipe may be a process that's not even git-pack-objects(1) in the first\n> place because of \"uploadpack.packObjectsHook\". So maybe we should have a\n> look at doing both.\n\nTrue, though I think that whatever is producing gobs of output from that\nhook should consider using a buffer size close to a pktline. In many\ncases it will be pack-objects itself (just wrapped with some extra\nmagic), but I guess you may have some kind of caching layer at GitLab\n(we did at GitHub).\n\nThat is getting specialized enough that I don't feel too bad suggesting\nthat authors of those tools should consider buffer sizes.\n\nAs far as doing both, I'm not sure if it's worth it. My two concerns\nare:\n\n 1. It re-opens the question of whether upload-pack might stall waiting\n to fill its buffer and fail to produce keepalives correctly.\n\n 2. I wonder if we could get some weird interactions between the two\n buffer sizes. E.g., if pack-objects sends 50k bytes at a time, but\n upload-pack wants to wait for 51k. So we read 50k then wait for the\n next chunk. Either:\n\n a. we read 50k again, pull off 13k of it to make a full pktline,\n and then memcpy around the other 37k to await more data.\n\t There's a bunch of extra copying as our buffer sizes don't\n\t line up.\n\n b. we read 13k (to fill up the pkt we're trying to send), send\n that, and then the next read gets a partial read(). So we end\n\t up issuing more reads, although sometimes pack-objects might\n\t catch up and fill the pipe buffer, and we'd get a full packet\n\t in one go. But depending on the timing, I wonder if things\n\t could get choppy and we'd end up issuing a bunch of extra\n\t reads (and possibly extra writes on the pack-objects side if\n\t it's waiting on us to create more space in the pipe).\n\n-Peff\n"},{"id":"537652","messageId":"aaaqgrmOBj-Ly1Vx@pks.im","threadId":"65085","inReplyTo":"20260302182023.GG28275@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T09:31:46Z","receivedAt":"2026-03-03T09:31:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 02, 2026 at 01:20:23PM -0500, Jeff King wrote:\n> On Mon, Mar 02, 2026 at 01:12:07PM +0100, Patrick Steinhardt wrote:\n> \n> > > Rather than buffering in upload-pack, would it not be simpler to just\n> > > increase the write size from pack-objects? Then we do not have to worry\n> > > about disrupting upload-pack's keepalive timeouts. And as a bonus, if\n> > > you are worried about the system-wide number of calls, you will likewise\n> > > be reducing the number of read() and write() calls over the pipe between\n> > > pack-objects and upload-pack.\n> > \n> > We can do that. But we also have to keep in mind that downstream in the\n> > pipe may be a process that's not even git-pack-objects(1) in the first\n> > place because of \"uploadpack.packObjectsHook\". So maybe we should have a\n> > look at doing both.\n> \n> True, though I think that whatever is producing gobs of output from that\n> hook should consider using a buffer size close to a pktline. In many\n> cases it will be pack-objects itself (just wrapped with some extra\n> magic), but I guess you may have some kind of caching layer at GitLab\n> (we did at GitHub).\n> \n> That is getting specialized enough that I don't feel too bad suggesting\n> that authors of those tools should consider buffer sizes.\n> \n> As far as doing both, I'm not sure if it's worth it. My two concerns\n> are:\n> \n> 1. It re-opens the question of whether upload-pack might stall waiting\n> to fill its buffer and fail to produce keepalives correctly.\n\nI've got a patch for that. The problem can even trigger right now as we\nalready do buffer some of the data, and that may cause the keepalives to\nbe missed. But this only happens initially in our current\ninfrastructure, before we see the \"PACK\" signature, so it's unlikely to\nbe a problem in practice.\n\n> 2. I wonder if we could get some weird interactions between the two\n> buffer sizes. E.g., if pack-objects sends 50k bytes at a time, but\n> upload-pack wants to wait for 51k. So we read 50k then wait for the\n> next chunk. Either:\n> \n> a. we read 50k again, pull off 13k of it to make a full pktline,\n> and then memcpy around the other 37k to await more data.\n> \t There's a bunch of extra copying as our buffer sizes don't\n> \t line up.\n> \n> b. we read 13k (to fill up the pkt we're trying to send), send\n> that, and then the next read gets a partial read(). So we end\n> \t up issuing more reads, although sometimes pack-objects might\n> \t catch up and fill the pipe buffer, and we'd get a full packet\n> \t in one go. But depending on the timing, I wonder if things\n> \t could get choppy and we'd end up issuing a bunch of extra\n> \t reads (and possibly extra writes on the pack-objects side if\n> \t it's waiting on us to create more space in the pipe).\n\nWe would likely hit this issue if we insist on the buffer being\ncompletely filled before sending it out. But that's why I adapted the\nlogic to say that we send out once we've filled it at least 2/3rds of\nthe pktline limit. So in your case above we wouldn't face an issue as\nwe'd already send the first 50kB, as it is smaller than 2/3rds of the\nmaximum length (~42kB).\n\nThat being said, you'll still be able to construct cases where we have\nweird edge cases. For example if you consistently send one byte less\nthan 2/3rds of the capacity.\n\nPatrick\n"},{"id":"537663","messageId":"20260303133540.GA818878@coredump.intra.peff.net","threadId":"65085","inReplyTo":"aaaqgrmOBj-Ly1Vx@pks.im","subject":"Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-03T13:35:40Z","receivedAt":"2026-03-03T13:35:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 03, 2026 at 10:31:46AM +0100, Patrick Steinhardt wrote:\n\n> > As far as doing both, I'm not sure if it's worth it. My two concerns\n> > are:\n> > \n> > 1. It re-opens the question of whether upload-pack might stall waiting\n> > to fill its buffer and fail to produce keepalives correctly.\n> \n> I've got a patch for that. The problem can even trigger right now as we\n> already do buffer some of the data, and that may cause the keepalives to\n> be missed. But this only happens initially in our current\n> infrastructure, before we see the \"PACK\" signature, so it's unlikely to\n> be a problem in practice.\n\nI'm not sure what you mean by \"this only happens initially\" here. If it\nis: we can only miss keepalives in that time, then I think that is\nprobably a real problem. The time we _most_ need keepalives is before we\nsee the PACK signature, because that is when pack-objects is chewing on\nthe input, looking for deltas, etc, and not producing any output.\n\nIt is usually \"solved\" by pack-objects producing progress over stderr,\nbut for \"--quiet\" fetches, it could produce nothing for a long time.\n\nBut anyway, if you are fixing it either way, then I am happy. :)\n\n> We would likely hit this issue if we insist on the buffer being\n> completely filled before sending it out. But that's why I adapted the\n> logic to say that we send out once we've filled it at least 2/3rds of\n> the pktline limit. So in your case above we wouldn't face an issue as\n> we'd already send the first 50kB, as it is smaller than 2/3rds of the\n> maximum length (~42kB).\n> \n> That being said, you'll still be able to construct cases where we have\n> weird edge cases. For example if you consistently send one byte less\n> than 2/3rds of the capacity.\n\nRight, my numbers were just meant as examples. Whatever the values, it\nmeans that whatever is generating the pack data (pack-objects or\notherwise) really wants to be in sync with how upload-pack is buffering.\nOr vice versa. If we just pass back whole chunks of what we read() in\nupload-pack, then that happens automatically.\n\n-Peff\n"},{"id":"537666","messageId":"aabmXF5D1rMRZgP3@pks.im","threadId":"65085","inReplyTo":"20260303133540.GA818878@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T13:47:08Z","receivedAt":"2026-03-03T13:47:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 03, 2026 at 08:35:40AM -0500, Jeff King wrote:\n> On Tue, Mar 03, 2026 at 10:31:46AM +0100, Patrick Steinhardt wrote:\n> > We would likely hit this issue if we insist on the buffer being\n> > completely filled before sending it out. But that's why I adapted the\n> > logic to say that we send out once we've filled it at least 2/3rds of\n> > the pktline limit. So in your case above we wouldn't face an issue as\n> > we'd already send the first 50kB, as it is smaller than 2/3rds of the\n> > maximum length (~42kB).\n> > \n> > That being said, you'll still be able to construct cases where we have\n> > weird edge cases. For example if you consistently send one byte less\n> > than 2/3rds of the capacity.\n> \n> Right, my numbers were just meant as examples. Whatever the values, it\n> means that whatever is generating the pack data (pack-objects or\n> otherwise) really wants to be in sync with how upload-pack is buffering.\n> Or vice versa. If we just pass back whole chunks of what we read() in\n> upload-pack, then that happens automatically.\n\nYeah, that's fair. I'll include this part in v2 for now so that we can\ndiscuss once the bigger picture emerges :)\n\nThanks!\n\nPatrick\n"},{"id":"537671","messageId":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260227-pks-upload-pack-write-contention-v1-0-7166fe255704@pks.im","subject":"[PATCH v2 00/10] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:15Z","receivedAt":"2026-03-03T15:00:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series fixes some heavy lock contention when writing\ndata from git-upload-pack(1) into pipes. This lock contention can be\nobserved when having hundreds of git-upload-pack(1) processes active at\nthe same time that write data into pipes at dozens of gigabits per\nsecond.\n\nI have uploaded the flame graph that clearly shows the lock contention\nat [1].\n\nChanges in v2:\n - Change the buffer size in git-pack-objects(1) to also reduce the\n number of write syscalls over there.\n - Introduce writev to half the number of syscalls when writing\n pktlines.\n - Use `sizeof(os->buffer)` instead of open-coding its size.\n - Improve keepalive logic in git-upload-pack(1) to account for\n buffering.\n - Link to v1: https://lore.kernel.org/r/20260227-pks-upload-pack-write-contention-v1-0-7166fe255704@pks.im\n\nThanks!\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/git/-/work_items/675\n\n---\nPatrick Steinhardt (10):\n upload-pack: fix debug statement when flushing packfile data\n upload-pack: adapt keepalives based on buffering\n upload-pack: reduce lock contention when writing packfile data\n git-compat-util: introduce `cast_size_t_to_ssize_t()`\n compat/posix: introduce writev(3p) wrapper\n wrapper: introduce writev(3p) wrappers\n sideband: use writev(3p) to send pktlines\n csum-file: introduce `hashfd_ext()`\n csum-file: drop `hashfd_throughput()`\n builtin/pack-objects: reduce lock contention when writing packfile data\n\n Makefile | 4 ++++\n builtin/pack-objects.c | 23 +++++++++++++++----\n compat/posix.h | 14 ++++++++++++\n compat/writev.c | 29 ++++++++++++++++++++++++\n config.mak.uname | 2 ++\n csum-file.c | 28 +++++++----------------\n csum-file.h | 16 ++++++++++++--\n git-compat-util.h | 8 +++++++\n meson.build | 1 +\n sideband.c | 14 +++++++++---\n upload-pack.c | 60 ++++++++++++++++++++++++++++++++++++++++----------\n wrapper.c | 41 ++++++++++++++++++++++++++++++++++\n wrapper.h | 9 ++++++++\n write-or-die.c | 8 +++++++\n write-or-die.h | 1 +\n 15 files changed, 217 insertions(+), 41 deletions(-)\n\nRange-diff versus v1:\n\n 1: 368a984880 = 1: 81c00e3b52 upload-pack: fix debug statement when flushing packfile data\n -: ---------- > 2: 01c86a9573 upload-pack: adapt keepalives based on buffering\n 2: f014e8005a ! 3: 16bc5661d4 upload-pack: reduce lock contention when writing packfile data\n @@ Commit message\n write(3p) syscalls from 130,000 to 100,000. So overall this doesn't seem\n worth it.\n \n + Note that the issue could also be fixed by adapting the write buffer\n + that we use in the downstream git-pack-objects(1) command, and such a\n + change would have roughly the same result. But the command that\n + generates the packfile data may not always be git-pack-objects(1) as it\n + can be changed via \"uploadpack.packObjectsHook\", so such a fix would\n + only help in _some_ cases. Regardless of that, we'll also adapt the\n + write buffer size of git-pack-objects(1) in a subsequent commit.\n +\n Helped-by: Matt Smiley \n Signed-off-by: Patrick Steinhardt \n \n @@ upload-pack.c: static int relay_pack_data(int pack_objects_out, struct output_st\n +\t * Make sure that we buffer some data before sending it to the client.\n +\t * This significantly reduces the number of write(3p) syscalls.\n +\t */\n -+\tif (readsz && os->used < (LARGE_PACKET_DATA_MAX * 2 / 3))\n ++\tif (readsz && os->used < (sizeof(os->buffer) * 2 / 3))\n +\t\treturn readsz;\n +\n \tif (os->used > 1) {\n -: ---------- > 4: f42a8a0558 git-compat-util: introduce `cast_size_t_to_ssize_t()`\n -: ---------- > 5: 830c72f588 compat/posix: introduce writev(3p) wrapper\n -: ---------- > 6: 733ef129f1 wrapper: introduce writev(3p) wrappers\n -: ---------- > 7: 18f7429e14 sideband: use writev(3p) to send pktlines\n -: ---------- > 8: 8eb2aee74d csum-file: introduce `hashfd_ext()`\n -: ---------- > 9: a634506b81 csum-file: drop `hashfd_throughput()`\n -: ---------- > 10: ca6d52b012 builtin/pack-objects: reduce lock contention when writing packfile data\n\n---\nbase-commit: 7b2bccb0d58d4f24705bf985de1f4612e4cf06e5\nchange-id: 20260227-pks-upload-pack-write-contention-435ce01f5fe9\n\n"},{"id":"537672","messageId":"20260303-pks-upload-pack-write-contention-v2-1-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","subject":"[PATCH v2 01/10] upload-pack: fix debug statement when flushing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:16Z","receivedAt":"2026-03-03T15:00:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When git-upload-pack(1) writes packfile data to the client we have some\nlogic in place that buffers some partial lines. When that buffer still\ncontains data after git-pack-objects(1) has finished we flush the buffer\nso that all remaining bytes are sent out.\n\nCuriously, when we do so we also print the string \"flushed.\" to stderr.\nThis statement has been introduced in b1c71b7281 (upload-pack: avoid\nsending an incomplete pack upon failure, 2006-06-20), so quite a while\nago. What's interesting though is that stderr is typically spliced\nthrough to the client-side, and consequently the client would see this\nmessage. Munging the way how we do the caching indeed confirms this:\n\n $ git clone file:///home/pks/Development/linux/\n Cloning into bare repository 'linux.git'...\n remote: Enumerating objects: 12980346, done.\n remote: Counting objects: 100% (131820/131820), done.\n remote: Compressing objects: 100% (50290/50290), done.\n remote: Total 12980346 (delta 96319), reused 104500 (delta 81217), pack-reused 12848526 (from 1)\n Receiving objects: 100% (12980346/12980346), 3.23 GiB | 57.44 MiB/s, done.\n flushed.\n Resolving deltas: 100% (10676718/10676718), done.\n\nIt's quite clear that this string shouldn't ever be visible to the\nclient, so it rather feels like this is a left-over debug statement. The\nmenitoned commit doesn't mention this line, either.\n\nRemove the debug output to prepare for a change in how we do the\nbuffering in the next commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 2d2b70cbf2..c2643c0295 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -457,11 +457,9 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t}\n \n \t/* flush the data */\n-\tif (output_state->used > 0) {\n+\tif (output_state->used > 0)\n \t\tsend_client_data(1, output_state->buffer, output_state->used,\n \t\t\t\t pack_data->use_sideband);\n-\t\tfprintf(stderr, \"flushed.\\n\");\n-\t}\n \tfree(output_state);\n \tif (pack_data->use_sideband)\n \t\tpacket_flush(1);\n\n-- \n2.53.0.697.g625c4fb2da.dirty\n\n"},{"id":"537673","messageId":"20260303-pks-upload-pack-write-contention-v2-2-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","subject":"[PATCH v2 02/10] upload-pack: adapt keepalives based on buffering","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:17Z","receivedAt":"2026-03-03T15:00:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `create_pack_file()` is responsible for sending the\npackfile data to the client of git-upload-pack(1). As generating the\nbytes may take significant computing resources we also have a mechanism\nin place that optionally sends keepalive pktlines in case we haven't\nsent out any data.\n\nThe keepalive logic is purely based poll(3p): we pass a timeout to that\nsyscall, and if the call times out we send out the keepalive pktline.\nWhile reasonable, this logic isn't entirely sufficient: even if the call\nto poll(3p) ends because we have received data on any of the file\ndescriptors we may not necessarily send data to the client.\n\nThe most important edge case here happens in `relay_pack_data()`. When\nwe haven't seen the initial \"PACK\" signature from git-pack-objects(1)\nyet we buffer incoming data. So in the worst case, if each of the bytes\nof that signature arrive shortly before the configured keepalive\ntimeout, then we may not send out any data for a time period that is\n(almost) four times as long as the configured timeout.\n\nThis edge case is rather unlikely to matter in practice. But in a\nsubsequent commit we're going to adapt our buffering mechanism to become\nmore aggressive, which makes it more likely that we don't send any data\nfor an extended amount of time.\n\nAdapt the logic so that instead of using a fixed timeout on every call\nto poll(3p), we instead figure out how much time has passed since the\nlast-sent data.\n\nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 49 ++++++++++++++++++++++++++++++++++++++++---------\n 1 file changed, 40 insertions(+), 9 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c2643c0295..04521e57c9 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -29,6 +29,7 @@\n #include \"commit-graph.h\"\n #include \"commit-reach.h\"\n #include \"shallow.h\"\n+#include \"trace.h\"\n #include \"write-or-die.h\"\n #include \"json-writer.h\"\n #include \"strmap.h\"\n@@ -218,7 +219,8 @@ struct output_state {\n };\n \n static int relay_pack_data(int pack_objects_out, struct output_state *os,\n-\t\t\t int use_sideband, int write_packfile_line)\n+\t\t\t int use_sideband, int write_packfile_line,\n+\t\t\t bool *did_send_data)\n {\n \t/*\n \t * We keep the last byte to ourselves\n@@ -232,6 +234,8 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t */\n \tssize_t readsz;\n \n+\t*did_send_data = false;\n+\n \treadsz = xread(pack_objects_out, os->buffer + os->used,\n \t\t sizeof(os->buffer) - os->used);\n \tif (readsz < 0) {\n@@ -247,6 +251,7 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t\t\t\tif (os->packfile_uris_started)\n \t\t\t\t\tpacket_delim(1);\n \t\t\t\tpacket_write_fmt(1, \"\\1packfile\\n\");\n+\t\t\t\t*did_send_data = true;\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n@@ -259,6 +264,7 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t\t\t}\n \t\t\t*p = '\\0';\n \t\t\tpacket_write_fmt(1, \"\\1%s\\n\", os->buffer);\n+\t\t\t*did_send_data = true;\n \n \t\t\tos->used -= p - os->buffer + 1;\n \t\t\tmemmove(os->buffer, p + 1, os->used);\n@@ -279,6 +285,7 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t\tos->used = 0;\n \t}\n \n+\t*did_send_data = true;\n \treturn readsz;\n }\n \n@@ -290,6 +297,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \tchar progress[128];\n \tchar abort_msg[] = \"aborting due to possible repository \"\n \t\t\"corruption on the remote side.\";\n+\tuint64_t last_sent_ms = 0;\n \tssize_t sz;\n \tint i;\n \tFILE *pipe_fd;\n@@ -365,10 +373,14 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t */\n \n \twhile (1) {\n+\t\tuint64_t now_ms = getnanotime() / 1000000;\n \t\tstruct pollfd pfd[2];\n-\t\tint pe, pu, pollsize, polltimeout;\n+\t\tint pe, pu, pollsize, polltimeout_ms;\n \t\tint ret;\n \n+\t\tif (!last_sent_ms)\n+\t\t\tlast_sent_ms = now_ms;\n+\n \t\treset_timeout(pack_data->timeout);\n \n \t\tpollsize = 0;\n@@ -390,11 +402,21 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\tif (!pollsize)\n \t\t\tbreak;\n \n-\t\tpolltimeout = pack_data->keepalive < 0\n-\t\t\t? -1\n-\t\t\t: 1000 * pack_data->keepalive;\n+\t\tif (pack_data->keepalive < 0) {\n+\t\t\tpolltimeout_ms = -1;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * The polling timeout needs to be adjusted based on\n+\t\t\t * the time we have sent our last package. The longer\n+\t\t\t * it's been in the past, the shorter the timeout\n+\t\t\t * becomes until we eventually don't block at all.\n+\t\t\t */\n+\t\t\tpolltimeout_ms = 1000 * pack_data->keepalive - (now_ms - last_sent_ms);\n+\t\t\tif (polltimeout_ms < 0)\n+\t\t\t\tpolltimeout_ms = 0;\n+\t\t}\n \n-\t\tret = poll(pfd, pollsize, polltimeout);\n+\t\tret = poll(pfd, pollsize, polltimeout_ms);\n \n \t\tif (ret < 0) {\n \t\t\tif (errno != EINTR) {\n@@ -403,16 +425,18 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+\n \t\tif (0 <= pe && (pfd[pe].revents & (POLLIN|POLLHUP))) {\n \t\t\t/* Status ready; we ship that in the side-band\n \t\t\t * or dump to the standard error.\n \t\t\t */\n \t\t\tsz = xread(pack_objects.err, progress,\n \t\t\t\t sizeof(progress));\n-\t\t\tif (0 < sz)\n+\t\t\tif (0 < sz) {\n \t\t\t\tsend_client_data(2, progress, sz,\n \t\t\t\t\t\t pack_data->use_sideband);\n-\t\t\telse if (sz == 0) {\n+\t\t\t\tlast_sent_ms = now_ms;\n+\t\t\t} else if (sz == 0) {\n \t\t\t\tclose(pack_objects.err);\n \t\t\t\tpack_objects.err = -1;\n \t\t\t}\n@@ -421,11 +445,14 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\t\t/* give priority to status messages */\n \t\t\tcontinue;\n \t\t}\n+\n \t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n+\t\t\tbool did_send_data;\n \t\t\tint result = relay_pack_data(pack_objects.out,\n \t\t\t\t\t\t output_state,\n \t\t\t\t\t\t pack_data->use_sideband,\n-\t\t\t\t\t\t !!uri_protocols);\n+\t\t\t\t\t\t !!uri_protocols,\n+\t\t\t\t\t\t &did_send_data);\n \n \t\t\tif (result == 0) {\n \t\t\t\tclose(pack_objects.out);\n@@ -433,6 +460,9 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\t\t} else if (result < 0) {\n \t\t\t\tgoto fail;\n \t\t\t}\n+\n+\t\t\tif (did_send_data)\n+\t\t\t\tlast_sent_ms = now_ms;\n \t\t}\n \n \t\t/*\n@@ -448,6 +478,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\tif (!ret && pack_data->use_sideband) {\n \t\t\tstatic const char buf[] = \"0005\\1\";\n \t\t\twrite_or_die(1, buf, 5);\n+\t\t\tlast_sent_ms = now_ms;\n \t\t}\n \t}\n \n\n-- \n2.53.0.697.g625c4fb2da.dirty\n\n"},{"id":"537674","messageId":"20260303-pks-upload-pack-write-contention-v2-3-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","subject":"[PATCH v2 03/10] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:18Z","receivedAt":"2026-03-03T15:00:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In our production systems we have recently observed write contention in\ngit-upload-pack(1). The system in question was consistently streaming\npackfiles at a rate of dozens of gigabits per second, but curiously the\nsystem was neither bottlenecked on CPU, memory or IOPS.\n\nWe eventually discovered that Git was spending 80% of its time in\n`pipe_write()`, out of which almost all of the time was spent in the\n`ep_poll_callback` function in the kernel. Quoting the reporter:\n\n This infrastructure is part of an event notification queue designed to\n allow for multiple producers to emit events, but that concurrency\n safety is guarded by 3 layers of locking. The layer we're hitting\n contention in uses a simple reader/writer lock mode (a.k.a. shared\n versus exclusive mode), where producers need shared-mode (read mode),\n and various other actions use exclusive (write) mode.\n\nThe system in question generates workloads where we have hundreds of\ngit-upload-pack(1) processes active at the same point in time. These\nprocesses end up contending around those locks, and the consequence is\nthat the Git processes stall.\n\nNow git-upload-pack(1) already has the infrastructure in place to buffer\nsome of the data it reads from git-pack-objects(1) before actually\nsending it out. We only use this infrastructure in very limited ways\nthough, so we generally end up matching one read(3p) call with one\nwrite(3p) call. Even worse, when the sideband is enabled we end up\nmatching one read with _two_ writes: one for the pkt-line length, and\none for the packfile data.\n\nExtend our use of the buffering infrastructure so that we soak up bytes\nuntil the buffer is filled up at least 2/3rds of its capacity. The\nchange is relatively simple to implement as we already know to flush the\nbuffer in `create_pack_file()` after git-pack-objects(1) has finished.\n\nThis significantly reduces the number of write(3p) syscalls we need to\ndo. Before this change, cloning the Linux repository resulted in around\n400,000 write(3p) syscalls. With the buffering in place we only do\naround 130,000 syscalls.\n\nNow we could of course go even further and make sure that we always fill\nup the whole buffer. But this might cause an increase in read(3p)\nsyscalls, and some tests show that this only reduces the number of\nwrite(3p) syscalls from 130,000 to 100,000. So overall this doesn't seem\nworth it.\n\nNote that the issue could also be fixed by adapting the write buffer\nthat we use in the downstream git-pack-objects(1) command, and such a\nchange would have roughly the same result. But the command that\ngenerates the packfile data may not always be git-pack-objects(1) as it\ncan be changed via \"uploadpack.packObjectsHook\", so such a fix would\nonly help in _some_ cases. Regardless of that, we'll also adapt the\nwrite buffer size of git-pack-objects(1) in a subsequent commit.\n\nHelped-by: Matt Smiley \nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 04521e57c9..1b1c81ea63 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -276,6 +276,13 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t\t}\n \t}\n \n+\t/*\n+\t * Make sure that we buffer some data before sending it to the client.\n+\t * This significantly reduces the number of write(3p) syscalls.\n+\t */\n+\tif (readsz && os->used < (sizeof(os->buffer) * 2 / 3))\n+\t\treturn readsz;\n+\n \tif (os->used > 1) {\n \t\tsend_client_data(1, os->buffer, os->used - 1, use_sideband);\n \t\tos->buffer[0] = os->buffer[os->used - 1];\n\n-- \n2.53.0.697.g625c4fb2da.dirty\n\n"},{"id":"537675","messageId":"20260303-pks-upload-pack-write-contention-v2-4-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","subject":"[PATCH v2 04/10] git-compat-util: introduce `cast_size_t_to_ssize_t()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:19Z","receivedAt":"2026-03-03T15:00:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Introduce a new helper function `cast_size_t_to_ssize_t()`. This\nfunction will be used in the next commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n git-compat-util.h | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex bebcf9f698..c6af04cd7a 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -665,6 +665,14 @@ static inline int cast_size_t_to_int(size_t a)\n \treturn (int)a;\n }\n \n+static inline ssize_t cast_size_t_to_ssize_t(size_t a)\n+{\n+\tif (a > maximum_signed_value_of_type(ssize_t))\n+\t\tdie(\"number too large to represent as ssize_t on this platform: %\"PRIuMAX,\n+\t\t (uintmax_t)a);\n+\treturn (ssize_t)a;\n+}\n+\n static inline uint64_t u64_mult(uint64_t a, uint64_t b)\n {\n \tif (unsigned_mult_overflows(a, b))\n\n-- \n2.53.0.697.g625c4fb2da.dirty\n\n"},{"id":"537676","messageId":"20260303-pks-upload-pack-write-contention-v2-5-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","subject":"[PATCH v2 05/10] compat/posix: introduce writev(3p) wrapper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:20Z","receivedAt":"2026-03-03T15:00:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In a subsequent commit we're going to add the first caller to\nwritev(3p). Introduce a compatibility wrapper for this syscall that we\ncan use on systems that don't have this syscall.\n\nThe syscall exists on modern Unixes like Linux and macOS, and seemingly\neven for NonStop according to [1]. It doesn't seem to exist on Windows\nthough.\n\n[1]: http://nonstoptools.com/manuals/OSS-SystemCalls.pdf\n[2]: https://www.gnu.org/software/gnulib/manual/html_node/writev.html\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 4 ++++\n compat/posix.h | 14 ++++++++++++++\n compat/writev.c | 29 +++++++++++++++++++++++++++++\n config.mak.uname | 2 ++\n meson.build | 1 +\n 5 files changed, 50 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 7f37ad8f58..cb95ff2daf 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2021,6 +2021,10 @@ ifdef NO_PREAD\n \tCOMPAT_CFLAGS += -DNO_PREAD\n \tCOMPAT_OBJS += compat/pread.o\n endif\n+ifdef NO_WRITEV\n+\tCOMPAT_CFLAGS += -DNO_WRITEV\n+\tCOMPAT_OBJS += compat/writev.o\n+endif\n ifdef NO_FAST_WORKING_DIRECTORY\n \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n endif\ndiff --git a/compat/posix.h b/compat/posix.h\nindex 245386fa4a..3c611d2736 100644\n--- a/compat/posix.h\n+++ b/compat/posix.h\n@@ -137,6 +137,9 @@\n #include \n #include \n #include \n+#ifndef NO_WRITEV\n+#include \n+#endif\n #include \n #ifndef NO_SYS_SELECT_H\n #include \n@@ -323,6 +326,17 @@ int git_lstat(const char *, struct stat *);\n ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);\n #endif\n \n+#ifdef NO_WRITEV\n+#define writev git_writev\n+#define iovec git_iovec\n+struct git_iovec {\n+\tvoid *iov_base;\n+\tsize_t iov_len;\n+};\n+\n+ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt);\n+#endif\n+\n #ifdef NO_SETENV\n #define setenv gitsetenv\n int gitsetenv(const char *, const char *, int);\ndiff --git a/compat/writev.c b/compat/writev.c\nnew file mode 100644\nindex 0000000000..b77e534d5d\n--- /dev/null\n+++ b/compat/writev.c\n@@ -0,0 +1,29 @@\n+#include \"../git-compat-util.h\"\n+#include \"../wrapper.h\"\n+\n+ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)\n+{\n+\tsize_t total_written = 0;\n+\n+\tfor (int i = 0; i < iovcnt; i++) {\n+\t\tconst char *bytes = iov[i].iov_base;\n+\t\tsize_t iovec_written = 0;\n+\n+\t\twhile (iovec_written < iov[i].iov_len) {\n+\t\t\tssize_t bytes_written = xwrite(fd, bytes + iovec_written,\n+\t\t\t\t\t\t iov[i].iov_len - iovec_written);\n+\t\t\tif (bytes_written < 0) {\n+\t\t\t\tif (total_written)\n+\t\t\t\t\tgoto out;\n+\t\t\t\treturn bytes_written;\n+\t\t\t}\n+\t\t\tif (!bytes_written)\n+\t\t\t\tgoto out;\n+\t\t\tiovec_written += bytes_written;\n+\t\t\ttotal_written += bytes_written;\n+\t\t}\n+\t}\n+\n+out:\n+\treturn cast_size_t_to_ssize_t(total_written);\n+}\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 5feb582558..ccb3f71881 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -459,6 +459,7 @@ ifeq ($(uname_S),Windows)\n \tSANE_TOOL_PATH ?= $(msvc_bin_dir_msys)\n \tHAVE_ALLOCA_H = YesPlease\n \tNO_PREAD = YesPlease\n+\tNO_WRITEV = YesPlease\n \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n \tNO_LIBGEN_H = YesPlease\n \tNO_POLL = YesPlease\n@@ -674,6 +675,7 @@ ifeq ($(uname_S),MINGW)\n \tpathsep = ;\n \tHAVE_ALLOCA_H = YesPlease\n \tNO_PREAD = YesPlease\n+\tNO_WRITEV = YesPlease\n \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n \tNO_LIBGEN_H = YesPlease\n \tNO_POLL = YesPlease\ndiff --git a/meson.build b/meson.build\nindex 762e2d0fc0..63514b6b84 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1409,6 +1409,7 @@ checkfuncs = {\n 'initgroups' : [],\n 'strtoumax' : ['strtoumax.c', 'strtoimax.c'],\n 'pread' : ['pread.c'],\n+ 'writev' : ['writev.c'],\n }\n \n if host_machine.system() == 'windows'\n\n-- \n2.53.0.697.g625c4fb2da.dirty\n\n"},{"id":"537677","messageId":"20260303-pks-upload-pack-write-contention-v2-6-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","subject":"[PATCH v2 06/10] wrapper: introduce writev(3p) wrappers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:21Z","receivedAt":"2026-03-03T15:00:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the preceding commit we have added a compatibility wrapper for the\nwritev(3p) syscall. Introduce some generic wrappers for this function\nthat we nowadays take for granted in the Git codebase.\n\nSigned-off-by: Patrick Steinhardt \n---\n wrapper.c | 41 +++++++++++++++++++++++++++++++++++++++++\n wrapper.h | 9 +++++++++\n write-or-die.c | 8 ++++++++\n write-or-die.h | 1 +\n 4 files changed, 59 insertions(+)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 16f5a63fbb..be8fa575e6 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -323,6 +323,47 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n \treturn total;\n }\n \n+ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)\n+{\n+\tssize_t total_written = 0;\n+\n+\twhile (iovcnt) {\n+\t\tssize_t bytes_written = writev(fd, iov, iovcnt);\n+\t\tif (bytes_written < 0) {\n+\t\t\tif (errno == EINTR || errno == EAGAIN)\n+\t\t\t\tcontinue;\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (!bytes_written) {\n+\t\t\terrno = ENOSPC;\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\ttotal_written += bytes_written;\n+\n+\t\t/*\n+\t\t * We first need to discard any iovec entities that have been\n+\t\t * fully written.\n+\t\t */\n+\t\twhile (iovcnt && (size_t)bytes_written >= iov->iov_len) {\n+\t\t\tbytes_written -= iov->iov_len;\n+\t\t\tiov++;\n+\t\t\tiovcnt--;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Finally, we need to adjust the last iovec in case we have\n+\t\t * performed a partial write.\n+\t\t */\n+\t\tif (iovcnt && bytes_written) {\n+\t\t\tiov->iov_base = (char *) iov->iov_base + bytes_written;\n+\t\t\tiov->iov_len -= bytes_written;\n+\t\t}\n+\t}\n+\n+\treturn total_written;\n+}\n+\n ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)\n {\n \tchar *p = buf;\ndiff --git a/wrapper.h b/wrapper.h\nindex 15ac3bab6e..27519b32d1 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -47,6 +47,15 @@ ssize_t read_in_full(int fd, void *buf, size_t count);\n ssize_t write_in_full(int fd, const void *buf, size_t count);\n ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);\n \n+/*\n+ * Try to write all iovecs. Returns -1 in case an error occurred with a proper\n+ * errno set, the number of bytes written otherwise.\n+ *\n+ * Note that the iovec will be modified as a result of this call to adjust for\n+ * partial writes!\n+ */\n+ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt);\n+\n static inline ssize_t write_str_in_full(int fd, const char *str)\n {\n \treturn write_in_full(fd, str, strlen(str));\ndiff --git a/write-or-die.c b/write-or-die.c\nindex 01a9a51fa2..5f522fb728 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -96,6 +96,14 @@ void write_or_die(int fd, const void *buf, size_t count)\n \t}\n }\n \n+void writev_or_die(int fd, struct iovec *iov, int iovlen)\n+{\n+\tif (writev_in_full(fd, iov, iovlen) < 0) {\n+\t\tcheck_pipe(errno);\n+\t\tdie_errno(\"writev error\");\n+\t}\n+}\n+\n void fwrite_or_die(FILE *f, const void *buf, size_t count)\n {\n \tif (fwrite(buf, 1, count, f) != count)\ndiff --git a/write-or-die.h b/write-or-die.h\nindex 65a5c42a47..ae3d7d88b8 100644\n--- a/write-or-die.h\n+++ b/write-or-die.h\n@@ -7,6 +7,7 @@ void fprintf_or_die(FILE *, const char *fmt, ...);\n void fwrite_or_die(FILE *f, const void *buf, size_t count);\n void fflush_or_die(FILE *f);\n void write_or_die(int fd, const void *buf, size_t count);\n+void writev_or_die(int fd, struct iovec *iov, int iovlen);\n \n /*\n * These values are used to help identify parts of a repository to fsync.\n\n-- \n2.53.0.697.g625c4fb2da.dirty\n\n"},{"id":"537678","messageId":"20260303-pks-upload-pack-write-contention-v2-7-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","subject":"[PATCH v2 07/10] sideband: use writev(3p) to send pktlines","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:22Z","receivedAt":"2026-03-03T15:00:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Every pktline that we send out via `send_sideband()` currently requires\ntwo syscalls: one to write the pktline's length, and one to send its\ndata. This typically isn't all that much of a problem, but under extreme\nload the syscalls may cause contention in the kernel.\n\nRefactor the code to instead use the newly introduced writev(3p) infra\nso that we can send out the data with a single syscall. This reduces the\nnumber of syscalls from around 133,000 calls to write(3p) to around\n67,000 calls to writev(3p).\n\nSuggested-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\n sideband.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex ea7c25211e..1ed6614eaf 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -264,6 +264,7 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n \tconst char *p = data;\n \n \twhile (sz) {\n+\t\tstruct iovec iov[2];\n \t\tunsigned n;\n \t\tchar hdr[5];\n \n@@ -273,12 +274,19 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n \t\tif (0 <= band) {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 5);\n \t\t\thdr[4] = band;\n-\t\t\twrite_or_die(fd, hdr, 5);\n+\t\t\tiov[0].iov_base = hdr;\n+\t\t\tiov[0].iov_len = 5;\n \t\t} else {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 4);\n-\t\t\twrite_or_die(fd, hdr, 4);\n+\t\t\tiov[0].iov_base = hdr;\n+\t\t\tiov[0].iov_len = 4;\n \t\t}\n-\t\twrite_or_die(fd, p, n);\n+\n+\t\tiov[1].iov_base = (void *) p;\n+\t\tiov[1].iov_len = n;\n+\n+\t\twritev_or_die(fd, iov, ARRAY_SIZE(iov));\n+\n \t\tp += n;\n \t\tsz -= n;\n \t}\n\n-- \n2.53.0.697.g625c4fb2da.dirty\n\n"},{"id":"537679","messageId":"20260303-pks-upload-pack-write-contention-v2-8-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","subject":"[PATCH v2 08/10] csum-file: introduce `hashfd_ext()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:23Z","receivedAt":"2026-03-03T15:00:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Introduce a new `hashfd_ext()` function that takes an options structure.\nThis function will replace `hashd_throughput()` in the next commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n csum-file.c | 22 +++++++++++++---------\n csum-file.h | 14 ++++++++++++++\n 2 files changed, 27 insertions(+), 9 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 6e21e3cac8..a50416247e 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -161,17 +161,16 @@ struct hashfile *hashfd_check(const struct git_hash_algo *algop,\n \treturn f;\n }\n \n-static struct hashfile *hashfd_internal(const struct git_hash_algo *algop,\n-\t\t\t\t\tint fd, const char *name,\n-\t\t\t\t\tstruct progress *tp,\n-\t\t\t\t\tsize_t buffer_len)\n+struct hashfile *hashfd_ext(const struct git_hash_algo *algop,\n+\t\t\t int fd, const char *name,\n+\t\t\t const struct hashfd_options *opts)\n {\n \tstruct hashfile *f = xmalloc(sizeof(*f));\n \tf->fd = fd;\n \tf->check_fd = -1;\n \tf->offset = 0;\n \tf->total = 0;\n-\tf->tp = tp;\n+\tf->tp = opts->progress;\n \tf->name = name;\n \tf->do_crc = 0;\n \tf->skip_hash = 0;\n@@ -179,8 +178,8 @@ static struct hashfile *hashfd_internal(const struct git_hash_algo *algop,\n \tf->algop = unsafe_hash_algo(algop);\n \tf->algop->init_fn(&f->ctx);\n \n-\tf->buffer_len = buffer_len;\n-\tf->buffer = xmalloc(buffer_len);\n+\tf->buffer_len = opts->buffer_len ? opts->buffer_len : 128 * 1024;\n+\tf->buffer = xmalloc(f->buffer_len);\n \tf->check_buffer = NULL;\n \n \treturn f;\n@@ -194,7 +193,8 @@ struct hashfile *hashfd(const struct git_hash_algo *algop,\n \t * measure the rate of data passing through this hashfile,\n \t * use a larger buffer size to reduce fsync() calls.\n \t */\n-\treturn hashfd_internal(algop, fd, name, NULL, 128 * 1024);\n+\tstruct hashfd_options opts = { 0 };\n+\treturn hashfd_ext(algop, fd, name, &opts);\n }\n \n struct hashfile *hashfd_throughput(const struct git_hash_algo *algop,\n@@ -206,7 +206,11 @@ struct hashfile *hashfd_throughput(const struct git_hash_algo *algop,\n \t * size so the progress indicators arrive at a more\n \t * frequent rate.\n \t */\n-\treturn hashfd_internal(algop, fd, name, tp, 8 * 1024);\n+\tstruct hashfd_options opts = {\n+\t\t.progress = tp,\n+\t\t.buffer_len = 8 * 1024,\n+\t};\n+\treturn hashfd_ext(algop, fd, name, &opts);\n }\n \n void hashfile_checkpoint_init(struct hashfile *f,\ndiff --git a/csum-file.h b/csum-file.h\nindex 07ae11024a..a03b60120d 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -45,6 +45,20 @@ int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *);\n #define CSUM_FSYNC\t\t2\n #define CSUM_HASH_IN_STREAM\t4\n \n+struct hashfd_options {\n+\t/*\n+\t * Throughput progress that counts the number of bytes that have been\n+\t * hashed.\n+\t */\n+\tstruct progress *progress;\n+\n+\t/* The length of the buffer that shall be used read read data. */\n+\tsize_t buffer_len;\n+};\n+\n+struct hashfile *hashfd_ext(const struct git_hash_algo *algop,\n+\t\t\t int fd, const char *name,\n+\t\t\t const struct hashfd_options *opts);\n struct hashfile *hashfd(const struct git_hash_algo *algop,\n \t\t\tint fd, const char *name);\n struct hashfile *hashfd_check(const struct git_hash_algo *algop,\n\n-- \n2.53.0.697.g625c4fb2da.dirty\n\n"},{"id":"537680","messageId":"20260303-pks-upload-pack-write-contention-v2-9-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","subject":"[PATCH v2 09/10] csum-file: drop `hashfd_throughput()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:24Z","receivedAt":"2026-03-03T15:00:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `hashfd_throughput()` function is used by a single callsite in\ngit-pack-objects(1). In contrast to `hashfd()`, this function uses a\nprogress meter to measure throughput and a smaller buffer length so that\nthe progress meter can provide more granular metrics.\n\nWe're going to change that caller in the next commit to be a bit more\nspecific to packing objects. As such, `hashfd_throughput()` will be a\nsomewhat unfitting mechanism for any potential new callers.\n\nDrop the function and replace it with a call to `hashfd_ext()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 19 +++++++++++++++----\n csum-file.c | 16 ----------------\n csum-file.h | 2 --\n 3 files changed, 15 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cfb03d4c09..db04e6dd0e 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1330,11 +1330,22 @@ static void write_pack_file(void)\n \t\tunsigned char hash[GIT_MAX_RAWSZ];\n \t\tchar *pack_tmp_name = NULL;\n \n-\t\tif (pack_to_stdout)\n-\t\t\tf = hashfd_throughput(the_repository->hash_algo, 1,\n-\t\t\t\t\t \"\", progress_state);\n-\t\telse\n+\t\tif (pack_to_stdout) {\n+\t\t\t/*\n+\t\t\t * Since we are expecting to report progress of the\n+\t\t\t * write into this hashfile, use a smaller buffer\n+\t\t\t * size so the progress indicators arrive at a more\n+\t\t\t * frequent rate.\n+\t\t\t */\n+\t\t\tstruct hashfd_options opts = {\n+\t\t\t\t.progress = progress_state,\n+\t\t\t\t.buffer_len = 8 * 1024,\n+\t\t\t};\n+\t\t\tf = hashfd_ext(the_repository->hash_algo, 1,\n+\t\t\t\t \"\", &opts);\n+\t\t} else {\n \t\t\tf = create_tmp_packfile(the_repository, &pack_tmp_name);\n+\t\t}\n \n \t\toffset = write_pack_header(f, nr_remaining);\n \ndiff --git a/csum-file.c b/csum-file.c\nindex a50416247e..5dfaca5543 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -197,22 +197,6 @@ struct hashfile *hashfd(const struct git_hash_algo *algop,\n \treturn hashfd_ext(algop, fd, name, &opts);\n }\n \n-struct hashfile *hashfd_throughput(const struct git_hash_algo *algop,\n-\t\t\t\t int fd, const char *name, struct progress *tp)\n-{\n-\t/*\n-\t * Since we are expecting to report progress of the\n-\t * write into this hashfile, use a smaller buffer\n-\t * size so the progress indicators arrive at a more\n-\t * frequent rate.\n-\t */\n-\tstruct hashfd_options opts = {\n-\t\t.progress = tp,\n-\t\t.buffer_len = 8 * 1024,\n-\t};\n-\treturn hashfd_ext(algop, fd, name, &opts);\n-}\n-\n void hashfile_checkpoint_init(struct hashfile *f,\n \t\t\t struct hashfile_checkpoint *checkpoint)\n {\ndiff --git a/csum-file.h b/csum-file.h\nindex a03b60120d..01472555c8 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -63,8 +63,6 @@ struct hashfile *hashfd(const struct git_hash_algo *algop,\n \t\t\tint fd, const char *name);\n struct hashfile *hashfd_check(const struct git_hash_algo *algop,\n \t\t\t const char *name);\n-struct hashfile *hashfd_throughput(const struct git_hash_algo *algop,\n-\t\t\t\t int fd, const char *name, struct progress *tp);\n \n /*\n * Free the hashfile without flushing its contents to disk. This only\n\n-- \n2.53.0.697.g625c4fb2da.dirty\n\n"},{"id":"537681","messageId":"20260303-pks-upload-pack-write-contention-v2-10-7321830f08fe@pks.im","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im","subject":"[PATCH v2 10/10] builtin/pack-objects: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-03T15:00:25Z","receivedAt":"2026-03-03T15:00:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When running `git pack-objects --stdout` we feed the data through\n`hashfd_ext()` with a progress meter and a smaller-than-usual buffer\nlength of 8kB so that we can track throughput more granularly. But as\npackfiles tend to be on the larger side, this small buffer size may\ncause a ton of write(3p) syscalls.\n\nOriginally, the buffer we used in `hashfd()` was 8kB for all use cases.\nThis was changed though in 2ca245f8be (csum-file.h: increase hashfile\nbuffer size, 2021-05-18) because we noticed that the number of writes\ncan have an impact on performance. So the buffer size was increased to\n128kB, which improved performance a bit for some use cases.\n\nBut the commit didn't touch the buffer size for `hashd_throughput()`.\nThe reasoning here was that callers expect the progress indicator to\nupdate frequently, and a larger buffer size would of course reduce the\nupdate frequency especially on slow networks.\n\nWhile that is of course true, there was (and still is, even though it's\nnow a call to `hashfd_ext()`) only a single caller of this function in\ngit-pack-objects(1). This command is responsible for writing packfiles,\nand those packfiles are often on the bigger side. So arguably:\n\n - The user won't care about increments of 8kB when packfiles tend to\n be megabytes or even gigabytes in size.\n\n - Reducing the number of syscalls would be even more valuable here\n than it would be for multi-pack indices, which was the benchmark\n done in the mentioned commit, as MIDXs are typically significantly\n smaller than packfiles.\n\n - Nowadays, many internet connections should be able to transfer data\n at a rate significantly higher than 8kB per second.\n\nUpdate the buffer to instead have a size of `LARGE_PACKET_DATA_MAX - 1`,\nwhich translates to ~64kB. This limit was chosen because `git\npack-objects --stdout` is most often used when sending packfiles via\ngit-upload-pack(1), where packfile data is chunked into pktlines when\nusing the sideband. Furthermore, most internet connections should have a\nbandwidth signifcantly higher than 64kB/s, so we'd still be able to\nobserve progress updates at a rate of at least once per second.\n\nThis change significantly reduces the number of write(3p) syscalls from\n355,000 to 44,000 when packing the Linux repository. While this results\nin a small performance improvement on an otherwise-unused system, this\nimprovement is mostly negligible. More importantly though, it will\nreduce lock contention in the kernel on an extremely busy system where\nwe have many processes writing data at once.\n\nSuggested-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex db04e6dd0e..b8d684522d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -41,6 +41,7 @@\n #include \"promisor-remote.h\"\n #include \"pack-mtimes.h\"\n #include \"parse-options.h\"\n+#include \"pkt-line.h\"\n #include \"blob.h\"\n #include \"tree.h\"\n #include \"path-walk.h\"\n@@ -1332,14 +1333,17 @@ static void write_pack_file(void)\n \n \t\tif (pack_to_stdout) {\n \t\t\t/*\n-\t\t\t * Since we are expecting to report progress of the\n-\t\t\t * write into this hashfile, use a smaller buffer\n-\t\t\t * size so the progress indicators arrive at a more\n-\t\t\t * frequent rate.\n+\t\t\t * This command is most often invoked via\n+\t\t\t * git-upload-pack(1), which will typically chunk data\n+\t\t\t * into pktlines. As such, we use the maximum data\n+\t\t\t * length of them as buffer length.\n+\t\t\t *\n+\t\t\t * Note that we need to subtract one though to\n+\t\t\t * accomodate for the sideband byte.\n \t\t\t */\n \t\t\tstruct hashfd_options opts = {\n \t\t\t\t.progress = progress_state,\n-\t\t\t\t.buffer_len = 8 * 1024,\n+\t\t\t\t.buffer_len = LARGE_PACKET_DATA_MAX - 1,\n \t\t\t};\n \t\t\tf = hashfd_ext(the_repository->hash_algo, 1,\n \t\t\t\t \"\", &opts);\n\n-- \n2.53.0.697.g625c4fb2da.dirty\n\n"},{"id":"537845","messageId":"xmqqseaf5k5t.fsf@gitster.g","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-5-7321830f08fe@pks.im","subject":"Re: [PATCH v2 05/10] compat/posix: introduce writev(3p) wrapper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-04T22:01:18Z","receivedAt":"2026-03-04T22:01:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> In a subsequent commit we're going to add the first caller to\n> writev(3p). Introduce a compatibility wrapper for this syscall that we\n> can use on systems that don't have this syscall.\n>\n> The syscall exists on modern Unixes like Linux and macOS, and seemingly\n> even for NonStop according to [1]. It doesn't seem to exist on Windows\n> though.\n\n\n\n>\n> [1]: http://nonstoptools.com/manuals/OSS-SystemCalls.pdf\n> [2]: https://www.gnu.org/software/gnulib/manual/html_node/writev.html\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> Makefile | 4 ++++\n> compat/posix.h | 14 ++++++++++++++\n> compat/writev.c | 29 +++++++++++++++++++++++++++++\n> config.mak.uname | 2 ++\n> meson.build | 1 +\n> 5 files changed, 50 insertions(+)\n>\n> diff --git a/Makefile b/Makefile\n> index 7f37ad8f58..cb95ff2daf 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2021,6 +2021,10 @@ ifdef NO_PREAD\n> \tCOMPAT_CFLAGS += -DNO_PREAD\n> \tCOMPAT_OBJS += compat/pread.o\n> endif\n> +ifdef NO_WRITEV\n> +\tCOMPAT_CFLAGS += -DNO_WRITEV\n> +\tCOMPAT_OBJS += compat/writev.o\n> +endif\n> ifdef NO_FAST_WORKING_DIRECTORY\n> \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n> endif\n> diff --git a/compat/posix.h b/compat/posix.h\n> index 245386fa4a..3c611d2736 100644\n> --- a/compat/posix.h\n> +++ b/compat/posix.h\n> @@ -137,6 +137,9 @@\n> #include \n> #include \n> #include \n> +#ifndef NO_WRITEV\n> +#include \n> +#endif\n> #include \n> #ifndef NO_SYS_SELECT_H\n> #include \n> @@ -323,6 +326,17 @@ int git_lstat(const char *, struct stat *);\n> ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);\n> #endif\n> \n> +#ifdef NO_WRITEV\n> +#define writev git_writev\n> +#define iovec git_iovec\n> +struct git_iovec {\n> +\tvoid *iov_base;\n> +\tsize_t iov_len;\n> +};\n> +\n> +ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt);\n> +#endif\n> +\n> #ifdef NO_SETENV\n> #define setenv gitsetenv\n> int gitsetenv(const char *, const char *, int);\n> diff --git a/compat/writev.c b/compat/writev.c\n> new file mode 100644\n> index 0000000000..b77e534d5d\n> --- /dev/null\n> +++ b/compat/writev.c\n> @@ -0,0 +1,29 @@\n> +#include \"../git-compat-util.h\"\n> +#include \"../wrapper.h\"\n> +\n> +ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)\n> +{\n> +\tsize_t total_written = 0;\n> +\n> +\tfor (int i = 0; i < iovcnt; i++) {\n> +\t\tconst char *bytes = iov[i].iov_base;\n> +\t\tsize_t iovec_written = 0;\n> +\n> +\t\twhile (iovec_written < iov[i].iov_len) {\n> +\t\t\tssize_t bytes_written = xwrite(fd, bytes + iovec_written,\n> +\t\t\t\t\t\t iov[i].iov_len - iovec_written);\n> +\t\t\tif (bytes_written < 0) {\n> +\t\t\t\tif (total_written)\n> +\t\t\t\t\tgoto out;\n> +\t\t\t\treturn bytes_written;\n> +\t\t\t}\n> +\t\t\tif (!bytes_written)\n> +\t\t\t\tgoto out;\n> +\t\t\tiovec_written += bytes_written;\n> +\t\t\ttotal_written += bytes_written;\n> +\t\t}\n> +\t}\n> +\n> +out:\n> +\treturn cast_size_t_to_ssize_t(total_written);\n> +}\n\nBecause we do not check the accumulation of bytes_written in the two\naccumulator variables inside the inner loop, it is very possible for\ntotal_written to wraparound size_t and end up below the largest\nvalue possible to be stored in ssize_t type.\n\nIOW, the cast_size_t_to_ssize_t() introduced in the previous step is\npointless, isn't it?\n\nAccording to [*1*], the real\n\n ssize_t writev(int fd, const struct iovec *iov, int iovcnt)\n\nis supposed to report error with errno set to EINVAL when the sum of\niov_len member of the iov[] array elements exceed half of the\nmaximum size_t.\n\n EINVAL The sum of the iov_len values overflows an ssize_t value.\n\nSo instead of dying with cast_size_t_to_ssize_t(), we probably would\nwant the check done in a more stupid and straight-forward way?\nAdding up iov[i].iov_len while the addition would not wraparound in\neach and every step, and return error with EINVAL before attempting\neven a single call to xwrite(), and then have the above double loop\nthat does not care about integer wraparound at all, and return\ntotal_written with simple cast to (ssize_t)?\n\n\n[Reference]\n\n *1* https://pubs.opengroup.org/onlinepubs/9799919799/functions/writev.html\n\n\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 5feb582558..ccb3f71881 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -459,6 +459,7 @@ ifeq ($(uname_S),Windows)\n> \tSANE_TOOL_PATH ?= $(msvc_bin_dir_msys)\n> \tHAVE_ALLOCA_H = YesPlease\n> \tNO_PREAD = YesPlease\n> +\tNO_WRITEV = YesPlease\n> \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n> \tNO_LIBGEN_H = YesPlease\n> \tNO_POLL = YesPlease\n> @@ -674,6 +675,7 @@ ifeq ($(uname_S),MINGW)\n> \tpathsep = ;\n> \tHAVE_ALLOCA_H = YesPlease\n> \tNO_PREAD = YesPlease\n> +\tNO_WRITEV = YesPlease\n> \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n> \tNO_LIBGEN_H = YesPlease\n> \tNO_POLL = YesPlease\n> diff --git a/meson.build b/meson.build\n> index 762e2d0fc0..63514b6b84 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -1409,6 +1409,7 @@ checkfuncs = {\n> 'initgroups' : [],\n> 'strtoumax' : ['strtoumax.c', 'strtoimax.c'],\n> 'pread' : ['pread.c'],\n> + 'writev' : ['writev.c'],\n> }\n> \n> if host_machine.system() == 'windows'\n"},{"id":"537847","messageId":"xmqqo6l35jzi.fsf@gitster.g","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-7-7321830f08fe@pks.im","subject":"Re: [PATCH v2 07/10] sideband: use writev(3p) to send pktlines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-04T22:05:05Z","receivedAt":"2026-03-04T22:05:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Every pktline that we send out via `send_sideband()` currently requires\n> two syscalls: one to write the pktline's length, and one to send its\n> data. This typically isn't all that much of a problem, but under extreme\n> load the syscalls may cause contention in the kernel.\n>\n> Refactor the code to instead use the newly introduced writev(3p) infra\n> so that we can send out the data with a single syscall. This reduces the\n> number of syscalls from around 133,000 calls to write(3p) to around\n> 67,000 calls to writev(3p).\n>\n> Suggested-by: Jeff King \n> Signed-off-by: Patrick Steinhardt \n> ---\n> sideband.c | 14 +++++++++++---\n> 1 file changed, 11 insertions(+), 3 deletions(-)\n\nQuite straight-forward. Looking good.\n\n> diff --git a/sideband.c b/sideband.c\n> index ea7c25211e..1ed6614eaf 100644\n> --- a/sideband.c\n> +++ b/sideband.c\n> @@ -264,6 +264,7 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n> \tconst char *p = data;\n> \n> \twhile (sz) {\n> +\t\tstruct iovec iov[2];\n> \t\tunsigned n;\n> \t\tchar hdr[5];\n> \n> @@ -273,12 +274,19 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n> \t\tif (0 <= band) {\n> \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 5);\n> \t\t\thdr[4] = band;\n> -\t\t\twrite_or_die(fd, hdr, 5);\n> +\t\t\tiov[0].iov_base = hdr;\n> +\t\t\tiov[0].iov_len = 5;\n> \t\t} else {\n> \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 4);\n> -\t\t\twrite_or_die(fd, hdr, 4);\n> +\t\t\tiov[0].iov_base = hdr;\n> +\t\t\tiov[0].iov_len = 4;\n> \t\t}\n> -\t\twrite_or_die(fd, p, n);\n> +\n> +\t\tiov[1].iov_base = (void *) p;\n> +\t\tiov[1].iov_len = n;\n> +\n> +\t\twritev_or_die(fd, iov, ARRAY_SIZE(iov));\n> +\n> \t\tp += n;\n> \t\tsz -= n;\n> \t}\n"},{"id":"537848","messageId":"xmqqjyvr5jos.fsf@gitster.g","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-8-7321830f08fe@pks.im","subject":"Re: [PATCH v2 08/10] csum-file: introduce `hashfd_ext()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-04T22:11:31Z","receivedAt":"2026-03-04T22:11:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> -static struct hashfile *hashfd_internal(const struct git_hash_algo *algop,\n> -\t\t\t\t\tint fd, const char *name,\n> -\t\t\t\t\tstruct progress *tp,\n> -\t\t\t\t\tsize_t buffer_len)\n> +struct hashfile *hashfd_ext(const struct git_hash_algo *algop,\n> +\t\t\t int fd, const char *name,\n> +\t\t\t const struct hashfd_options *opts)\n\nWhat does _ext stand for?\n\nMore seriously, this essentially chooses to pick two parameters\nhashfd_internal() takes and put them in a struct, which would give\nus a clear upgrade path to add more to the structure without having\nto change the function signature. But what is the criteria used to\nchoose these two among 5 parameters the original function takes?\n\nSpecifically, I am wondering if fd and algop should be part of the\nstructure, as these would be exactly the same for repeated calls to\nthis function to write to a single stream.\n\n> +struct hashfd_options {\n> +\t/*\n> +\t * Throughput progress that counts the number of bytes that have been\n> +\t * hashed.\n> +\t */\n> +\tstruct progress *progress;\n> +\n> +\t/* The length of the buffer that shall be used read read data. */\n> +\tsize_t buffer_len;\n> +};\n> +\n> +struct hashfile *hashfd_ext(const struct git_hash_algo *algop,\n> +\t\t\t int fd, const char *name,\n> +\t\t\t const struct hashfd_options *opts);\n> struct hashfile *hashfd(const struct git_hash_algo *algop,\n> \t\t\tint fd, const char *name);\n> struct hashfile *hashfd_check(const struct git_hash_algo *algop,\n"},{"id":"537854","messageId":"20260305003745.GA4943@coredump.intra.peff.net","threadId":"65085","inReplyTo":"xmqqseaf5k5t.fsf@gitster.g","subject":"Re: [PATCH v2 05/10] compat/posix: introduce writev(3p) wrapper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-05T00:37:45Z","receivedAt":"2026-03-05T00:37:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 04, 2026 at 02:01:18PM -0800, Junio C Hamano wrote:\n\n> Because we do not check the accumulation of bytes_written in the two\n> accumulator variables inside the inner loop, it is very possible for\n> total_written to wraparound size_t and end up below the largest\n> value possible to be stored in ssize_t type.\n\nCoverity complained about it, too.\n\n> IOW, the cast_size_t_to_ssize_t() introduced in the previous step is\n> pointless, isn't it?\n> \n> According to [*1*], the real\n> \n> ssize_t writev(int fd, const struct iovec *iov, int iovcnt)\n> \n> is supposed to report error with errno set to EINVAL when the sum of\n> iov_len member of the iov[] array elements exceed half of the\n> maximum size_t.\n> \n> EINVAL The sum of the iov_len values overflows an ssize_t value.\n> \n> So instead of dying with cast_size_t_to_ssize_t(), we probably would\n> want the check done in a more stupid and straight-forward way?\n> Adding up iov[i].iov_len while the addition would not wraparound in\n> each and every step, and return error with EINVAL before attempting\n> even a single call to xwrite(), and then have the above double loop\n> that does not care about integer wraparound at all, and return\n> total_written with simple cast to (ssize_t)?\n\nI like that writev() can work as a drop-in replacement for write() at\nthe lowest level. But given that our main use is likely to be pkt-lines,\nI do kind of wonder if we should just try to be more clever in forming\nour buffers. That makes all of the portability and compat questions go\naway (and gives the benefit to platforms that don't even have writev).\n\nE.g., the somewhat ugly patch below is enough to do single write()s for\nthe pack data in upload-pack. I think if the pkt-writing API were less\nad-hoc (and had an actual buffer in \"struct packet_writer\"), this could\nbe made a bit more universal.\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 1b1c81ea63..d01b547b10 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -179,11 +179,32 @@ static void reset_timeout(unsigned int timeout)\n \talarm(timeout);\n }\n \n-static void send_client_data(int fd, const char *data, ssize_t sz,\n-\t\t\t int use_sideband)\n+#define send_client_data(fd, data, sz, use_sideband) \\\n+\tsend_client_data_1((fd), (data), (sz), (use_sideband), NULL)\n+static void send_client_data_1(int fd, const char *data, ssize_t sz,\n+\t\t\t int use_sideband, char *hdr_buf)\n {\n \tif (use_sideband) {\n-\t\tsend_sideband(1, fd, data, sz, use_sideband);\n+\t\t/*\n+\t\t * If we don't have a contiguous header buf, or if the data is\n+\t\t * too big for a single packet (which I don't think happens\n+\t\t * with any of our current callers), bail to the old-style\n+\t\t * send_sideband.\n+\t\t */\n+\t\tif (!hdr_buf || sz > LARGE_PACKET_DATA_MAX - 1) {\n+\t\t\tsend_sideband(1, fd, data, sz, use_sideband);\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tif (hdr_buf + 5 != data)\n+\t\t\tBUG(\"hdr_buf should be contiguous with pkt data\");\n+\n+\t\tif (use_sideband < 0)\n+\t\t\tBUG(\"negative sideband!?\");\n+\n+\t\tset_packet_header(hdr_buf, sz + 5);\n+\t\thdr_buf[4] = 1; /* sideband 1 */\n+\t\twrite_or_die(fd, hdr_buf, sz + 5);\n \t\treturn;\n \t}\n \tif (fd == 3)\n@@ -211,7 +232,14 @@ struct output_state {\n \t * sideband-64k the band designator takes up 1 byte of space. Because\n \t * relay_pack_data keeps the last byte to itself, we make the buffer 1\n \t * byte bigger than the intended maximum write size.\n+\t *\n+\t * However, we also reserve 5 bytes before the buffer so we can format\n+\t * a packet header and send it with a single write(). We may be pushing\n+\t * our luck on assuming there will be no padding here (though we do\n+\t * check it with a BUG(), but there are other ways to write it\n+\t * (e.g., a single large buffer with \"buffer\" as a pointer into it).\n \t */\n+\tchar hdr[5];\n \tchar buffer[(LARGE_PACKET_DATA_MAX - 1) + 1];\n \tint used;\n \tunsigned packfile_uris_started : 1;\n@@ -284,7 +312,7 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t\treturn readsz;\n \n \tif (os->used > 1) {\n-\t\tsend_client_data(1, os->buffer, os->used - 1, use_sideband);\n+\t\tsend_client_data_1(1, os->buffer, os->used - 1, use_sideband, os->hdr);\n \t\tos->buffer[0] = os->buffer[os->used - 1];\n \t\tos->used = 1;\n \t} else {\n\n-Peff\n"},{"id":"537869","messageId":"20260305005611.GB4943@coredump.intra.peff.net","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-2-7321830f08fe@pks.im","subject":"Re: [PATCH v2 02/10] upload-pack: adapt keepalives based on buffering","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-05T00:56:11Z","receivedAt":"2026-03-05T00:56:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 03, 2026 at 04:00:17PM +0100, Patrick Steinhardt wrote:\n\n> The most important edge case here happens in `relay_pack_data()`. When\n> we haven't seen the initial \"PACK\" signature from git-pack-objects(1)\n> yet we buffer incoming data. So in the worst case, if each of the bytes\n> of that signature arrive shortly before the configured keepalive\n> timeout, then we may not send out any data for a time period that is\n> (almost) four times as long as the configured timeout.\n\nThanks for laying out this case. I think this is all-but-impossible in\npractice, as anybody writing \"PACK\" is going to do so all at once. Even\n4 separate write() calls would be fine, as long as it does not pause in\nbetween!\n\nI think there's another one, too. If we are getting packfile_uris, and\npack-objects writes half a line, we will pause waiting for the complete\nline to show up. This also seems quite unlikely in practice.\n\n> This edge case is rather unlikely to matter in practice. But in a\n> subsequent commit we're going to adapt our buffering mechanism to become\n> more aggressive, which makes it more likely that we don't send any data\n> for an extended amount of time.\n> \n> Adapt the logic so that instead of using a fixed timeout on every call\n> to poll(3p), we instead figure out how much time has passed since the\n> last-sent data.\n\nOK. That should not be too bad to do, though...\n\n> @@ -365,10 +373,14 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n> \t */\n> \n> \twhile (1) {\n> +\t\tuint64_t now_ms = getnanotime() / 1000000;\n\n...now we are talking about wall-clock time since the epoch. What\nhappens if time goes backwards due to a clock reset?\n\nThen now_ms may be less than last_sent_ms, and then here:\n\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * The polling timeout needs to be adjusted based on\n> +\t\t\t * the time we have sent our last package. The longer\n> +\t\t\t * it's been in the past, the shorter the timeout\n> +\t\t\t * becomes until we eventually don't block at all.\n> +\t\t\t */\n> +\t\t\tpolltimeout_ms = 1000 * pack_data->keepalive - (now_ms - last_sent_ms);\n> +\t\t\tif (polltimeout_ms < 0)\n> +\t\t\t\tpolltimeout_ms = 0;\n> +\t\t}\n\n...we end up with a value higher than the keepalive, and we wait too\nlong. That's probably an OK outcome for such an exceptional condition.\nThe worst case if you set your clock back is that we fail to send\nkeepalives until the next actual data chunk arrives.\n\n> \t\tif (0 <= pe && (pfd[pe].revents & (POLLIN|POLLHUP))) {\n> \t\t\t/* Status ready; we ship that in the side-band\n> \t\t\t * or dump to the standard error.\n> \t\t\t */\n> \t\t\tsz = xread(pack_objects.err, progress,\n> \t\t\t\t sizeof(progress));\n> -\t\t\tif (0 < sz)\n> +\t\t\tif (0 < sz) {\n> \t\t\t\tsend_client_data(2, progress, sz,\n> \t\t\t\t\t\t pack_data->use_sideband);\n> -\t\t\telse if (sz == 0) {\n> +\t\t\t\tlast_sent_ms = now_ms;\n> +\t\t\t} else if (sz == 0) {\n> \t\t\t\tclose(pack_objects.err);\n> \t\t\t\tpack_objects.err = -1;\n> \t\t\t}\n\nOK, so here we know we sent data, because we pass along stderr as\nquickly as possible. If we got EOF on stderr, that doesn't count as\nsending data, because we don't pass along that EOF to the client. Makes\nsense.\n\n> \t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n> +\t\t\tbool did_send_data;\n> \t\t\tint result = relay_pack_data(pack_objects.out,\n> \t\t\t\t\t\t output_state,\n> \t\t\t\t\t\t pack_data->use_sideband,\n> -\t\t\t\t\t\t !!uri_protocols);\n> +\t\t\t\t\t\t !!uri_protocols,\n> +\t\t\t\t\t\t &did_send_data);\n\nAnd then for stdout, we have to ask relay_pack_data() if it sent\nanything. That makes sense.\n\n-Peff\n"},{"id":"537883","messageId":"20260305011638.GC4943@coredump.intra.peff.net","threadId":"65085","inReplyTo":"20260303-pks-upload-pack-write-contention-v2-3-7321830f08fe@pks.im","subject":"Re: [PATCH v2 03/10] upload-pack: reduce lock contention when writing packfile data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-05T01:16:38Z","receivedAt":"2026-03-05T01:16:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 03, 2026 at 04:00:18PM +0100, Patrick Steinhardt wrote:\n\n> Extend our use of the buffering infrastructure so that we soak up bytes\n> until the buffer is filled up at least 2/3rds of its capacity. The\n> change is relatively simple to implement as we already know to flush the\n> buffer in `create_pack_file()` after git-pack-objects(1) has finished.\n\nThis 2/3rds feels kind of arbitrary. Isn't our best bet to try to fill\npkt-lines? Later you say:\n\n> Now we could of course go even further and make sure that we always fill\n> up the whole buffer. But this might cause an increase in read(3p)\n> syscalls, and some tests show that this only reduces the number of\n> write(3p) syscalls from 130,000 to 100,000. So overall this doesn't seem\n> worth it.\n\nBut I am not clear how it increases the number of read() calls. I guess\nyou are concerned that we'll get 50k, and then do a read for the\nremaining 14k, and then read 50k, and then 14k, and so on. But I'm\nunconvinced that 2/3 is really any better here, as it depends on the\nbuffering patterns of the upstream writer. They could be writing 1 byte\nless than 2/3, and we'd wait to buffer, then read half their next\npacket, write it, then read the second of of their next packet, wait to\nbuffer, and so on.\n\nEven just doing:\n\n git clone --upload-pack='strace -e write git-upload-pack' --bare --no-local . foo.git\n\nwith this patch (and not the later one to increase the buffer size of\npack-objects), I see an interesting flip-flop between packets of size\n65515 and 61461. But we never send a single full-size one, even though\npack-objects should be outpacing us (because we're slowed by running\nunder strace). That's probably an OK loss of efficiency in practice, but\nit's very dependent on pack-objects buffering.\n\nI'm still a little bit negative on the whole concept of buffering in\nupload-pack, just because the interactions between buffering layers can\nbe so subtle. But I guess I'm not really making any argument that I\ndidn't make in v1, and you kept this in v2, so I suppose you are not\nswayed by it. ;)\n\n\nIf we are going to buffer in upload-pack, there is an obvious\noptimization that I didn't see in your series. When we send a keepalive,\nwe should just send whatever we have in os->buffer (even if it is\nnothing). If we are wasting 5 bytes of pkt-line header and a write()\ncall to send the keepalive, we may as well send what data we do have.\n\nI don't know how much it would help in practice, though. Most keepalives\nwill come before the pack data starts, as once pack-objects starts\nproducing data, it tends to do so pretty consistently. And of course we\ncan't send os->buffer before we see the PACK header, because the whole\npoint is to buffer the early bit waiting for packfile uris.\n\nSo it might not be worth adding.\n\n-Peff\n"},{"id":"537886","messageId":"aajnZZI5yqL9CYeR@fruit.crustytoothpaste.net","threadId":"65085","inReplyTo":"20260305003745.GA4943@coredump.intra.peff.net","subject":"Re: [PATCH v2 05/10] compat/posix: introduce writev(3p) wrapper","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-03-05T02:16:05Z","receivedAt":"2026-03-05T02:16:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2026-03-05 at 00:37:45, Jeff King wrote:\n> I like that writev() can work as a drop-in replacement for write() at\n> the lowest level. But given that our main use is likely to be pkt-lines,\n> I do kind of wonder if we should just try to be more clever in forming\n> our buffers. That makes all of the portability and compat questions go\n> away (and gives the benefit to platforms that don't even have writev).\n\nThis does work and it is clever, but I think the writev is clearer and\nmore explicit. In addition, this is literally the kind of use case that\nit's designed for and the kernel will have a highly optimized\nimplementation handling it. I could also see myself making use of\nwritev in my future work as well, although I don't have any concrete\ncode depending on it at the moment (but I could probably add it in the\ninterop code).\n\nThere is a Windows equivalent as well (according to the Rust\ndocumentation[0]) which doesn't have to be added at this point, but\ncould in the future as a quality-of-implementation issue. And almost\nall Unix systems will support this, with the possible exception of\nNonStop, since it's part of XSI and effectively everyone implements\nthose C functions. (Also, writev was in 4.2BSD, released in 1983, so\nyou'd have to be really behind the times to not support it.)\n\n[0] Rust defines the `std::io::IoSlice` type which is documented as\nfollows:\n\n A buffer type used with `Write::write_vectored`.\n\n It is semantically a wrapper around a `&[u8]`, but is guaranteed to\n be ABI compatible with the `iovec` type on Unix platforms and\n `WSABUF` on Windows.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"537890","messageId":"7e8b51d1-382d-47f8-be71-74e76f0c9888@kdbg.org","threadId":"65085","inReplyTo":"aajnZZI5yqL9CYeR@fruit.crustytoothpaste.net","subject":"Re: [PATCH v2 05/10] compat/posix: introduce writev(3p) wrapper","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2026-03-05T06:39:13Z","receivedAt":"2026-03-05T06:39:24Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.03.26 um 03:16 schrieb brian m. carlson:\n> On 2026-03-05 at 00:37:45, Jeff King wrote:\n>> I like that writev() can work as a drop-in replacement for write() at\n>> the lowest level. But given that our main use is likely to be pkt-lines,\n>> I do kind of wonder if we should just try to be more clever in forming\n>> our buffers. That makes all of the portability and compat questions go\n>> away (and gives the benefit to platforms that don't even have writev).\n> \n> This does work and it is clever, but I think the writev is clearer and\n> more explicit. In addition, this is literally the kind of use case that\n> it's designed for and the kernel will have a highly optimized\n> implementation handling it. I could also see myself making use of\n> writev in my future work as well,...\n\nPlease don't. The use of writev may mislead you to depend on guarantees\nthat a kernel implementation of writev can provide, but a compat/\nimplementation cannot. (For example, I read something about \"the file\npointer shall be unchanged\" in the case of errors.) Please use simple\nand stupid functions (write). I highly doubt that you can squeeze out a\nnoticable performance improvement with writev.\n\n-- Hannes\n\n"},{"id":"538003","messageId":"aaoCNUY3XVyFaDcu@fruit.crustytoothpaste.net","threadId":"65085","inReplyTo":"7e8b51d1-382d-47f8-be71-74e76f0c9888@kdbg.org","subject":"Re: [PATCH v2 05/10] compat/posix: introduce writev(3p) wrapper","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-03-05T22:22:45Z","receivedAt":"2026-03-05T22:22:47Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2026-03-05 at 06:39:13, Johannes Sixt wrote:\n> Please don't. The use of writev may mislead you to depend on guarantees\n> that a kernel implementation of writev can provide, but a compat/\n> implementation cannot. (For example, I read something about \"the file\n> pointer shall be unchanged\" in the case of errors.) Please use simple\n> and stupid functions (write). I highly doubt that you can squeeze out a\n> noticable performance improvement with writev.\n\nI have Rust code that does show a substantial performance improvement\nwith writev and I use it for a very similar purpose (printing the size\nbefore a packet of data).\n\nI am okay with slightly loosening the guarantees to support our compat\nimplementation if that's what we need to do.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"538401","messageId":"abAJ2iMVa9rwGgr5@pks.im","threadId":"65085","inReplyTo":"20260305005611.GB4943@coredump.intra.peff.net","subject":"Re: [PATCH v2 02/10] upload-pack: adapt keepalives based on buffering","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T12:08:58Z","receivedAt":"2026-03-10T12:09:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 04, 2026 at 07:56:11PM -0500, Jeff King wrote:\n> On Tue, Mar 03, 2026 at 04:00:17PM +0100, Patrick Steinhardt wrote:\n> \n> > The most important edge case here happens in `relay_pack_data()`. When\n> > we haven't seen the initial \"PACK\" signature from git-pack-objects(1)\n> > yet we buffer incoming data. So in the worst case, if each of the bytes\n> > of that signature arrive shortly before the configured keepalive\n> > timeout, then we may not send out any data for a time period that is\n> > (almost) four times as long as the configured timeout.\n> \n> Thanks for laying out this case. I think this is all-but-impossible in\n> practice, as anybody writing \"PACK\" is going to do so all at once. Even\n> 4 separate write() calls would be fine, as long as it does not pause in\n> between!\n> \n> I think there's another one, too. If we are getting packfile_uris, and\n> pack-objects writes half a line, we will pause waiting for the complete\n> line to show up. This also seems quite unlikely in practice.\n\nIt could happen in case the data was written by the pack-objects hook.\nBut I fully agree that this is a rather unlikely scenario.\n\n> > This edge case is rather unlikely to matter in practice. But in a\n> > subsequent commit we're going to adapt our buffering mechanism to become\n> > more aggressive, which makes it more likely that we don't send any data\n> > for an extended amount of time.\n> > \n> > Adapt the logic so that instead of using a fixed timeout on every call\n> > to poll(3p), we instead figure out how much time has passed since the\n> > last-sent data.\n> \n> OK. That should not be too bad to do, though...\n> \n> > @@ -365,10 +373,14 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n> > \t */\n> > \n> > \twhile (1) {\n> > +\t\tuint64_t now_ms = getnanotime() / 1000000;\n> \n> ...now we are talking about wall-clock time since the epoch. What\n> happens if time goes backwards due to a clock reset?\n> \n> Then now_ms may be less than last_sent_ms, and then here:\n> \n> > +\t\t} else {\n> > +\t\t\t/*\n> > +\t\t\t * The polling timeout needs to be adjusted based on\n> > +\t\t\t * the time we have sent our last package. The longer\n> > +\t\t\t * it's been in the past, the shorter the timeout\n> > +\t\t\t * becomes until we eventually don't block at all.\n> > +\t\t\t */\n> > +\t\t\tpolltimeout_ms = 1000 * pack_data->keepalive - (now_ms - last_sent_ms);\n> > +\t\t\tif (polltimeout_ms < 0)\n> > +\t\t\t\tpolltimeout_ms = 0;\n> > +\t\t}\n> \n> ...we end up with a value higher than the keepalive, and we wait too\n> long. That's probably an OK outcome for such an exceptional condition.\n> The worst case if you set your clock back is that we fail to send\n> keepalives until the next actual data chunk arrives.\n\nRight. It's rather unlikely to happen, and in addition to that we use a\nmonotonic clock in `getnanotime()` on systems with `CLOCK_MONOTONIC` and\non Windows. Which probably covers almost all platforms out there.\n\nPatrick\n"},{"id":"538402","messageId":"abAJ6mkYp5chzXrt@pks.im","threadId":"65085","inReplyTo":"xmqqjyvr5jos.fsf@gitster.g","subject":"Re: [PATCH v2 08/10] csum-file: introduce `hashfd_ext()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T12:09:14Z","receivedAt":"2026-03-10T12:09:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 04, 2026 at 02:11:31PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > -static struct hashfile *hashfd_internal(const struct git_hash_algo *algop,\n> > -\t\t\t\t\tint fd, const char *name,\n> > -\t\t\t\t\tstruct progress *tp,\n> > -\t\t\t\t\tsize_t buffer_len)\n> > +struct hashfile *hashfd_ext(const struct git_hash_algo *algop,\n> > +\t\t\t int fd, const char *name,\n> > +\t\t\t const struct hashfd_options *opts)\n> \n> What does _ext stand for?\n\n\"extended\". It's a pattern that we use in lots of other places, too.\n\n> More seriously, this essentially chooses to pick two parameters\n> hashfd_internal() takes and put them in a struct, which would give\n> us a clear upgrade path to add more to the structure without having\n> to change the function signature. But what is the criteria used to\n> choose these two among 5 parameters the original function takes?\n\nThe criteria is mostly whether the parameter is optional or not. The\nalgorithm, file descriptor and name are all mandatory, whereas all the\nother parts in the options structure are optional.\n\nPatrick\n"},{"id":"538403","messageId":"abAKD91MweDTS4jv@pks.im","threadId":"65085","inReplyTo":"aaoCNUY3XVyFaDcu@fruit.crustytoothpaste.net","subject":"Re: [PATCH v2 05/10] compat/posix: introduce writev(3p) wrapper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T12:09:51Z","receivedAt":"2026-03-10T12:09:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 05, 2026 at 10:22:45PM +0000, brian m. carlson wrote:\n> On 2026-03-05 at 06:39:13, Johannes Sixt wrote:\n> > Please don't. The use of writev may mislead you to depend on guarantees\n> > that a kernel implementation of writev can provide, but a compat/\n> > implementation cannot. (For example, I read something about \"the file\n> > pointer shall be unchanged\" in the case of errors.) Please use simple\n> > and stupid functions (write). I highly doubt that you can squeeze out a\n> > noticable performance improvement with writev.\n> \n> I have Rust code that does show a substantial performance improvement\n> with writev and I use it for a very similar purpose (printing the size\n> before a packet of data).\n> \n> I am okay with slightly loosening the guarantees to support our compat\n> implementation if that's what we need to do.\n\nYeah, I think that having a syscall like writev(3p) in our codebase\ncould likely also lead to improvements in other areas. Nice to see that\nit'll also help your use case.\n\nSo for now I'll stick with it, but will fix overflow handling as\nsuggested by Junio.\n\nThanks!\n\nPatrick\n"},{"id":"538405","messageId":"abALMbYE0C0UlwbD@pks.im","threadId":"65085","inReplyTo":"20260305011638.GC4943@coredump.intra.peff.net","subject":"Re: [PATCH v2 03/10] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T12:14:41Z","receivedAt":"2026-03-10T12:14:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 04, 2026 at 08:16:38PM -0500, Jeff King wrote:\n> On Tue, Mar 03, 2026 at 04:00:18PM +0100, Patrick Steinhardt wrote:\n> \n> > Extend our use of the buffering infrastructure so that we soak up bytes\n> > until the buffer is filled up at least 2/3rds of its capacity. The\n> > change is relatively simple to implement as we already know to flush the\n> > buffer in `create_pack_file()` after git-pack-objects(1) has finished.\n> \n> This 2/3rds feels kind of arbitrary. Isn't our best bet to try to fill\n> pkt-lines? Later you say:\n> \n> > Now we could of course go even further and make sure that we always fill\n> > up the whole buffer. But this might cause an increase in read(3p)\n> > syscalls, and some tests show that this only reduces the number of\n> > write(3p) syscalls from 130,000 to 100,000. So overall this doesn't seem\n> > worth it.\n> \n> But I am not clear how it increases the number of read() calls. I guess\n> you are concerned that we'll get 50k, and then do a read for the\n> remaining 14k, and then read 50k, and then 14k, and so on. But I'm\n> unconvinced that 2/3 is really any better here, as it depends on the\n> buffering patterns of the upstream writer. They could be writing 1 byte\n> less than 2/3, and we'd wait to buffer, then read half their next\n> packet, write it, then read the second of of their next packet, wait to\n> buffer, and so on.\n> \n> Even just doing:\n> \n> git clone --upload-pack='strace -e write git-upload-pack' --bare --no-local . foo.git\n> \n> with this patch (and not the later one to increase the buffer size of\n> pack-objects), I see an interesting flip-flop between packets of size\n> 65515 and 61461. But we never send a single full-size one, even though\n> pack-objects should be outpacing us (because we're slowed by running\n> under strace). That's probably an OK loss of efficiency in practice, but\n> it's very dependent on pack-objects buffering.\n\nThe big question though is whether it's a loss of efficiency compared to\nthe previous state where we didn't buffer at all.\n\n> I'm still a little bit negative on the whole concept of buffering in\n> upload-pack, just because the interactions between buffering layers can\n> be so subtle. But I guess I'm not really making any argument that I\n> didn't make in v1, and you kept this in v2, so I suppose you are not\n> swayed by it. ;)\n\nYeah. Its usefulness highly depends on the downstream command indeed, so\nit may or may not help. What I'm not convinced of though is that there\nare likely scenarios where it'll perform _worse_ than before. If we can\ncome up with such a scenario then I'll be happy to drop this patch.\n\nFor downstream git-pack-objects(1) it shouldn't really make much of a\ndifference at the end of this series anyway, as it starts to write data\nat pktline lengh. But it may help users of the pack-objects hook.\n\n> If we are going to buffer in upload-pack, there is an obvious\n> optimization that I didn't see in your series. When we send a keepalive,\n> we should just send whatever we have in os->buffer (even if it is\n> nothing). If we are wasting 5 bytes of pkt-line header and a write()\n> call to send the keepalive, we may as well send what data we do have.\n> \n> I don't know how much it would help in practice, though. Most keepalives\n> will come before the pack data starts, as once pack-objects starts\n> producing data, it tends to do so pretty consistently. And of course we\n> can't send os->buffer before we see the PACK header, because the whole\n> point is to buffer the early bit waiting for packfile uris.\n> \n> So it might not be worth adding.\n\nIt's an interesting idea indeed. We also need to be mindful that we try\nto hold one byte back so that we can \"fail\" downstream callers in case\ngit-pack-objects(1) fails.\n\nSo we need to repeat some conditions here, but overall it's not too bad:\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 1b1c81ea63..199bbd1ad6 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -484,7 +484,16 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\t */\n \t\tif (!ret && pack_data->use_sideband) {\n \t\t\tstatic const char buf[] = \"0005\\1\";\n-\t\t\twrite_or_die(1, buf, 5);\n+\n+\t\t\tif (output_state->packfile_started && output_state->used > 1) {\n+\t\t\t\tsend_client_data(1, output_state->buffer, output_state->used - 1,\n+\t\t\t\t\t\t pack_data->use_sideband);\n+\t\t\t\toutput_state->buffer[0] = output_state->buffer[output_state->used - 1];\n+\t\t\t\toutput_state->used = 1;\n+\t\t\t} else {\n+\t\t\t\twrite_or_die(1, buf, 5);\n+\t\t\t}\n+\n \t\t\tlast_sent_ms = now_ms;\n \t\t}\n \t}\n\nI'll include this in the next version, thanks!\n\nPatrick\n"},{"id":"538420","messageId":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260227-pks-upload-pack-write-contention-v1-0-7166fe255704@pks.im","subject":"[PATCH v3 00/10] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:24:56Z","receivedAt":"2026-03-10T13:25:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series fixes some heavy lock contention when writing\ndata from git-upload-pack(1) into pipes. This lock contention can be\nobserved when having hundreds of git-upload-pack(1) processes active at\nthe same time that write data into pipes at dozens of gigabits per\nsecond.\n\nI have uploaded the flame graph that clearly shows the lock contention\nat [1].\n\nChanges in v3:\n - Fix handling of `iov_len` overflows in writev(3p) wrapper.\n - Add another patch that causes us to flush out data instead of\n sending a 0005 keepalive packet.\n - Link to v2: https://lore.kernel.org/r/20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im\n\nChanges in v2:\n - Change the buffer size in git-pack-objects(1) to also reduce the\n number of write syscalls over there.\n - Introduce writev to half the number of syscalls when writing\n pktlines.\n - Use `sizeof(os->buffer)` instead of open-coding its size.\n - Improve keepalive logic in git-upload-pack(1) to account for\n buffering.\n - Link to v1: https://lore.kernel.org/r/20260227-pks-upload-pack-write-contention-v1-0-7166fe255704@pks.im\n\nThanks!\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/git/-/work_items/675\n\n---\nPatrick Steinhardt (10):\n upload-pack: fix debug statement when flushing packfile data\n upload-pack: adapt keepalives based on buffering\n upload-pack: prefer flushing data over sending keepalive\n upload-pack: reduce lock contention when writing packfile data\n compat/posix: introduce writev(3p) wrapper\n wrapper: introduce writev(3p) wrappers\n sideband: use writev(3p) to send pktlines\n csum-file: introduce `hashfd_ext()`\n csum-file: drop `hashfd_throughput()`\n builtin/pack-objects: reduce lock contention when writing packfile data\n\n Makefile | 4 +++\n builtin/pack-objects.c | 23 +++++++++++---\n compat/posix.h | 14 +++++++++\n compat/writev.c | 44 +++++++++++++++++++++++++++\n config.mak.uname | 2 ++\n csum-file.c | 28 +++++------------\n csum-file.h | 16 ++++++++--\n meson.build | 1 +\n sideband.c | 14 +++++++--\n upload-pack.c | 81 +++++++++++++++++++++++++++++++++++++++-----------\n wrapper.c | 41 +++++++++++++++++++++++++\n wrapper.h | 9 ++++++\n write-or-die.c | 8 +++++\n write-or-die.h | 1 +\n 14 files changed, 239 insertions(+), 47 deletions(-)\n\nRange-diff versus v2:\n\n 1: ef3244ffcc = 1: 540b577540 upload-pack: fix debug statement when flushing packfile data\n 2: 3297edd609 = 2: 253d119b10 upload-pack: adapt keepalives based on buffering\n -: ---------- > 3: 960c650063 upload-pack: prefer flushing data over sending keepalive\n 3: 84671fb222 = 4: 0e147c41cf upload-pack: reduce lock contention when writing packfile data\n 4: bd1f070cb4 < -: ---------- git-compat-util: introduce `cast_size_t_to_ssize_t()`\n 5: d5acbd1584 ! 5: b6fbd89b8e compat/posix: introduce writev(3p) wrapper\n @@ compat/writev.c (new)\n +ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)\n +{\n +\tsize_t total_written = 0;\n ++\tsize_t sum = 0;\n ++\n ++\t/*\n ++\t * According to writev(3p), the syscall shall error with EINVAL in case\n ++\t * the sum of `iov_len` overflows `ssize_t`.\n ++\t */\n ++\t for (int i = 0; i < iovcnt; i++) {\n ++\t\tif (iov[i].iov_len > maximum_signed_value_of_type(ssize_t) ||\n ++\t\t iov[i].iov_len + sum > maximum_signed_value_of_type(ssize_t)) {\n ++\t\t\terrno = EINVAL;\n ++\t\t\treturn -1;\n ++\t\t}\n ++\n ++\t\tsum += iov[i].iov_len;\n ++\t}\n +\n +\tfor (int i = 0; i < iovcnt; i++) {\n +\t\tconst char *bytes = iov[i].iov_base;\n @@ compat/writev.c (new)\n +\t}\n +\n +out:\n -+\treturn cast_size_t_to_ssize_t(total_written);\n ++\treturn (ssize_t) total_written;\n +}\n \n ## config.mak.uname ##\n 6: e3525cd25e = 6: 0a90ab2f62 wrapper: introduce writev(3p) wrappers\n 7: fe07f2d331 = 7: 5e29cfbf10 sideband: use writev(3p) to send pktlines\n 8: 4563ad923f = 8: 32b59d5f29 csum-file: introduce `hashfd_ext()`\n 9: 549b3dbefc = 9: a5b0eb3627 csum-file: drop `hashfd_throughput()`\n10: 155f79b128 = 10: a82959772e builtin/pack-objects: reduce lock contention when writing packfile data\n\n---\nbase-commit: fb1b83bcddff60463f6e86bb021784c88d0b748c\nchange-id: 20260227-pks-upload-pack-write-contention-435ce01f5fe9\n\n"},{"id":"538421","messageId":"20260310-pks-upload-pack-write-contention-v3-1-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"[PATCH v3 01/10] upload-pack: fix debug statement when flushing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:24:57Z","receivedAt":"2026-03-10T13:25:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When git-upload-pack(1) writes packfile data to the client we have some\nlogic in place that buffers some partial lines. When that buffer still\ncontains data after git-pack-objects(1) has finished we flush the buffer\nso that all remaining bytes are sent out.\n\nCuriously, when we do so we also print the string \"flushed.\" to stderr.\nThis statement has been introduced in b1c71b7281 (upload-pack: avoid\nsending an incomplete pack upon failure, 2006-06-20), so quite a while\nago. What's interesting though is that stderr is typically spliced\nthrough to the client-side, and consequently the client would see this\nmessage. Munging the way how we do the caching indeed confirms this:\n\n $ git clone file:///home/pks/Development/linux/\n Cloning into bare repository 'linux.git'...\n remote: Enumerating objects: 12980346, done.\n remote: Counting objects: 100% (131820/131820), done.\n remote: Compressing objects: 100% (50290/50290), done.\n remote: Total 12980346 (delta 96319), reused 104500 (delta 81217), pack-reused 12848526 (from 1)\n Receiving objects: 100% (12980346/12980346), 3.23 GiB | 57.44 MiB/s, done.\n flushed.\n Resolving deltas: 100% (10676718/10676718), done.\n\nIt's quite clear that this string shouldn't ever be visible to the\nclient, so it rather feels like this is a left-over debug statement. The\nmenitoned commit doesn't mention this line, either.\n\nRemove the debug output to prepare for a change in how we do the\nbuffering in the next commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex e8c5cce1c7..b3a8561ef5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -457,11 +457,9 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t}\n \n \t/* flush the data */\n-\tif (output_state->used > 0) {\n+\tif (output_state->used > 0)\n \t\tsend_client_data(1, output_state->buffer, output_state->used,\n \t\t\t\t pack_data->use_sideband);\n-\t\tfprintf(stderr, \"flushed.\\n\");\n-\t}\n \tfree(output_state);\n \tif (pack_data->use_sideband)\n \t\tpacket_flush(1);\n\n-- \n2.53.0.880.g73c4285caa.dirty\n\n"},{"id":"538422","messageId":"20260310-pks-upload-pack-write-contention-v3-2-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"[PATCH v3 02/10] upload-pack: adapt keepalives based on buffering","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:24:58Z","receivedAt":"2026-03-10T13:25:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `create_pack_file()` is responsible for sending the\npackfile data to the client of git-upload-pack(1). As generating the\nbytes may take significant computing resources we also have a mechanism\nin place that optionally sends keepalive pktlines in case we haven't\nsent out any data.\n\nThe keepalive logic is purely based poll(3p): we pass a timeout to that\nsyscall, and if the call times out we send out the keepalive pktline.\nWhile reasonable, this logic isn't entirely sufficient: even if the call\nto poll(3p) ends because we have received data on any of the file\ndescriptors we may not necessarily send data to the client.\n\nThe most important edge case here happens in `relay_pack_data()`. When\nwe haven't seen the initial \"PACK\" signature from git-pack-objects(1)\nyet we buffer incoming data. So in the worst case, if each of the bytes\nof that signature arrive shortly before the configured keepalive\ntimeout, then we may not send out any data for a time period that is\n(almost) four times as long as the configured timeout.\n\nThis edge case is rather unlikely to matter in practice. But in a\nsubsequent commit we're going to adapt our buffering mechanism to become\nmore aggressive, which makes it more likely that we don't send any data\nfor an extended amount of time.\n\nAdapt the logic so that instead of using a fixed timeout on every call\nto poll(3p), we instead figure out how much time has passed since the\nlast-sent data.\n\nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 49 ++++++++++++++++++++++++++++++++++++++++---------\n 1 file changed, 40 insertions(+), 9 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b3a8561ef5..f6f380a601 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -29,6 +29,7 @@\n #include \"commit-graph.h\"\n #include \"commit-reach.h\"\n #include \"shallow.h\"\n+#include \"trace.h\"\n #include \"write-or-die.h\"\n #include \"json-writer.h\"\n #include \"strmap.h\"\n@@ -218,7 +219,8 @@ struct output_state {\n };\n \n static int relay_pack_data(int pack_objects_out, struct output_state *os,\n-\t\t\t int use_sideband, int write_packfile_line)\n+\t\t\t int use_sideband, int write_packfile_line,\n+\t\t\t bool *did_send_data)\n {\n \t/*\n \t * We keep the last byte to ourselves\n@@ -232,6 +234,8 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t */\n \tssize_t readsz;\n \n+\t*did_send_data = false;\n+\n \treadsz = xread(pack_objects_out, os->buffer + os->used,\n \t\t sizeof(os->buffer) - os->used);\n \tif (readsz < 0) {\n@@ -247,6 +251,7 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t\t\t\tif (os->packfile_uris_started)\n \t\t\t\t\tpacket_delim(1);\n \t\t\t\tpacket_write_fmt(1, \"\\1packfile\\n\");\n+\t\t\t\t*did_send_data = true;\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n@@ -259,6 +264,7 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t\t\t}\n \t\t\t*p = '\\0';\n \t\t\tpacket_write_fmt(1, \"\\1%s\\n\", os->buffer);\n+\t\t\t*did_send_data = true;\n \n \t\t\tos->used -= p - os->buffer + 1;\n \t\t\tmemmove(os->buffer, p + 1, os->used);\n@@ -279,6 +285,7 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t\tos->used = 0;\n \t}\n \n+\t*did_send_data = true;\n \treturn readsz;\n }\n \n@@ -290,6 +297,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \tchar progress[128];\n \tchar abort_msg[] = \"aborting due to possible repository \"\n \t\t\"corruption on the remote side.\";\n+\tuint64_t last_sent_ms = 0;\n \tssize_t sz;\n \tint i;\n \tFILE *pipe_fd;\n@@ -365,10 +373,14 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t */\n \n \twhile (1) {\n+\t\tuint64_t now_ms = getnanotime() / 1000000;\n \t\tstruct pollfd pfd[2];\n-\t\tint pe, pu, pollsize, polltimeout;\n+\t\tint pe, pu, pollsize, polltimeout_ms;\n \t\tint ret;\n \n+\t\tif (!last_sent_ms)\n+\t\t\tlast_sent_ms = now_ms;\n+\n \t\treset_timeout(pack_data->timeout);\n \n \t\tpollsize = 0;\n@@ -390,11 +402,21 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\tif (!pollsize)\n \t\t\tbreak;\n \n-\t\tpolltimeout = pack_data->keepalive < 0\n-\t\t\t? -1\n-\t\t\t: 1000 * pack_data->keepalive;\n+\t\tif (pack_data->keepalive < 0) {\n+\t\t\tpolltimeout_ms = -1;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * The polling timeout needs to be adjusted based on\n+\t\t\t * the time we have sent our last package. The longer\n+\t\t\t * it's been in the past, the shorter the timeout\n+\t\t\t * becomes until we eventually don't block at all.\n+\t\t\t */\n+\t\t\tpolltimeout_ms = 1000 * pack_data->keepalive - (now_ms - last_sent_ms);\n+\t\t\tif (polltimeout_ms < 0)\n+\t\t\t\tpolltimeout_ms = 0;\n+\t\t}\n \n-\t\tret = poll(pfd, pollsize, polltimeout);\n+\t\tret = poll(pfd, pollsize, polltimeout_ms);\n \n \t\tif (ret < 0) {\n \t\t\tif (errno != EINTR) {\n@@ -403,16 +425,18 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+\n \t\tif (0 <= pe && (pfd[pe].revents & (POLLIN|POLLHUP))) {\n \t\t\t/* Status ready; we ship that in the side-band\n \t\t\t * or dump to the standard error.\n \t\t\t */\n \t\t\tsz = xread(pack_objects.err, progress,\n \t\t\t\t sizeof(progress));\n-\t\t\tif (0 < sz)\n+\t\t\tif (0 < sz) {\n \t\t\t\tsend_client_data(2, progress, sz,\n \t\t\t\t\t\t pack_data->use_sideband);\n-\t\t\telse if (sz == 0) {\n+\t\t\t\tlast_sent_ms = now_ms;\n+\t\t\t} else if (sz == 0) {\n \t\t\t\tclose(pack_objects.err);\n \t\t\t\tpack_objects.err = -1;\n \t\t\t}\n@@ -421,11 +445,14 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\t\t/* give priority to status messages */\n \t\t\tcontinue;\n \t\t}\n+\n \t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n+\t\t\tbool did_send_data;\n \t\t\tint result = relay_pack_data(pack_objects.out,\n \t\t\t\t\t\t output_state,\n \t\t\t\t\t\t pack_data->use_sideband,\n-\t\t\t\t\t\t !!uri_protocols);\n+\t\t\t\t\t\t !!uri_protocols,\n+\t\t\t\t\t\t &did_send_data);\n \n \t\t\tif (result == 0) {\n \t\t\t\tclose(pack_objects.out);\n@@ -433,6 +460,9 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\t\t} else if (result < 0) {\n \t\t\t\tgoto fail;\n \t\t\t}\n+\n+\t\t\tif (did_send_data)\n+\t\t\t\tlast_sent_ms = now_ms;\n \t\t}\n \n \t\t/*\n@@ -448,6 +478,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\tif (!ret && pack_data->use_sideband) {\n \t\t\tstatic const char buf[] = \"0005\\1\";\n \t\t\twrite_or_die(1, buf, 5);\n+\t\t\tlast_sent_ms = now_ms;\n \t\t}\n \t}\n \n\n-- \n2.53.0.880.g73c4285caa.dirty\n\n"},{"id":"538423","messageId":"20260310-pks-upload-pack-write-contention-v3-3-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"[PATCH v3 03/10] upload-pack: prefer flushing data over sending keepalive","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:24:59Z","receivedAt":"2026-03-10T13:25:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When using the sideband in git-upload-pack(1) we know to send out\nkeepalive packets in case generating the pack takes too long. These\nkeepalives take the form of a simple empty pktline.\n\nIn the preceding commit we have adapted git-upload-pack(1) to buffer\ndata more aggressively before sending it to the client. This creates an\nobvious optimization opportunity: when we hit the keepalive timeout\nwhile we still hold on to some buffered data, then it makes more sense\nto flush out the data instead of sending the empty keepalive packet.\n\nThis is overall not going to be a significant win. Most keepalives will\ncome before the pack data starts, and once pack-objects starts producing\ndata, it tends to do so pretty consistently. And of course we can't send\ndata before we see the PACK header, because the whole point is to buffer\nthe early bit waiting for packfile URIs. But the optimization is easy\nenough to realize.\n\nDo so and flush out data instead of sending an empty pktline. While at\nit, drop the useless\n\nSuggested-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 21 +++++++++++++++------\n 1 file changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex f6f380a601..7a165d226d 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -466,18 +466,27 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\t}\n \n \t\t/*\n-\t\t * We hit the keepalive timeout without saying anything; send\n-\t\t * an empty message on the data sideband just to let the other\n-\t\t * side know we're still working on it, but don't have any data\n-\t\t * yet.\n+\t\t * We hit the keepalive timeout without saying anything. If we\n+\t\t * have pending data we flush it out to the caller now.\n+\t\t * Otherwise, we send an empty message on the data sideband\n+\t\t * just to let the other side know we're still working on it,\n+\t\t * but don't have any data yet.\n \t\t *\n \t\t * If we don't have a sideband channel, there's no room in the\n \t\t * protocol to say anything, so those clients are just out of\n \t\t * luck.\n \t\t */\n \t\tif (!ret && pack_data->use_sideband) {\n-\t\t\tstatic const char buf[] = \"0005\\1\";\n-\t\t\twrite_or_die(1, buf, 5);\n+\t\t\tif (output_state->packfile_started && output_state->used > 1) {\n+\t\t\t\tsend_client_data(1, output_state->buffer, output_state->used - 1,\n+\t\t\t\t\t\t pack_data->use_sideband);\n+\t\t\t\toutput_state->buffer[0] = output_state->buffer[output_state->used - 1];\n+\t\t\t\toutput_state->used = 1;\n+\t\t\t} else {\n+\t\t\t\tstatic const char buf[] = \"0005\\1\";\n+\t\t\t\twrite_or_die(1, buf, 5);\n+\t\t\t}\n+\n \t\t\tlast_sent_ms = now_ms;\n \t\t}\n \t}\n\n-- \n2.53.0.880.g73c4285caa.dirty\n\n"},{"id":"538424","messageId":"20260310-pks-upload-pack-write-contention-v3-4-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"[PATCH v3 04/10] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:25:00Z","receivedAt":"2026-03-10T13:25:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In our production systems we have recently observed write contention in\ngit-upload-pack(1). The system in question was consistently streaming\npackfiles at a rate of dozens of gigabits per second, but curiously the\nsystem was neither bottlenecked on CPU, memory or IOPS.\n\nWe eventually discovered that Git was spending 80% of its time in\n`pipe_write()`, out of which almost all of the time was spent in the\n`ep_poll_callback` function in the kernel. Quoting the reporter:\n\n This infrastructure is part of an event notification queue designed to\n allow for multiple producers to emit events, but that concurrency\n safety is guarded by 3 layers of locking. The layer we're hitting\n contention in uses a simple reader/writer lock mode (a.k.a. shared\n versus exclusive mode), where producers need shared-mode (read mode),\n and various other actions use exclusive (write) mode.\n\nThe system in question generates workloads where we have hundreds of\ngit-upload-pack(1) processes active at the same point in time. These\nprocesses end up contending around those locks, and the consequence is\nthat the Git processes stall.\n\nNow git-upload-pack(1) already has the infrastructure in place to buffer\nsome of the data it reads from git-pack-objects(1) before actually\nsending it out. We only use this infrastructure in very limited ways\nthough, so we generally end up matching one read(3p) call with one\nwrite(3p) call. Even worse, when the sideband is enabled we end up\nmatching one read with _two_ writes: one for the pkt-line length, and\none for the packfile data.\n\nExtend our use of the buffering infrastructure so that we soak up bytes\nuntil the buffer is filled up at least 2/3rds of its capacity. The\nchange is relatively simple to implement as we already know to flush the\nbuffer in `create_pack_file()` after git-pack-objects(1) has finished.\n\nThis significantly reduces the number of write(3p) syscalls we need to\ndo. Before this change, cloning the Linux repository resulted in around\n400,000 write(3p) syscalls. With the buffering in place we only do\naround 130,000 syscalls.\n\nNow we could of course go even further and make sure that we always fill\nup the whole buffer. But this might cause an increase in read(3p)\nsyscalls, and some tests show that this only reduces the number of\nwrite(3p) syscalls from 130,000 to 100,000. So overall this doesn't seem\nworth it.\n\nNote that the issue could also be fixed by adapting the write buffer\nthat we use in the downstream git-pack-objects(1) command, and such a\nchange would have roughly the same result. But the command that\ngenerates the packfile data may not always be git-pack-objects(1) as it\ncan be changed via \"uploadpack.packObjectsHook\", so such a fix would\nonly help in _some_ cases. Regardless of that, we'll also adapt the\nwrite buffer size of git-pack-objects(1) in a subsequent commit.\n\nHelped-by: Matt Smiley \nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7a165d226d..9f6d6fe48c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -276,6 +276,13 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t\t}\n \t}\n \n+\t/*\n+\t * Make sure that we buffer some data before sending it to the client.\n+\t * This significantly reduces the number of write(3p) syscalls.\n+\t */\n+\tif (readsz && os->used < (sizeof(os->buffer) * 2 / 3))\n+\t\treturn readsz;\n+\n \tif (os->used > 1) {\n \t\tsend_client_data(1, os->buffer, os->used - 1, use_sideband);\n \t\tos->buffer[0] = os->buffer[os->used - 1];\n\n-- \n2.53.0.880.g73c4285caa.dirty\n\n"},{"id":"538425","messageId":"20260310-pks-upload-pack-write-contention-v3-5-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"[PATCH v3 05/10] compat/posix: introduce writev(3p) wrapper","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:25:01Z","receivedAt":"2026-03-10T13:25:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In a subsequent commit we're going to add the first caller to\nwritev(3p). Introduce a compatibility wrapper for this syscall that we\ncan use on systems that don't have this syscall.\n\nThe syscall exists on modern Unixes like Linux and macOS, and seemingly\neven for NonStop according to [1]. It doesn't seem to exist on Windows\nthough.\n\n[1]: http://nonstoptools.com/manuals/OSS-SystemCalls.pdf\n[2]: https://www.gnu.org/software/gnulib/manual/html_node/writev.html\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 4 ++++\n compat/posix.h | 14 ++++++++++++++\n compat/writev.c | 44 ++++++++++++++++++++++++++++++++++++++++++++\n config.mak.uname | 2 ++\n meson.build | 1 +\n 5 files changed, 65 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex f3264d0a37..493851162d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2021,6 +2021,10 @@ ifdef NO_PREAD\n \tCOMPAT_CFLAGS += -DNO_PREAD\n \tCOMPAT_OBJS += compat/pread.o\n endif\n+ifdef NO_WRITEV\n+\tCOMPAT_CFLAGS += -DNO_WRITEV\n+\tCOMPAT_OBJS += compat/writev.o\n+endif\n ifdef NO_FAST_WORKING_DIRECTORY\n \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n endif\ndiff --git a/compat/posix.h b/compat/posix.h\nindex 245386fa4a..3c611d2736 100644\n--- a/compat/posix.h\n+++ b/compat/posix.h\n@@ -137,6 +137,9 @@\n #include \n #include \n #include \n+#ifndef NO_WRITEV\n+#include \n+#endif\n #include \n #ifndef NO_SYS_SELECT_H\n #include \n@@ -323,6 +326,17 @@ int git_lstat(const char *, struct stat *);\n ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);\n #endif\n \n+#ifdef NO_WRITEV\n+#define writev git_writev\n+#define iovec git_iovec\n+struct git_iovec {\n+\tvoid *iov_base;\n+\tsize_t iov_len;\n+};\n+\n+ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt);\n+#endif\n+\n #ifdef NO_SETENV\n #define setenv gitsetenv\n int gitsetenv(const char *, const char *, int);\ndiff --git a/compat/writev.c b/compat/writev.c\nnew file mode 100644\nindex 0000000000..3a94870a2f\n--- /dev/null\n+++ b/compat/writev.c\n@@ -0,0 +1,44 @@\n+#include \"../git-compat-util.h\"\n+#include \"../wrapper.h\"\n+\n+ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)\n+{\n+\tsize_t total_written = 0;\n+\tsize_t sum = 0;\n+\n+\t/*\n+\t * According to writev(3p), the syscall shall error with EINVAL in case\n+\t * the sum of `iov_len` overflows `ssize_t`.\n+\t */\n+\t for (int i = 0; i < iovcnt; i++) {\n+\t\tif (iov[i].iov_len > maximum_signed_value_of_type(ssize_t) ||\n+\t\t iov[i].iov_len + sum > maximum_signed_value_of_type(ssize_t)) {\n+\t\t\terrno = EINVAL;\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\tsum += iov[i].iov_len;\n+\t}\n+\n+\tfor (int i = 0; i < iovcnt; i++) {\n+\t\tconst char *bytes = iov[i].iov_base;\n+\t\tsize_t iovec_written = 0;\n+\n+\t\twhile (iovec_written < iov[i].iov_len) {\n+\t\t\tssize_t bytes_written = xwrite(fd, bytes + iovec_written,\n+\t\t\t\t\t\t iov[i].iov_len - iovec_written);\n+\t\t\tif (bytes_written < 0) {\n+\t\t\t\tif (total_written)\n+\t\t\t\t\tgoto out;\n+\t\t\t\treturn bytes_written;\n+\t\t\t}\n+\t\t\tif (!bytes_written)\n+\t\t\t\tgoto out;\n+\t\t\tiovec_written += bytes_written;\n+\t\t\ttotal_written += bytes_written;\n+\t\t}\n+\t}\n+\n+out:\n+\treturn (ssize_t) total_written;\n+}\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 5feb582558..ccb3f71881 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -459,6 +459,7 @@ ifeq ($(uname_S),Windows)\n \tSANE_TOOL_PATH ?= $(msvc_bin_dir_msys)\n \tHAVE_ALLOCA_H = YesPlease\n \tNO_PREAD = YesPlease\n+\tNO_WRITEV = YesPlease\n \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n \tNO_LIBGEN_H = YesPlease\n \tNO_POLL = YesPlease\n@@ -674,6 +675,7 @@ ifeq ($(uname_S),MINGW)\n \tpathsep = ;\n \tHAVE_ALLOCA_H = YesPlease\n \tNO_PREAD = YesPlease\n+\tNO_WRITEV = YesPlease\n \tNEEDS_CRYPTO_WITH_SSL = YesPlease\n \tNO_LIBGEN_H = YesPlease\n \tNO_POLL = YesPlease\ndiff --git a/meson.build b/meson.build\nindex 4b536e0124..381974ab57 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1414,6 +1414,7 @@ checkfuncs = {\n 'initgroups' : [],\n 'strtoumax' : ['strtoumax.c', 'strtoimax.c'],\n 'pread' : ['pread.c'],\n+ 'writev' : ['writev.c'],\n }\n \n if host_machine.system() == 'windows'\n\n-- \n2.53.0.880.g73c4285caa.dirty\n\n"},{"id":"538426","messageId":"20260310-pks-upload-pack-write-contention-v3-6-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"[PATCH v3 06/10] wrapper: introduce writev(3p) wrappers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:25:02Z","receivedAt":"2026-03-10T13:25:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the preceding commit we have added a compatibility wrapper for the\nwritev(3p) syscall. Introduce some generic wrappers for this function\nthat we nowadays take for granted in the Git codebase.\n\nSigned-off-by: Patrick Steinhardt \n---\n wrapper.c | 41 +++++++++++++++++++++++++++++++++++++++++\n wrapper.h | 9 +++++++++\n write-or-die.c | 8 ++++++++\n write-or-die.h | 1 +\n 4 files changed, 59 insertions(+)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 16f5a63fbb..be8fa575e6 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -323,6 +323,47 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n \treturn total;\n }\n \n+ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)\n+{\n+\tssize_t total_written = 0;\n+\n+\twhile (iovcnt) {\n+\t\tssize_t bytes_written = writev(fd, iov, iovcnt);\n+\t\tif (bytes_written < 0) {\n+\t\t\tif (errno == EINTR || errno == EAGAIN)\n+\t\t\t\tcontinue;\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (!bytes_written) {\n+\t\t\terrno = ENOSPC;\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\ttotal_written += bytes_written;\n+\n+\t\t/*\n+\t\t * We first need to discard any iovec entities that have been\n+\t\t * fully written.\n+\t\t */\n+\t\twhile (iovcnt && (size_t)bytes_written >= iov->iov_len) {\n+\t\t\tbytes_written -= iov->iov_len;\n+\t\t\tiov++;\n+\t\t\tiovcnt--;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Finally, we need to adjust the last iovec in case we have\n+\t\t * performed a partial write.\n+\t\t */\n+\t\tif (iovcnt && bytes_written) {\n+\t\t\tiov->iov_base = (char *) iov->iov_base + bytes_written;\n+\t\t\tiov->iov_len -= bytes_written;\n+\t\t}\n+\t}\n+\n+\treturn total_written;\n+}\n+\n ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)\n {\n \tchar *p = buf;\ndiff --git a/wrapper.h b/wrapper.h\nindex 15ac3bab6e..27519b32d1 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -47,6 +47,15 @@ ssize_t read_in_full(int fd, void *buf, size_t count);\n ssize_t write_in_full(int fd, const void *buf, size_t count);\n ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);\n \n+/*\n+ * Try to write all iovecs. Returns -1 in case an error occurred with a proper\n+ * errno set, the number of bytes written otherwise.\n+ *\n+ * Note that the iovec will be modified as a result of this call to adjust for\n+ * partial writes!\n+ */\n+ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt);\n+\n static inline ssize_t write_str_in_full(int fd, const char *str)\n {\n \treturn write_in_full(fd, str, strlen(str));\ndiff --git a/write-or-die.c b/write-or-die.c\nindex 01a9a51fa2..5f522fb728 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -96,6 +96,14 @@ void write_or_die(int fd, const void *buf, size_t count)\n \t}\n }\n \n+void writev_or_die(int fd, struct iovec *iov, int iovlen)\n+{\n+\tif (writev_in_full(fd, iov, iovlen) < 0) {\n+\t\tcheck_pipe(errno);\n+\t\tdie_errno(\"writev error\");\n+\t}\n+}\n+\n void fwrite_or_die(FILE *f, const void *buf, size_t count)\n {\n \tif (fwrite(buf, 1, count, f) != count)\ndiff --git a/write-or-die.h b/write-or-die.h\nindex 65a5c42a47..ae3d7d88b8 100644\n--- a/write-or-die.h\n+++ b/write-or-die.h\n@@ -7,6 +7,7 @@ void fprintf_or_die(FILE *, const char *fmt, ...);\n void fwrite_or_die(FILE *f, const void *buf, size_t count);\n void fflush_or_die(FILE *f);\n void write_or_die(int fd, const void *buf, size_t count);\n+void writev_or_die(int fd, struct iovec *iov, int iovlen);\n \n /*\n * These values are used to help identify parts of a repository to fsync.\n\n-- \n2.53.0.880.g73c4285caa.dirty\n\n"},{"id":"538427","messageId":"20260310-pks-upload-pack-write-contention-v3-7-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"[PATCH v3 07/10] sideband: use writev(3p) to send pktlines","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:25:03Z","receivedAt":"2026-03-10T13:25:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Every pktline that we send out via `send_sideband()` currently requires\ntwo syscalls: one to write the pktline's length, and one to send its\ndata. This typically isn't all that much of a problem, but under extreme\nload the syscalls may cause contention in the kernel.\n\nRefactor the code to instead use the newly introduced writev(3p) infra\nso that we can send out the data with a single syscall. This reduces the\nnumber of syscalls from around 133,000 calls to write(3p) to around\n67,000 calls to writev(3p).\n\nSuggested-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\n sideband.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex ea7c25211e..1ed6614eaf 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -264,6 +264,7 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n \tconst char *p = data;\n \n \twhile (sz) {\n+\t\tstruct iovec iov[2];\n \t\tunsigned n;\n \t\tchar hdr[5];\n \n@@ -273,12 +274,19 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma\n \t\tif (0 <= band) {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 5);\n \t\t\thdr[4] = band;\n-\t\t\twrite_or_die(fd, hdr, 5);\n+\t\t\tiov[0].iov_base = hdr;\n+\t\t\tiov[0].iov_len = 5;\n \t\t} else {\n \t\t\txsnprintf(hdr, sizeof(hdr), \"%04x\", n + 4);\n-\t\t\twrite_or_die(fd, hdr, 4);\n+\t\t\tiov[0].iov_base = hdr;\n+\t\t\tiov[0].iov_len = 4;\n \t\t}\n-\t\twrite_or_die(fd, p, n);\n+\n+\t\tiov[1].iov_base = (void *) p;\n+\t\tiov[1].iov_len = n;\n+\n+\t\twritev_or_die(fd, iov, ARRAY_SIZE(iov));\n+\n \t\tp += n;\n \t\tsz -= n;\n \t}\n\n-- \n2.53.0.880.g73c4285caa.dirty\n\n"},{"id":"538428","messageId":"20260310-pks-upload-pack-write-contention-v3-8-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"[PATCH v3 08/10] csum-file: introduce `hashfd_ext()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:25:04Z","receivedAt":"2026-03-10T13:25:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Introduce a new `hashfd_ext()` function that takes an options structure.\nThis function will replace `hashd_throughput()` in the next commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n csum-file.c | 22 +++++++++++++---------\n csum-file.h | 14 ++++++++++++++\n 2 files changed, 27 insertions(+), 9 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 6e21e3cac8..a50416247e 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -161,17 +161,16 @@ struct hashfile *hashfd_check(const struct git_hash_algo *algop,\n \treturn f;\n }\n \n-static struct hashfile *hashfd_internal(const struct git_hash_algo *algop,\n-\t\t\t\t\tint fd, const char *name,\n-\t\t\t\t\tstruct progress *tp,\n-\t\t\t\t\tsize_t buffer_len)\n+struct hashfile *hashfd_ext(const struct git_hash_algo *algop,\n+\t\t\t int fd, const char *name,\n+\t\t\t const struct hashfd_options *opts)\n {\n \tstruct hashfile *f = xmalloc(sizeof(*f));\n \tf->fd = fd;\n \tf->check_fd = -1;\n \tf->offset = 0;\n \tf->total = 0;\n-\tf->tp = tp;\n+\tf->tp = opts->progress;\n \tf->name = name;\n \tf->do_crc = 0;\n \tf->skip_hash = 0;\n@@ -179,8 +178,8 @@ static struct hashfile *hashfd_internal(const struct git_hash_algo *algop,\n \tf->algop = unsafe_hash_algo(algop);\n \tf->algop->init_fn(&f->ctx);\n \n-\tf->buffer_len = buffer_len;\n-\tf->buffer = xmalloc(buffer_len);\n+\tf->buffer_len = opts->buffer_len ? opts->buffer_len : 128 * 1024;\n+\tf->buffer = xmalloc(f->buffer_len);\n \tf->check_buffer = NULL;\n \n \treturn f;\n@@ -194,7 +193,8 @@ struct hashfile *hashfd(const struct git_hash_algo *algop,\n \t * measure the rate of data passing through this hashfile,\n \t * use a larger buffer size to reduce fsync() calls.\n \t */\n-\treturn hashfd_internal(algop, fd, name, NULL, 128 * 1024);\n+\tstruct hashfd_options opts = { 0 };\n+\treturn hashfd_ext(algop, fd, name, &opts);\n }\n \n struct hashfile *hashfd_throughput(const struct git_hash_algo *algop,\n@@ -206,7 +206,11 @@ struct hashfile *hashfd_throughput(const struct git_hash_algo *algop,\n \t * size so the progress indicators arrive at a more\n \t * frequent rate.\n \t */\n-\treturn hashfd_internal(algop, fd, name, tp, 8 * 1024);\n+\tstruct hashfd_options opts = {\n+\t\t.progress = tp,\n+\t\t.buffer_len = 8 * 1024,\n+\t};\n+\treturn hashfd_ext(algop, fd, name, &opts);\n }\n \n void hashfile_checkpoint_init(struct hashfile *f,\ndiff --git a/csum-file.h b/csum-file.h\nindex 07ae11024a..a03b60120d 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -45,6 +45,20 @@ int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *);\n #define CSUM_FSYNC\t\t2\n #define CSUM_HASH_IN_STREAM\t4\n \n+struct hashfd_options {\n+\t/*\n+\t * Throughput progress that counts the number of bytes that have been\n+\t * hashed.\n+\t */\n+\tstruct progress *progress;\n+\n+\t/* The length of the buffer that shall be used read read data. */\n+\tsize_t buffer_len;\n+};\n+\n+struct hashfile *hashfd_ext(const struct git_hash_algo *algop,\n+\t\t\t int fd, const char *name,\n+\t\t\t const struct hashfd_options *opts);\n struct hashfile *hashfd(const struct git_hash_algo *algop,\n \t\t\tint fd, const char *name);\n struct hashfile *hashfd_check(const struct git_hash_algo *algop,\n\n-- \n2.53.0.880.g73c4285caa.dirty\n\n"},{"id":"538429","messageId":"20260310-pks-upload-pack-write-contention-v3-9-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"[PATCH v3 09/10] csum-file: drop `hashfd_throughput()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:25:05Z","receivedAt":"2026-03-10T13:25:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `hashfd_throughput()` function is used by a single callsite in\ngit-pack-objects(1). In contrast to `hashfd()`, this function uses a\nprogress meter to measure throughput and a smaller buffer length so that\nthe progress meter can provide more granular metrics.\n\nWe're going to change that caller in the next commit to be a bit more\nspecific to packing objects. As such, `hashfd_throughput()` will be a\nsomewhat unfitting mechanism for any potential new callers.\n\nDrop the function and replace it with a call to `hashfd_ext()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 19 +++++++++++++++----\n csum-file.c | 16 ----------------\n csum-file.h | 2 --\n 3 files changed, 15 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c1ee4d5ed7..f5cb80e870 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1330,11 +1330,22 @@ static void write_pack_file(void)\n \t\tunsigned char hash[GIT_MAX_RAWSZ];\n \t\tchar *pack_tmp_name = NULL;\n \n-\t\tif (pack_to_stdout)\n-\t\t\tf = hashfd_throughput(the_repository->hash_algo, 1,\n-\t\t\t\t\t \"\", progress_state);\n-\t\telse\n+\t\tif (pack_to_stdout) {\n+\t\t\t/*\n+\t\t\t * Since we are expecting to report progress of the\n+\t\t\t * write into this hashfile, use a smaller buffer\n+\t\t\t * size so the progress indicators arrive at a more\n+\t\t\t * frequent rate.\n+\t\t\t */\n+\t\t\tstruct hashfd_options opts = {\n+\t\t\t\t.progress = progress_state,\n+\t\t\t\t.buffer_len = 8 * 1024,\n+\t\t\t};\n+\t\t\tf = hashfd_ext(the_repository->hash_algo, 1,\n+\t\t\t\t \"\", &opts);\n+\t\t} else {\n \t\t\tf = create_tmp_packfile(the_repository, &pack_tmp_name);\n+\t\t}\n \n \t\toffset = write_pack_header(f, nr_remaining);\n \ndiff --git a/csum-file.c b/csum-file.c\nindex a50416247e..5dfaca5543 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -197,22 +197,6 @@ struct hashfile *hashfd(const struct git_hash_algo *algop,\n \treturn hashfd_ext(algop, fd, name, &opts);\n }\n \n-struct hashfile *hashfd_throughput(const struct git_hash_algo *algop,\n-\t\t\t\t int fd, const char *name, struct progress *tp)\n-{\n-\t/*\n-\t * Since we are expecting to report progress of the\n-\t * write into this hashfile, use a smaller buffer\n-\t * size so the progress indicators arrive at a more\n-\t * frequent rate.\n-\t */\n-\tstruct hashfd_options opts = {\n-\t\t.progress = tp,\n-\t\t.buffer_len = 8 * 1024,\n-\t};\n-\treturn hashfd_ext(algop, fd, name, &opts);\n-}\n-\n void hashfile_checkpoint_init(struct hashfile *f,\n \t\t\t struct hashfile_checkpoint *checkpoint)\n {\ndiff --git a/csum-file.h b/csum-file.h\nindex a03b60120d..01472555c8 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -63,8 +63,6 @@ struct hashfile *hashfd(const struct git_hash_algo *algop,\n \t\t\tint fd, const char *name);\n struct hashfile *hashfd_check(const struct git_hash_algo *algop,\n \t\t\t const char *name);\n-struct hashfile *hashfd_throughput(const struct git_hash_algo *algop,\n-\t\t\t\t int fd, const char *name, struct progress *tp);\n \n /*\n * Free the hashfile without flushing its contents to disk. This only\n\n-- \n2.53.0.880.g73c4285caa.dirty\n\n"},{"id":"538430","messageId":"20260310-pks-upload-pack-write-contention-v3-10-8bc97aa3e267@pks.im","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"[PATCH v3 10/10] builtin/pack-objects: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T13:25:06Z","receivedAt":"2026-03-10T13:25:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When running `git pack-objects --stdout` we feed the data through\n`hashfd_ext()` with a progress meter and a smaller-than-usual buffer\nlength of 8kB so that we can track throughput more granularly. But as\npackfiles tend to be on the larger side, this small buffer size may\ncause a ton of write(3p) syscalls.\n\nOriginally, the buffer we used in `hashfd()` was 8kB for all use cases.\nThis was changed though in 2ca245f8be (csum-file.h: increase hashfile\nbuffer size, 2021-05-18) because we noticed that the number of writes\ncan have an impact on performance. So the buffer size was increased to\n128kB, which improved performance a bit for some use cases.\n\nBut the commit didn't touch the buffer size for `hashd_throughput()`.\nThe reasoning here was that callers expect the progress indicator to\nupdate frequently, and a larger buffer size would of course reduce the\nupdate frequency especially on slow networks.\n\nWhile that is of course true, there was (and still is, even though it's\nnow a call to `hashfd_ext()`) only a single caller of this function in\ngit-pack-objects(1). This command is responsible for writing packfiles,\nand those packfiles are often on the bigger side. So arguably:\n\n - The user won't care about increments of 8kB when packfiles tend to\n be megabytes or even gigabytes in size.\n\n - Reducing the number of syscalls would be even more valuable here\n than it would be for multi-pack indices, which was the benchmark\n done in the mentioned commit, as MIDXs are typically significantly\n smaller than packfiles.\n\n - Nowadays, many internet connections should be able to transfer data\n at a rate significantly higher than 8kB per second.\n\nUpdate the buffer to instead have a size of `LARGE_PACKET_DATA_MAX - 1`,\nwhich translates to ~64kB. This limit was chosen because `git\npack-objects --stdout` is most often used when sending packfiles via\ngit-upload-pack(1), where packfile data is chunked into pktlines when\nusing the sideband. Furthermore, most internet connections should have a\nbandwidth signifcantly higher than 64kB/s, so we'd still be able to\nobserve progress updates at a rate of at least once per second.\n\nThis change significantly reduces the number of write(3p) syscalls from\n355,000 to 44,000 when packing the Linux repository. While this results\nin a small performance improvement on an otherwise-unused system, this\nimprovement is mostly negligible. More importantly though, it will\nreduce lock contention in the kernel on an extremely busy system where\nwe have many processes writing data at once.\n\nSuggested-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f5cb80e870..59876b024d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -41,6 +41,7 @@\n #include \"promisor-remote.h\"\n #include \"pack-mtimes.h\"\n #include \"parse-options.h\"\n+#include \"pkt-line.h\"\n #include \"blob.h\"\n #include \"tree.h\"\n #include \"path-walk.h\"\n@@ -1332,14 +1333,17 @@ static void write_pack_file(void)\n \n \t\tif (pack_to_stdout) {\n \t\t\t/*\n-\t\t\t * Since we are expecting to report progress of the\n-\t\t\t * write into this hashfile, use a smaller buffer\n-\t\t\t * size so the progress indicators arrive at a more\n-\t\t\t * frequent rate.\n+\t\t\t * This command is most often invoked via\n+\t\t\t * git-upload-pack(1), which will typically chunk data\n+\t\t\t * into pktlines. As such, we use the maximum data\n+\t\t\t * length of them as buffer length.\n+\t\t\t *\n+\t\t\t * Note that we need to subtract one though to\n+\t\t\t * accomodate for the sideband byte.\n \t\t\t */\n \t\t\tstruct hashfd_options opts = {\n \t\t\t\t.progress = progress_state,\n-\t\t\t\t.buffer_len = 8 * 1024,\n+\t\t\t\t.buffer_len = LARGE_PACKET_DATA_MAX - 1,\n \t\t\t};\n \t\t\tf = hashfd_ext(the_repository->hash_algo, 1,\n \t\t\t\t \"\", &opts);\n\n-- \n2.53.0.880.g73c4285caa.dirty\n\n"},{"id":"538475","messageId":"xmqqa4wfwrgn.fsf@gitster.g","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-5-8bc97aa3e267@pks.im","subject":"Re: [PATCH v3 05/10] compat/posix: introduce writev(3p) wrapper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-10T16:59:36Z","receivedAt":"2026-03-10T16:59:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)\n> +{\n> +\tsize_t total_written = 0;\n> +\tsize_t sum = 0;\n> +\n> +\t/*\n> +\t * According to writev(3p), the syscall shall error with EINVAL in case\n> +\t * the sum of `iov_len` overflows `ssize_t`.\n> +\t */\n> +\t for (int i = 0; i < iovcnt; i++) {\n> +\t\tif (iov[i].iov_len > maximum_signed_value_of_type(ssize_t) ||\n> +\t\t iov[i].iov_len + sum > maximum_signed_value_of_type(ssize_t)) {\n\nThis made me pause, but I think you cannot wrap-around size_t and\nend up with a small positive result that would fit in ssize_t by\nadding two quantities that are smaller than the max_ssize_t so this\ncheck should be sufficient.\n\n> +\t\t\terrno = EINVAL;\n> +\t\t\treturn -1;\n> +\t\t}\n"},{"id":"538477","messageId":"xmqq5x73wqzk.fsf@gitster.g","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-3-8bc97aa3e267@pks.im","subject":"Re: [PATCH v3 03/10] upload-pack: prefer flushing data over sending keepalive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-10T17:09:51Z","receivedAt":"2026-03-10T17:09:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When using the sideband in git-upload-pack(1) we know to send out\n> keepalive packets in case generating the pack takes too long. These\n> keepalives take the form of a simple empty pktline.\n>\n> In the preceding commit we have adapted git-upload-pack(1) to buffer\n> data more aggressively before sending it to the client. This creates an\n> obvious optimization opportunity: when we hit the keepalive timeout\n> while we still hold on to some buffered data, then it makes more sense\n> to flush out the data instead of sending the empty keepalive packet.\n>\n> This is overall not going to be a significant win. Most keepalives will\n> come before the pack data starts, and once pack-objects starts producing\n> data, it tends to do so pretty consistently. And of course we can't send\n> data before we see the PACK header, because the whole point is to buffer\n> the early bit waiting for packfile URIs. But the optimization is easy\n> enough to realize.\n>\n> Do so and flush out data instead of sending an empty pktline. While at\n> it, drop the useless\n\nUseless what?\n\n> Suggested-by: Jeff King \n> Signed-off-by: Patrick Steinhardt \n> ---\n> upload-pack.c | 21 +++++++++++++++------\n> 1 file changed, 15 insertions(+), 6 deletions(-)\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index f6f380a601..7a165d226d 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -466,18 +466,27 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n> \t\t}\n> \n> \t\t/*\n> -\t\t * We hit the keepalive timeout without saying anything; send\n> -\t\t * an empty message on the data sideband just to let the other\n> -\t\t * side know we're still working on it, but don't have any data\n> -\t\t * yet.\n> +\t\t * We hit the keepalive timeout without saying anything. If we\n> +\t\t * have pending data we flush it out to the caller now.\n> +\t\t * Otherwise, we send an empty message on the data sideband\n> +\t\t * just to let the other side know we're still working on it,\n> +\t\t * but don't have any data yet.\n> \t\t *\n> \t\t * If we don't have a sideband channel, there's no room in the\n> \t\t * protocol to say anything, so those clients are just out of\n> \t\t * luck.\n> \t\t */\n> \t\tif (!ret && pack_data->use_sideband) {\n> -\t\t\tstatic const char buf[] = \"0005\\1\";\n> -\t\t\twrite_or_die(1, buf, 5);\n> +\t\t\tif (output_state->packfile_started && output_state->used > 1) {\n> +\t\t\t\tsend_client_data(1, output_state->buffer, output_state->used - 1,\n> +\t\t\t\t\t\t pack_data->use_sideband);\n> +\t\t\t\toutput_state->buffer[0] = output_state->buffer[output_state->used - 1];\n> +\t\t\t\toutput_state->used = 1;\n> +\t\t\t} else {\n> +\t\t\t\tstatic const char buf[] = \"0005\\1\";\n> +\t\t\t\twrite_or_die(1, buf, 5);\n> +\t\t\t}\n> +\n\nOK. The new part of the comment, \"If we have pending data, flush\nit\", is what the new code is doing (i.e., flush all the bytes before\nthe final byte, and make the buffer hold only that final byte we\nkept for ourselves). Otherwise we give a keepalive packet (i.e., 0\nbyte thrown at the sideband #1) as before.\n\nOK.\n\n> \t\t\tlast_sent_ms = now_ms;\n> \t\t}\n> \t}\n"},{"id":"538478","messageId":"xmqq1phrwqwr.fsf@gitster.g","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"Re: [PATCH v3 00/10] upload-pack: reduce lock contention when writing packfile data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-10T17:11:32Z","receivedAt":"2026-03-10T17:11:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Changes in v3:\n> - Fix handling of `iov_len` overflows in writev(3p) wrapper.\n> - Add another patch that causes us to flush out data instead of\n> sending a 0005 keepalive packet.\n> - Link to v2: https://lore.kernel.org/r/20260303-pks-upload-pack-write-contention-v2-0-7321830f08fe@pks.im\n\nExcept for the tail part of the proposed log message of [PATCH 03/10]\nthat I couldn't understand, all the differences since v2 looked\nsensible to me.\n\nThanks. Will replace.\n"},{"id":"538482","messageId":"abBYK8uNkv84uydC@pks.im","threadId":"65085","inReplyTo":"xmqq5x73wqzk.fsf@gitster.g","subject":"Re: [PATCH v3 03/10] upload-pack: prefer flushing data over sending keepalive","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T17:43:07Z","receivedAt":"2026-03-10T17:43:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 10, 2026 at 10:09:51AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > When using the sideband in git-upload-pack(1) we know to send out\n> > keepalive packets in case generating the pack takes too long. These\n> > keepalives take the form of a simple empty pktline.\n> >\n> > In the preceding commit we have adapted git-upload-pack(1) to buffer\n> > data more aggressively before sending it to the client. This creates an\n> > obvious optimization opportunity: when we hit the keepalive timeout\n> > while we still hold on to some buffered data, then it makes more sense\n> > to flush out the data instead of sending the empty keepalive packet.\n> >\n> > This is overall not going to be a significant win. Most keepalives will\n> > come before the pack data starts, and once pack-objects starts producing\n> > data, it tends to do so pretty consistently. And of course we can't send\n> > data before we see the PACK header, because the whole point is to buffer\n> > the early bit waiting for packfile URIs. But the optimization is easy\n> > enough to realize.\n> >\n> > Do so and flush out data instead of sending an empty pktline. While at\n> > it, drop the useless\n> \n> Useless what?\n\nUgh. I was initially turning the allocation of `output_state` into an\non-stack variable only to later realize that we explicitly allocate it\nbecause it might otherwise blow the stack. Seems like I didn't manage to\nfully drop the sentence that mentioned this change.\n\nI'll remove this half-sentence.\n\nPatrick\n"},{"id":"538523","messageId":"2116166d-1aa2-46e5-b912-6268ebbcad4f@kdbg.org","threadId":"65085","inReplyTo":"20260310-pks-upload-pack-write-contention-v3-0-8bc97aa3e267@pks.im","subject":"Re: [PATCH v3 00/10] upload-pack: reduce lock contention when writing packfile data","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2026-03-10T20:56:14Z","receivedAt":"2026-03-10T20:56:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.03.26 um 14:24 schrieb Patrick Steinhardt:\n> compat/posix: introduce writev(3p) wrapper\n> wrapper: introduce writev(3p) wrappers\n\nI looked at these two patches.\n\nAt first, I was thrown off by the early exits when bytes_written == 0.\nBut I convinced myself that it makes sense:\n\n- In the emulation, if the underlying write(2) does return 0, it makes\nsense to signal a partial successful write to the caller of writev. It's\nthe best we can do when earlier blocks have already been written. A\nreturn value of 0 is strange and may be an indication of future\nproblems, so let's just stop the work we are doing and let the upper\nlayers deal with the problem.\n\n- In writev_in_full we need to treat the case specially in order to\nguarantee forward progress. Treating it as ENOSPC is plucked out of\nthin air, I guess, but not unreasonable.\n\nIn conclusion, these patches look good.\n\n-- Hannes\n\n"},{"id":"538556","messageId":"abELa6EfYexb06Wu@pks.im","threadId":"65085","inReplyTo":"2116166d-1aa2-46e5-b912-6268ebbcad4f@kdbg.org","subject":"Re: [PATCH v3 00/10] upload-pack: reduce lock contention when writing packfile data","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-11T06:27:55Z","receivedAt":"2026-03-11T06:28:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 10, 2026 at 09:56:14PM +0100, Johannes Sixt wrote:\n> Am 10.03.26 um 14:24 schrieb Patrick Steinhardt:\n> > compat/posix: introduce writev(3p) wrapper\n> > wrapper: introduce writev(3p) wrappers\n> \n> I looked at these two patches.\n> \n> At first, I was thrown off by the early exits when bytes_written == 0.\n> But I convinced myself that it makes sense:\n> \n> - In the emulation, if the underlying write(2) does return 0, it makes\n> sense to signal a partial successful write to the caller of writev. It's\n> the best we can do when earlier blocks have already been written. A\n> return value of 0 is strange and may be an indication of future\n> problems, so let's just stop the work we are doing and let the upper\n> layers deal with the problem.\n> \n> - In writev_in_full we need to treat the case specially in order to\n> guarantee forward progress. Treating it as ENOSPC is plucked out of\n> thin air, I guess, but not unreasonable.\n\nThis part is basically copied from `write_in_full()`, which does the\nsame.\n\n> In conclusion, these patches look good.\n\nThanks!\n\nPatrick\n"}]}