On 20/11/2022 03:50, Paul Eggert wrote:
Although we inadvertently removed support for weird devices in 2009 by
commit 55efc5f3ee485b3e31a91c331f07c89aeccc4e89, and nobody seems to
care (because people use dd or whatever to deal with weird devices), I
think it'd be better to limit the fix to regular files. And while we're
at it we might as well resurrect support for weird devices.
Paul I'm happy with your patch for this.
I've rebased and tweaked the summary and NEWS slightly in the attached.
OK for me to apply?
Preemptively marking this as done.
cheers,
Pádraig
From 573979e21af8f9ea7f83660e949902b3330ce25e Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 19 Nov 2022 19:04:36 -0800
Subject: [PATCH] copy: fix possible over allocation for regular files
* bootstrap.conf (gnulib_modules): Add count-leading-zeros,
which was already an indirect dependency, since ioblksize.h
now uses it directly.
* src/ioblksize.h: Include count-leading-zeros.h.
(io_blksize): Treat impossible blocksizes as IO_BUFSIZE.
When growing a blocksize to IO_BUFSIZE, keep it a multiple of the
stated blocksize. Work around the ZFS performance bug.
* NEWS: Mention the bug fix.
Problem reported by Korn Andras at https://bugs.gnu.org/59382
---
NEWS | 5 +++++
bootstrap.conf | 1 +
src/ioblksize.h | 28 +++++++++++++++++++++++++++-
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index 0fbfe7b09..40eb66968 100644
--- a/NEWS
+++ b/NEWS
@@ -20,6 +20,11 @@ GNU coreutils NEWS -*- outline -*-
which may have resulted in data corruption.
[bug introduced in coreutils-7.5 and enabled by default in coreutils-9.0]
+ cp, mv, and install avoid allocating too much memory, and possibly
+ triggering "memory exhausted" failures, on file systems like ZFS,
+ which can return varied file system I/O block size values for files.
+ [bug introduced in coreutils-6.0]
+
'mv --backup=simple f d/' no longer mistakenly backs up d/f to f~.
[bug introduced in coreutils-9.1]
diff --git a/bootstrap.conf b/bootstrap.conf
index 247734673..f04c90624 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -59,6 +59,7 @@ gnulib_modules="
config-h
configmake
copy-file-range
+ count-leading-zeros
crypto/md5
crypto/sha1
crypto/sha256
diff --git a/src/ioblksize.h b/src/ioblksize.h
index 7a56c1a51..80d7931ba 100644
--- a/src/ioblksize.h
+++ b/src/ioblksize.h
@@ -18,6 +18,7 @@
/* sys/stat.h and minmax.h will already have been included by system.h. */
#include "idx.h"
+#include "count-leading-zeros.h"
#include "stat-size.h"
@@ -75,8 +76,33 @@ enum { IO_BUFSIZE = 128 * 1024 };
static inline idx_t
io_blksize (struct stat sb)
{
+ /* Treat impossible blocksizes as if they were IO_BUFSIZE. */
+ idx_t blocksize = ST_BLKSIZE (sb) <= 0 ? IO_BUFSIZE : ST_BLKSIZE (sb);
+
+ /* Use a blocksize of at least IO_BUFSIZE bytes, keeping it a
+ multiple of the original blocksize. */
+ blocksize += (IO_BUFSIZE - 1) - (IO_BUFSIZE - 1) % blocksize;
+
+ /* For regular files we can ignore the blocksize if we think we know better.
+ ZFS sometimes understates the blocksize, because it thinks
+ apps stupidly allocate a block that large even for small files.
+ This misinformation can cause coreutils to use wrong-sized blocks.
+ Work around some of the performance bug by substituting the next
+ power of two when the reported blocksize is not a power of two. */
+ if (S_ISREG (sb.st_mode)
+ && blocksize & (blocksize - 1))
+ {
+ int leading_zeros = count_leading_zeros_ll (blocksize);
+ if (IDX_MAX < ULLONG_MAX || leading_zeros)
+ {
+ unsigned long long power = 1ull << (ULLONG_WIDTH - leading_zeros);
+ if (power <= IDX_MAX)
+ blocksize = power;
+ }
+ }
+
/* Don’t go above the largest power of two that fits in idx_t and size_t,
as that is asking for trouble. */
return MIN (MIN (IDX_MAX, SIZE_MAX) / 2 + 1,
- MAX (IO_BUFSIZE, ST_BLKSIZE (sb)));
+ blocksize);
}
--
2.26.2
