Subject: bug#19681: [PATCH] sync: use syncfs(2) if any argument is specified

From: Pádraig Brady <[email-masked]>
Date: Wed, 28 Jan 2015 10:19:59 +0000

On 28/01/15 08:17, Bernhard Voelker wrote:
> On 01/27/2015 03:58 PM, Pádraig Brady wrote:
>> From 12c6f0fd7f44133a2af8950c69b2bfa46ea5d3a4 Mon Sep 17 00:00:00 2001
>> From: Giuseppe Scrivano <[email-masked]>
>> Date: Sun, 25 Jan 2015 01:33:45 +0100
>> Subject: [PATCH] sync: support syncing specified arguments
> 
>> --- a/doc/coreutils.texi
>> +++ b/doc/coreutils.texi
>> @@ -12043,18 +12043,40 @@ with @env{TZ}, libc, The GNU C Library Reference Manual}.
>>  @command{sync} writes any data buffered in memory out to disk.  This can
>>  include (but is not limited to) modified superblocks, modified inodes,
>>  and delayed reads and writes.  This must be implemented by the kernel;
>> -The @command{sync} program does nothing but exercise the @code{sync} system
>> -call.
>> +The @command{sync} program does nothing but exercise the @code{sync},
>> +@code{syncfs}, @code{fsync}, and @code{fdatasync} system calls.
> 
> I think sync's info page now deserves a "Synopsis" line ... as the command
> now takes more than just --help/--version.

Done.

> Maybe the first line of 'man sync'
> 
>   sync - flush file system buffers
> 
> and 'info sync'
> 
>   "synchronize memory and disk" (in the parent table), and
>   "sync data on disk with memory" (sync invocation)
> 
> should be harmonized, too?

Good point. I went with this summary everywhere:

  "Synchronize cached writes to persistent storage"

>> diff --git a/src/sync.c b/src/sync.c
>> index e9f4d7e..80d1403 100644
>> --- a/src/sync.c
>> +++ b/src/sync.c
> 
>> @@ -37,11 +61,20 @@ usage (int status)
>>      emit_try_help ();
>>    else
>>      {
>> -      printf (_("Usage: %s [OPTION]\n"), program_name);
>> +      printf (_("Usage: %s [OPTION] [FILE]...\n"), program_name);
>>        fputs (_("\
>>  Force changed blocks to disk, update the super block.\n\
>>  \n\
>> +If one or more file paths are specified, sync only them,\n\
>> +use --data and --file-system to change the default behavior\n\
>> +\n\
>>  "), stdout);
>> +
>> +      fputs (_("\
>> +  --file-system              sync the file systems that contain the files\n\
>> +  --data                     only sync data for files, no unneeded metadata\n\
>> +"), stdout);
>> +
> 
> '--d' should go before '--f'.
> 
> And shouldn't we also be more translator-friendly, and split the
> 2 options into 2 fputs calls?

Good point. also the spacing was off.
Also I added short options to to align with -f, --file-system in stat(1).
If this ever was to be standardised, or used in other systems
short options would be used, so we might as well give some precedence
here to ease compat.

> The rest of the patch including the test almost LGTM:
> when running against a non-accessible directory, then the correct error
> diagnostic ("permission denied") is eclipsed by a non-descriptive
> diagnostic:
> 
>   $ src/sync --file /tmp /root
>   src/sync: error opening ‘/root’: Is a directory
> 
> strace output of the above:
> 
>   open("/root", O_RDONLY|O_NONBLOCK)      = -1 EACCES (Permission denied)
>   open("/root", O_WRONLY|O_NONBLOCK)      = -1 EISDIR (Is a directory)

Good catch!
I've gone with always reporting the first errno.

thanks again for the excellent review.

Latest is attached.

Pádraig.
From 2caa949db1fc16d1bdcf547d65001a0e430a2b27 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <[email-masked]>
Date: Sun, 25 Jan 2015 01:33:45 +0100
Subject: [PATCH] sync: support syncing specified arguments

* m4/jm-macros.m4 (coreutils_MACROS): Check for syncfs().
* man/sync.x: Add references to syncfs, fsync and fdatasync.
* doc/coreutils.texi (sync invocation): Document the new feature.
* src/sync.c: Include "quote.h".
(AUTHORS): Include myself.
(MODE_FILE, MODE_DATA, MODE_FILE_SYSTEM, MODE_SYNC): New enum values.
(long_options): Define.
(sync_arg): New function.
(usage): Describe that arguments are now accepted.
(main): Add arguments parsing and add support for fsync(2),
fdatasync(2) and syncfs(2).
* tests/misc/sync.sh: New (and only) test for sync.
* tests/local.mk: Reference the new test.
* AUTHORS: Add myself to sync's authors.
* NEWS: Mention the new feature.
---
 AUTHORS            |   2 +-
 NEWS               |   3 +
 doc/coreutils.texi |  52 +++++++++++++---
 m4/jm-macros.m4    |   1 +
 man/sync.x         |   4 +-
 src/sync.c         | 177 +++++++++++++++++++++++++++++++++++++++++++++++++----
 tests/local.mk     |   1 +
 tests/misc/sync.sh |  43 +++++++++++++
 8 files changed, 258 insertions(+), 25 deletions(-)
 create mode 100755 tests/misc/sync.sh

diff --git a/AUTHORS b/AUTHORS
index 0296830..64c11d7 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -83,7 +83,7 @@ stat: Michael Meskes
 stdbuf: Pádraig Brady
 stty: David MacKenzie
 sum: Kayvan Aghaiepour, David MacKenzie
-sync: Jim Meyering
+sync: Jim Meyering, Giuseppe Scrivano
 tac: Jay Lepreau, David MacKenzie
 tail: Paul Rubin, David MacKenzie, Ian Lance Taylor, Jim Meyering
 tee: Mike Parker, Richard M. Stallman, David MacKenzie
diff --git a/NEWS b/NEWS
index 73314d7..b3641ca 100644
--- a/NEWS
+++ b/NEWS
@@ -51,6 +51,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   stty allows setting the "extproc" option where supported, which is
   a useful setting with high latency links.
 
+  sync no longer ignores arguments, and syncs each specified file, or with the
+  --file-system option, the file systems associated with each specified file.
+
 ** Changes in behavior
 
   df no longer suppresses separate exports of the same remote device, as
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 4a15939..99c7df3 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -114,7 +114,7 @@
 * stdbuf: (coreutils)stdbuf invocation.         Modify stdio buffering.
 * stty: (coreutils)stty invocation.             Print/change terminal settings.
 * sum: (coreutils)sum invocation.               Print traditional checksum.
-* sync: (coreutils)sync invocation.             Synchronize memory and disk.
+* sync: (coreutils)sync invocation.             Synchronize memory to disk.
 * tac: (coreutils)tac invocation.               Reverse files.
 * tail: (coreutils)tail invocation.             Output the last part of files.
 * tee: (coreutils)tee invocation.               Redirect to multiple files.
@@ -346,7 +346,7 @@ Disk usage
 * df invocation::                Report file system disk space usage
 * du invocation::                Estimate file space usage
 * stat invocation::              Report file or file system status
-* sync invocation::              Synchronize data on disk with memory
+* sync invocation::              Synchronize cached writes to persistent storage
 * truncate invocation::          Shrink or extend the size of a file
 
 Printing text
@@ -11191,7 +11191,7 @@ file status information, and write buffers to disk.
 * df invocation::               Report file system disk space usage.
 * du invocation::               Estimate file space usage.
 * stat invocation::             Report file or file system status.
-* sync invocation::             Synchronize memory and disk.
+* sync invocation::             Synchronize cached writes to persistent storage.
 * truncate invocation::         Shrink or extend the size of a file.
 @end menu
 
@@ -12033,28 +12033,60 @@ with @env{TZ}, libc, The GNU C Library Reference Manual}.

 @node sync invocation
-@section @command{sync}: Synchronize data on disk with memory
+@section @command{sync}: Synchronize cached writes to persistent storage
 
 @pindex sync
 @cindex synchronize disk and memory
+@cindex Synchronize cached writes to persistent storage
+
+@command{sync} synchronizes in memory files or file systems to persistent
+storage.  Synopsis:
+
+@example
+sync [@var{option}] [@var{file}]@dots{}
+@end example
 
 @cindex superblock, writing
 @cindex inodes, written buffered
 @command{sync} writes any data buffered in memory out to disk.  This can
 include (but is not limited to) modified superblocks, modified inodes,
 and delayed reads and writes.  This must be implemented by the kernel;
-The @command{sync} program does nothing but exercise the @code{sync} system
-call.
+The @command{sync} program does nothing but exercise the @code{sync},
+@code{syncfs}, @code{fsync}, and @code{fdatasync} system calls.
 
 @cindex crashes and corruption
 The kernel keeps data in memory to avoid doing (relatively slow) disk
 reads and writes.  This improves performance, but if the computer
 crashes, data may be lost or the file system corrupted as a
-result.  The @command{sync} command ensures everything in memory
-is written to disk.
+result.  The @command{sync} command instructs the kernel to write
+data in memory to persistent storage.
 
-Any arguments are ignored, except for a lone @option{--help} or
-@option{--version} (@pxref{Common options}).
+If any argument is specified then only those files will be
+synchronized using the fsync(2) syscall by default.
+
+If at least one file is specified, it is possible to change the
+synchronization method with the following options.  Also see
+@ref{Common options}.
+
+@table @samp
+@item -d
+@itemx --data
+@opindex --data
+Use fdatasync(2) to sync only the data for the file,
+and any metadata required to maintain file system consistency.
+
+@item -f
+@itemx --file-system
+@opindex --file-system
+Synchronize all the I/O waiting for the file systems that contain the file,
+using the syscall syncfs(2).  Note you would usually @emph{not} specify
+this option if passing a device node like @samp{/dev/sda} for example,
+as that would sync the containing file system rather than the referenced one.
+Note also that depending on the system, passing individual device nodes or files
+may have different sync characteristics than using no arguments.
+I.E. arguments passed to fsync(2) may provide greater guarantees through
+write barriers, than a global sync(2) used when no arguments are provided.
+@end table
 
 @exitstatus
 
diff --git a/m4/jm-macros.m4 b/m4/jm-macros.m4
index 58fdd97..79f124b 100644
--- a/m4/jm-macros.m4
+++ b/m4/jm-macros.m4
@@ -89,6 +89,7 @@ AC_DEFUN([coreutils_MACROS],
     sethostname
     siginterrupt
     sync
+    syncfs
     sysctl
     sysinfo
     tcgetpgrp
diff --git a/man/sync.x b/man/sync.x
index 7947bb7..79fee22 100644
--- a/man/sync.x
+++ b/man/sync.x
@@ -1,6 +1,6 @@
 [NAME]
-sync \- flush file system buffers
+sync \- Synchronize cached writes to persistent storage
 [DESCRIPTION]
 .\" Add any additional description here
 [SEE ALSO]
-sync(2)
+fdatasync(2), fsync(2), sync(2), syncfs(2)
diff --git a/src/sync.c b/src/sync.c
index e9f4d7e..0b2b178 100644
--- a/src/sync.c
+++ b/src/sync.c
@@ -17,18 +17,42 @@
 /* Written by Jim Meyering */
 
 #include <config.h>
+#include <assert.h>
 #include <getopt.h>
 #include <stdio.h>
 #include <sys/types.h>
 
 #include "system.h"
 #include "error.h"
-#include "long-options.h"
+#include "quote.h"
 
 /* The official name of this program (e.g., no 'g' prefix).  */
 #define PROGRAM_NAME "sync"
 
-#define AUTHORS proper_name ("Jim Meyering")
+#define AUTHORS                                 \
+  proper_name ("Jim Meyering"),                 \
+  proper_name ("Giuseppe Scrivano")
+
+#ifndef HAVE_SYNCFS
+# define HAVE_SYNCFS 0
+#endif
+
+enum sync_mode
+{
+  MODE_FILE,
+  MODE_DATA,
+  MODE_FILE_SYSTEM,
+  MODE_SYNC
+};
+
+static struct option const long_options[] =
+{
+  {"data", no_argument, NULL, MODE_DATA},
+  {"file-system", no_argument, NULL, MODE_FILE_SYSTEM},
+  {GETOPT_HELP_OPTION_DECL},
+  {GETOPT_VERSION_OPTION_DECL},
+  {NULL, 0, NULL, 0}
+};
 
 void
 usage (int status)
@@ -37,11 +61,22 @@ usage (int status)
     emit_try_help ();
   else
     {
-      printf (_("Usage: %s [OPTION]\n"), program_name);
+      printf (_("Usage: %s [OPTION] [FILE]...\n"), program_name);
       fputs (_("\
-Force changed blocks to disk, update the super block.\n\
+Synchronize cached writes to persistent storage\n\
+\n\
+If one or more files are specified, sync only them,\n\
+or their containing file systems.\n\
 \n\
 "), stdout);
+
+      fputs (_("\
+  -d, --data             sync only file data, no unneeded metadata\n\
+"), stdout);
+      fputs (_("\
+  -f, --file-system      sync the file systems that contain the files\n\
+"), stdout);
+
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
       emit_ancillary_info (PROGRAM_NAME);
@@ -49,9 +84,86 @@ Force changed blocks to disk, update the super block.\n\
   exit (status);
 }
 
+/* Sync the specified FILE, or file systems associated with FILE.
+   Return 1 on success.  */
+
+static bool
+sync_arg (enum sync_mode mode, char const *file)
+{
+  bool ret = true;
+  int fd;
+
+  /* Note O_PATH might be supported with syncfs(),
+     though as of Linux 3.18 is not.  */
+  if ((fd = open (file, O_RDONLY | O_NONBLOCK)) < 0)
+    {
+      /* Use the O_RDONLY errno, which is significant
+         with directories for example.  */
+      int rd_errno = errno;
+      if ((fd = open (file, O_WRONLY | O_NONBLOCK)) < 0)
+        error (0, rd_errno, _("error opening %s"), quote (file));
+      return false;
+    }
+
+  /* We used O_NONBLOCK above to not hang with fifos,
+     so reset that here.  */
+  int fdflags;
+  if ((fdflags = fcntl (fd, F_GETFL)) == -1
+      || fcntl (fd, F_SETFL, fdflags & ~O_NONBLOCK) < 0)
+    {
+      error (0, errno, _("couldn't reset non-blocking mode %s"), quote (file));
+      ret = false;
+    }
+
+  if (ret == true)
+    {
+      int sync_status;
+
+      switch (mode)
+        {
+        case MODE_DATA:
+          sync_status = fdatasync (fd);
+          break;
+
+        case MODE_FILE:
+          sync_status = fsync (fd);
+          break;
+
+#if HAVE_SYNCFS
+        case MODE_FILE_SYSTEM:
+          sync_status = syncfs (fd);
+          break;
+#endif
+
+        default:
+          assert ("invalid sync_mode");
+        }
+
+      if (sync_status < 0)
+        {
+          error (0, errno, _("error syncing %s"), quote (file));
+          ret = false;
+        }
+    }
+
+  if (close (fd) < 0)
+    {
+      error (0, errno, _("failed to close %s"), quote (file));
+      ret = false;
+    }
+
+  return ret;
+}
+
 int
 main (int argc, char **argv)
 {
+  int c;
+  bool args_specified;
+  bool arg_data = false, arg_file_system = false;
+  enum sync_mode mode;
+  bool ok = true;
+
   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
   setlocale (LC_ALL, "");
@@ -60,14 +172,55 @@ main (int argc, char **argv)
 
   atexit (close_stdout);
 
-  parse_long_options (argc, argv, PROGRAM_NAME, PACKAGE, Version,
-                      usage, AUTHORS, (char const *) NULL);
-  if (getopt_long (argc, argv, "", NULL, NULL) != -1)
-    usage (EXIT_FAILURE);
+  while ((c = getopt_long (argc, argv, "", long_options, NULL))
+         != -1)
+    {
+      switch (c)
+        {
+        case MODE_DATA:
+          arg_data = true;
+          break;
+
+        case MODE_FILE_SYSTEM:
+          arg_file_system = true;
+          break;
+
+        case_GETOPT_HELP_CHAR;
+
+        case_GETOPT_VERSION_CHAR (PROGRAM_NAME, AUTHORS);
 
-  if (optind < argc)
-    error (0, 0, _("ignoring all arguments"));
+        default:
+          usage (EXIT_FAILURE);
+        }
+    }
+
+  args_specified = optind < argc;
+
+  if (arg_data && arg_file_system)
+    {
+      error (EXIT_FAILURE, 0,
+             _("cannot specify both --data and --file-system"));
+    }
+
+  if (!args_specified && arg_data)
+    error (EXIT_FAILURE, 0, _("--data needs at least one argument"));
+
+  if (! args_specified || (arg_file_system && ! HAVE_SYNCFS))
+    mode = MODE_SYNC;
+  else if (arg_file_system)
+    mode = MODE_FILE_SYSTEM;
+  else if (! arg_data)
+    mode = MODE_FILE;
+  else
+    mode = MODE_DATA;
+
+  if (mode == MODE_SYNC)
+    sync ();
+  else
+    {
+      for (; optind < argc; optind++)
+        ok &= sync_arg (mode, argv[optind]);
+    }
 
-  sync ();
-  return EXIT_SUCCESS;
+  return ok ? EXIT_SUCCESS : EXIT_FAILURE;
 }
diff --git a/tests/local.mk b/tests/local.mk
index 5dcbd55..bfa447c 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -375,6 +375,7 @@ all_tests =					\
   tests/misc/stty-row-col.sh			\
   tests/misc/sum.pl				\
   tests/misc/sum-sysv.sh			\
+  tests/misc/sync.sh				\
   tests/misc/tac.pl				\
   tests/misc/tac-continue.sh			\
   tests/misc/tac-2-nonseekable.sh		\
diff --git a/tests/misc/sync.sh b/tests/misc/sync.sh
new file mode 100755
index 0000000..8b0caf4
--- /dev/null
+++ b/tests/misc/sync.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+# Test various sync(1) operations
+
+# Copyright (C) 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_ sync
+
+touch file
+
+# fdatasync+syncfs is nonsensical
+returns_ 1 sync --data --file-system || fail=1
+
+# fdatasync needs an operand
+returns_ 1 sync --data || fail=1
+
+# Test syncing of file (fsync) (little side effects)
+sync file || fail=1
+
+# Ensure multiple args are processed and diagnosed
+returns_ 1 sync file nofile || fail=1
+
+if test "$fail" != '1'; then
+  # Ensure a fifo doesn't block
+  mkfifo_or_skip_ fifo
+  timeout 10 sync fifo
+  test $? = 124 && fail=1
+fi
+
+Exit $fail
-- 
2.1.0