bug#59382: cp(1) tries to allocate too much memory if filesystem blocksizes are unusual

[Pádraig Brady]

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