]> git.ipfire.org Git - thirdparty/coreutils.git/commitdiff
dd: fix nocache regions passed to posix_fadvise()
authorPádraig Brady <P@draigBrady.com>
Wed, 11 Oct 2017 06:29:08 +0000 (23:29 -0700)
committerPádraig Brady <P@draigBrady.com>
Wed, 25 Oct 2017 02:38:12 +0000 (19:38 -0700)
Previously with oflag=direct the call to invalidate_cache()
was not passed to the kernel, as it was less than a page size,
and a subsequent call was not made to invalidate the pending space.
Similarly with oflag=nocache the pending space at EOF was
not invalidated.  Even though these amount to only a single page
in the page cache it can be significant.  For example on
XFS before kernel patch v4.9-rc1-4-g0ee7a3f, O_DIRECT files
would have been read inefficiently if any pages were cached,
even if they were already synced to storage.

* src/dd.c (i_nocache_eof, o_nocache_eof): New bools used
to control when we want invalidate_cache(,0) to clear to EOF.
(cache_round): Use IO_BUFSIZE (currently 132KiB) to minimize
calls to the relatively expensive advise function, rather
than page_size.  This also makes it clear that while the
kernel function operates on pages, this size is chosen for
performance reasons.
(invalidate_cache): Refactor to share more code between
input and output paths. Use i_nocache_eof and o_nocache_eof
rather than proxying off max_records.  Ensure we
invalidate full pages when clearing to EOF as the kernel
will ignore any non complete pages.  Fix the offset used
for the output path.
(dd_copy): Invalidate the cache of the input after the
offset is updated, for consistency and so we don't try to
invalidate before the start of the file.  When we read
EOF on input, set flags so that we invalidate to EOF.
(main): Invalidate to EOF in more cases, by depending
on the i_nocache_eof and o_nocache_eof flags.
* doc/coreutils.texi (dd invocation): Clarify the alignment
and persisted caveats on the example applying "nocache"
to part of a file.
* tests/dd/nocache_eof.sh: A new test.
* tests/local.mk: Reference the new test.
* NEWS: Mention the bug fix.
Issue reported by Eric Bergen.

NEWS
THANKS.in
doc/coreutils.texi
src/dd.c
tests/dd/nocache_eof.sh [new file with mode: 0755]
tests/local.mk

diff --git a/NEWS b/NEWS
index 7a163c72a804df4a09dc24e440c2c5a2ffb436fc..806e3efd681877a9a10b223ce1f4742fcecf5b69 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,11 @@ GNU coreutils NEWS                                    -*- outline -*-
   b2sum no longer crashes when processing certain truncated check files.
   [bug introduced with b2sum coreutils-8.26]
 
+  dd now ensures the correct cache ranges are specified for the "nocache"
+  and "direct" flags.  Previously some pages in the page cache were not
+  invalidated.  [bug introduced for "direct" in coreutils-7.5,
+  and with the "nocache" implementation in coreutils-8.11]
+
   ptx -S no longer infloops for a pattern which returns zero-length matches.
   [the bug dates back to the initial implementation]
 
index b5e11f0a461ac61b00a09225b87ad48679cbf24d..46d78ace22953b642224dfa429fbf34687d9a9a4 100644 (file)
--- a/THANKS.in
+++ b/THANKS.in
@@ -192,6 +192,7 @@ Eli Zaretskii                       eliz@is.elta.co.il
 Emile LeBlanc                       leblanc@math.toronto.edu
 Emmanuel Lacour                     elacour@home-dn.net
 Eric Backus                         ericb@lsid.hp.com
+Eric Bergen                         eric.bergen@gmail.com
 Eric G. Miller                      egm2@jps.net
 Eric Pemente                        pemente@northpark.edu
 Eric S. Raymond                     esr@snark.thyrsus.com
index 47a9f6d253005d1c2decfd8392777ba230947b56..70e2b76c5d35ca9f1a6e96038fbbce1ea62706d8 100644 (file)
@@ -9187,7 +9187,9 @@ dd if=ifile iflag=nocache count=0
 # Ensure drop cache for the whole file
 dd of=ofile oflag=nocache conv=notrunc,fdatasync count=0
 
-# Drop cache for part of file
+# Advise to drop cache for part of file
+# Note the kernel will only consider complete and
+# already persisted pages.
 dd if=ifile iflag=nocache skip=10 count=10 of=/dev/null
 
 # Stream data using just the read-ahead cache.
