Git 2.54.0-rc1, subtests of t5310, t5326, t5327
29 messages between Apr 7, 2026 and Apr 10, 2026, from rsbecker@nexbridge.com, Jeff King, Junio C Hamano, brian m. carlson, Patrick Steinhardt, Phillip Wood, Johannes Sixt.
Plain Markdown or JSON for tools and agents.
I can getting numerous issues in t5310, t5326, t2527 relating to the following use of --git-dir:
In t5310: fatal: not a git repository: 'clone.git' not ok 55 - fetch (full bitmap) # # git --git-dir=clone.git fetch origin second:second && # git rev-parse HEAD >expect && # git --git-dir=clone.git rev-parse HEAD >actual && # test_cmp expect actual #
In t5326 and t5327: fatal: writev error: Invalid function argument fetch-pack: unexpected disconnect while reading sideband packet fatal: early EOF fatal: fetch-pack: invalid index-pack output not ok 24 - clone from bitmapped repository # # rm -fr clone.git && # git clone --no-local --bare . clone.git && # git rev-parse HEAD >expect && # git --git-dir=clone.git rev-parse HEAD >actual && # test_cmp expect actual #
It appears that the --git-dir argument is not working properly in this release. Any opinions?
I would have expected HOME or .gitconfig to have been set or redirected elsewhere in the test.
--Randall
-- Brief whoami: NonStop&UNIX developer since approximately UNIX(421664400) NonStop(211288444200000000) -- In real life, I talk too much.
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Tue, Apr 07, 2026 at 07:29:50PM -0400, rsbecker@nexbridge.com wrote:
Show 13 quoted lines
> I can getting numerous issues in t5310, t5326, t2527 relating to the
> following use of --git-dir:
>
> In t5310:
> fatal: not a git repository: 'clone.git'
> not ok 55 - fetch (full bitmap)
> #
> # git --git-dir=clone.git fetch origin second:second
> &&
> # git rev-parse HEAD >expect &&
> # git --git-dir=clone.git rev-parse HEAD >actual &&
> # test_cmp expect actual
> #
This test hasn't changed recently. The clone.git directory should have been created by an earlier test. Can you try running with "-i" and make sure that this is the first failing test, and we didn't fail earlier?
Especially because...
Show 13 quoted lines
> In t5326 and t5327:
> fatal: writev error: Invalid function argument
> fetch-pack: unexpected disconnect while reading sideband packet
> fatal: early EOF
> fatal: fetch-pack: invalid index-pack output
> not ok 24 - clone from bitmapped repository
> #
> # rm -fr clone.git &&
> # git clone --no-local --bare . clone.git &&
> # git rev-parse HEAD >expect &&
> # git --git-dir=clone.git rev-parse HEAD >actual &&
> # test_cmp expect actual
> #
...it looks less like --git-dir is a problem here, and more like the introduction of writev() is. It is now used in sideband_send(), so it seems plausible that a similar failure might have broken the git-clone operation that the other test was using to create clone.git.
As for why writev() is failing, I don't know. If it were totally broken on your system I'd expect almost everything to be failing. But maybe try building with "make NO_WRITEV=Nope" and see if that makes the problems go away? The compat implementation just does a series of write() calls, which is what send_sideband() was doing before.
-Peff
RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On April 8, 2026 12:17 AM, Jeff King wrote:
Show 45 quoted lines
>On Tue, Apr 07, 2026 at 07:29:50PM -0400, rsbecker@nexbridge.com wrote:
>
>> I can getting numerous issues in t5310, t5326, t2527 relating to the
>> following use of --git-dir:
>>
>> In t5310:
>> fatal: not a git repository: 'clone.git'
>> not ok 55 - fetch (full bitmap)
>> #
>> # git --git-dir=clone.git fetch origin second:second
>> &&
>> # git rev-parse HEAD >expect &&
>> # git --git-dir=clone.git rev-parse HEAD >actual &&
>> # test_cmp expect actual
>> #
>
>This test hasn't changed recently. The clone.git directory should have been created
>by an earlier test. Can you try running with "-i" and make sure that this is the first
>failing test, and we didn't fail earlier?
>
>Especially because...
>
>> In t5326 and t5327:
>> fatal: writev error: Invalid function argument
>> fetch-pack: unexpected disconnect while reading sideband packet
>> fatal: early EOF
>> fatal: fetch-pack: invalid index-pack output not ok 24 - clone from
>> bitmapped repository #
>> # rm -fr clone.git &&
>> # git clone --no-local --bare . clone.git &&
>> # git rev-parse HEAD >expect &&
>> # git --git-dir=clone.git rev-parse HEAD >actual &&
>> # test_cmp expect actual
>> #
>
>...it looks less like --git-dir is a problem here, and more like the introduction of
>writev() is. It is now used in sideband_send(), so it seems plausible that a similar
>failure might have broken the git-clone operation that the other test was using to
>create clone.git.
>
>As for why writev() is failing, I don't know. If it were totally broken on your system
>I'd expect almost everything to be failing. But maybe try building with "make
>NO_WRITEV=Nope" and see if that makes the problems go away? The compat
>implementation just does a series of write() calls, which is what send_sideband()
>was doing before.
First fail is as follows in subtest 25:
expecting success of 5310.25 'clone from bitmapped repository':
rm -fr clone.git &&
git clone --no-local --bare . clone.git &&
git rev-parse HEAD >expect &&
git --git-dir=clone.git rev-parse HEAD >actual &&
test_cmp expect actualCloning into bare repository 'clone.git'... remote: Enumerating objects: 629, done. fatal: writev error: Invalid function argument fetch-pack: unexpected disconnect while reading sideband packet fatal: early EOF fatal: fetch-pack: invalid index-pack output not ok 25 - clone from bitmapped repository
I think the invalid function argument maybe an ioctl or socketioctl not supported for the file type.
RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On April 8, 2026 10:54 AM, I wrote:
Show 70 quoted lines
>To: 'Jeff King' <peff@peff.net>
>Cc: git@vger.kernel.org
>Subject: RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
>
>On April 8, 2026 12:17 AM, Jeff King wrote:
>>On Tue, Apr 07, 2026 at 07:29:50PM -0400, rsbecker@nexbridge.com wrote:
>>
>>> I can getting numerous issues in t5310, t5326, t2527 relating to the
>>> following use of --git-dir:
>>>
>>> In t5310:
>>> fatal: not a git repository: 'clone.git'
>>> not ok 55 - fetch (full bitmap)
>>> #
>>> # git --git-dir=clone.git fetch origin second:second
>>> &&
>>> # git rev-parse HEAD >expect &&
>>> # git --git-dir=clone.git rev-parse HEAD >actual &&
>>> # test_cmp expect actual
>>> #
>>
>>This test hasn't changed recently. The clone.git directory should have been created
>>by an earlier test. Can you try running with "-i" and make sure that this is the first
>>failing test, and we didn't fail earlier?
>>
>>Especially because...
>>
>>> In t5326 and t5327:
>>> fatal: writev error: Invalid function argument
>>> fetch-pack: unexpected disconnect while reading sideband packet
>>> fatal: early EOF
>>> fatal: fetch-pack: invalid index-pack output not ok 24 - clone from
>>> bitmapped repository #
>>> # rm -fr clone.git &&
>>> # git clone --no-local --bare . clone.git &&
>>> # git rev-parse HEAD >expect &&
>>> # git --git-dir=clone.git rev-parse HEAD >actual &&
>>> # test_cmp expect actual
>>> #
>>
>>...it looks less like --git-dir is a problem here, and more like the introduction of
>>writev() is. It is now used in sideband_send(), so it seems plausible that a similar
>>failure might have broken the git-clone operation that the other test was using to
>>create clone.git.
>>
>>As for why writev() is failing, I don't know. If it were totally broken on your system
>>I'd expect almost everything to be failing. But maybe try building with "make
>>NO_WRITEV=Nope" and see if that makes the problems go away? The compat
>>implementation just does a series of write() calls, which is what send_sideband()
>>was doing before.
>
>First fail is as follows in subtest 25:
>
>expecting success of 5310.25 'clone from bitmapped repository':
> rm -fr clone.git &&
> git clone --no-local --bare . clone.git &&
> git rev-parse HEAD >expect &&
> git --git-dir=clone.git rev-parse HEAD >actual &&
> test_cmp expect actual
>
>Cloning into bare repository 'clone.git'...
>remote: Enumerating objects: 629, done.
>fatal: writev error: Invalid function argument
>fetch-pack: unexpected disconnect while reading sideband packet
>fatal: early EOF
>fatal: fetch-pack: invalid index-pack output
>not ok 25 - clone from bitmapped repository
>
>I think the invalid function argument maybe an ioctl or socketioctl not supported for
>the file type.
This is also impacting t5608 and t7700. Anywhere where writev() is used, seemingly. We went through MAX_IO_SIZE issues years ago, instead of using ssize_t as a basis of how big communication is. I think writev() is not valid. It worked on Lunix, but had issues elsewhere. This broke the compat layer.
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Wed, Apr 08, 2026 at 10:54:00AM -0400, rsbecker@nexbridge.com wrote:
Show 16 quoted lines
> First fail is as follows in subtest 25:
>
> expecting success of 5310.25 'clone from bitmapped repository':
> rm -fr clone.git &&
> git clone --no-local --bare . clone.git &&
> git rev-parse HEAD >expect &&
> git --git-dir=clone.git rev-parse HEAD >actual &&
> test_cmp expect actual
>
> Cloning into bare repository 'clone.git'...
> remote: Enumerating objects: 629, done.
> fatal: writev error: Invalid function argument
> fetch-pack: unexpected disconnect while reading sideband packet
> fatal: early EOF
> fatal: fetch-pack: invalid index-pack output
> not ok 25 - clone from bitmapped repository
OK, good. Well, not good, but at least absolves --git-dir, and we know the problem is just writev() everywhere.
> I think the invalid function argument maybe an ioctl or socketioctl
> not supported for the file type.
Yeah, this is weird. We are just calling writev() here, and not trying to do anything exotic. I could believe that writev() isn't supported on certain descriptor types or something, but this should just be a pipe. But why does it consistently fail here, but not in every clone? Weird.
It might be interesting to use strace or a debugger (or just printf from writev_or_die()) to see if there's something interesting about the arguments here. But finding the root cause may not actually be that helpful (or at least not worth the effort).
Does building with NO_WRITEV make the problem go away?
-Peff
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Wed, Apr 08, 2026 at 12:25:47PM -0400, rsbecker@nexbridge.com wrote:
Show 5 quoted lines
> This is also impacting t5608 and t7700. Anywhere where writev() is
> used, seemingly. We went through MAX_IO_SIZE issues years ago, instead
> of using ssize_t as a basis of how big communication is. I think
> writev() is not valid. It worked on Lunix, but had issues elsewhere.
> This broke the compat layer.
I wondered briefly if the problem could be that we're violating MAX_IO_SIZE here, as our use of writev() does not respect it at all. But the only spot that uses it is feeding pkt-line packets, which max out at 64k. So unless your MAX_IO_SIZE is smaller than that, I doubt that is the problem.
-Peff
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> On Wed, Apr 08, 2026 at 12:25:47PM -0400, rsbecker@nexbridge.com wrote:
>
>> This is also impacting t5608 and t7700. Anywhere where writev() is
>> used, seemingly. We went through MAX_IO_SIZE issues years ago, instead
>> of using ssize_t as a basis of how big communication is. I think
>> writev() is not valid. It worked on Lunix, but had issues elsewhere.
>> This broke the compat layer.
>
> I wondered briefly if the problem could be that we're violating
> MAX_IO_SIZE here, as our use of writev() does not respect it at all. But
> the only spot that uses it is feeding pkt-line packets, which max out at
> 64k. So unless your MAX_IO_SIZE is smaller than that, I doubt that is
> the problem.
Good point. The original did not use write(2) directly but used write_or_die(), that is write_in_full(), that loops over xwrite(), so it would have worked even with a lot lower MAX_IO_SIZE limit.
According to man7.org, writev() is allowed to transfer fewer bytes than requested, so our use of writev() may have to be a bit more careful, though.
RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On April 8, 2026 1:40 PM, Jeff King wrote:
Show 12 quoted lines
>On Wed, Apr 08, 2026 at 12:25:47PM -0400, rsbecker@nexbridge.com wrote:
>
>> This is also impacting t5608 and t7700. Anywhere where writev() is
>> used, seemingly. We went through MAX_IO_SIZE issues years ago, instead
>> of using ssize_t as a basis of how big communication is. I think
>> writev() is not valid. It worked on Lunix, but had issues elsewhere.
>> This broke the compat layer.
>
>I wondered briefly if the problem could be that we're violating MAX_IO_SIZE here,
>as our use of writev() does not respect it at all. But the only spot that uses it is
>feeding pkt-line packets, which max out at 64k. So unless your MAX_IO_SIZE is
>smaller than that, I doubt that is the problem.
SSIZE_MAX on platform is 53248, so yes. We expected git-compat-util.h at line 696 to be honoured.
RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On April 8, 2026 2:13 PM, Junio C Hamano wrote:
Show 18 quoted lines
>Jeff King <peff@peff.net> writes:
>
>> On Wed, Apr 08, 2026 at 12:25:47PM -0400, rsbecker@nexbridge.com wrote:
>>
>>> This is also impacting t5608 and t7700. Anywhere where writev() is
>>> used, seemingly. We went through MAX_IO_SIZE issues years ago,
>>> instead of using ssize_t as a basis of how big communication is. I
>>> think
>>> writev() is not valid. It worked on Lunix, but had issues elsewhere.
>>> This broke the compat layer.
>>
>> I wondered briefly if the problem could be that we're violating
>> MAX_IO_SIZE here, as our use of writev() does not respect it at all.
>> But the only spot that uses it is feeding pkt-line packets, which max
>> out at 64k. So unless your MAX_IO_SIZE is smaller than that, I doubt
>> that is the problem.
>
>Good point. The original did not use write(2) directly but used
>is write_in_full(), that loops over xwrite(), so it would have worked even
>lower MAX_IO_SIZE limit.
>
>According to man7.org, writev() is allowed to transfer fewer bytes than
>so our use of writev() may have to be a bit more careful, though.
On my box, I have the following note:
Specifying the sum of the iov_len values in the iov array greater than the OSS I/O size limit for that open causes the writev() function to return -1 and set errno to [EINVAL].
This can be either 52K (SSIZE_MAX) or 868K depending on the build settings. We are currently stuck with 32-bit builds, with large file support, so really no limit on the file sizes, but due to dependencies that are stuck at 32-bit until I convince the powers that be to fix those. Please, can we be compliant with MAX_IO_SIZE?
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
<rsbecker@nexbridge.com> writes:
Show 5 quoted lines
> On my box, I have the following note:
>
> Specifying the sum of the iov_len values in the iov array greater than
> the OSS I/O size limit for that open causes the writev() function to
> return -1 and set errno to [EINVAL].
That is unexpected.
writev() may fail if the sum of iov_len would not fit within ssize_t with EINVAL, but unless your "the OSS I/O size limit" is the same as SSIZE_MAX, what you have above is not quite the same.
Does your build work with NO_WRITEV=Nope? I think I saw it asked a few times but I do not recall seeing it answered. At least we know xwrite() seems to work well enough on your system, which is what the writev() emulation is written in terms of, so I suspect it would.
RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On April 8, 2026 4:22 PM, Junio C Hamano wrote:
Show 11 quoted lines
><rsbecker@nexbridge.com> writes:
>
>> On my box, I have the following note:
>>
>> Specifying the sum of the iov_len values in the iov array greater
>> than the OSS I/O size limit for that open causes the writev()
>> function to return -1 and set errno to [EINVAL].
>
>That is unexpected.
>
>writev() may fail if the sum of iov_len would not fit within ssize_t with
>unless your "the OSS I/O size limit" is the same as SSIZE_MAX, what you
>is not quite the same.
>
>Does your build work with NO_WRITEV=Nope? I think I saw it asked a few
>but I do not recall seeing it answered. At least we know
>xwrite() seems to work well enough on your system, which is what the
>writev() emulation is written in terms of, so I suspect it would.
Yes, NO_WRITEV=Nope does compile and execute. I am including it in our CI/CD job for now. Can we plan on a fix for this?
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
<rsbecker@nexbridge.com> writes:
Show 25 quoted lines
> On April 8, 2026 4:22 PM, Junio C Hamano wrote:
>><rsbecker@nexbridge.com> writes:
>>
>>> On my box, I have the following note:
>>>
>>> Specifying the sum of the iov_len values in the iov array greater
>>> than the OSS I/O size limit for that open causes the writev()
>>> function to return -1 and set errno to [EINVAL].
>>
>>That is unexpected.
>>
>>writev() may fail if the sum of iov_len would not fit within ssize_t with
> EINVAL, but
>>unless your "the OSS I/O size limit" is the same as SSIZE_MAX, what you
> have above
>>is not quite the same.
>>
>>Does your build work with NO_WRITEV=Nope? I think I saw it asked a few
> times
>>but I do not recall seeing it answered. At least we know
>>xwrite() seems to work well enough on your system, which is what the
>>writev() emulation is written in terms of, so I suspect it would.
>
> Yes, NO_WRITEV=Nope does compile and execute. I am including it
> in our CI/CD job for now. Can we plan on a fix for this?
What I have heard so far indicate that the code that uses writev() would need to loop over to prepare for short writes, but your writev() that fails for "the OSS I/O size limit" (whatever it is) does not sound like something we want to change the callers to chomp the writev() calls into smaller chunks for. Such a platform is far better off using the compat/writev for the code path we recently started using writev() in.
To be quite honest, I am not sure if it is even worth using writev() if we need a loop that protects against shrot writes, so unless I am grossly mistaken (e.g., perhaps there is some guarantee that there won't be any short writes for writev() that sends data smaller than 64k that I missed in the docs), the best course of action might be to revert the change to use writev() and use the two write(2)s as before, *if* we actually observe that the current code is broken by short writes.
RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On April 8, 2026 5:43 PM, Junio C Hamano wrote:
Show 32 quoted lines
><rsbecker@nexbridge.com> writes:
>
>> On April 8, 2026 4:22 PM, Junio C Hamano wrote:
>>><rsbecker@nexbridge.com> writes:
>>>
>>>> On my box, I have the following note:
>>>>
>>>> Specifying the sum of the iov_len values in the iov array greater
>>>> than the OSS I/O size limit for that open causes the writev()
>>>> function to return -1 and set errno to [EINVAL].
>>>
>>>That is unexpected.
>>>
>>>writev() may fail if the sum of iov_len would not fit within ssize_t
>>>with
>> EINVAL, but
>>>unless your "the OSS I/O size limit" is the same as SSIZE_MAX, what
>>>you
>> have above
>>>is not quite the same.
>>>
>>>Does your build work with NO_WRITEV=Nope? I think I saw it asked a
>>>few
>> times
>>>but I do not recall seeing it answered. At least we know
>>>xwrite() seems to work well enough on your system, which is what the
>>>writev() emulation is written in terms of, so I suspect it would.
>>
>> Yes, NO_WRITEV=Nope does compile and execute. I am including it in our
>> CI/CD job for now. Can we plan on a fix for this?
>
>What I have heard so far indicate that the code that uses writev() would
>loop over to prepare for short writes, but your
>writev() that fails for "the OSS I/O size limit" (whatever it is) does not
>something we want to change the callers to chomp the writev() calls into
>chunks for. Such a platform is far better off using the compat/writev for
>path we recently started using writev() in.
>
>To be quite honest, I am not sure if it is even worth using writev() if we
>that protects against shrot writes, so unless I am grossly mistaken (e.g.,
>there is some guarantee that there won't be any short writes for writev()
>data smaller than 64k that I missed in the docs), the best course of action
>to revert the change to use writev() and use the two write(2)s as before,
>actually observe that the current code is broken by short writes.
I am 100% sure that EINVAL is returned by writev() on NonStop if the size exceeds 52K In 32-bit models. Whether it supports 868K for 64-bit is conjecture. NO_WRITEV=Nope works, which I am trying for everything at RC1, then we can live with this.
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Wed, Apr 08, 2026 at 02:36:27PM -0400, rsbecker@nexbridge.com wrote:
Show 16 quoted lines
> On April 8, 2026 1:40 PM, Jeff King wrote:
> >On Wed, Apr 08, 2026 at 12:25:47PM -0400, rsbecker@nexbridge.com wrote:
> >
> >> This is also impacting t5608 and t7700. Anywhere where writev() is
> >> used, seemingly. We went through MAX_IO_SIZE issues years ago, instead
> >> of using ssize_t as a basis of how big communication is. I think
> >> writev() is not valid. It worked on Lunix, but had issues elsewhere.
> >> This broke the compat layer.
> >
> >I wondered briefly if the problem could be that we're violating MAX_IO_SIZE here,
> >as our use of writev() does not respect it at all. But the only spot that uses it is
> >feeding pkt-line packets, which max out at 64k. So unless your MAX_IO_SIZE is
> >smaller than that, I doubt that is the problem.
>
> SSIZE_MAX on platform is 53248, so yes. We expected git-compat-util.h at line
> 696 to be honoured.
Oof, that is small. So yeah, that is almost certainly the problem (and explains why it only happens for some writes in the test suite).
-Peff
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> To be quite honest, I am not sure if it is even worth using writev()
> if we need a loop that protects against shrot writes, so unless I am
> grossly mistaken (e.g., perhaps there is some guarantee that there
> won't be any short writes for writev() that sends data smaller than
> 64k that I missed in the docs), the best course of action might be
> to revert the change to use writev() and use the two write(2)s as
> before, *if* we actually observe that the current code is broken by
> short writes.
Ah, sorry, I should have double checked the actual code. We already use a looping writev_in_full() that wraps writev(), so there is nothing extra that we still need to do to prepare for short writes.
Unfortunately, comparing write_in_full() vs writev_in_full(), there is nothing that corresponds to xwrite() that can be used to hide the short writes and chomps an originally larger I/O into smaller pieces. Unlike write() that we may receive a single linear large sequence of bytes, which we can choose to chomp into artificially smaller pieces and write them out (up to 8MB by default), writev() API lets the caller to prepare chunks of memory and I do not think there is a good way for the writev_in_full() at the lower layer to chomp these into smaller pieces, and even if we could, that would defeat the whole reason why we rewrote the original code that used write_in_full() into using writev(), i.e., to avoid extra allocation (and extra system calls---but if your I/O layer is limited to very small writes, no matter how we chop it, you will have to issue extra system calls to flush all of the data out).
So, I dunno.
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Wed, Apr 08, 2026 at 02:43:20PM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> > Yes, NO_WRITEV=Nope does compile and execute. I am including it
> > in our CI/CD job for now. Can we plan on a fix for this?
>
> What I have heard so far indicate that the code that uses writev()
> would need to loop over to prepare for short writes, but your
> writev() that fails for "the OSS I/O size limit" (whatever it is)
> does not sound like something we want to change the callers to chomp
> the writev() calls into smaller chunks for. Such a platform is far
> better off using the compat/writev for the code path we recently
> started using writev() in.
Yeah, we definitely do not want callers to worry about this. I think it would be possible to have xwritev() handle this, but it gets ugly. If we are worried about an individual iovec being larger than MAX_IO_SIZE, then we may need to rewrite the iovec array to break it apart. And if we are worried about the sum of the pieces being larger than MAX_IO_SIZE, then we end up with multiple writev() calls anyway.
At which point the least-painful thing is probably just calling write() on each segment anyway, which is exactly what the compat wrapper does.
Show 8 quoted lines
> To be quite honest, I am not sure if it is even worth using writev()
> if we need a loop that protects against shrot writes, so unless I am
> grossly mistaken (e.g., perhaps there is some guarantee that there
> won't be any short writes for writev() that sends data smaller than
> 64k that I missed in the docs), the best course of action might be
> to revert the change to use writev() and use the two write(2)s as
> before, *if* we actually observe that the current code is broken by
> short writes.
I think writev() is buying us something when it works (it is halving the number of writes for sideband packets). And it works when either:
1. the platform is OK with writing up to 64k in a single writev()
2. the platform has a limit that is small (like NonStop here), but
writes less than MAX_IO_SIZE work and will save a write() callIf we just care about (1), then the right solution is to declare that writev() isn't fully functional for us on some platforms, and they should build with NO_WRITEV. And we should probably embed that in config.mak.uname.
If we want to care about (2), then we could make the decision at runtime, something like:
diff --git a/compat/writev.c b/compat/writev.c
index 3a94870a2f..cf2fbb39c9 100644
--- a/compat/writev.c
+++ b/compat/writev.c
@@ -1,7 +1,7 @@
#include "../git-compat-util.h"
#include "../wrapper.h"
-ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)
+ssize_t git_writev_with_write(int fd, const struct iovec *iov, int iovcnt)
{
size_t total_written = 0;
size_t sum = 0;
@@ -42,3 +42,17 @@ ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)
out:
return (ssize_t) total_written;
}
+
+ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)
+{
+ size_t total = 0;
+ for (int i = 0; i < iovcnt; i++)
+ total += iov[i].iov_len;
+ if (total > MAX_IO_SIZE) {
+ /* too big; bail to wrapper which will limit individual writes */
+ return git_writev_with_write(fd, iov, iovcnt);
+ }
+
+ /* otherwise, use the real system writev */
+ return writev(fd, iov, iovcnt);
+}
I'm not sure that complexity is worth it, though. If your write-limit is
less than 64k you are already doing lots of extra write() calls, and
trying to squeeze out a tiny bit of performance by omitting a few of
them is not worth the trouble.
Though it does make things Just Work on such platforms without having to
set another build-time knob.
I do wonder what it all means for systems with MAX_IO_SIZE that is more
reasonable. Right now we are using writev() only for sideband packets.
But one of the stated reasons for using it there (instead of tweaking
the packet buffer so that there's room for the header in it) is that
people wanted to be able to start using writev() elsewhere. If we start
feeding unbounded data to it (say, a blob buffer), then we're going to
be violating MAX_IO_SIZE for those calls. So there may be more of this
headache down the road.
-Peff
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> To be quite honest, I am not sure if it is even worth using writev()
>> if we need a loop that protects against shrot writes, so unless I am
>> grossly mistaken (e.g., perhaps there is some guarantee that there
>> won't be any short writes for writev() that sends data smaller than
>> 64k that I missed in the docs), the best course of action might be
>> to revert the change to use writev() and use the two write(2)s as
>> before, *if* we actually observe that the current code is broken by
>> short writes.
>
> Ah, sorry, I should have double checked the actual code. We already
> use a looping writev_in_full() that wraps writev(), so there is
> nothing extra that we still need to do to prepare for short writes.
>
> Unfortunately, comparing write_in_full() vs writev_in_full(), there
> is nothing that corresponds to xwrite() that can be used to hide the
> short writes and chomps an originally larger I/O into smaller
> pieces.
Oops, the beauty of having xwrite() is *not* that it hides short writes (it doesn't), but it can be used to pretend that short writes happened on platforms with unreasonably small I/O limit by setting MAX_IO_SIZE to unusually low. But the point that ...
Show 13 quoted lines
> Unlike write() that we may receive a single linear large
> sequence of bytes, which we can choose to chomp into artificially
> smaller pieces and write them out (up to 8MB by default), writev()
> API lets the caller to prepare chunks of memory and I do not think
> there is a good way for the writev_in_full() at the lower layer to
> chomp these into smaller pieces, and even if we could, that would
> defeat the whole reason why we rewrote the original code that used
> write_in_full() into using writev(), i.e., to avoid extra allocation
> (and extra system calls---but if your I/O layer is limited to very
> small writes, no matter how we chop it, you will have to issue extra
> system calls to flush all of the data out).
>
> So, I dunno.
... I doubt that there exists a good way to have xwritev() that wraps around writev() and pretend that a short write happened, instead of issuing a large I/O, still stands.
RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On April 8, 2026 6:35 PM, Junio C Hamano wrote
Show 22 quoted lines
>Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> To be quite honest, I am not sure if it is even worth using writev()
>>> if we need a loop that protects against shrot writes, so unless I am
>>> grossly mistaken (e.g., perhaps there is some guarantee that there
>>> won't be any short writes for writev() that sends data smaller than
>>> 64k that I missed in the docs), the best course of action might be to
>>> revert the change to use writev() and use the two write(2)s as
>>> before, *if* we actually observe that the current code is broken by
>>> short writes.
>>
>> Ah, sorry, I should have double checked the actual code. We already
>> use a looping writev_in_full() that wraps writev(), so there is
>> nothing extra that we still need to do to prepare for short writes.
>>
>> Unfortunately, comparing write_in_full() vs writev_in_full(), there is
>> nothing that corresponds to xwrite() that can be used to hide the
>> short writes and chomps an originally larger I/O into smaller pieces.
>
>Oops, the beauty of having xwrite() is *not* that it hides short writes (it
>but it can be used to pretend that short writes happened on platforms with
>unreasonably small I/O limit by setting MAX_IO_SIZE to unusually low. But
Show 17 quoted lines
>point that ...
>
>> Unlike write() that we may receive a single linear large sequence of
>> bytes, which we can choose to chomp into artificially smaller pieces
>> and write them out (up to 8MB by default), writev() API lets the
>> caller to prepare chunks of memory and I do not think there is a good
>> way for the writev_in_full() at the lower layer to chomp these into
>> smaller pieces, and even if we could, that would defeat the whole
>> reason why we rewrote the original code that used
>> write_in_full() into using writev(), i.e., to avoid extra allocation
>> (and extra system calls---but if your I/O layer is limited to very
>> small writes, no matter how we chop it, you will have to issue extra
>> system calls to flush all of the data out).
>>
>> So, I dunno.
>
>... I doubt that there exists a good way to have xwritev() that wraps
>and pretend that a short write happened, instead of issuing a large I/O,
still stands.
I am partial to Peff's runtime approach, personally.
If it helps (probably does not), the limit is due to the DMA/VMM limits on the hardware. The NonStop Message system is blindingly fast when sending messages around between processes on other CPUs - far faster than I have seen on most other platforms. But there are physical limits in the chipset. It is interesting to use it in an abstracted way, like hacking the TCP/IP stack to pass handles around.
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On 2026-04-08 at 22:32:33, Jeff King wrote:
Show 12 quoted lines
> I think writev() is buying us something when it works (it is hlving the
> number of writes for sideband packets). And it works when either:
>
> 1. the platform is OK with writing up to 64k in a single writev()
>
> 2. the platform has a limit that is small (like NonStop here), but
> writes less than MAX_IO_SIZE work and will save a write() call
>
> If we just care about (1), then the right solution is to declare that
> writev() isn't fully functional for us on some platforms, and they
> should build with NO_WRITEV. And we should probably embed that in
> config.mak.uname.
Looking at POSIX, there doesn't seem to be any constraints on the size of individual vectors other than that they must total to less than SSIZE_MAX. iovcnt can be limited to 16, but I don't think we're hitting that here. POSIX does say that SSIZE_MAX does not need to exceed 32767, which may be what's going on here, although that does seem like an unreasonable value for a real system. Linux, FreeBSD, and NetBSD all set SSIZE_MAX to either INT_MAX or LONG_MAX.
I also think that 64 KiB is more than reasonable in terms of the size that people should be able to send. I'd personally expect to be able to send values much larger, at least 512 KiB, and I have code that expects even larger (16 MiB).
So I'd simply say that for systems that have a constraint on the size that is "too small", they should just use NO_WRITEV.
However, I don't have a strong opinion on this and if people want to do the proposal for option 2, that's fine with me.
I will say that we may find ourselves in a pickle with Rust code in the future if we use `write_vectored` since that will probably just use the OS implementation, but we can worry about that when we get there.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Thu, Apr 09, 2026 at 12:20:26AM +0000, brian m. carlson wrote:
Show 13 quoted lines
> On 2026-04-08 at 22:32:33, Jeff King wrote:
> > I think writev() is buying us something when it works (it is hlving the
> > number of writes for sideband packets). And it works when either:
> >
> > 1. the platform is OK with writing up to 64k in a single writev()
> >
> > 2. the platform has a limit that is small (like NonStop here), but
> > writes less than MAX_IO_SIZE work and will save a write() call
> >
> > If we just care about (1), then the right solution is to declare that
> > writev() isn't fully functional for us on some platforms, and they
> > should build with NO_WRITEV. And we should probably embed that in
> > config.mak.uname.
Yeah, agreed. I think we shouldn't make ourselves a hostage to platforms that don't have reasonable support for writev(3p), as it does buy us something on the majority of platforms that actually support it well.
That of course doesn't mean that we shouldn't support such platforms.
Show 15 quoted lines
> Looking at POSIX, there doesn't seem to be any constraints on the size
> of individual vectors other than that they must total to less than
> SSIZE_MAX. iovcnt can be limited to 16, but I don't think we're hitting
> that here. POSIX does say that SSIZE_MAX does not need to exceed 32767,
> which may be what's going on here, although that does seem like an
> unreasonable value for a real system. Linux, FreeBSD, and NetBSD all
> set SSIZE_MAX to either INT_MAX or LONG_MAX.
>
> I also think that 64 KiB is more than reasonable in terms of the size
> that people should be able to send. I'd personally expect to be able to
> send values much larger, at least 512 KiB, and I have code that expects
> even larger (16 MiB).
>
> So I'd simply say that for systems that have a constraint on the size
> that is "too small", they should just use NO_WRITEV.
I would be happy with this as an intermediate step, as Randall has confirmed it would fix the issue. It is the least intrusive step and has the lowest risk.
> However, I don't have a strong opinion on this and if people want to do
> the proposal for option 2, that's fine with me.
I think in the long term this is the most sensible approach though so that we don't have to special-case platforms. I've crafted the below alternative to Peff's patch, and I think it's ultimately not too bad.
One question to Randall though: does MAX_IO_SIZE apply to the overall size of the iovec or to the individual iovec entries? I think it should be the latter, but I cannot easily verify and couldn't find any docs around this. So could you please try the patch at the end of this mail to verify that it works on your system?
In any case, I've tested that my patch also works when defining MAX_IO_SIZE to 128 bytes on my system, which hopefully demonstrates that it works as expected:
diff --git a/git-compat-util.h b/git-compat-util.h
index 4b4ea2498f..8e02b5f673 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -690,14 +690,8 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
* to override this, if the definition of SSIZE_MAX given by the platform
* is broken.
*/
-#ifndef MAX_IO_SIZE
-# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
-# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
-# define MAX_IO_SIZE SSIZE_MAX
-# else
-# define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
-# endif
-#endif
+#undef MAX_IO_SIZE
+#define MAX_IO_SIZE 128
#ifdef HAVE_ALLOCA_H
# include <alloca.h>
I'm happy to go either way, but think that we should definitely aim for
the below patch eventually. Just let me know which way you prefer and
I'm happy to polish up the patch.
Patrick
diff --git a/wrapper.c b/wrapper.c
index be8fa575e6..645dbc5f20 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -323,21 +323,50 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
return total;
}
+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt)
+{
+ ssize_t bytes_written;
+ int i;
+
+ /*
+ * We need to make sure that no individual iovec entry exceeds
+ * `MAX_IO_SIZE`. If there's any entry that does exceed this limit
+ * we'll pass all entries up to it to `writev()`, and then process the
+ * exceeding entry via a call to `xwrite()`.
+ */
+ for (i = 0; i < iovcnt; i++)
+ if (iov[i].iov_len > MAX_IO_SIZE)
+ break;
+ if (i < iovcnt) {
+ /*
+ * The first entry exceeds MAX_IO_SIZE, so we pass it to
+ * xwrite, which knows to handle his case.
+ */
+ if (!i)
+ return xwrite(fd, iov->iov_base, iov->iov_len);
+ iovcnt = i;
+ }
+
+ bytes_written = writev(fd, iov, iovcnt);
+ if (!bytes_written) {
+ errno = ENOSPC;
+ return -1;
+ }
+
+ return bytes_written;
+}
+
ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)
{
ssize_t total_written = 0;
while (iovcnt) {
- ssize_t bytes_written = writev(fd, iov, iovcnt);
- if (bytes_written < 0) {
+ ssize_t bytes_written = xwritev(fd, iov, iovcnt);
+ if (bytes_written <= 0) {
if (errno == EINTR || errno == EAGAIN)
continue;
return -1;
}
- if (!bytes_written) {
- errno = ENOSPC;
- return -1;
- }
total_written += bytes_written;
diff --git a/wrapper.h b/wrapper.h
index 27519b32d1..a6287d7f4d 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_
int xopen(const char *path, int flags, ...);
ssize_t xread(int fd, void *buf, size_t len);
ssize_t xwrite(int fd, const void *buf, size_t len);
+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);
ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
int xdup(int fd);
FILE *xfopen(const char *path, const char *mode);
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On 09/04/2026 09:17, Patrick Steinhardt wrote:
>
> One question to Randall though: does MAX_IO_SIZE apply to the overall
> size of the iovec or to the individual iovec entries?
In <014e01dcc793$8a9bab90$9fd302b0$@nexbridge.com> Randall says
Specifying the sum of the iov_len values in the iov array greater
than the OSS I/O size limit for that open causes the writev()
function to return -1 and set errno to [EINVAL].So it is the overall size which fits with POSIX limiting to overall size to SSIZE_MAX.
Thanks
Phillip
Show 111 quoted lines
> I think it should
> be the latter, but I cannot easily verify and couldn't find any docs
> around this. So could you please try the patch at the end of this mail
> to verify that it works on your system?
>
> In any case, I've tested that my patch also works when defining
> MAX_IO_SIZE to 128 bytes on my system, which hopefully demonstrates that
> it works as expected:
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 4b4ea2498f..8e02b5f673 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -690,14 +690,8 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
> * to override this, if the definition of SSIZE_MAX given by the platform
> * is broken.
> */
> -#ifndef MAX_IO_SIZE
> -# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
> -# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
> -# define MAX_IO_SIZE SSIZE_MAX
> -# else
> -# define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
> -# endif
> -#endif
> +#undef MAX_IO_SIZE
> +#define MAX_IO_SIZE 128
>
> #ifdef HAVE_ALLOCA_H
> # include <alloca.h>
>
> I'm happy to go either way, but think that we should definitely aim for
> the below patch eventually. Just let me know which way you prefer and
> I'm happy to polish up the patch.
>
> Patrick
>
> diff --git a/wrapper.c b/wrapper.c
> index be8fa575e6..645dbc5f20 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -323,21 +323,50 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
> return total;
> }
>
> +ssize_t xwritev(int fd, struct iovec *iov, int iovcnt)
> +{
> + ssize_t bytes_written;
> + int i;
> +
> + /*
> + * We need to make sure that no individual iovec entry exceeds
> + * `MAX_IO_SIZE`. If there's any entry that does exceed this limit
> + * we'll pass all entries up to it to `writev()`, and then process the
> + * exceeding entry via a call to `xwrite()`.
> + */
> + for (i = 0; i < iovcnt; i++)
> + if (iov[i].iov_len > MAX_IO_SIZE)
> + break;
> + if (i < iovcnt) {
> + /*
> + * The first entry exceeds MAX_IO_SIZE, so we pass it to
> + * xwrite, which knows to handle his case.
> + */
> + if (!i)
> + return xwrite(fd, iov->iov_base, iov->iov_len);
> + iovcnt = i;
> + }
> +
> + bytes_written = writev(fd, iov, iovcnt);
> + if (!bytes_written) {
> + errno = ENOSPC;
> + return -1;
> + }
> +
> + return bytes_written;
> +}
> +
> ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)
> {
> ssize_t total_written = 0;
>
> while (iovcnt) {
> - ssize_t bytes_written = writev(fd, iov, iovcnt);
> - if (bytes_written < 0) {
> + ssize_t bytes_written = xwritev(fd, iov, iovcnt);
> + if (bytes_written <= 0) {
> if (errno == EINTR || errno == EAGAIN)
> continue;
> return -1;
> }
> - if (!bytes_written) {
> - errno = ENOSPC;
> - return -1;
> - }
>
> total_written += bytes_written;
>
> diff --git a/wrapper.h b/wrapper.h
> index 27519b32d1..a6287d7f4d 100644
> --- a/wrapper.h
> +++ b/wrapper.h
> @@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_
> int xopen(const char *path, int flags, ...);
> ssize_t xread(int fd, void *buf, size_t len);
> ssize_t xwrite(int fd, const void *buf, size_t len);
> +ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);
> ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
> int xdup(int fd);
> FILE *xfopen(const char *path, const char *mode);
> Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Thu, Apr 09, 2026 at 10:48:02AM +0100, Phillip Wood wrote:
Show 13 quoted lines
> On 09/04/2026 09:17, Patrick Steinhardt wrote:
> >
> > One question to Randall though: does MAX_IO_SIZE apply to the overall
> > size of the iovec or to the individual iovec entries?
>
> In <014e01dcc793$8a9bab90$9fd302b0$@nexbridge.com> Randall says
>
> Specifying the sum of the iov_len values in the iov array greater
> than the OSS I/O size limit for that open causes the writev()
> function to return -1 and set errno to [EINVAL].
>
> So it is the overall size which fits with POSIX limiting to overall size to
> SSIZE_MAX.
Ah, thanks for the pointer. I've adapted the patch a bit to the below one. Again, I've tested it with `#define MAX_IO_SIZE 100` to verify that it works as expected.
I guess I'll send a polished version to the mailing list in a bit.
Patrick
diff --git a/wrapper.c b/wrapper.c
index be8fa575e6..d989c78b4b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -323,21 +323,60 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
return total;
}
+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt)
+{
+ ssize_t bytes_written;
+ size_t total_length;
+ int i;
+
+ /*
+ * We need to make sure that writev(3p) call does not write more than
+ * `MAX_IO_SIZE` many bytes. If we do exceed that limit, we only pass
+ * those iovecs to writev(3p) that sum up to less than the limit.
+ *
+ * If on the other hand the first iovec entry already exceeds this
+ * limit we'll instead use xwrite() to write it, which knows to handle
+ * `MAX_IO_SIZE` for us.
+ */
+ for (i = 0, total_length = 0; i < iovcnt; i++) {
+ if (unsigned_add_overflows(total_length, iov[i].iov_len))
+ break;
+
+ total_length += iov[i].iov_len;
+ if (total_length > MAX_IO_SIZE)
+ break;
+ }
+
+ if (i < iovcnt) {
+ /*
+ * The first entry exceeds MAX_IO_SIZE, so we pass it to
+ * xwrite, which knows to handle this case.
+ */
+ if (!i)
+ return xwrite(fd, iov->iov_base, iov->iov_len);
+ iovcnt = i;
+ }
+
+ bytes_written = writev(fd, iov, iovcnt);
+ if (!bytes_written) {
+ errno = ENOSPC;
+ return -1;
+ }
+
+ return bytes_written;
+}
+
ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)
{
ssize_t total_written = 0;
while (iovcnt) {
- ssize_t bytes_written = writev(fd, iov, iovcnt);
- if (bytes_written < 0) {
+ ssize_t bytes_written = xwritev(fd, iov, iovcnt);
+ if (bytes_written <= 0) {
if (errno == EINTR || errno == EAGAIN)
continue;
return -1;
}
- if (!bytes_written) {
- errno = ENOSPC;
- return -1;
- }
total_written += bytes_written;
diff --git a/wrapper.h b/wrapper.h
index 27519b32d1..a6287d7f4d 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_
int xopen(const char *path, int flags, ...);
ssize_t xread(int fd, void *buf, size_t len);
ssize_t xwrite(int fd, const void *buf, size_t len);
+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);
ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
int xdup(int fd);
FILE *xfopen(const char *path, const char *mode);
RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On April 9, 2026 4:17 AM, Patrick Steinhardt wrote
Show 17 quoted lines
>On Thu, Apr 09, 2026 at 12:20:26AM +0000, brian m. carlson wrote:
>> On 2026-04-08 at 22:32:33, Jeff King wrote:
>> > I think writev() is buying us something when it works (it is hlving
>> > the number of writes for sideband packets). And it works when either:
>> >
>> > 1. the platform is OK with writing up to 64k in a single writev()
>> >
>> > 2. the platform has a limit that is small (like NonStop here), but
>> > writes less than MAX_IO_SIZE work and will save a write() call
>> >
>> > If we just care about (1), then the right solution is to declare
>> > that
>> > writev() isn't fully functional for us on some platforms, and they
>> > should build with NO_WRITEV. And we should probably embed that in
>> > config.mak.uname.
>
>Yeah, agreed. I think we shouldn't make ourselves a hostage to platforms
Show 22 quoted lines
>have reasonable support for writev(3p), as it does buy us something on the
>majority of platforms that actually support it well.
>
>That of course doesn't mean that we shouldn't support such platforms.
>
>> Looking at POSIX, there doesn't seem to be any constraints on the size
>> of individual vectors other than that they must total to less than
>> SSIZE_MAX. iovcnt can be limited to 16, but I don't think we're
>> hitting that here. POSIX does say that SSIZE_MAX does not need to
>> exceed 32767, which may be what's going on here, although that does
>> seem like an unreasonable value for a real system. Linux, FreeBSD,
>> and NetBSD all set SSIZE_MAX to either INT_MAX or LONG_MAX.
>>
>> I also think that 64 KiB is more than reasonable in terms of the size
>> that people should be able to send. I'd personally expect to be able
>> to send values much larger, at least 512 KiB, and I have code that
>> expects even larger (16 MiB).
>>
>> So I'd simply say that for systems that have a constraint on the size
>> that is "too small", they should just use NO_WRITEV.
>
>I would be happy with this as an intermediate step, as Randall has
>would fix the issue. It is the least intrusive step and has the lowest
Show 5 quoted lines
>
>> However, I don't have a strong opinion on this and if people want to
>> do the proposal for option 2, that's fine with me.
>
>I think in the long term this is the most sensible approach though so that
>have to special-case platforms. I've crafted the below alternative to
>I think it's ultimately not too bad.
>
>One question to Randall though: does MAX_IO_SIZE apply to the overall size
>iovec or to the individual iovec entries? I think it should be the latter,
>easily verify and couldn't find any docs around this. So could you please
>patch at the end of this mail to verify that it works on your system?
>
>In any case, I've tested that my patch also works when defining MAX_IO_SIZE
>128 bytes on my system, which hopefully demonstrates that it works as
>
>diff --git a/git-compat-util.h b/git-compat-util.h index
Show 10 quoted lines
>100644
>--- a/git-compat-util.h
>+++ b/git-compat-util.h
>@@ -690,14 +690,8 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
> * to override this, if the definition of SSIZE_MAX given by the platform
> * is broken.
> */
>-#ifndef MAX_IO_SIZE
>-# define MAX_IO_SIZE_DEFAULT (8*1024*1024) -# if defined(SSIZE_MAX) &&
>(SSIZE_MAX < MAX_IO_SIZE_DEFAULT) -# define MAX_IO_SIZE SSIZE_MAX -# else
Show 8 quoted lines
># define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT -# endif -#endif
>+#undef MAX_IO_SIZE
>+#define MAX_IO_SIZE 128
>
> #ifdef HAVE_ALLOCA_H
> # include <alloca.h>
>
>I'm happy to go either way, but think that we should definitely aim for the
>patch eventually. Just let me know which way you prefer and I'm happy to
Show 9 quoted lines
>the patch.
>
>Patrick
>
>diff --git a/wrapper.c b/wrapper.c
>index be8fa575e6..645dbc5f20 100644
>--- a/wrapper.c
>+++ b/wrapper.c
>@@ -323,21 +323,50 @@ ssize_t write_in_full(int fd, const void *buf, size_t
Show 11 quoted lines
> return total;
> }
>
>+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt) {
>+ ssize_t bytes_written;
>+ int i;
>+
>+ /*
>+ * We need to make sure that no individual iovec entry exceeds
>+ * `MAX_IO_SIZE`. If there's any entry that does exceed this limit
>+ * we'll pass all entries up to it to `writev()`, and then processShow 48 quoted lines
>+ * exceeding entry via a call to `xwrite()`.
>+ */
>+ for (i = 0; i < iovcnt; i++)
>+ if (iov[i].iov_len > MAX_IO_SIZE)
>+ break;
>+ if (i < iovcnt) {
>+ /*
>+ * The first entry exceeds MAX_IO_SIZE, so we pass it to
>+ * xwrite, which knows to handle his case.
>+ */
>+ if (!i)
>+ return xwrite(fd, iov->iov_base, iov->iov_len);
>+ iovcnt = i;
>+ }
>+
>+ bytes_written = writev(fd, iov, iovcnt);
>+ if (!bytes_written) {
>+ errno = ENOSPC;
>+ return -1;
>+ }
>+
>+ return bytes_written;
>+}
>+
> ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt) {
> ssize_t total_written = 0;
>
> while (iovcnt) {
>- ssize_t bytes_written = writev(fd, iov, iovcnt);
>- if (bytes_written < 0) {
>+ ssize_t bytes_written = xwritev(fd, iov, iovcnt);
>+ if (bytes_written <= 0) {
> if (errno == EINTR || errno == EAGAIN)
> continue;
> return -1;
> }
>- if (!bytes_written) {
>- errno = ENOSPC;
>- return -1;
>- }
>
> total_written += bytes_written;
>
>diff --git a/wrapper.h b/wrapper.h
>index 27519b32d1..a6287d7f4d 100644
>--- a/wrapper.h
>+++ b/wrapper.h
>@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot,>flags, int fd, off_ int xopen(const char *path, int flags, ...); ssize_t
>*buf, size_t len); ssize_t xwrite(int fd, const void *buf, size_t len);
>+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);
> ssize_t xpread(int fd, void *buf, size_t len, off_t offset); int xdup(int
>*xfopen(const char *path, const char *mode);
Please do not make the change in git-compat-util. This will break xwrite(). We already have MAX_IO_SIZE working and verified from years ago. Changing that will remove our platform from being supportable.
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Thu, Apr 09, 2026 at 09:46:39AM -0400, rsbecker@nexbridge.com wrote:
Show 17 quoted lines
> >--- a/git-compat-util.h
> >+++ b/git-compat-util.h
> >@@ -690,14 +690,8 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
> > * to override this, if the definition of SSIZE_MAX given by the platform
> > * is broken.
> > */
> >-#ifndef MAX_IO_SIZE
> >-# define MAX_IO_SIZE_DEFAULT (8*1024*1024) -# if defined(SSIZE_MAX) &&
> >(SSIZE_MAX < MAX_IO_SIZE_DEFAULT) -# define MAX_IO_SIZE SSIZE_MAX -# else
> -
> ># define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT -# endif -#endif
> >+#undef MAX_IO_SIZE
> >+#define MAX_IO_SIZE 128
> [...]
> Please do not make the change in git-compat-util. This will break xwrite().
> We already have MAX_IO_SIZE working and verified from years ago. Changing
> that will remove our platform from being supportable.
I think that was just there to demonstrate that the patch works regardless of the size, and would not be included in the final. Building with:
make CFLAGS=-DMAX_IO_SIZE=128
is probably a nicer way of doing that, though. ;)
-Peff
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Thu, Apr 09, 2026 at 10:17:22AM +0200, Patrick Steinhardt wrote:
Show 10 quoted lines
> > that people should be able to send. I'd personally expect to be able to
> > send values much larger, at least 512 KiB, and I have code that expects
> > even larger (16 MiB).
> >
> > So I'd simply say that for systems that have a constraint on the size
> > that is "too small", they should just use NO_WRITEV.
>
> I would be happy with this as an intermediate step, as Randall has
> confirmed it would fix the issue. It is the least intrusive step and has
> the lowest risk.
The only downside is that if there are other systems with a small SSIZE_MAX, they'll need similar treatment.
I took a look at auto-detecting this at the preprocessor stage. The conditional itself is not too bad, but we have to compile writev.o unconditionally, as well as re-order some definitions:
diff --git a/Makefile b/Makefile
index 5d22394c2e..f9b4dd8ae3 100644
--- a/Makefile
+++ b/Makefile
@@ -1125,6 +1125,7 @@ LIB_OBJS += compat/nonblock.o
LIB_OBJS += compat/obstack.o
LIB_OBJS += compat/open.o
LIB_OBJS += compat/terminal.o
+LIB_OBJS += compat/writev.o
LIB_OBJS += compiler-tricks/not-constant.o
LIB_OBJS += config.o
LIB_OBJS += connect.o
@@ -2031,7 +2032,6 @@ ifdef NO_PREAD
endif
ifdef NO_WRITEV
COMPAT_CFLAGS += -DNO_WRITEV
- COMPAT_OBJS += compat/writev.o
endif
ifdef NO_FAST_WORKING_DIRECTORY
BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
diff --git a/compat/posix.h b/compat/posix.h
index 94699a03fa..57c11938c7 100644
--- a/compat/posix.h
+++ b/compat/posix.h
@@ -326,6 +326,39 @@ int git_lstat(const char *, struct stat *);
ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);
#endif
+/*
+ * Limit size of IO chunks, because huge chunks only cause pain. OS X
+ * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in
+ * the absence of bugs, large chunks can result in bad latencies when
+ * you decide to kill the process.
+ *
+ * We pick 8 MiB as our default, but if the platform defines SSIZE_MAX
+ * that is smaller than that, clip it to SSIZE_MAX, as a call to
+ * read(2) or write(2) larger than that is allowed to fail. As the last
+ * resort, we allow a port to pass via CFLAGS e.g. "-DMAX_IO_SIZE=value"
+ * to override this, if the definition of SSIZE_MAX given by the platform
+ * is broken.
+ */
+#ifndef MAX_IO_SIZE
+# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
+# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
+# define MAX_IO_SIZE SSIZE_MAX
+# else
+# define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
+# endif
+#endif
+
+/*
+ * Temporary hack: our use of writev() does not respect MAX_IO_SIZE, but we
+ * never feed more than a single pkt-line packet to it. Disable writev()
+ * and use our fallback wrapper if the system can't support that size.
+ *
+ * This should be removed once writev() supports MAX_IO_SIZE.
+ */
+#if !defined(NO_WRITEV) && (MAX_IO_SIZE < 65536)
+#define NO_WRITEV
+#endif
+
#ifdef NO_WRITEV
#define writev git_writev
#define iovec git_iovec
diff --git a/git-compat-util.h b/git-compat-util.h
index 4b4ea2498f..60108459f3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -677,28 +677,6 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
return a + b;
}
-/*
- * Limit size of IO chunks, because huge chunks only cause pain. OS X
- * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in
- * the absence of bugs, large chunks can result in bad latencies when
- * you decide to kill the process.
- *
- * We pick 8 MiB as our default, but if the platform defines SSIZE_MAX
- * that is smaller than that, clip it to SSIZE_MAX, as a call to
- * read(2) or write(2) larger than that is allowed to fail. As the last
- * resort, we allow a port to pass via CFLAGS e.g. "-DMAX_IO_SIZE=value"
- * to override this, if the definition of SSIZE_MAX given by the platform
- * is broken.
- */
-#ifndef MAX_IO_SIZE
-# define MAX_IO_SIZE_DEFAULT (8*1024*1024)
-# if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
-# define MAX_IO_SIZE SSIZE_MAX
-# else
-# define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
-# endif
-#endif
-
#ifdef HAVE_ALLOCA_H
# include <alloca.h>
# define xalloca(size) (alloca(size))
It's not _too_ complicated, but it's a little more than I'd like for an
-rc2. Yet another option is to swap out NO_WRITEV for USE_WRITEV,
effectively reverting the feature temporarily until it supports
MAX_IO_SIZE. But that's also slightly non-trivial.
So I dunno. Do we want just:
diff --git a/config.mak.uname b/config.mak.uname
index ccb3f71881..b672b1d077 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -654,6 +654,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
CSPRNG_METHOD = openssl
SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
SHELL_PATH = /usr/coreutils/bin/bash
+ NO_WRITEV = NotUntilItSupportsMaxIOSize
endif
ifeq ($(uname_S),OS/390)
NO_SYS_POLL_H = YesPlease
and hope it's the only one?
-Peff
RE: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On April 9, 2026 4:34 PM, Jeff King wrote:
Show 30 quoted lines
>On Thu, Apr 09, 2026 at 09:46:39AM -0400, rsbecker@nexbridge.com wrote:
>
>> >--- a/git-compat-util.h
>> >+++ b/git-compat-util.h
>> >@@ -690,14 +690,8 @@ static inline uint64_t u64_add(uint64_t a,
>> >uint64_t b)
>> > * to override this, if the definition of SSIZE_MAX given by the
>> >platform
>> > * is broken.
>> > */
>> >-#ifndef MAX_IO_SIZE
>> >-# define MAX_IO_SIZE_DEFAULT (8*1024*1024) -# if defined(SSIZE_MAX)
>> >&& (SSIZE_MAX < MAX_IO_SIZE_DEFAULT) -# define MAX_IO_SIZE SSIZE_MAX
>> >-# else
>> -
>> ># define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT -# endif -#endif
>> >+#undef MAX_IO_SIZE
>> >+#define MAX_IO_SIZE 128
>> [...]
>> Please do not make the change in git-compat-util. This will break xwrite().
>> We already have MAX_IO_SIZE working and verified from years ago.
>> Changing that will remove our platform from being supportable.
>
>I think that was just there to demonstrate that the patch works regardless of the
>size, and would not be included in the final.
>Building with:
>
> make CFLAGS=-DMAX_IO_SIZE=128
>
>is probably a nicer way of doing that, though. ;)
We had that set properly in git-compat-util.h for years. MAX_IO_SIZE should be set to SSIZE_MAX if SSIZE_MAX is defined. #ifndef MAX_IO_SIZE # define MAX_IO_SIZE_DEFAULT (8*1024*1024) # if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT) # define MAX_IO_SIZE SSIZE_MAX # else # define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Thu, Apr 09, 2026 at 06:40:00PM -0400, rsbecker@nexbridge.com wrote:
Show 19 quoted lines
> >> Please do not make the change in git-compat-util. This will break xwrite().
> >> We already have MAX_IO_SIZE working and verified from years ago.
> >> Changing that will remove our platform from being supportable.
> >
> >I think that was just there to demonstrate that the patch works regardless of the
> >size, and would not be included in the final.
> >Building with:
> >
> > make CFLAGS=-DMAX_IO_SIZE=128
> >
> >is probably a nicer way of doing that, though. ;)
>
> We had that set properly in git-compat-util.h for years. MAX_IO_SIZE should be set to SSIZE_MAX if SSIZE_MAX is defined.
> #ifndef MAX_IO_SIZE
> # define MAX_IO_SIZE_DEFAULT (8*1024*1024)
> # if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
> # define MAX_IO_SIZE SSIZE_MAX
> # else
> # define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
Right. We would retain that. I think the point was that Patrick was dropping MAX_IO_SIZE artificially on his Linux system to exercise the new code.
-Peff
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
On Thu, Apr 09, 2026 at 06:58:06PM -0400, Jeff King wrote:
Show 25 quoted lines
> On Thu, Apr 09, 2026 at 06:40:00PM -0400, rsbecker@nexbridge.com wrote:
>
> > >> Please do not make the change in git-compat-util. This will break xwrite().
> > >> We already have MAX_IO_SIZE working and verified from years ago.
> > >> Changing that will remove our platform from being supportable.
> > >
> > >I think that was just there to demonstrate that the patch works regardless of the
> > >size, and would not be included in the final.
> > >Building with:
> > >
> > > make CFLAGS=-DMAX_IO_SIZE=128
> > >
> > >is probably a nicer way of doing that, though. ;)
> >
> > We had that set properly in git-compat-util.h for years. MAX_IO_SIZE should be set to SSIZE_MAX if SSIZE_MAX is defined.
> > #ifndef MAX_IO_SIZE
> > # define MAX_IO_SIZE_DEFAULT (8*1024*1024)
> > # if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE_DEFAULT)
> > # define MAX_IO_SIZE SSIZE_MAX
> > # else
> > # define MAX_IO_SIZE MAX_IO_SIZE_DEFAULT
>
> Right. We would retain that. I think the point was that Patrick was
> dropping MAX_IO_SIZE artificially on his Linux system to exercise the
> new code.
Yup, exactly. I don't have any intent to change the above snippet in our code base.
Patrick
Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
Am 09.04.26 um 10:17 schrieb Patrick Steinhardt:
Show 5 quoted lines
> Yeah, agreed. I think we shouldn't make ourselves a hostage to platforms
> that don't have reasonable support for writev(3p), as it does buy us
> something on the majority of platforms that actually support it well.
>
> That of course doesn't mean that we shouldn't support such platforms.
My take on this matter is that the use writev in Git's code gives POSIX centered contributors a false sense of security. POSIX's writev offers a number of guarantees that a compatibility implementation cannot provide. If uses of writev proliferate, I forsee the time when someone reports a problem on a compatibility platform, and one of us will point at POSIX and say "but that cannot happen because we have this and that guarantee".
What did we gain with writev? There are half that many system calls. So what? Have there been any hard numbers about better performance, for example? Not even the call sites became simpler, see the commit that introduced the only caller: 26986f4cbaf3 ("sideband: use writev(3p) to send pktlines", 2026-03-13).
-- Hannes