Subject: Re: bug#21790: [PATCH] coreutils/cp: handle EOF extents correctly
From: Pádraig Brady <[email-masked]>
To: Dmitry Monakhov <[email-masked]>, [email-masked]
Cc: [email-masked], [email-masked]
Date: Fri, 20 Nov 2015 18:26:57 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

On 30/10/15 18:54, Pádraig Brady wrote:
> On 30/10/15 16:57, Pádraig Brady wrote:
>> On 30/10/15 09:02, Dmitry Monakhov wrote:
>>> fallocate can allocate extens beyond EOF via FALLOC_FL_KEEP_SIZE.
>>> Currenly sparse engine tries to copy such extents which is wrong and
>>> result in silent data corruption (leave file with incorrect size).
>>>
>>> ##TESTCASE
>>> echo blabla > sparse_falloc.in
>>> truncate -s 2M sparse_falloc.in
>>> fallocate -n -o 4M -l 1M sparse_falloc.in
>>> cp sparse_falloc.in sparse_falloc.out
>>> cmp sparse_falloc.in sparse_falloc.out
>>
>> Ouch.  Thanks for the analysis and patch.
>> It looks correct.  I'll analyze further before applying.
> 
> This doesn't handle the --sparse==never case
> (which is broken with and without the patch).
> 
> Also one might have an extent spanning the file size boundary,
> in which this patch could miss the remaining data?
> 
> Also currently if the source file is being extended
> while being copied, we continue to read, whereas we now wont.
> Theoretically this is an issue when st_size doesn't match
> what's available to be read, though maybe not a practical issue
> since we don't use this path for a zero st_size, nor
> sources that don't support fiemap anyway.
> 
> This might be an opportune time to rip out the fiemap stuff
> in favor of SEEK{DATA,HOLE} anyway, which I intended to do
> during this cycle.  For fix backporting sake though,
> we should apply the minimal fix to the fiemap code first.
> 
> Attached is the current minimally tested patch.

I'll apply the attached in your name soon.
Please review.

thanks,
Pádraig.
From f79f3a83e4e43b70d2a209e6d1ec8185dac16625 Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <[email-masked]>
Date: Fri, 30 Oct 2015 22:04:46 +0000
Subject: [PATCH] copy: fix copying of extents beyond the apparent file size

fallocate can allocate extents beyond EOF via FALLOC_FL_KEEP_SIZE.
Where there is a gap (hole) between the extents, and EOF is within
that gap, the final hole wasn't reproduced, resulting in silent
data corruption in the copied file (size too small).

* src/copy.c (extent_copy): Ensure we don't process extents
beyond the apparent file size, since processing and allocating
those is not currently supported.
* tests/cp/fiemap-extents.sh: Rename from tests/cp/fiemap-empty.sh
and renable parts checking the extents at and beyond EOF.
* tests/local.mk: Reference the renamed test.
* NEWS: Mention the bug fix.
---
 NEWS                       |   5 +++
 src/copy.c                 |  18 +++++++-
 tests/cp/fiemap-empty.sh   | 102 ---------------------------------------------
 tests/cp/fiemap-extents.sh |  81 +++++++++++++++++++++++++++++++++++
 tests/local.mk             |   2 +-
 5 files changed, 104 insertions(+), 104 deletions(-)
 delete mode 100755 tests/cp/fiemap-empty.sh
 create mode 100755 tests/cp/fiemap-extents.sh

diff --git a/NEWS b/NEWS
index fc5e927..814e29d 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,11 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Bug fixes
 
+  cp now correctly copies files with a hole at the end of the file,
+  and extents allocated beyond the apparent size of the file.
+  That combination resulted in the trailing hole not being reproduced.
+  [bug introduced in coreutils-8.10]
+
   ls no longer prematurely wraps lines when printing short file names.
   [bug introduced in 5.1.0]
 
diff --git a/src/copy.c b/src/copy.c
index dc1cd29..4fb3fb2 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -432,6 +432,19 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size,
               ext_len = 0;
             }
 
+          /* Truncate extent to EOF.
+             Generally this will trigger with an extent starting after
+             src_total_size, and result in creating a hole or zeros until EOF.
+             Though in a file with changing extents since src_total_size
+             was determined, we might have an extent spanning that size,
+             in which case we'll only copy data up to that size.  */
+          if (src_total_size < ext_start + ext_len)
+            {
+              if (src_total_size < ext_start)
+                ext_start = src_total_size;
+              ext_len = src_total_size - ext_start;
+            }
+
           ext_hole_size = ext_start - last_ext_start - last_ext_len;
 
           wrote_hole_at_eof = false;
