Git development
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Patrick Steinhardt <ps@pks.im>
Cc: "brian m. carlson" <sandals@crustytoothpaste.net>,
	Junio C Hamano <gitster@pobox.com>,
	rsbecker@nexbridge.com, git@vger.kernel.org
Subject: Re: Git 2.54.0-rc1, subtests of t5310, t5326, t5327
Date: Thu, 9 Apr 2026 16:51:45 -0400	[thread overview]
Message-ID: <20260409205145.GC3076846@coredump.intra.peff.net> (raw)
In-Reply-To: <addgkjiB80pgKw69@pks.im>

On Thu, Apr 09, 2026 at 10:17:22AM +0200, Patrick Steinhardt wrote:

> > 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

  parent reply	other threads:[~2026-04-09 20:51 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-07 23:29 Git 2.54.0-rc1, subtests of t5310, t5326, t5327 rsbecker
2026-04-08  4:17 ` Jeff King
2026-04-08 14:54   ` rsbecker
2026-04-08 16:25     ` rsbecker
2026-04-08 17:39       ` Jeff King
2026-04-08 18:12         ` Junio C Hamano
2026-04-08 20:08           ` rsbecker
2026-04-08 20:21             ` Junio C Hamano
2026-04-08 21:27               ` rsbecker
2026-04-08 21:43                 ` Junio C Hamano
2026-04-08 22:04                   ` rsbecker
2026-04-08 22:24                   ` Junio C Hamano
2026-04-08 22:35                     ` Junio C Hamano
2026-04-08 23:15                       ` rsbecker
2026-04-08 22:32                   ` Jeff King
2026-04-09  0:20                     ` brian m. carlson
2026-04-09  8:17                       ` Patrick Steinhardt
2026-04-09  9:48                         ` Phillip Wood
2026-04-09 11:29                           ` Patrick Steinhardt
2026-04-09 13:46                         ` rsbecker
2026-04-09 20:33                           ` Jeff King
2026-04-09 22:40                             ` rsbecker
2026-04-09 22:58                               ` Jeff King
2026-04-10  4:34                                 ` Patrick Steinhardt
2026-04-09 20:51                         ` Jeff King [this message]
2026-04-10  7:35                         ` Johannes Sixt
2026-04-08 18:36         ` rsbecker
2026-04-08 22:14           ` Jeff King
2026-04-08 17:37     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260409205145.GC3076846@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=ps@pks.im \
    --cc=rsbecker@nexbridge.com \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox