Subject: bug#21790: closed (Re: bug#21790: [PATCH] coreutils/cp: handle EOF extents correctly)

From: [email-masked] (GNU bug Tracking System)
Date: Fri, 20 Nov 2015 18:28:02 +0000

Your bug report

#21790: [PATCH] coreutils/cp: handle EOF extents correctly

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
21790: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=21790
GNU Bug Tracking System
Contact [email-masked] with problems

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

Subject: [PATCH] coreutils/cp: handle EOF extents correctly
From: Dmitry Monakhov <[email-masked]>
To: [email-masked]
Cc: Dmitry Monakhov <[email-masked]>, [email-masked], [email-masked]
Date: Fri, 30 Oct 2015 13:02:39 +0400
List-Id: <debbugs-submit.debbugs.gnu.org>

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

Signed-off-by: Dmitry Monakhov <[email-masked]>
---
 src/copy.c                   |  7 ++++++-
 tests/cp/sparse-unwritten.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100755 tests/cp/sparse-unwritten.sh

diff --git a/src/copy.c b/src/copy.c
index edf022e..de197a4 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -430,7 +430,12 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size,
               ext_start = last_ext_start + scan.ext_info[i].ext_length;
               ext_len = 0;
             }
-
+	  /* Is this extent beyond EOF? */
+	  if (ext_start + ext_len >= src_total_size)
+	    {
+	      wrote_hole_at_eof = true;
+	      break;
+	    }
           ext_hole_size = ext_start - last_ext_start - last_ext_len;
 
           wrote_hole_at_eof = false;
diff --git a/tests/cp/sparse-unwritten.sh b/tests/cp/sparse-unwritten.sh
new file mode 100755
index 0000000..4666535
--- /dev/null
+++ b/tests/cp/sparse-unwritten.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+# Test cp for a file which has extents beyond EOF
+
+# Copyright (C) 2006-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_
+
+echo blabla > sparse_falloc.in || framework_failure_
+truncate -s 2M sparse_falloc.in || framework_failure_
+fallocate -n -o 4M -l 1M sparse_falloc.in || framework_failure_
+cp sparse_falloc.in sparse_falloc.out || fail=1
+cmp sparse_falloc.in sparse_falloc.out || fail=1
+
+Exit $fail
-- 
2.1.4