index 8e3224a2174e2b33a2c85feeffe1e06ae6af6602..b6ceb4fd7cc6af18c6227ccc86c4cc8b4c34a4cc 100644 (file)
--- a/src/dd.c
+++ b/src/dd.c
@@ -31,6 +31,7 @@
 #include "fd-reopen.h"
 #include "gethrxtime.h"
 #include "human.h"
+#include "ioblksize.h"
 #include "long-options.h"
 #include "quote.h"
 #include "verror.h"
@@ -266,6 +267,9 @@ static sig_atomic_t volatile info_signal_count;
 /* Whether to discard cache for input or output.  */
 static bool i_nocache, o_nocache;
 
+/* Whether to instruct the kernel to discard the complete file.  */
+static bool i_nocache_eof, o_nocache_eof;
+
 /* Function used for read (to handle iflag=fullblock parameter).  */
 static ssize_t (*iread_fnc) (int fd, char *buf, size_t size);
 
@@ -1000,7 +1004,8 @@ quit (int code)
   exit (code);
 }
 
-/* Return LEN rounded down to a multiple of PAGE_SIZE
+/* Return LEN rounded down to a multiple of IO_BUFSIZE
+   (to minimize calls to the expensive posix_fadvise(,POSIX_FADV_DONTNEED),
    while storing the remainder internally per FD.
    Pass LEN == 0 to get the current remainder.  */
 
@@ -1013,7 +1018,7 @@ cache_round (int fd, off_t len)
   if (len)
     {
       uintmax_t c_pending = *pending + len;
-      *pending = c_pending % page_size;
+      *pending = c_pending % IO_BUFSIZE;
       if (c_pending > *pending)
         len = c_pending - *pending;
       else
@@ -1033,54 +1038,64 @@ static bool
 invalidate_cache (int fd, off_t len)
 {
   int adv_ret = -1;
+  off_t offset;
+  bool nocache_eof = (fd == STDIN_FILENO ? i_nocache_eof : o_nocache_eof);
 
   /* Minimize syscalls.  */
   off_t clen = cache_round (fd, len);
   if (len && !clen)
     return true; /* Don't advise this time.  */
-  if (!len && !clen && max_records)
-    return true; /* Nothing pending.  */
+  else if (! len && ! clen && ! nocache_eof)
+    return true;
   off_t pending = len ? cache_round (fd, 0) : 0;
 
   if (fd == STDIN_FILENO)
     {
       if (input_seekable)
+        offset = input_offset;
+      else
         {
-          /* Note we're being careful here to only invalidate what
-             we've read, so as not to dump any read ahead cache.  */
-#if HAVE_POSIX_FADVISE
-            adv_ret = posix_fadvise (fd, input_offset - clen - pending, clen,
-                                     POSIX_FADV_DONTNEED);
-#else
-            errno = ENOTSUP;
-#endif
+          offset = -1;
+          errno = ESPIPE;
         }
-      else
-        errno = ESPIPE;
     }
-  else if (fd == STDOUT_FILENO)
+  else
     {
       static off_t output_offset = -2;
 
       if (output_offset != -1)
         {
-          if (0 > output_offset)
-            {
-              output_offset = lseek (fd, 0, SEEK_CUR);
-              output_offset -= clen + pending;
-            }
-          if (0 <= output_offset)
-            {
+          if (output_offset < 0)
+            output_offset = lseek (fd, 0, SEEK_CUR);
+          else if (len)
+            output_offset += clen + pending;
+        }
+
+      offset = output_offset;
+    }
+
+  if (0 <= offset)
+   {
+     if (! len && clen && nocache_eof)
+       {
+         pending = clen;
+         clen = 0;
+       }
+
+     /* Note we're being careful here to only invalidate what
+        we've read, so as not to dump any read ahead cache.
+        Note also the kernel is conservative and only invalidates
+        full pages in the specified range.  */
 #if HAVE_POSIX_FADVISE
-              adv_ret = posix_fadvise (fd, output_offset, clen,
-                                       POSIX_FADV_DONTNEED);
+     offset = offset - clen - pending;
+     /* ensure full page specified when invalidating to eof.  */
+     if (clen == 0)
+       offset -= offset % page_size;
+     adv_ret = posix_fadvise (fd, offset, clen, POSIX_FADV_DONTNEED);
 #else
-              errno = ENOTSUP;
+     errno = ENOTSUP;
 #endif
-              output_offset += clen + pending;
-            }
-        }
-    }
+   }
 
   return adv_ret != -1 ? true : false;
 }
