Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Apr 9, 2026, 08:17 UTC
- Message-ID
- <addgkjiB80pgKw69@pks.im>
- In-Reply-To
- <adbwyvQ-R2Ag1vox@fruit.crustytoothpaste.net>
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);