@@ -495,14 +508,17 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size,
               off_t n_read;
               empty_extent = false;
               last_ext_len = ext_len;
+              bool read_hole;
 
               if ( ! sparse_copy (src_fd, dest_fd, buf, buf_size,
                                   sparse_mode == SPARSE_ALWAYS ? hole_size: 0,
                                   true, src_name, dst_name, ext_len, &n_read,
-                                  &wrote_hole_at_eof))
+                                  &read_hole))
                 goto fail;
 
               dest_pos = ext_start + n_read;
+              if (n_read)
+                wrote_hole_at_eof = read_hole;
             }
 
           /* If the file ends with unwritten extents not accounted for in the
diff --git a/tests/cp/fiemap-empty.sh b/tests/cp/fiemap-empty.sh
deleted file mode 100755
index b3b2cd7..0000000
--- a/tests/cp/fiemap-empty.sh
+++ /dev/null
@@ -1,102 +0,0 @@
-#!/bin/sh
-# Test cp reads unwritten extents efficiently
-
-# Copyright (C) 2011-2015 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=.}/tests/init.sh"; path_prepend_ ./src
-print_ver_ cp
-
-# FIXME: enable any part of this test that is still relevant,
-# or, if none are relevant (now that cp does not handle unwritten
-# extents), just remove the test altogether.
-# Note also if checking allocations may need to sync first on BTRFS at least
-skip_ 'disabled for now'
-
-touch fiemap_chk
-fiemap_capable_ fiemap_chk ||
-  skip_ 'this file system lacks FIEMAP support'
-rm fiemap_chk
-
-# TODO: rather than requiring $(fallocate), possible add
-# this functionality to truncate --alloc
-fallocate --help >/dev/null || skip_ 'The fallocate utility is required'
-fallocate -l 1 -n falloc.test ||
-  skip_ 'this file system lacks FALLOCATE support'
-rm falloc.test
-
-# Require more space than we'll actually use, so that
-# tests run in parallel do not run out of space.
-# Otherwise, with inadequate space, simply running the following
-# fallocate command would induce a temporary disk-full condition,
-# which would cause failure of unrelated tests run in parallel.
-require_file_system_bytes_free_ 800000000
-
-fallocate -l 600MiB space.test ||
-  skip_ 'this test needs at least 600MiB free space'
-
-# Disable this test on old BTRFS (e.g. Fedora 14)
-# which reports ordinary extents for unwritten ones.
-filefrag space.test || skip_ 'the 'filefrag' utility is missing'
-filefrag -v space.test | grep -F 'unwritten' > /dev/null ||
-  skip_ 'this file system does not report empty extents as "unwritten"'
-
-rm space.test
-
-# Ensure we read a large empty file quickly
-fallocate -l 300MiB empty.big || framework_failure_
-timeout 3 cp --sparse=always empty.big cp.test || fail=1
-test $(stat -c %s empty.big) = $(stat -c %s cp.test) || fail=1
-rm empty.big cp.test
-
-# Ensure we handle extents beyond file size correctly.
-# Note until we support fallocate, we will not maintain
-# the file allocation.  FIXME: amend this test when fallocate is supported.
-fallocate -l 10MiB -n unwritten.withdata || framework_failure_
-dd count=10 if=/dev/urandom conv=notrunc iflag=fullblock of=unwritten.withdata
-cp unwritten.withdata cp.test || fail=1
-test $(stat -c %s unwritten.withdata) = $(stat -c %s cp.test) || fail=1
-cmp unwritten.withdata cp.test || fail=1
-rm unwritten.withdata cp.test
-
-# The following to generate unaccounted extents followed by a hole, is not
-# supported by ext4 at least. The ftruncate discards all extents not
-# accounted for in the size.
-#  fallocate -l 10MiB -n unacc.withholes
-#  dd count=10 if=/dev/urandom conv=notrunc iflag=fullblock of=unacc.withholes
-#  truncate -s20M unacc.withholes
-
-# Ensure we handle a hole after empty extents correctly.
-# Since all extents are accounted for in the size,
-# we can maintain the allocation independently from
-# fallocate() support.
-fallocate -l 10MiB empty.withholes
-truncate -s 20M empty.withholes
-sectors_per_block=$(expr $(stat -c %o .) / 512)
-cp empty.withholes cp.test || fail=1
-test $(stat -c %s empty.withholes) = $(stat -c %s cp.test) || fail=1
-# These are usually equal but can vary by an IO block due to alignment
-alloc_diff=$(expr $(stat -c %b empty.withholes) - $(stat -c %b cp.test))
-alloc_diff=$(echo $alloc_diff | tr -d -- -) # abs()
-test $alloc_diff -le $sectors_per_block || fail=1
-# Again with SPARSE_ALWAYS
-cp --sparse=always empty.withholes cp.test || fail=1
-test $(stat -c %s empty.withholes) = $(stat -c %s cp.test) || fail=1
-# cp.test should take 0 space, but allowing for some systems
-# that store default extended attributes in data blocks
-test $(stat -c %b cp.test) -le $sectors_per_block || fail=1
-rm empty.withholes cp.test
-
-Exit $fail
diff --git a/tests/cp/fiemap-extents.sh b/tests/cp/fiemap-extents.sh
new file mode 100755
index 0000000..55ec5df
--- /dev/null
+++ b/tests/cp/fiemap-extents.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+# Test cp handles extents correctly
+
+# Copyright (C) 2011-2015 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=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ cp
+
+require_sparse_support_
+
+touch fiemap_chk
+fiemap_capable_ fiemap_chk ||
+  skip_ 'this file system lacks FIEMAP support'
+rm fiemap_chk
+
+fallocate --help >/dev/null || skip_ 'The fallocate utility is required'
+touch falloc.test || framework_failure_
+fallocate -l 1 -o 0 -n falloc.test ||
+  skip_ 'this file system lacks FALLOCATE support'
+rm falloc.test
+
+# We don't currently handle unwritten extents specially
+if false; then
+# Require more space than we'll actually use, so that
+# tests run in parallel do not run out of space.
+# Otherwise, with inadequate space, simply running the following
+# fallocate command would induce a temporary disk-full condition,
+# which would cause failure of unrelated tests run in parallel.
+require_file_system_bytes_free_ 800000000
+
+fallocate -l 600MiB space.test ||
+  skip_ 'this test needs at least 600MiB free space'
+
+# Disable this test on old BTRFS (e.g. Fedora 14)
+# which reports ordinary extents for unwritten ones.
+filefrag space.test || skip_ 'the 'filefrag' utility is missing'
+filefrag -v space.test | grep -F 'unwritten' > /dev/null ||
+  skip_ 'this file system does not report empty extents as "unwritten"'
+
+rm space.test
+
+# Ensure we read a large empty file quickly
+fallocate -l 300MiB empty.big || framework_failure_
+timeout 3 cp --sparse=always empty.big cp.test || fail=1
+test $(stat -c %s empty.big) = $(stat -c %s cp.test) || fail=1
+rm empty.big cp.test
+fi
+
+# Ensure we handle extents beyond file size correctly.
+# Note until we support fallocate, we will not maintain
+# the file allocation.  FIXME: amend this test if fallocate is supported.
+# Note currently this only uses fiemap logic when the allocation (-l)
+# is smaller than the size, thus identifying the file as sparse.
+# Note the '-l 1' case is an effective noop, and just checks
+# a file with a trailing hole is copied correctly.
+for sparse_mode in always auto never; do
+  for alloc in '-l 4MiB ' '-l 1MiB -o 4MiB' '-l 1'; do
+    dd count=10 if=/dev/urandom iflag=fullblock of=unwritten.withdata
+    truncate -s 2MiB unwritten.withdata || framework_failure_
+    fallocate $alloc -n unwritten.withdata || framework_failure_
+    cp --sparse=$sparse_mode unwritten.withdata cp.test || fail=1
+    test $(stat -c %s unwritten.withdata) = $(stat -c %s cp.test) || fail=1
+    cmp unwritten.withdata cp.test || fail=1
+    rm unwritten.withdata cp.test
+  done
+done
+
+Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index adf96f0..89fdbb0 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -446,7 +446,7 @@ all_tests =					\
   tests/cp/existing-perm-dir.sh			\
   tests/cp/existing-perm-race.sh		\
   tests/cp/fail-perm.sh				\
-  tests/cp/fiemap-empty.sh			\
+  tests/cp/fiemap-extents.sh			\
   tests/cp/fiemap-FMR.sh			\
   tests/cp/fiemap-perf.sh			\
   tests/cp/fiemap-2.sh				\
-- 
2.5.0