GNU bug report logs - #59382
cp(1) tries to allocate too much memory if filesystem blocksizes are unusual

Previous Next

Package: coreutils;

Reported by: Korn Andras <korn-gnu.org <at> elan.rulez.org>

Date: Sat, 19 Nov 2022 09:26:03 UTC

Severity: normal

Done: Pádraig Brady <P <at> draigBrady.com>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 59382 in the body.
You can then email your comments to 59382 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-coreutils <at> gnu.org:
bug#59382; Package coreutils. (Sat, 19 Nov 2022 09:26:03 GMT) Full text and rfc822 format available.

Acknowledgement sent to Korn Andras <korn-gnu.org <at> elan.rulez.org>:
New bug report received and forwarded. Copy sent to bug-coreutils <at> gnu.org. (Sat, 19 Nov 2022 09:26:03 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Korn Andras <korn-gnu.org <at> elan.rulez.org>
To: bug-coreutils <at> gnu.org
Subject: cp(1) tries to allocate too much memory if filesystem blocksizes are
 unusual
Date: Sat, 19 Nov 2022 09:14:50 +0100
Hi,

on zfs, newfstatat() can return an st_blksize that is approximately equal to the file size in bytes (if the file fit into a single zfs record).

The block size for filesystems can also be quite large (currently, up to 16M).

The code at https://github.com/coreutils/coreutils/blob/master/src/copy.c#L1343 will try to compute the least common multiple of the input and output block sizes, then allocate a buffer of that size using mmap(). With such unusual block sizes, the least common multiple can be very large, causing the mmap() to return ENOMEM and cp to abort with "memory exhausted".

Example:

openat(AT_FDCWD, "tmp", O_RDONLY|O_PATH|O_DIRECTORY) = 3</t/tmp>
newfstatat(AT_FDCWD, "usr/src/zfs-2.1.6/configure", {st_dev=makedev(0, 0x30), st_ino=29267, st_mode=S_IFREG|0755, st_nlink=1, st_uid=0, st_gid=0, st_blksize=2647552, st_blocks=5177, st_size=2647046, st_atime=1668786119 /* 2022-11-18T16:41:59.760703544+0100 */, st_atime_nsec=760703544, st_mtime=1667705386 /* 2022-11-06T04:29:46+0100 */, st_mtime_nsec=0, st_ctime=1668785062 /* 2022-11-18T16:24:22.866737598+0100 */, st_ctime_nsec=866737598}, AT_SYMLINK_NOFOLLOW) = 0
newfstatat(3</t/tmp>, "configure", {st_dev=makedev(0, 0x32), st_ino=3838, st_mode=S_IFREG|0700, st_nlink=1, st_uid=0, st_gid=0, st_blksize=4194304, st_blocks=1, st_size=0, st_atime=1668788205 /* 2022-11-18T17:16:45.416328293+0100 */, st_atime_nsec=416328293, st_mtime=1668788255 /* 2022-11-18T17:17:35.809758585+0100 */, st_mtime_nsec=809758585, st_ctime=1668788255 /* 2022-11-18T17:17:35.809758585+0100 */, st_ctime_nsec=809758585}, 0) = 0
openat(AT_FDCWD, "usr/src/zfs-2.1.6/configure", O_RDONLY|O_NOFOLLOW) = 4</t/usr/src/zfs-2.1.6/configure>
newfstatat(4</t/usr/src/zfs-2.1.6/configure>, "", {st_dev=makedev(0, 0x30), st_ino=29267, st_mode=S_IFREG|0755, st_nlink=1, st_uid=0, st_gid=0, st_blksize=2647552, st_blocks=5177, st_size=2647046, st_atime=1668786119 /* 2022-11-18T16:41:59.760703544+0100 */, st_atime_nsec=760703544, st_mtime=1667705386 /* 2022-11-06T04:29:46+0100 */, st_mtime_nsec=0, st_ctime=1668785062 /* 2022-11-18T16:24:22.866737598+0100 */, st_ctime_nsec=866737598}, AT_EMPTY_PATH) = 0
openat(3</t/tmp>, "configure", O_WRONLY|O_TRUNC) = 5</t/tmp/configure>
ioctl(5</t/tmp/configure>, BTRFS_IOC_CLONE or FICLONE, 4) = -1 EXDEV (Invalid cross-device link)
newfstatat(5</t/tmp/configure>, "", {st_dev=makedev(0, 0x32), st_ino=3838, st_mode=S_IFREG|0700, st_nlink=1, st_uid=0, st_gid=0, st_blksize=4194304, st_blocks=1, st_size=0, st_atime=1668788205 /* 2022-11-18T17:16:45.416328293+0100 */, st_atime_nsec=416328293, st_mtime=1668788266 /* 2022-11-18T17:17:46.326056957+0100 */, st_mtime_nsec=326056957, st_ctime=1668788266 /* 2022-11-18T17:17:46.326056957+0100 */, st_ctime_nsec=326056957}, AT_EMPTY_PATH) = 0
fadvise64(4</t/usr/src/zfs-2.1.6/configure>, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
copy_file_range(4</t/usr/src/zfs-2.1.6/configure>, NULL, 5</t/tmp/configure>, NULL, 9223372035781033984, 0) = -1 EXDEV (Invalid cross-device link)
mmap(NULL, 21688754176, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory)
brk(0x55a7fa4b0000)                     = 0x55a2ed8ab000
mmap(NULL, 21689794560, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory)
openat(AT_FDCWD, "/usr/share/locale/en_US.UTF-8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en_US.utf8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en_US/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en.UTF-8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en.utf8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
write(2</dev/pts/3<char 136:3>>, "cp: ", 4) = 4
write(2</dev/pts/3<char 136:3>>, "memory exhausted", 16) = 16
write(2</dev/pts/3<char 136:3>>, "\n", 1) = 1
lseek(0</dev/pts/3<char 136:3>>, 0, SEEK_CUR) = -1 ESPIPE (Illegal seek)
close(0</dev/pts/3<char 136:3>>)        = 0
close(1</dev/pts/3<char 136:3>>)        = 0
close(2</dev/pts/3<char 136:3>>)        = 0
exit_group(1)                           = ?

I think cp(1) shouldn't insist on using the lcm if it's very large; additionally, it should never try to allocate a buffer that is (much) larger than the source file.

Perhaps you could also try to use successively smaller buffers if allocating a large one fails?

András

-- 
               Write your complaints in this box:  ---------> []




Information forwarded to bug-coreutils <at> gnu.org:
bug#59382; Package coreutils. (Sat, 19 Nov 2022 17:26:01 GMT) Full text and rfc822 format available.

Message #8 received at 59382 <at> debbugs.gnu.org (full text, mbox):

From: "Pádraig Brady" <P <at> draigBrady.com>
To: Korn Andras <korn-gnu.org <at> elan.rulez.org>, 59382 <at> debbugs.gnu.org
Subject: Re: bug#59382: cp(1) tries to allocate too much memory if filesystem
 blocksizes are unusual
Date: Sat, 19 Nov 2022 17:25:10 +0000
[Message part 1 (text/plain, inline)]
On 19/11/2022 08:14, Korn Andras wrote:
> Hi,
> 
> on zfs, newfstatat() can return an st_blksize that is approximately equal to the file size in bytes (if the file fit into a single zfs record).
> 
> The block size for filesystems can also be quite large (currently, up to 16M).
> 
> The code at https://github.com/coreutils/coreutils/blob/master/src/copy.c#L1343 will try to compute the least common multiple of the input and output block sizes, then allocate a buffer of that size using mmap(). With such unusual block sizes, the least common multiple can be very large, causing the mmap() to return ENOMEM and cp to abort with "memory exhausted".
> 
> Example:
> 
> openat(AT_FDCWD, "tmp", O_RDONLY|O_PATH|O_DIRECTORY) = 3</t/tmp>
> newfstatat(AT_FDCWD, "usr/src/zfs-2.1.6/configure", {st_dev=makedev(0, 0x30), st_ino=29267, st_mode=S_IFREG|0755, st_nlink=1, st_uid=0, st_gid=0, st_blksize=2647552, st_blocks=5177, st_size=2647046, st_atime=1668786119 /* 2022-11-18T16:41:59.760703544+0100 */, st_atime_nsec=760703544, st_mtime=1667705386 /* 2022-11-06T04:29:46+0100 */, st_mtime_nsec=0, st_ctime=1668785062 /* 2022-11-18T16:24:22.866737598+0100 */, st_ctime_nsec=866737598}, AT_SYMLINK_NOFOLLOW) = 0
> newfstatat(3</t/tmp>, "configure", {st_dev=makedev(0, 0x32), st_ino=3838, st_mode=S_IFREG|0700, st_nlink=1, st_uid=0, st_gid=0, st_blksize=4194304, st_blocks=1, st_size=0, st_atime=1668788205 /* 2022-11-18T17:16:45.416328293+0100 */, st_atime_nsec=416328293, st_mtime=1668788255 /* 2022-11-18T17:17:35.809758585+0100 */, st_mtime_nsec=809758585, st_ctime=1668788255 /* 2022-11-18T17:17:35.809758585+0100 */, st_ctime_nsec=809758585}, 0) = 0
> openat(AT_FDCWD, "usr/src/zfs-2.1.6/configure", O_RDONLY|O_NOFOLLOW) = 4</t/usr/src/zfs-2.1.6/configure>
> newfstatat(4</t/usr/src/zfs-2.1.6/configure>, "", {st_dev=makedev(0, 0x30), st_ino=29267, st_mode=S_IFREG|0755, st_nlink=1, st_uid=0, st_gid=0, st_blksize=2647552, st_blocks=5177, st_size=2647046, st_atime=1668786119 /* 2022-11-18T16:41:59.760703544+0100 */, st_atime_nsec=760703544, st_mtime=1667705386 /* 2022-11-06T04:29:46+0100 */, st_mtime_nsec=0, st_ctime=1668785062 /* 2022-11-18T16:24:22.866737598+0100 */, st_ctime_nsec=866737598}, AT_EMPTY_PATH) = 0
> openat(3</t/tmp>, "configure", O_WRONLY|O_TRUNC) = 5</t/tmp/configure>
> ioctl(5</t/tmp/configure>, BTRFS_IOC_CLONE or FICLONE, 4) = -1 EXDEV (Invalid cross-device link)
> newfstatat(5</t/tmp/configure>, "", {st_dev=makedev(0, 0x32), st_ino=3838, st_mode=S_IFREG|0700, st_nlink=1, st_uid=0, st_gid=0, st_blksize=4194304, st_blocks=1, st_size=0, st_atime=1668788205 /* 2022-11-18T17:16:45.416328293+0100 */, st_atime_nsec=416328293, st_mtime=1668788266 /* 2022-11-18T17:17:46.326056957+0100 */, st_mtime_nsec=326056957, st_ctime=1668788266 /* 2022-11-18T17:17:46.326056957+0100 */, st_ctime_nsec=326056957}, AT_EMPTY_PATH) = 0
> fadvise64(4</t/usr/src/zfs-2.1.6/configure>, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
> copy_file_range(4</t/usr/src/zfs-2.1.6/configure>, NULL, 5</t/tmp/configure>, NULL, 9223372035781033984, 0) = -1 EXDEV (Invalid cross-device link)
> mmap(NULL, 21688754176, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory)
> brk(0x55a7fa4b0000)                     = 0x55a2ed8ab000
> mmap(NULL, 21689794560, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = -1 ENOMEM (Cannot allocate memory)
> openat(AT_FDCWD, "/usr/share/locale/en_US.UTF-8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
> openat(AT_FDCWD, "/usr/share/locale/en_US.utf8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
> openat(AT_FDCWD, "/usr/share/locale/en_US/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
> openat(AT_FDCWD, "/usr/share/locale/en.UTF-8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
> openat(AT_FDCWD, "/usr/share/locale/en.utf8/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
> openat(AT_FDCWD, "/usr/share/locale/en/LC_MESSAGES/coreutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
> write(2</dev/pts/3<char 136:3>>, "cp: ", 4) = 4
> write(2</dev/pts/3<char 136:3>>, "memory exhausted", 16) = 16
> write(2</dev/pts/3<char 136:3>>, "\n", 1) = 1
> lseek(0</dev/pts/3<char 136:3>>, 0, SEEK_CUR) = -1 ESPIPE (Illegal seek)
> close(0</dev/pts/3<char 136:3>>)        = 0
> close(1</dev/pts/3<char 136:3>>)        = 0
> close(2</dev/pts/3<char 136:3>>)        = 0
> exit_group(1)                           = ?
> 
> I think cp(1) shouldn't insist on using the lcm if it's very large; additionally, it should never try to allocate a buffer that is (much) larger than the source file.
> 
> Perhaps you could also try to use successively smaller buffers if allocating a large one fails?

Thank you for the detailed report.
This buffer_lcm code has been present for a long time,
but I agree in the presence of disparate st_blksize values
it can result in inappropriate buffer sizes.

In your case:
  src st_blksize = 2647552 (0x286600) (st_size = 2647046)
  dst st_blksize = 4194304 (0x400000) (st_size = 0)
Which implies a lowest common multiple of 21688745984

Once the st_blksize is based on st_size we can get very large values like this.
In the zfs case it seems to set st_blksize to st_size rounded to next multiple of 512.

The proposed patch attached removes the use of buffer_lcm()
and just picks the largest st_blksize, which would be 4MiB in your case.
It also limits the max buffer size to 32MiB in the edge case
where st_blksize returns a larger value that this.

cheers,
Pádraig
[copy-buffer-limit.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#59382; Package coreutils. (Sun, 20 Nov 2022 03:51:02 GMT) Full text and rfc822 format available.

Message #11 received at 59382 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>,
 Korn Andras <korn-gnu.org <at> elan.rulez.org>, 59382 <at> debbugs.gnu.org
Subject: Re: bug#59382: cp(1) tries to allocate too much memory if filesystem
 blocksizes are unusual
Date: Sat, 19 Nov 2022 19:50:06 -0800
[Message part 1 (text/plain, inline)]
>> The block size for filesystems can also be quite large (currently, up 
>> to 16M).

It seems ZFS tries to "help" apps by reporting misinformation (namely a 
smaller block size than actually preferred) when the file is small. This 
is unfortunate, since it messes up cp and similar programs that need to 
juggle multiple block sizes. Plus, it messes up any program that assumes 
st_blksize is constant for the life of a file descriptor, which "cp" 
does assume elsewhere.

GNU cp doesn't need ZFS's "help", as it's already smart enough to not 
over-allocate a buffer when the input file is small but its blocksize is 
large. Instead, this "help" from ZFS causes GNU cp to over-allocate 
because it naively trusts the blocksize ZFS that reports.


> The proposed patch attached removes the use of buffer_lcm()
> and just picks the largest st_blksize, which would be 4MiB in your case.
> It also limits the max buffer size to 32MiB in the edge case
> where st_blksize returns a larger value that this.

I suppose this could break cp if st_blksize is not a power of 2, and if 
the file is not a regular file, and reads must be a multiple of the 
block size. POSIX allows such things though I expect nowadays it'd be 
limited to weird devices.

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.


> +#include <assert.h>

No need for this, as static_assert works without <assert.h> in C23, and 
Gnulib's assert-h module support this.


> +/* Set a max constraint to avoid excessive mem usage or type overflow.  */
> +enum { IO_BUFSIZE_MAX = 128 * IO_BUFSIZE };
> +static_assert (IO_BUFSIZE_MAX <= MIN (IDX_MAX, SIZE_MAX) / 2 + 1);

I'm leery of putting in a maximum as low as 16 MiB. Although that's OK 
now (it matches OpenZFS's current maximum), cp in the future will surely 
deal with bigger block sizes. Instead, how about if we stick with GNU's 
"no arbitrary limits" policy and work around the ZFS bug instead?

Something like the attached patch, perhaps?
[0001-cp-work-around-ZFS-misinformation.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#59382; Package coreutils. (Sun, 20 Nov 2022 11:42:03 GMT) Full text and rfc822 format available.

Message #14 received at 59382 <at> debbugs.gnu.org (full text, mbox):

From: Korn Andras <korn-gnu.org <at> elan.rulez.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 59382 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigBrady.com>
Subject: Re: bug#59382: cp(1) tries to allocate too much memory if filesystem
 blocksizes are unusual
Date: Sun, 20 Nov 2022 07:43:19 +0100
On Sat, Nov 19, 2022 at 07:50:06PM -0800, Paul Eggert wrote:

> > > The block size for filesystems can also be quite large (currently,
> > > up to 16M).
> 
> It seems ZFS tries to "help" apps by reporting misinformation (namely a
> smaller block size than actually preferred) when the file is small. This is

Just a nit: this isn't actually misinformation. ZFS uses variable "block"
sizes (it calls these blocks "records"). There is a configurable
per-filesystem maximum, which records of new writes will not exceed (but may
not reach); but existing files may use larger record sizes than what is
currently configured for the fs.

The currently-configured recordsize of the filesystem and the recordsize a
particular file was written with are not necessarily related. Depending on
the write pattern and whether the recordsize of the fs was changed during
the lifetime of the file, the same file can contain records of different
sizes. Reductio ad absurdum: the "optimal" blocksize for reading may in fact
depend on the position within the file (and only apply to the next read).

If a file fits into a single record, then, IIUC, it is actually optimal to
read it in a single operation; this is the case even if the currently
configured recordsize is smaller than what the allowed maximum was when the
file was written. If the file is highly fragmented and chunks of it are
stored on different physical media (this can easily happen if the zfs pool
was expanded with a new "vdev" during the lifetime of the file), it will in
fact be fastest to issue reads for several chunks in parallel, with read
speed possibly scaling almost linearly with the number of parallel requests.
(Not that I'm proposing cp(1) should try to figure this out, presumably in a
zfs-specific way, and actually do it.)

Since the file may contain records of various sizes that bear no relation to
the current per-fs recordsize setting, it's not immediately obvious (at
least to me) what st_blocksize zfs should report that can't be construed as
misinformation.

If you have strong opinions on the matter, you may want to explain them in
the pertinent OpenZFS issue: https://github.com/openzfs/zfs/issues/14195.

András

-- 
 I was once thrown out of a mental hospital for depressing the other patients.




Information forwarded to bug-coreutils <at> gnu.org:
bug#59382; Package coreutils. (Sun, 20 Nov 2022 17:03:01 GMT) Full text and rfc822 format available.

Message #17 received at 59382 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>,
 Korn Andras <korn-gnu.org <at> elan.rulez.org>, 59382 <at> debbugs.gnu.org
Subject: Re: bug#59382: cp(1) tries to allocate too much memory if filesystem
 blocksizes are unusual
Date: Sun, 20 Nov 2022 17:02:40 +0000
On 20/11/2022 03:50, Paul Eggert wrote:
>>> The block size for filesystems can also be quite large (currently, up
>>> to 16M).
> 
> It seems ZFS tries to "help" apps by reporting misinformation (namely a
> smaller block size than actually preferred) when the file is small. This
> is unfortunate, since it messes up cp and similar programs that need to
> juggle multiple block sizes. Plus, it messes up any program that assumes
> st_blksize is constant for the life of a file descriptor, which "cp"
> does assume elsewhere.
> 
> GNU cp doesn't need ZFS's "help", as it's already smart enough to not
> over-allocate a buffer when the input file is small but its blocksize is
> large. Instead, this "help" from ZFS causes GNU cp to over-allocate
> because it naively trusts the blocksize ZFS that reports.
> 
> 
>> The proposed patch attached removes the use of buffer_lcm()
>> and just picks the largest st_blksize, which would be 4MiB in your case.
>> It also limits the max buffer size to 32MiB in the edge case
>> where st_blksize returns a larger value that this.
> 
> I suppose this could break cp if st_blksize is not a power of 2, and if
> the file is not a regular file, and reads must be a multiple of the
> block size. POSIX allows such things though I expect nowadays it'd be
> limited to weird devices.
> 
> 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.

I wouldn't worry about weird devices TBH, for the reasons you state,
but it doesn't cost too much to support so fair enough.
I would definitely add a comment to the code in this regard though
as I certainly wasn't aware of that issue when I added commit 55efc5f3.

>> +#include <assert.h>
> 
> No need for this, as static_assert works without <assert.h> in C23, and
> Gnulib's assert-h module support this.

cool

>> +/* Set a max constraint to avoid excessive mem usage or type overflow.  */
>> +enum { IO_BUFSIZE_MAX = 128 * IO_BUFSIZE };
>> +static_assert (IO_BUFSIZE_MAX <= MIN (IDX_MAX, SIZE_MAX) / 2 + 1);
> 
> I'm leery of putting in a maximum as low as 16 MiB. Although that's OK
> now (it matches OpenZFS's current maximum), cp in the future will surely
> deal with bigger block sizes. Instead, how about if we stick with GNU's
> "no arbitrary limits" policy and work around the ZFS bug instead?

Well I wouldn't think of this as a functional limit,
more of a defensive programming technique with possible perf benefits.
Note as per the table in ioblksize.h, performance is seen to max out
at around 2^17 on most systems, and degrades on some systems beyond that.
So cp would be faster while being less memory efficient.
But yes if we're relying on these multiples for "weird devices" then fair enough,
that would be a functional limit, so we need to consider such large buffers.

Also to state it explicitly, for regular files, your change to clamp to a power of two
will effectively get buffer_lcm to pick the max of the two io_blksize() values.

I would change the commit summary from:
  cp: work around ZFS misinformation
to:
  copy: fix possible over allocation for regular files
because:
  - This also applies to cross device mv etc.
  - ZFS is giving more info TBH, just a bit naïvely
  - I do see this is a coreutils bug (as well)

Here is a NEWS entry as well:

  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]

thanks!
Pádraig




Information forwarded to bug-coreutils <at> gnu.org:
bug#59382; Package coreutils. (Sun, 20 Nov 2022 17:30:04 GMT) Full text and rfc822 format available.

Message #20 received at 59382 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Korn Andras <korn-gnu.org <at> elan.rulez.org>
Cc: 59382 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigBrady.com>
Subject: Re: bug#59382: cp(1) tries to allocate too much memory if filesystem
 blocksizes are unusual
Date: Sun, 20 Nov 2022 09:29:33 -0800
On 2022-11-19 22:43, Korn Andras wrote:
> the same file can contain records of different
> sizes. Reductio ad absurdum: the "optimal" blocksize for reading may in fact
> depend on the position within the file (and only apply to the next read).

This sort of problem exists on traditional devices as well. A tape drive 
can have records of different sizes. For these devices, the best 
approach is to allocate a buffer of the maximum blocksize the drive 
supports.

For the file you describe the situation is different, since ZFS will 
straddle small blocks during I/O. Although there's no single "best" I 
would guess that it'd typically be better to report the blocksize 
currently in use for creating new blocks (which would be a power of two 
for ZFS), as that will map better to how programs like cp deal with 
blocksizes. This may not be perfect but it'd be better than what ZFS 
does now, at least for the instances of 'cp' that are already out there.





Reply sent to Pádraig Brady <P <at> draigBrady.com>:
You have taken responsibility. (Mon, 02 Jan 2023 23:05:01 GMT) Full text and rfc822 format available.

Notification sent to Korn Andras <korn-gnu.org <at> elan.rulez.org>:
bug acknowledged by developer. (Mon, 02 Jan 2023 23:05:01 GMT) Full text and rfc822 format available.

Message #25 received at 59382-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>,
 Korn Andras <korn-gnu.org <at> elan.rulez.org>, 59382-done <at> debbugs.gnu.org
Subject: Re: bug#59382: cp(1) tries to allocate too much memory if filesystem
 blocksizes are unusual
Date: Mon, 2 Jan 2023 23:03:50 +0000
[Message part 1 (text/plain, inline)]
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
[copy-fix-large-alloc.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#59382; Package coreutils. (Mon, 02 Jan 2023 23:19:02 GMT) Full text and rfc822 format available.

Message #28 received at 59382-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>,
 Korn Andras <korn-gnu.org <at> elan.rulez.org>, 59382-done <at> debbugs.gnu.org
Subject: Re: bug#59382: cp(1) tries to allocate too much memory if filesystem
 blocksizes are unusual
Date: Mon, 2 Jan 2023 15:18:09 -0800
On 2023-01-02 15:03, Pádraig Brady wrote:
> 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?

Sure, that looks good, thanks.





Information forwarded to bug-coreutils <at> gnu.org:
bug#59382; Package coreutils. (Mon, 02 Jan 2023 23:25:02 GMT) Full text and rfc822 format available.

Message #31 received at 59382-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>,
 Korn Andras <korn-gnu.org <at> elan.rulez.org>, 59382-done <at> debbugs.gnu.org
Subject: Re: bug#59382: cp(1) tries to allocate too much memory if filesystem
 blocksizes are unusual
Date: Mon, 2 Jan 2023 23:24:01 +0000
[Message part 1 (text/plain, inline)]
On 02/01/2023 23:18, Paul Eggert wrote:
> On 2023-01-02 15:03, Pádraig Brady wrote:
>> 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?
> 
> Sure, that looks good, thanks.

I forgot about documenting the reinstated I/O block size multiple constraint,
which I'll do with the attached.

cheers,
Pádraig
[copy-doc-io-size.patch (text/x-patch, attachment)]

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Tue, 31 Jan 2023 12:24:09 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 85 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.