Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327

From: Jeff King <peff@peff.net>
Date: Apr 8, 2026, 22:32 UTC
Message-ID: <20260408223233.GB2873736@coredump.intra.peff.net>
In-Reply-To: <xmqqcy09xh53.fsf@gitster.g>

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() 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.

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