On 29/01/11 09:47, Jim Meyering wrote: > Jim Meyering wrote: >> Jeff liu wrote: >>> Now make check passed against the following combination: >>> 1. Refresh installed host in Ubuntu10.0.4, >>> filefrag comes from E2fsprogs 1.41.11 && Kernel: 2.6.32-16 >>> 2. filefrag in e2fsprogs-1.4.12 && kernel-2.6.36. >> [passes] >> >> Glad to here it passes for you, now. >> FYI, I have spent pretty much time on cp over the last >> couple days, factoring out the hole-inducing code and >> making extent_copy use it. Part of the motivation was >> to fix cp --sparse=always, which was broken on the branch. >> It would not induce holes when going through extent_copy. >> I've added a couple more tests and will post the series as >> soon I've cleaned things up a little more. > > Here are 9 more patches, just pushed to the fiemap-copy-2 branch: > > http://git.savannah.gnu.org/cgit/coreutils.git/log/?h=fiemap-copy-2 > > The first and last add tests, and the others consolidate, > clean up, and fix a few bugs. > > 1/9 tests: ensure that FIEMAP-enabled cp copies a sparse file efficiently > Ensure that copying a sparse 1TiB file completes in less than 3 seconds > That can only succeed with FIEMAP (or --reflink=, which is off by default) > > 2/9 fiemap copy: rename some locals > The _logical suffix was not useful. Change it to _start > > 3/9 fiemap copy: simplify post-loop logic; improve comments > > 4/9 fiemap copy: avoid a performance hit due to very small buffer > I didn't measure this, but once you see it, it's an obvious bug. > Using an arbitrarily small buffer size is bound to cause trouble. > > 5/9 fiemap copy: avoid leak-on-error > Failing from within the loop, we have to free the extent buffer. > > 6/9 copy: factor sparse-copying code into its own function, because > we're going to have to use it from within extent_copy, too. > I realized that cp --sparse=always could no longer create holes > in the destination. Factoring this out is the first step. > > 7/9 copy: remove obsolete comment > unrelated to the rest, but hard to pull out since it's in moved code > > 8/9 copy: make extent_copy use sparse_copy, rather than its own code > Now that sparse_copy is separate, and used by copy_reg, adapt it > so that it can also be used by extent_copy. > > 9/9 tests: cp/fiemap: exercise previously-failing parts > This is a hole-inducing test that would have failed with previous > fiemap-based copying code. > > I may change or remove the sparse_copy_finalize function, which just calls > ftruncate, especially now that it's used from only one place (initially > I was using it from each sparse_copy caller, but that didn't work out), > and don't particularly like the added lseek call that is performed for > each file copied, but keeping track of total written/offset byte counts > and inflicting the need to do that on both callers seems like too much > added code/complexity to justify avoiding that single lseek call. > >>From 8e4f0efd3ad17f1dd7a561369da22dfaf43ab3e8 Mon Sep 17 00:00:00 2001 > From: Jim Meyering <[email protected]> > Date: Fri, 28 Jan 2011 22:31:23 +0100 > Subject: [PATCH 1/9] tests: ensure that FIEMAP-enabled cp copies a sparse > file efficiently > > * tests/cp/fiemap-perf: New file. > * tests/Makefile.am (TESTS): Add it. > --- > tests/Makefile.am | 1 + > tests/cp/fiemap-perf | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 33 insertions(+), 0 deletions(-) > create mode 100755 tests/cp/fiemap-perf > > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 847f181..7855ac5 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -320,6 +320,7 @@ TESTS = \ > cp/dir-vs-file \ > cp/existing-perm-race \ > cp/fail-perm \ > + cp/fiemap-perf \ > cp/file-perm-race \ > cp/into-self \ > cp/link \ > diff --git a/tests/cp/fiemap-perf b/tests/cp/fiemap-perf > new file mode 100755 > index 0000000..429e59b > --- /dev/null > +++ b/tests/cp/fiemap-perf > @@ -0,0 +1,32 @@ > +#!/bin/sh > +# ensure that a sparse file is copied efficiently, by default > + > +# Copyright (C) 2011 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/>. > + > +. "${srcdir=.}/init.sh"; path_prepend_ ../src > +print_ver_ cp > + > +# Require a fiemap-enabled FS. > +df -T -t btrfs -t xfs -t ext4 -t ocfs2 . \ > + || skip_ "this file system lacks FIEMAP support" > + > +# Create a large-but-sparse file. > +timeout 1 dd bs=1 seek=1T of=f < /dev/null || framework_failure_ > + > +# Nothing can read (much less write) that many bytes in so little time. > +timeout 3 cp f f2 || framework_failure_
I'm a bit worried with a 1s timeout. The following will only give false negatives over 100GB/s timeout 10 truncate -s1T f || framework_failure_ timeout 10 cp f f2 || framework_failure_ I wouldn't worry about filling file systems either, as we're already limiting to ext4 etc. >>From 7f154dcfc5641c9616921d4c5ac5005bcb2507eb Mon Sep 17 00:00:00 20: Jim >>Meyering <[email protected]> > Date: Thu, 27 Jan 2011 15:17:42 +0100 > Subject: [PATCH 9/9] tests: cp/fiemap: exercise previously-failing parts > +# Ensure that --sparse=always can restore holes. > +rm -f k > +# Create a file starting with an "x", followed by 257K-1 0 bytes. > +printf x > k || framework_failure_ > +dd bs=1k seek=1 of=k count=255 < /dev/zero || framework_failure_ S/257/256/ ?
