[PATCH] dd: work around buffer length restrictions with oflag=direct (O_DIRECT)

[Jim Meyering]

Eric Sandeen mentioned that dd's O_DIRECT-exposing code
didn't always work.  The problem is that the kernel imposes
draconian restrictions on the size of buffer that one may
write to an FD opened with O_DIRECT.  Currently, at least on
ext4 and with a recent linux kernel, that buffer size must
be a multiple of 512, which happens to be dd's default buffer size.

This means you may well copy most of an 8GB iso with
dd bs=2048 iflag=direct oflag=direct if=/dev/sr0 of=foo.iso
but if the image size is not a multiple of 512 bytes,
the final write will fail with EINVAL.

That can be frustrating.

Here's a tiny example:

    $ echo > in; /bin/dd if=in oflag=direct of=out
    /bin/dd: writing `out': Invalid argument
    ...
    [Exit 1]

Use a size > 512, to get the same failure via a different code path:

    $ truncate -s 513 in; /cu/src/dd if=in oflag=direct of=out
    /bin/dd: writing `out': Invalid argument
    ...
    [Exit 1]

Initially I changed the iwrite function to
write output_blocksize bytes for oflag=direct,
and then ftruncate back to the desired size:

+  if ((output_flags & O_DIRECT) && size < output_blocksize)
+    {
+      off_t cur_pos = lseek (STDOUT_FILENO, 0, SEEK_CUR);
+      if (0 < cur_pos)
+        {
+          ssize_t nwritten = write (fd, buf, output_blocksize);
+          if (nwritten < output_blocksize)
+            return nwritten;
+          cur_pos += size;
+          if (ftruncate (STDOUT_FILENO, cur_pos) != 0)
+            error (EXIT_FAILURE, errno,
+                   _("failed to truncate to %"PRIuMAX" bytes in output file 
%s"),
+                   cur_pos, quote (output_file));
+          return size;
+        }
+    }
+
   while (total_written < size)
     {
       ssize_t nwritten;

But I didn't like writing beyond the desired length,
even briefly, so was glad to realize there's a better way:
turn off O_DIRECT for that final write:

The above discusses only *output* flags, oflags=direct.
So far I haven't looked much at iflags=odirect,
but the few tests I've run suggest that it has no such problem.

Note also that the code makes no attempt to determine the
appropriate block size.  So if you chose obs=42, you lose.
The adventurous user who experiments with oflags=direct
must also select a block size that works with O_DIRECT
and the destination file system.

Here's the patch I expect to push soon:

>From 2f4004be6131e049a1452ee68377ae1756673bd4 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 4 Aug 2009 19:54:58 +0200
Subject: [PATCH] dd: work around buffer length restrictions with oflag=direct 
(O_DIRECT)

* src/dd.c (iwrite): Turn off O_DIRECT for any
smaller-than-obs-sized write.  Don't bother to restore it.
* tests/dd/direct: New test for the above.
* tests/Makefile.am (TESTS): Add od/direct.
* doc/coreutils.texi (dd invocation): Mention oflags=direct
buffer size restriction.
Reported by Eric Sandeen.
---
 doc/coreutils.texi |    4 ++++
 src/dd.c           |   10 +++++++++-
 tests/Makefile.am  |    1 +
 tests/dd/direct    |   40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 1 deletions(-)
 create mode 100755 tests/dd/direct

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index acec76e..6fa2602 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -7861,6 +7861,10 @@ dd invocation
 @opindex direct
 @cindex direct I/O
 Use direct I/O for data, avoiding the buffer cache.
+Note that the kernel may impose restrictions on read or write buffer sizes.
+For example, with an ext4 destination file system and a linux-based kernel,
+using @samp{oflags=direct} will cause writes to fail with @code{EINVAL} if the
+output buffer size is not a multiple of 512.

 @item directory
 @opindex directory
diff --git a/src/dd.c b/src/dd.c
index 9a9d22a..43ad718 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -837,6 +837,14 @@ iwrite (int fd, char const *buf, size_t size)
 {
   size_t total_written = 0;

+  if ((output_flags & O_DIRECT) && size < output_blocksize)
+    {
+      int old_flags = fcntl (STDOUT_FILENO, F_GETFL);
+      if (fcntl (STDOUT_FILENO, F_SETFL, old_flags & ~O_DIRECT) != 0)
+        error (0, errno, _("failed to turn off O_DIRECT: %s"),
+               quote (output_file));
+    }
+
   while (total_written < size)
     {
       ssize_t nwritten;
@@ -1897,7 +1905,7 @@ main (int argc, char **argv)
                  || S_ISDIR (stdout_stat.st_mode)
                  || S_TYPEISSHM (&stdout_stat))
                error (EXIT_FAILURE, ftruncate_errno,
-                      _("truncating at %"PRIuMAX" bytes in output file %s"),
+                   _("failed to truncate to %"PRIuMAX" bytes in output file 
%s"),
                       size, quote (output_file));
            }
        }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ad19d02..6ad6133 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -298,6 +298,7 @@ TESTS =                                             \
   cp/src-base-dot                              \
   cp/symlink-slash                             \
   cp/thru-dangling                             \
+  dd/direct                                    \
   dd/misc                                      \
   dd/not-rewound                               \
   dd/reblock                                   \
diff --git a/tests/dd/direct b/tests/dd/direct
new file mode 100755
index 0000000..7e80bee
--- /dev/null
+++ b/tests/dd/direct
@@ -0,0 +1,40 @@
+#!/bin/sh
+# ensure that dd's oflag=direct works
+
+# 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
+  dd --version
+fi
+
+. $srcdir/test-lib.sh
+
+truncate -s 8192 in || framework_failure
+dd if=in oflag=direct of=out 2> /dev/null \
+  || skip_test_ 'this file system lacks support for O_DIRECT'
+
+truncate -s 511 short || framework_failure
+truncate -s 8191 m1 || framework_failure
+truncate -s 8193 p1 || framework_failure
+
+fail=0
+for i in short m1 p1; do
+  rm -f out
+  dd if=$i oflag=direct of=out || fail=1
+done
+
+Exit $fail
--
1.6.4.70.g9c084