@@ -1168,15 +1183,19 @@ iwrite (int fd, char const *buf, size_t size)
                quotef (output_file));
 
       /* Since we have just turned off O_DIRECT for the final write,
-         here we try to preserve some of its semantics.  First, use
-         posix_fadvise to tell the system not to pollute the buffer
-         cache with this data.  Don't bother to diagnose lseek or
-         posix_fadvise failure. */
+         we try to preserve some of its semantics.  */
+
+      /* Call invalidate_cache() to setup the appropriate offsets
+         for subsequent calls.  */
+      o_nocache_eof = true;
       invalidate_cache (STDOUT_FILENO, 0);
 
       /* Attempt to ensure that that final block is committed
          to disk as quickly as possible.  */
       conversions_mask |= C_FSYNC;
+
+      /* After the subsequent fsync() we'll call invalidate_cache()
+         to attempt to clear all data from the page cache.  */
     }
 
   while (total_written < size)
@@ -1562,11 +1581,13 @@ scanargs (int argc, char *const *argv)
   if (input_flags & O_NOCACHE)
     {
       i_nocache = true;
+      i_nocache_eof = (max_records == 0 && max_bytes == 0);
       input_flags &= ~O_NOCACHE;
     }
   if (output_flags & O_NOCACHE)
     {
       o_nocache = true;
+      o_nocache_eof = (max_records == 0 && max_bytes == 0);
       output_flags &= ~O_NOCACHE;
     }
 }
@@ -2149,13 +2170,19 @@ dd_copy (void)
       else
         nread = iread_fnc (STDIN_FILENO, ibuf, input_blocksize);
 
-      if (nread >= 0 && i_nocache)
-        invalidate_cache (STDIN_FILENO, nread);
-
-      if (nread == 0)
-        break;                 /* EOF.  */
-
-      if (nread < 0)
+      if (nread > 0)
+        {
+          advance_input_offset (nread);
+          if (i_nocache)
+            invalidate_cache (STDIN_FILENO, nread);
+        }
+      else if (nread == 0)
+        {
+          i_nocache_eof |= i_nocache;
+          o_nocache_eof |= o_nocache && ! (conversions_mask & C_NOTRUNC);
+          break;                       /* EOF.  */
+        }
+      else
         {
           if (!(conversions_mask & C_NOERROR) || status_level != STATUS_NONE)
             error (0, errno, _("error reading %s"), quoteaf (input_file));
@@ -2194,7 +2221,6 @@ dd_copy (void)
         }
 
       n_bytes_read = nread;
-      advance_input_offset (nread);
 
       if (n_bytes_read < input_blocksize)
         {
@@ -2470,13 +2496,12 @@ main (int argc, char **argv)
           exit_status = EXIT_FAILURE;
         }
     }
-  else if (max_records != (uintmax_t) -1)
+  else
     {
-      /* Invalidate any pending region less than page size,
-         in case the kernel might round up.  */
-      if (i_nocache)
+      /* Invalidate any pending region or to EOF if appropriate.  */
+      if (i_nocache || i_nocache_eof)
         invalidate_cache (STDIN_FILENO, 0);
-      if (o_nocache)
+      if (o_nocache || o_nocache_eof)
         invalidate_cache (STDOUT_FILENO, 0);
     }
 
