[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: cp(1) fails to copy file from /proc
From: |
Jim Meyering |
Subject: |
Re: cp(1) fails to copy file from /proc |
Date: |
Sat, 18 Apr 2009 22:58:52 +0200 |
Mike Frysinger wrote:
> On Friday 17 April 2009 18:28:07 James Youngman wrote:
...
>> The patch itself looks good, but it might be worth leaving in a
>> comment indicating why the optimisation should not be reintroduced...
>
> and/or a new test (i prefer the "and"):
> if [ -e /proc/cpuinfo ] ; then
> cp /proc/cpuinfo cpuinfo.cp
> cat /proc/cpuinfo > cpuinfo.cat
> cmp cpuinfo.cp cpuinfo.cat
> fi
Of course ;-)
As promised, I've added a test for this below.
We can't use /proc/cpuinfo, at least not precisely like that,
because its cpu speed line can change due to frequency scaling.
Also, that file is usually too small to trigger the failure.
Here's a more complete patch, with a title and NEWS reflecting
that I now think it's a linux kernel bug.
I'll wait a few days before pushing, in case I learn otherwise.
>From a248490b206eca42b9018e596f1c7a234566838a Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 17 Apr 2009 18:44:18 +0200
Subject: [PATCH] cp: work around linux kernel bug: short-read != EOF on /proc
Remove the optimization that avoided up to 50% of cp's read syscalls.
Do not assume that a short read on a regular file indicates EOF.
When reading from a file in /proc on linux [at least 2.6.9 - 2.6.29]
into a 4k-byte buffer or larger, a short read does not
always indicate EOF. For example, "cp /proc/slabinfo /tmp"
copies only 4068 of the total 7493 bytes. This optimization
(25719a33154f0c62ea9881f0c79ae312dd4cec7a, Improve performance a bit
by optimizing away; 2005-11-24) appears to have been worth less than
a 2% speed-up (and usually much less), so the impact of removing it
is negligible.
* src/copy.c (copy_reg): Don't exit the loop early.
* tests/cp/proc-short-read: New test, lightly based on a suggestion
from Mike Frysinger, to exercise this fix.
* tests/Makefile.am (TESTS): Add cp/proc-short-read.
* NEWS (Improve robustness): Mention this change.
---
NEWS | 12 ++++++++++++
src/copy.c | 7 ++++---
tests/Makefile.am | 1 +
tests/cp/proc-short-read | 45 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 62 insertions(+), 3 deletions(-)
create mode 100755 tests/cp/proc-short-read
diff --git a/NEWS b/NEWS
index 5951bb5..18db634 100644
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,18 @@ GNU coreutils NEWS -*-
outline -*-
default should proceed at the speed of the disk. Previously /dev/urandom
was used if available, which is relatively slow on GNU/Linux systems.
+** Improved robustness
+
+ cp would exit successfully after copying less than the full contents
+ of a file larger than ~4000 bytes from a linux-/proc file system to a
+ destination file system with a fundamental block size of 4KiB or greater.
+ Reading into a 4KiB-or-larger buffer, cp's "read" syscall would return
+ a value smaller than 4096, and cp would interpret that as EOF (POSIX
+ allows this). This optimization, now removed, saved 50% of cp's read
+ syscalls when copying small files. Affected linux kernels: at least
+ 2.6.9 through 2.6.29.
+ [the optimization was introduced in coreutils-6.0]
+
** Portability
`id -G $USER` now works correctly even on Darwin and NetBSD. Previously it
diff --git a/src/copy.c b/src/copy.c
index 9b0e139..c45224c 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -700,9 +700,10 @@ copy_reg (char const *src_name, char const *dst_name,
}
last_write_made_hole = false;
- /* A short read on a regular file means EOF. */
- if (n_read != buf_size && S_ISREG (src_open_sb.st_mode))
- break;
+ /* It is tempting to return early here upon a short read from a
+ regular file. That would save the final read syscall for each
+ file. Unfortunately that doesn't work for certain files in
+ /proc with linux kernels from at least 2.6.9 .. 2.6.29. */
}
}
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 8ce6a21..c02f2de 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -281,6 +281,7 @@ TESTS = \
cp/parent-perm-race \
cp/perm \
cp/preserve-2 \
+ cp/proc-short-read \
cp/proc-zero-len \
cp/r-vs-symlink \
cp/same-file \
diff --git a/tests/cp/proc-short-read b/tests/cp/proc-short-read
new file mode 100755
index 0000000..e06143c
--- /dev/null
+++ b/tests/cp/proc-short-read
@@ -0,0 +1,45 @@
+#!/bin/sh
+# exercise cp's short-read failure when operating on >4KB files in /proc
+
+# Copyright (C) 2009 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+if test "$VERBOSE" = yes; then
+ set -x
+ cp --version
+fi
+
+. $srcdir/test-lib.sh
+
+fail=0
+kall=/proc/kallsyms
+
+test -r $kall || skip_test_ "your system lacks $kall"
+
+# Before coreutils-7.3, cp would copy less than 4KiB of this 1MB+ file.
+cp $kall 1 || fail=1
+cat $kall > 2 || fail=1
+compare 1 2 || fail=1
+
+# Also check md5sum, just for good measure.
+md5sum $kall > 3 || fail=1
+md5sum 2 > 4 || fail=1
+
+# Remove each file name before comparing checksums.
+sed 's/ .*//' 3 > sum.proc || fail=1
+sed 's/ .*//' 4 > sum.2 || fail=1
+compare sum.proc sum.2 || fail=1
+
+Exit $fail
--
1.6.3.rc0.230.g3edd6
- cp(1) fails to copy file from /proc, Jukka Salmi, 2009/04/16
- Re: cp(1) fails to copy file from /proc, Jim Meyering, 2009/04/16
- Re: cp(1) fails to copy file from /proc, Jim Meyering, 2009/04/16
- Re: cp(1) fails to copy file from /proc, James Youngman, 2009/04/17
- Re: cp(1) fails to copy file from /proc, Jim Meyering, 2009/04/17
- Re: cp(1) fails to copy file from /proc, James Youngman, 2009/04/17
- Re: cp(1) fails to copy file from /proc, Mike Frysinger, 2009/04/18
- Re: cp(1) fails to copy file from /proc,
Jim Meyering <=
- Re: cp(1) fails to copy file from /proc, Mike Frysinger, 2009/04/18
- Re: cp(1) fails to copy file from /proc, Jim Meyering, 2009/04/18
- Re: cp(1) fails to copy file from /proc, Andreas Schwab, 2009/04/19
- Re: cp(1) fails to copy file from /proc, Jim Meyering, 2009/04/19
- Re: cp(1) fails to copy file from /proc, James Youngman, 2009/04/19
- Re: cp(1) fails to copy file from /proc, Jim Meyering, 2009/04/23
- Re: cp(1) fails to copy file from /proc, Mike Frysinger, 2009/04/26