diff --git a/tests/dd/nocache_eof.sh b/tests/dd/nocache_eof.sh
new file mode 100755 (executable)
index 0000000..96d5988
--- /dev/null
@@ -0,0 +1,98 @@
+#!/bin/sh
+# Ensure dd invalidates to EOF when appropriate
+
+# Copyright (C) 2017 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 <https://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ dd
+require_strace_ fadvise64
+
+head -c1234567 /dev/zero > in.f || framework_failure_
+
+# Check basic operation or skip.
+# We could check for 'Operation not supported' error here,
+# but that was seen to be brittle. HPUX returns ENOTTY for example.
+# So assume that if this basic operation fails, it's due to lack
+# of support by the system.
+dd if=in.f iflag=nocache count=0 ||
+  skip_ 'this file system lacks support for posix_fadvise()'
+
+strace_dd() {
+  strace -o dd.strace -e fadvise64 dd status=none "$@" || fail=1
+}
+
+advised_to_eof() {
+  grep -F ' 0, POSIX_FADV_DONTNEED' dd.strace >/dev/null
+}
+
+# The commented fadvise64 calls are what are expected with
+# a 4KiB page size and 128KiB IO_BUFSIZE.
+
+strace_dd if=in.f of=out.f bs=1M oflag=direct
+#The first call is redundant but inconsequential
+#fadvise64(1, 1048576, 0, POSIX_FADV_DONTNEED) = 0
+#fadvise64(1, 1048576, 0, POSIX_FADV_DONTNEED) = 0
+advised_to_eof || fail=1
+
+strace_dd if=in.f of=out.f bs=1M oflag=nocache,sync
+#fadvise64(1, 0, 1048576, POSIX_FADV_DONTNEED) = 0
+#fadvise64(1, 1048576, 131072, POSIX_FADV_DONTNEED) = 0
+#fadvise64(1, 1179648, 0, POSIX_FADV_DONTNEED) = 0
+advised_to_eof || fail=1
+
+strace_dd if=in.f count=0 iflag=nocache
+#fadvise64(0, 0, 0, POSIX_FADV_DONTNEED) = 0
+advised_to_eof || fail=1
+
+strace_dd if=in.f of=/dev/null iflag=nocache skip=10 count=300
+#fadvise64(0, 5120, 131072, POSIX_FADV_DONTNEED) = 0
+#fadvise64(0, 136192, 22528, POSIX_FADV_DONTNEED) = 0
+returns_ 1 advised_to_eof || fail=1
+
+strace_dd if=in.f of=/dev/null iflag=nocache bs=1M count=3000
+#fadvise64(0, 0, 1048576, POSIX_FADV_DONTNEED) = 0
+#fadvise64(0, 1048576, 131072, POSIX_FADV_DONTNEED) = 0
+#fadvise64(0, 1179648, 0, POSIX_FADV_DONTNEED) = 0
+advised_to_eof || fail=1
+
+strace_dd if=in.f of=/dev/null bs=1M count=1 iflag=nocache
+#fadvise64(0, 0, 1048576, POSIX_FADV_DONTNEED) = 0
+returns_ 1 advised_to_eof || fail=1
+
+strace_dd if=in.f of=out.f bs=1M iflag=nocache oflag=nocache,sync
+#fadvise64(0, 0, 1048576, POSIX_FADV_DONTNEED) = 0
+#fadvise64(1, 0, 1048576, POSIX_FADV_DONTNEED) = 0
+#fadvise64(0, 1048576, 131072, POSIX_FADV_DONTNEED) = 0
+#fadvise64(1, 1048576, 131072, POSIX_FADV_DONTNEED) = 0
+#fadvise64(0, 1179648, 0, POSIX_FADV_DONTNEED) = 0
+#fadvise64(1, 1179648, 0, POSIX_FADV_DONTNEED) = 0
+advised_to_eof || fail=1
+
+# Ensure sub page size offsets are handled.
+# I.e., only page aligned offsets are sent to fadvise.
+if ! strace -o dd.strace -e fadvise64 dd status=none \
+ if=in.f of=out.f bs=1M oflag=direct seek=512 oflag=seek_bytes 2>err; then
+  # older XFS had a page size alignment requirement
+  echo "dd: error writing 'out.f': Invalid argument" > err_ok
+  compare err_ok err || fail=1
+else
+  #The first call is redundant but inconsequential
+  #fadvise64(1, 1048576, 0, POSIX_FADV_DONTNEED) = 0
+  #fadvise64(1, 1048576, 0, POSIX_FADV_DONTNEED) = 0
+  advised_to_eof || fail=1
+fi
+
+Exit $fail
index 42e247e6f2ec6ed0449e887f0012a0ebe53d3f60..74426f6085ffb058e7f2747085bff567f8ca0907 100644 (file)
@@ -511,6 +511,7 @@ all_tests =                                 \
   tests/dd/misc.sh                             \
   tests/dd/no-allocate.sh                      \
   tests/dd/nocache.sh                          \
+  tests/dd/nocache_eof.sh                      \
   tests/dd/not-rewound.sh                      \
   tests/dd/reblock.sh                          \
   tests/dd/skip-seek.pl                                \