GNU bug report logs - #19681
[PATCH] sync: use syncfs(2) if any argument is specified

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 19681 in the body.
You can then email your comments to 19681 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Giuseppe Scrivano <gscrivan <at> redhat.com>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] sync: use syncfs(2) if any argument is specified
Date: Sun, 25 Jan 2015 01:48:32 +0100

* configure.ac: Check if syncfs(2) is available.
* NEWS: Mention the new feature.
* doc/coreutils.texi (sync invocation): Document the new feature.
* src/sync.c (usage): Describe that arguments are now accepted.
(main): Use syncfs(2) to flush buffers for the file system which
contain the specified arguments.  Silently fallback to sync(2) on
errors.
---
 NEWS               |  3 +++
 configure.ac       |  2 ++
 doc/coreutils.texi |  7 ++++++-
 src/sync.c         | 31 +++++++++++++++++++++++++++++--
 4 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/NEWS b/NEWS
index e0a2893..42bd02f 100644
--- a/NEWS
+++ b/NEWS
@@ -48,6 +48,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   split accepts a new --separator option to select a record separator character
   other than the default newline character.
 
+  sync accepts arguments, and if any is specified use syncfs(2) to
+  flush the buffers for the file systems which cointain these paths.
+
 ** Changes in behavior
 
   df no longer suppresses separate exports of the same remote device, as
diff --git a/configure.ac b/configure.ac
index 3918f43..8fcfec9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -328,6 +328,8 @@ if test $ac_cv_func_syslog = no; then
   done
 fi
 
+AC_CHECK_FUNCS([syncfs])
+
 AC_CACHE_CHECK([for 3-argument setpriority function],
   [utils_cv_func_setpriority],
   [AC_LINK_IFELSE(
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 5a3c31a..6cc7414 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -12053,6 +12053,10 @@ 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.
 
+If any argument is specified and the system supports the synfcs(2)
+syscall, then only the file systems containing these paths will be
+synchronized.  If multiple paths point to the same file system, the
+syncfs(2) syscall will be invoked for each one of them.
 Any arguments are ignored, except for a lone @option{--help} or
 @option{--version} (@pxref{Common options}).
 
@@ -12081,7 +12085,8 @@ If a @var{file} is larger than the specified size, the extra data is lost.
 If a @var{file} is shorter, it is extended and the extended part (or hole)
 reads as zero bytes.
 
-The program accepts the following options.  Also see @ref{Common options}.
+The only options are a lone @option{--help} or @option{--version}.
+@xref{Common options}.
 
 @table @samp
 
diff --git a/src/sync.c b/src/sync.c
index e9f4d7e..940836e 100644
--- a/src/sync.c
+++ b/src/sync.c
@@ -37,10 +37,13 @@ usage (int status)
     emit_try_help ();
   else
     {
-      printf (_("Usage: %s [OPTION]\n"), program_name);
+      printf (_("Usage: %s [OPTION] [PATH]...\n"), program_name);
       fputs (_("\
 Force changed blocks to disk, update the super block.\n\
 \n\
+If one or more file paths are specified, update only the\n\
+file-systems which contain those files.\n\
+\n\
 "), stdout);
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
@@ -65,9 +68,33 @@ main (int argc, char **argv)
   if (getopt_long (argc, argv, "", NULL, NULL) != -1)
     usage (EXIT_FAILURE);
 
+#if HAVE_SYNCFS
+  /* If arguments are specified, use syncfs on any of them.
+     On any error, silently fallback to sync.  */
   if (optind < argc)
-    error (0, 0, _("ignoring all arguments"));
+    {
+      while (optind < argc)
+        {
+          int fd = open (argv[optind], O_RDONLY);
+          if (fd < 0)
+            goto sync;
+
+          if (syncfs (fd) < 0)
+            {
+              close (fd);
+              goto sync;
+            }
+
+          if (close (fd) < 0)
+            goto sync;
+
+          optind++;
+        }
+      return EXIT_SUCCESS;
+    }
+#endif
 
+sync:
   sync ();
   return EXIT_SUCCESS;
 }
-- 
2.1.0

Message #8 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Giuseppe Scrivano <gscrivan <at> redhat.com>, 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Sun, 25 Jan 2015 03:42:00 +0000

thanks I like it!
Tweaks below...

On 25/01/15 00:48, Giuseppe Scrivano wrote:
> * configure.ac: Check if syncfs(2) is available.
> * NEWS: Mention the new feature.
> * doc/coreutils.texi (sync invocation): Document the new feature.
> * src/sync.c (usage): Describe that arguments are now accepted.
> (main): Use syncfs(2) to flush buffers for the file system which
> contain the specified arguments.  Silently fallback to sync(2) on
> errors.
> ---
>  NEWS               |  3 +++
>  configure.ac       |  2 ++
>  doc/coreutils.texi |  7 ++++++-
>  src/sync.c         | 31 +++++++++++++++++++++++++++++--
>  4 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index e0a2893..42bd02f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -48,6 +48,9 @@ GNU coreutils NEWS                                    -*- outline -*-
>    split accepts a new --separator option to select a record separator character
>    other than the default newline character.
>  
> +  sync accepts arguments, and if any is specified use syncfs(2) to
> +  flush the buffers for the file systems which cointain these paths.

sync no longer ignores arguments, and now uses syncfs(2) to sync
the file systems associated with each specified path.

> +
>  ** Changes in behavior
>  
>    df no longer suppresses separate exports of the same remote device, as
> diff --git a/configure.ac b/configure.ac
> index 3918f43..8fcfec9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -328,6 +328,8 @@ if test $ac_cv_func_syslog = no; then
>    done
>  fi
>  
> +AC_CHECK_FUNCS([syncfs])
> +
>  AC_CACHE_CHECK([for 3-argument setpriority function],
>    [utils_cv_func_setpriority],
>    [AC_LINK_IFELSE(
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 5a3c31a..6cc7414 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -12053,6 +12053,10 @@ 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.
>  
> +If any argument is specified and the system supports the synfcs(2)
> +syscall, then only the file systems containing these paths will be
> +synchronized.  If multiple paths point to the same file system, the
> +syncfs(2) syscall will be invoked for each one of them.
>  Any arguments are ignored, except for a lone @option{--help} or
>  @option{--version} (@pxref{Common options}).
>  
> @@ -12081,7 +12085,8 @@ If a @var{file} is larger than the specified size, the extra data is lost.
>  If a @var{file} is shorter, it is extended and the extended part (or hole)
>  reads as zero bytes.
>  
> -The program accepts the following options.  Also see @ref{Common options}.
> +The only options are a lone @option{--help} or @option{--version}.
> +@xref{Common options}.
>  
>  @table @samp
>  
> diff --git a/src/sync.c b/src/sync.c
> index e9f4d7e..940836e 100644
> --- a/src/sync.c
> +++ b/src/sync.c
> @@ -37,10 +37,13 @@ usage (int status)
>      emit_try_help ();
>    else
>      {
> -      printf (_("Usage: %s [OPTION]\n"), program_name);
> +      printf (_("Usage: %s [OPTION] [PATH]...\n"), program_name);
>        fputs (_("\
>  Force changed blocks to disk, update the super block.\n\
>  \n\
> +If one or more file paths are specified, update only the\n\
> +file-systems which contain those files.\n\
> +\n\
>  "), stdout);
>        fputs (HELP_OPTION_DESCRIPTION, stdout);
>        fputs (VERSION_OPTION_DESCRIPTION, stdout);
> @@ -65,9 +68,33 @@ main (int argc, char **argv)
>    if (getopt_long (argc, argv, "", NULL, NULL) != -1)
>      usage (EXIT_FAILURE);
>  
> +#if HAVE_SYNCFS
> +  /* If arguments are specified, use syncfs on any of them.
> +     On any error, silently fallback to sync.  */
>    if (optind < argc)
> -    error (0, 0, _("ignoring all arguments"));

The warning above should be moved down to the sync: case
rather than removing it.

> +    {
> +      while (optind < argc)
> +        {
> +          int fd = open (argv[optind], O_RDONLY);
> +          if (fd < 0)
> +            goto sync;
> +
> +          if (syncfs (fd) < 0)
> +            {
> +              close (fd);
> +              goto sync;
> +            }
> +
> +          if (close (fd) < 0)
> +            goto sync;
> +
> +          optind++;
> +        }
> +      return EXIT_SUCCESS;
> +    }
> +#endif
>  
> +sync:
>    sync ();
>    return EXIT_SUCCESS;
>  }
> 

thanks!
Pádraig

Message #11 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Giuseppe Scrivano <gscrivan <at> redhat.com>
To: P <at> draigbrady.com
Cc: 19681 <at> debbugs.gnu.org
Subject: [PATCH] sync: use syncfs(2) if any argument is specified
Date: Sun, 25 Jan 2015 14:03:04 +0100

* configure.ac: Check if syncfs(2) is available.
* NEWS: Mention the new feature.
* doc/coreutils.texi (sync invocation): Document the new feature.
* src/sync.c (usage): Describe that arguments are now accepted.
(main): Use syncfs(2) to flush buffers for the file system which
contain the specified arguments.  Silently fallback to sync(2) on
errors.
---

I've amended the suggested fixes in this version.

 NEWS               |  3 +++
 configure.ac       |  2 ++
 doc/coreutils.texi |  7 ++++++-
 src/sync.c         | 37 +++++++++++++++++++++++++++++++++++--
 4 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/NEWS b/NEWS
index e0a2893..611fde6 100644
--- a/NEWS
+++ b/NEWS
@@ -48,6 +48,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   split accepts a new --separator option to select a record separator character
   other than the default newline character.
 
+  sync no longer ignores arguments, and now uses syncfs(2) to sync
+  the file systems associated with each specified path.
+
 ** Changes in behavior
 
   df no longer suppresses separate exports of the same remote device, as
diff --git a/configure.ac b/configure.ac
index 3918f43..8fcfec9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -328,6 +328,8 @@ if test $ac_cv_func_syslog = no; then
   done
 fi
 
+AC_CHECK_FUNCS([syncfs])
+
 AC_CACHE_CHECK([for 3-argument setpriority function],
   [utils_cv_func_setpriority],
   [AC_LINK_IFELSE(
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 5a3c31a..6cc7414 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -12053,6 +12053,10 @@ 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.
 
+If any argument is specified and the system supports the synfcs(2)
+syscall, then only the file systems containing these paths will be
+synchronized.  If multiple paths point to the same file system, the
+syncfs(2) syscall will be invoked for each one of them.
 Any arguments are ignored, except for a lone @option{--help} or
 @option{--version} (@pxref{Common options}).
 
@@ -12081,7 +12085,8 @@ If a @var{file} is larger than the specified size, the extra data is lost.
 If a @var{file} is shorter, it is extended and the extended part (or hole)
 reads as zero bytes.
 
-The program accepts the following options.  Also see @ref{Common options}.
+The only options are a lone @option{--help} or @option{--version}.
+@xref{Common options}.
 
 @table @samp
 
diff --git a/src/sync.c b/src/sync.c
index e9f4d7e..c160e54 100644
--- a/src/sync.c
+++ b/src/sync.c
@@ -37,10 +37,13 @@ usage (int status)
     emit_try_help ();
   else
     {
-      printf (_("Usage: %s [OPTION]\n"), program_name);
+      printf (_("Usage: %s [OPTION] [PATH]...\n"), program_name);
       fputs (_("\
 Force changed blocks to disk, update the super block.\n\
 \n\
+If one or more file paths are specified, update only the\n\
+file-systems which contain those files.\n\
+\n\
 "), stdout);
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
@@ -52,6 +55,7 @@ Force changed blocks to disk, update the super block.\n\
 int
 main (int argc, char **argv)
 {
+  bool args_specified;
   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
   setlocale (LC_ALL, "");
@@ -65,7 +69,36 @@ main (int argc, char **argv)
   if (getopt_long (argc, argv, "", NULL, NULL) != -1)
     usage (EXIT_FAILURE);
 
-  if (optind < argc)
+  args_specified = optind < argc;
+
+#if HAVE_SYNCFS
+  /* If arguments are specified, use syncfs on each of them.
+     On any error, silently fallback to sync.  */
+  if (args_specified)
+    {
+      while (optind < argc)
+        {
+          int fd = open (argv[optind], O_RDONLY);
+          if (fd < 0)
+            goto sync;
+
+          if (syncfs (fd) < 0)
+            {
+              close (fd);
+              goto sync;
+            }
+
+          if (close (fd) < 0)
+            goto sync;
+
+          optind++;
+        }
+      return EXIT_SUCCESS;
+    }
+#endif
+
+sync:
+  if (args_specified)
     error (0, 0, _("ignoring all arguments"));
 
   sync ();
-- 
2.1.0

Message #14 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Sun, 25 Jan 2015 13:13:51 +0000

On 25/01/15 13:03, Giuseppe Scrivano wrote:
> diff --git a/configure.ac b/configure.ac

> +AC_CHECK_FUNCS([syncfs])
> +

I might move this to one of the other config time files.

> diff --git a/src/sync.c b/src/sync.c

> @@ -37,10 +37,13 @@ usage (int status)

> +If one or more file paths are specified, update only the\n\
> +file-systems which contain those files.\n\

s/file-systems/file systems/

I'll tweak as above before pushing.

thanks!
Pádraig.

Message #17 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Sun, 25 Jan 2015 07:38:43 -0800

If we're adding this sort of option, shouldn't we also give users the ability to 
invoke fsync and fdatasync on a single file, as opposed to syncfs on an entire 
file system?

Message #20 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Giuseppe Scrivano <gscrivan <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Pádraig Brady <P <at> draigBrady.com>, 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Sun, 25 Jan 2015 18:19:18 +0100

Paul Eggert <eggert <at> cs.ucla.edu> writes:

> If we're adding this sort of option, shouldn't we also give users the
> ability to invoke fsync and fdatasync on a single file, as opposed to
> syncfs on an entire file system?

Good point.  Should we instead add something like --file-system and
--data-only, respectively for syncfs and fdatasync and use fsync if no
option is specified?

Giuseppe

Message #23 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: Pádraig Brady <P <at> draigBrady.com>, 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Sun, 25 Jan 2015 09:28:34 -0800

Giuseppe Scrivano wrote:
> Should we instead add something like --file-system and
> --data-only, respectively for syncfs and fdatasync and use fsync if no
> option is specified?

Yes, thanks, that sounds reasonable.  I suggest --data rather than --data-only, 
as fdatasync is allowed to synchronize metadata and "--data-only" implies 
otherwise.

Message #26 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Sun, 25 Jan 2015 17:41:12 +0000

On 25/01/15 15:38, Paul Eggert wrote:
> If we're adding this sort of option, shouldn't we also give users the ability to 
> invoke fsync and fdatasync on a single file, as opposed to syncfs on an entire 
> file system?

Yes good point on also integrating per file syncing.
Per file syncing is already supported by dd,
but would be a natural fit to sync(1).

BTW I grepped my internal notes on sync and noticed http://lwn.net/Articles/433384/
which details syncfs() and also suggests `sync file...` as an interface.

So we have: fdatasync < fsync < syncfs < sync
referring to:: file data, file data + metadata, file system, all file systems

You'd need an option to distinguish between file and file system.
The option could refer to _what_ to sync, or what _method_ to use to sync.

An advantage of specifying the method is they're well
understood and described elsewhere:
  --mode={fdatasync, fsync, syncfs, sync}
The first two would give an error without arguments,
the last would give an error with arguments.

The alternative _what_ interface might look like:
  --what={data, file-system}
or split out to separate options like:
  -d,--data, -f,--file-system
An advantage of specifying what to sync is that
it's a bit more general, avoiding specifying implementation methods.
Also the options would not imply needing or not needing to specify files.

I'd be incline to go with the _what_ interface above.

thanks,
Pádraig

Message #29 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Paul Eggert <eggert <at> cs.ucla.edu>, Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Sun, 25 Jan 2015 19:05:42 +0100

On 01/25/2015 06:41 PM, Pádraig Brady wrote:
> So we have: fdatasync < fsync < syncfs < sync
> referring to:: file data, file data + metadata, file system, all file systems

> [...]

> I'd be incline to go with the _what_ interface above.

Either way, I think it's important to document sync is falling back
to the bigger hammer if the smaller failed.
... or shouldn't do sync this?

Have a nice day,
Berny

Message #32 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>, 
 Paul Eggert <eggert <at> cs.ucla.edu>, Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Sun, 25 Jan 2015 18:32:29 +0000

On 25/01/15 18:05, Bernhard Voelker wrote:
> On 01/25/2015 06:41 PM, Pádraig Brady wrote:
>> So we have: fdatasync < fsync < syncfs < sync
>> referring to:: file data, file data + metadata, file system, all file systems
> 
>> [...]
> 
>> I'd be incline to go with the _what_ interface above.
> 
> Either way, I think it's important to document sync is falling back
> to the bigger hammer if the smaller failed.
> ... or shouldn't do sync this?

It should fall back where possible.

Now there is a difference between the file and file system(s) interfaces
in that the former can return EIO error for example, while the latter
are specified to always return success. You wouldn't fall back to
a syncfs() if an fsync() gave an EIO for example.  Also gnulib
guarantees that fsync() and fdatasync() are available, so I wouldn't
fallback from file -> file system interfaces, nor between file interfaces.
There is an edge case with fsync("fifo") for example where EINVAL is given,
though again since there is no data in the file system associated with that,
I wouldn't bother falling back to a file system interface.

As for documenting the fall back, that's one of the reasons I
suggested keeping the warning if syncfs() is not supported,
which should be enough direct feedback about the extra syncing happening.

cheers,
Pádraig.

Message #35 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Giuseppe Scrivano <gscrivan <at> redhat.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: Bernhard Voelker <mail <at> bernhard-voelker.de>, 19681 <at> debbugs.gnu.org,
 Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Mon, 26 Jan 2015 09:36:05 +0100

Pádraig Brady <P <at> draigbrady.com> writes:

> On 25/01/15 18:05, Bernhard Voelker wrote:
>> On 01/25/2015 06:41 PM, Pádraig Brady wrote:
>>> So we have: fdatasync < fsync < syncfs < sync
>>> referring to:: file data, file data + metadata, file system, all file systems
>> 
>>> [...]
>> 
>>> I'd be incline to go with the _what_ interface above.
>> 
>> Either way, I think it's important to document sync is falling back
>> to the bigger hammer if the smaller failed.
>> ... or shouldn't do sync this?
>
> It should fall back where possible.
>
> Now there is a difference between the file and file system(s) interfaces
> in that the former can return EIO error for example, while the latter
> are specified to always return success. You wouldn't fall back to
> a syncfs() if an fsync() gave an EIO for example.  Also gnulib
> guarantees that fsync() and fdatasync() are available, so I wouldn't
> fallback from file -> file system interfaces, nor between file interfaces.

one risk here is when multiple arguments are specified and the fsync
will return EIO more than once, we will fallback to syncfs multiple
times.  Couldn't in this case a single sync be a better choice?

Regards,
Giuseppe

Message #38 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: Bernhard Voelker <mail <at> bernhard-voelker.de>, 19681 <at> debbugs.gnu.org,
 Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Mon, 26 Jan 2015 11:11:52 +0000

On 26/01/15 08:36, Giuseppe Scrivano wrote:
> Pádraig Brady <P <at> draigbrady.com> writes:
> 
>> On 25/01/15 18:05, Bernhard Voelker wrote:
>>> On 01/25/2015 06:41 PM, Pádraig Brady wrote:
>>>> So we have: fdatasync < fsync < syncfs < sync
>>>> referring to:: file data, file data + metadata, file system, all file systems
>>>
>>>> [...]
>>>
>>>> I'd be incline to go with the _what_ interface above.
>>>
>>> Either way, I think it's important to document sync is falling back
>>> to the bigger hammer if the smaller failed.
>>> ... or shouldn't do sync this?
>>
>> It should fall back where possible.
>>
>> Now there is a difference between the file and file system(s) interfaces
>> in that the former can return EIO error for example, while the latter
>> are specified to always return success. You wouldn't fall back to
>> a syncfs() if an fsync() gave an EIO for example.  Also gnulib
>> guarantees that fsync() and fdatasync() are available, so I wouldn't
>> fallback from file -> file system interfaces, nor between file interfaces.
> 
> one risk here is when multiple arguments are specified and the fsync
> will return EIO more than once, we will fallback to syncfs multiple
> times.  Couldn't in this case a single sync be a better choice?

I was saying we shouldn't fall back from fsync() to syncfs().
Just process each argument. Diagnose any errors and EXIT_FAILURE
if there was any error?

Pádraig.

Message #41 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Giuseppe Scrivano <gscrivan <at> redhat.com>, 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Mon, 26 Jan 2015 09:02:50 -0700

[Message part 1 (text/plain, inline)]

On 01/24/2015 05:48 PM, Giuseppe Scrivano wrote:
> * configure.ac: Check if syncfs(2) is available.
> * NEWS: Mention the new feature.
> * doc/coreutils.texi (sync invocation): Document the new feature.
> * src/sync.c (usage): Describe that arguments are now accepted.
> (main): Use syncfs(2) to flush buffers for the file system which
> contain the specified arguments.  Silently fallback to sync(2) on
> errors.
> ---

> @@ -65,9 +68,33 @@ main (int argc, char **argv)
>    if (getopt_long (argc, argv, "", NULL, NULL) != -1)
>      usage (EXIT_FAILURE);
>  
> +#if HAVE_SYNCFS
> +  /* If arguments are specified, use syncfs on any of them.
> +     On any error, silently fallback to sync.  */
>    if (optind < argc)
> -    error (0, 0, _("ignoring all arguments"));
> +    {
> +      while (optind < argc)
> +        {
> +          int fd = open (argv[optind], O_RDONLY);
> +          if (fd < 0)
> +            goto sync;
> +
> +          if (syncfs (fd) < 0)
> +            {
> +              close (fd);
> +              goto sync;
> +            }
> +
> +          if (close (fd) < 0)
> +            goto sync;
> +
> +          optind++;
> +        }
> +      return EXIT_SUCCESS;
> +    }
> +#endif
>  
> +sync:

This label is unused if !HAVE_SYNCFS.  I suggest moving it inside the #if.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #44 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Giuseppe Scrivano <gscrivan <at> redhat.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: Bernhard Voelker <mail <at> bernhard-voelker.de>, Eric Blake <eblake <at> redhat.com>,
 19681 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Mon, 26 Jan 2015 22:27:31 +0100

Pádraig Brady <P <at> draigbrady.com> writes:

> On 26/01/15 08:36, Giuseppe Scrivano wrote:
>> Pádraig Brady <P <at> draigbrady.com> writes:
>> 
>>> On 25/01/15 18:05, Bernhard Voelker wrote:
>>>> On 01/25/2015 06:41 PM, Pádraig Brady wrote:
>>>>> So we have: fdatasync < fsync < syncfs < sync
>>>>> referring to:: file data, file data + metadata, file system, all file systems
>>>>
>>>>> [...]
>>>>
>>>>> I'd be incline to go with the _what_ interface above.
>>>>
>>>> Either way, I think it's important to document sync is falling back
>>>> to the bigger hammer if the smaller failed.
>>>> ... or shouldn't do sync this?
>>>
>>> It should fall back where possible.
>>>
>>> Now there is a difference between the file and file system(s) interfaces
>>> in that the former can return EIO error for example, while the latter
>>> are specified to always return success. You wouldn't fall back to
>>> a syncfs() if an fsync() gave an EIO for example.  Also gnulib
>>> guarantees that fsync() and fdatasync() are available, so I wouldn't
>>> fallback from file -> file system interfaces, nor between file interfaces.
>> 
>> one risk here is when multiple arguments are specified and the fsync
>> will return EIO more than once, we will fallback to syncfs multiple
>> times.  Couldn't in this case a single sync be a better choice?
>
> I was saying we shouldn't fall back from fsync() to syncfs().
> Just process each argument. Diagnose any errors and EXIT_FAILURE
> if there was any error?

sorry for misunderstanding that.

I've worked out a new version that includes these suggestions, also
since now the user can explicitly ask for the sync mechanism to use, I
agree with you and we should raise an error if something goes wrong.

Since sync is completely different now, I took the freedom to add myself
to the AUTHORS, feel free to drop this part if you disagree.

Regards,
Giuseppe



From 0dbc5ce9c78bc97ec5a678803270767ad9980618 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivan <at> redhat.com>
Date: Sun, 25 Jan 2015 01:33:45 +0100
Subject: [PATCH] sync: add support for fsync(2), fdatasync(2) and syncfs(2)

* AUTHORS: Add myself to sync's authors.
* NEWS: Mention the new feature.
* m4/jm-macros.m4 (coreutils_MACROS): Check for syncfs.
* doc/coreutils.texi (sync invocation): Document the new feature.
* src/sync.c: Include "quote.h".
(AUTHORS): Include myself.
(MODE_FILE, MODE_FILE_DATA, MODE_FILE_SYSTEM): New enum values.
(long_options): Define.
(usage): Describe that arguments are now accepted.
(main): Add arguments parsing and add support for fsync(2),
fdatasync(2) and syncfs(2).
---
 AUTHORS            |   2 +-
 NEWS               |   3 ++
 doc/coreutils.texi |  20 ++++++++-
 m4/jm-macros.m4    |   1 +
 src/sync.c         | 116 +++++++++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 131 insertions(+), 11 deletions(-)

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 e0a2893..3d4190b 100644
--- a/NEWS
+++ b/NEWS
@@ -48,6 +48,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   split accepts a new --separator option to select a record separator character
   other than the default newline character.
 
+  sync no longer ignores arguments and it uses fsync(2), fdatasync(2)
+  and syncfs(2) synchronization in addition to sync(2).
+
 ** 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 5a3c31a..c99b8ed 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -12053,8 +12053,24 @@ 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.
 
-Any arguments are ignored, except for a lone @option{--help} or
-@option{--version} (@pxref{Common options}).
+If any argument is specified then only the specified paths will be
+synchronized.  It uses internally the syscall fsync(2) on each of them.
+
+If at least one path is specified, it is possible to change the
+synchronization policy with the following options.  Also see
+@ref{Common options}.
+
+@table @samp
+@item --data
+@opindex --data
+Do not synchronize the file metadata unless it is required to maintain
+data integrity.  It uses the syscall fdatasync(2).
+
+@item --file-system
+@opindex --file-system
+Synchronize all the I/O waiting for the file system that contains the path.
+It uses the syscall syncfs(2).
+@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/src/sync.c b/src/sync.c
index e9f4d7e..4a15d75 100644
--- a/src/sync.c
+++ b/src/sync.c
@@ -23,12 +23,30 @@
 
 #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")
+
+enum
+{
+  MODE_FILE,
+  MODE_FILE_DATA,
+  MODE_FILE_SYSTEM
+};
+
+static struct option const long_options[] =
+{
+  {"data", no_argument, NULL, MODE_FILE_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 +55,21 @@ usage (int status)
     emit_try_help ();
   else
     {
-      printf (_("Usage: %s [OPTION]\n"), program_name);
+      printf (_("Usage: %s [OPTION] [PATH]...\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 path\n\
+  --data                     flush the metadata only if needed later for\n\
+                             data retrieval to be correctly handled\n\
 "), stdout);
+
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
       emit_ancillary_info (PROGRAM_NAME);
@@ -52,6 +80,11 @@ Force changed blocks to disk, update the super block.\n\
 int
 main (int argc, char **argv)
 {
+  bool args_specified;
+  int c;
+  int mode = MODE_FILE;
+  int (*sync_func)(int) = NULL;
+
   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
   setlocale (LC_ALL, "");
@@ -60,12 +93,79 @@ 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_FILE_DATA:
+          mode = MODE_FILE_DATA;
+          break;
+
+        case MODE_FILE_SYSTEM:
+          mode = MODE_FILE_SYSTEM;
+          break;
+
+        case_GETOPT_HELP_CHAR;
+
+        case_GETOPT_VERSION_CHAR (PROGRAM_NAME, AUTHORS);
+
+        default:
+          usage (EXIT_FAILURE);
+        }
+    }
+
+  args_specified = optind < argc;
+
+  if (! args_specified)
+    goto sync;
+
+  if (!args_specified && mode != MODE_FILE)
+    error (EXIT_FAILURE, errno, _("mode specified without any argument"));
+
+  switch (mode)
+    {
+    case MODE_FILE_DATA:
+      sync_func = fdatasync;
+      break;
+
+    case MODE_FILE:
+      sync_func = fsync;
+      break;
+#if HAVE_SYNCFS
+      /* On systems where syncfs is not available, fallback to sync.  */
+    case MODE_FILE_SYSTEM:
+      sync_func = syncfs;
+      break;
+#endif
+    default:
+      goto sync;
+    }
+
+  while (optind < argc)
+    {
+      int fd = open (argv[optind], O_RDONLY);
+      if (fd < 0)
+        {
+          error (EXIT_FAILURE, errno, _("cannot open %s for reading"),
+                 quote (argv[optind]));
+        }
+
+      if (sync_func (fd) < 0)
+        error (EXIT_FAILURE, errno, _("syncing error"));
+
+      if (close (fd) < 0)
+        {
+          error (EXIT_FAILURE, errno, _("failed to close %s"),
+                 quote (argv[optind]));
+        }
+
+      optind++;
+    }
+  return EXIT_SUCCESS;
 
-  if (optind < argc)
+sync:
+  if (args_specified)
     error (0, 0, _("ignoring all arguments"));
 
   sync ();
-- 
2.1.0

Message #47 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: Bernhard Voelker <mail <at> bernhard-voelker.de>, Eric Blake <eblake <at> redhat.com>,
 19681 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Mon, 26 Jan 2015 23:19:34 +0000

On 26/01/15 21:27, Giuseppe Scrivano wrote:
> Pádraig Brady <P <at> draigbrady.com> writes:
> 
>> On 26/01/15 08:36, Giuseppe Scrivano wrote:
>>> Pádraig Brady <P <at> draigbrady.com> writes:
>>>
>>>> On 25/01/15 18:05, Bernhard Voelker wrote:
>>>>> On 01/25/2015 06:41 PM, Pádraig Brady wrote:
>>>>>> So we have: fdatasync < fsync < syncfs < sync
>>>>>> referring to:: file data, file data + metadata, file system, all file systems
>>>>>
>>>>>> [...]
>>>>>
>>>>>> I'd be incline to go with the _what_ interface above.
>>>>>
>>>>> Either way, I think it's important to document sync is falling back
>>>>> to the bigger hammer if the smaller failed.
>>>>> ... or shouldn't do sync this?
>>>>
>>>> It should fall back where possible.
>>>>
>>>> Now there is a difference between the file and file system(s) interfaces
>>>> in that the former can return EIO error for example, while the latter
>>>> are specified to always return success. You wouldn't fall back to
>>>> a syncfs() if an fsync() gave an EIO for example.  Also gnulib
>>>> guarantees that fsync() and fdatasync() are available, so I wouldn't
>>>> fallback from file -> file system interfaces, nor between file interfaces.
>>>
>>> one risk here is when multiple arguments are specified and the fsync
>>> will return EIO more than once, we will fallback to syncfs multiple
>>> times.  Couldn't in this case a single sync be a better choice?
>>
>> I was saying we shouldn't fall back from fsync() to syncfs().
>> Just process each argument. Diagnose any errors and EXIT_FAILURE
>> if there was any error?
> 
> sorry for misunderstanding that.
> 
> I've worked out a new version that includes these suggestions, also
> since now the user can explicitly ask for the sync mechanism to use, I
> agree with you and we should raise an error if something goes wrong.
> 
> Since sync is completely different now, I took the freedom to add myself
> to the AUTHORS, feel free to drop this part if you disagree.
> 
> Regards,
> Giuseppe
> 
> 
> 
>>From 0dbc5ce9c78bc97ec5a678803270767ad9980618 Mon Sep 17 00:00:00 2001
> From: Giuseppe Scrivano <gscrivan <at> redhat.com>
> Date: Sun, 25 Jan 2015 01:33:45 +0100
> Subject: [PATCH] sync: add support for fsync(2), fdatasync(2) and syncfs(2)
> 
> * AUTHORS: Add myself to sync's authors.
> * NEWS: Mention the new feature.
> * m4/jm-macros.m4 (coreutils_MACROS): Check for syncfs.
> * doc/coreutils.texi (sync invocation): Document the new feature.
> * src/sync.c: Include "quote.h".
> (AUTHORS): Include myself.
> (MODE_FILE, MODE_FILE_DATA, MODE_FILE_SYSTEM): New enum values.
> (long_options): Define.
> (usage): Describe that arguments are now accepted.
> (main): Add arguments parsing and add support for fsync(2),
> fdatasync(2) and syncfs(2).
> ---
>  AUTHORS            |   2 +-
>  NEWS               |   3 ++
>  doc/coreutils.texi |  20 ++++++++-
>  m4/jm-macros.m4    |   1 +
>  src/sync.c         | 116 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  5 files changed, 131 insertions(+), 11 deletions(-)
> 
> 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 e0a2893..3d4190b 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -48,6 +48,9 @@ GNU coreutils NEWS                                    -*- outline -*-
>    split accepts a new --separator option to select a record separator character
>    other than the default newline character.
>  
> +  sync no longer ignores arguments and it uses fsync(2), fdatasync(2)
> +  and syncfs(2) synchronization in addition to sync(2).

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 5a3c31a..c99b8ed 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -12053,8 +12053,24 @@ 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.
>  
> -Any arguments are ignored, except for a lone @option{--help} or
> -@option{--version} (@pxref{Common options}).
> +If any argument is specified then only the specified paths will be

s/paths/files/.  paths is a bit ambiguous, while files implies dirs too.

> +synchronized.  It uses internally the syscall fsync(2) on each of them.
> +
> +If at least one path is specified, it is possible to change the
> +synchronization policy with the following options.  Also see
> +@ref{Common options}.
> +
> +@table @samp
> +@item --data
> +@opindex --data
> +Do not synchronize the file metadata unless it is required to maintain
> +data integrity.  It uses the syscall fdatasync(2).
> +
> +@item --file-system
> +@opindex --file-system
> +Synchronize all the I/O waiting for the file system that contains the path.

s/file system/file systems/
s/path/file/

> +It uses the syscall syncfs(2).
> +@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/src/sync.c b/src/sync.c
> index e9f4d7e..4a15d75 100644
> --- a/src/sync.c
> +++ b/src/sync.c
> @@ -23,12 +23,30 @@
>  
>  #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")
> +
> +enum
> +{
> +  MODE_FILE,
> +  MODE_FILE_DATA,
> +  MODE_FILE_SYSTEM
> +};
> +
> +static struct option const long_options[] =
> +{
> +  {"data", no_argument, NULL, MODE_FILE_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 +55,21 @@ usage (int status)
>      emit_try_help ();
>    else
>      {
> -      printf (_("Usage: %s [OPTION]\n"), program_name);
> +      printf (_("Usage: %s [OPTION] [PATH]...\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 path\n\
> +  --data                     flush the metadata only if needed later for\n\

s/flush/sync/ as flush has ambiguous connotations with discard rather than drain.

> +                             data retrieval to be correctly handled\n\
>  "), stdout);
> +
>        fputs (HELP_OPTION_DESCRIPTION, stdout);
>        fputs (VERSION_OPTION_DESCRIPTION, stdout);
>        emit_ancillary_info (PROGRAM_NAME);
> @@ -52,6 +80,11 @@ Force changed blocks to disk, update the super block.\n\
>  int
>  main (int argc, char **argv)
>  {
> +  bool args_specified;
> +  int c;
> +  int mode = MODE_FILE;
> +  int (*sync_func)(int) = NULL;

This assumes these are never function like macros.
Probably OK, but it would be better to avoid that assumption
about external functions.

> +
>    initialize_main (&argc, &argv);
>    set_program_name (argv[0]);
>    setlocale (LC_ALL, "");
> @@ -60,12 +93,79 @@ 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_FILE_DATA:
> +          mode = MODE_FILE_DATA;
> +          break;
> +
> +        case MODE_FILE_SYSTEM:
> +          mode = MODE_FILE_SYSTEM;
> +          break;
> +
> +        case_GETOPT_HELP_CHAR;
> +
> +        case_GETOPT_VERSION_CHAR (PROGRAM_NAME, AUTHORS);
> +
> +        default:
> +          usage (EXIT_FAILURE);
> +        }
> +    }
> +
> +  args_specified = optind < argc;
> +
> +  if (! args_specified)
> +    goto sync;
> +
> +  if (!args_specified && mode != MODE_FILE)
> +    error (EXIT_FAILURE, errno, _("mode specified without any argument"));

That's a big aggressive. I'd allow --file-system with no args,
as then --file-system means to sync the specified or all file systems.

Requiring arguments with --data probably make sense.

Another validation worth applying is to disallow --data with --file-system
as they're invalid to specify together.

> +
> +  switch (mode)
> +    {
> +    case MODE_FILE_DATA:
> +      sync_func = fdatasync;
> +      break;
> +
> +    case MODE_FILE:
> +      sync_func = fsync;
> +      break;
> +#if HAVE_SYNCFS
> +      /* On systems where syncfs is not available, fallback to sync.  */
> +    case MODE_FILE_SYSTEM:
> +      sync_func = syncfs;
> +      break;
> +#endif
> +    default:
> +      goto sync;
> +    }
> +

I'd probably put the following loop body in a sync_arg() function returning a bool.

> +  while (optind < argc)
> +    {
> +      int fd = open (argv[optind], O_RDONLY);
> +      if (fd < 0)
> +        {
> +          error (EXIT_FAILURE, errno, _("cannot open %s for reading"),
> +                 quote (argv[optind]));
> +        }
> +
> +      if (sync_func (fd) < 0)
> +        error (EXIT_FAILURE, errno, _("syncing error"));
> +
> +      if (close (fd) < 0)
> +        {
> +          error (EXIT_FAILURE, errno, _("failed to close %s"),
> +                 quote (argv[optind]));
> +        }
> +
> +      optind++;
> +    }

It's probably better to register the failure and try subsequent arguments,
as then it's ambiguous as to what is sync'd if you only get the warning
about the first failure.

> +  return EXIT_SUCCESS;
>  
> -  if (optind < argc)

There aren't any deep nestings here, so things could probably be reworked
easily to avoid the goto.

> +sync:
> +  if (args_specified)
>      error (0, 0, _("ignoring all arguments"));
>  
>    sync ();

It's probably worth adding references to syncfs(2), fsync(2), and fdatasync(2)
to man/sync.x

thanks!
Pádraig.

Message #50 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Giuseppe Scrivano <gscrivan <at> redhat.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: Bernhard Voelker <mail <at> bernhard-voelker.de>, Eric Blake <eblake <at> redhat.com>,
 19681 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Tue, 27 Jan 2015 11:48:44 +0100

Pádraig Brady <P <at> draigbrady.com> writes:

> thanks!
> Pádraig.

Thanks for the review, I've amended the changes you suggested:

From b2babc9838b52892e2cdc46bc4590fa852daa0eb Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivan <at> redhat.com>
Date: Sun, 25 Jan 2015 01:33:45 +0100
Subject: [PATCH] sync: add support for fsync(2), fdatasync(2) and syncfs(2)

* AUTHORS: Add myself to sync's authors.
* NEWS: Mention the new feature.
* 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).
---
 AUTHORS            |   2 +-
 NEWS               |   3 +
 doc/coreutils.texi |  20 ++++++-
 m4/jm-macros.m4    |   1 +
 man/sync.x         |   2 +-
 src/sync.c         | 157 +++++++++++++++++++++++++++++++++++++++++++++++++----
 6 files changed, 170 insertions(+), 15 deletions(-)

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 e0a2893..3d4190b 100644
--- a/NEWS
+++ b/NEWS
@@ -48,6 +48,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   split accepts a new --separator option to select a record separator character
   other than the default newline character.
 
+  sync no longer ignores arguments and it uses fsync(2), fdatasync(2)
+  and syncfs(2) synchronization in addition to sync(2).
+
 ** 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 5a3c31a..a1e664b 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -12053,8 +12053,24 @@ 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.
 
-Any arguments are ignored, except for a lone @option{--help} or
-@option{--version} (@pxref{Common options}).
+If any argument is specified then only the specified files will be
+synchronized.  It uses internally the syscall fsync(2) on each of them.
+
+If at least one file is specified, it is possible to change the
+synchronization policy with the following options.  Also see
+@ref{Common options}.
+
+@table @samp
+@item --data
+@opindex --data
+Do not synchronize the file metadata unless it is required to maintain
+data integrity.  It uses the syscall fdatasync(2).
+
+@item --file-system
+@opindex --file-system
+Synchronize all the I/O waiting for the file systems that contain the file.
+It uses the syscall syncfs(2).
+@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..6ced07e 100644
--- a/man/sync.x
+++ b/man/sync.x
@@ -3,4 +3,4 @@ sync \- flush file system buffers
 [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..6c4d571 100644
--- a/src/sync.c
+++ b/src/sync.c
@@ -23,12 +23,31 @@
 
 #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")
+
+enum
+{
+  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 +56,21 @@ usage (int status)
     emit_try_help ();
   else
     {
-      printf (_("Usage: %s [OPTION]\n"), program_name);
+      printf (_("Usage: %s [OPTION] [PATH]...\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 path\n\
+  --data                     flush the metadata only if needed later for\n\
+                             data retrieval to be correctly handled\n\
+"), stdout);
+
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
       emit_ancillary_info (PROGRAM_NAME);
@@ -49,9 +78,61 @@ Force changed blocks to disk, update the super block.\n\
   exit (status);
 }
 
+static bool
+sync_arg (int mode, char *file)
+{
+  int ret = -1;
+  int fd = open (file, O_RDONLY);
+  if (fd < 0)
+    {
+      error (0, errno, _("cannot open %s for reading"), quote (file));
+      return false;
+    }
+
+  switch (mode)
+    {
+    case MODE_FILE:
+      ret = fsync (fd);
+      break;
+
+    case MODE_DATA:
+      ret = fdatasync (fd);
+      break;
+
+#if HAVE_SYNCFS
+    case MODE_FILE_SYSTEM:
+      ret = syncfs (fd);
+      break;
+#endif
+
+    default:
+      error (EXIT_FAILURE, errno, _("invalid mode specified"));
+      break;
+    }
+
+  if (ret < 0)
+    {
+      error (0, errno, _("syncing error"));
+      return false;
+    }
+
+  if (close (fd) < 0)
+    {
+      error (0, errno, _("failed to close %s"), quote (file));
+      return false;
+    }
+
+  return true;
+}
+
 int
 main (int argc, char **argv)
 {
+  bool args_specified;
+  int c, ret;
+  bool arg_data = false, arg_file_system = false;
+  int mode;
+
   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
   setlocale (LC_ALL, "");
@@ -60,14 +141,68 @@ 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);
+
+        default:
+          usage (EXIT_FAILURE);
+        }
+    }
+
+  args_specified = optind < argc;
+
+  if (arg_data && arg_file_system)
+    {
+      error (EXIT_FAILURE, errno,
+             _("cannot specify both --data and --file-system"));
+    }
+
+  if (!args_specified && arg_data)
+    error (EXIT_FAILURE, errno, _("--data needs at least one argument"));
+
+  if (arg_data)
+    mode = MODE_DATA;
+  else if (! arg_file_system)
+    {
+      mode = args_specified ? MODE_FILE : MODE_SYNC;
+    }
+  else
+    {
+      /* On systems where syncfs is not available, or if no args are specified,
+         fallback to sync.  */
+      if (HAVE_SYNCFS && args_specified)
+          mode = MODE_FILE_SYSTEM;
+      else
+        {
+          mode = MODE_SYNC;
+        }
+    }
 
-  if (optind < argc)
-    error (0, 0, _("ignoring all arguments"));
+  if (mode == MODE_SYNC)
+    {
+      sync ();
+      return EXIT_SUCCESS;
+    }
 
-  sync ();
-  return EXIT_SUCCESS;
+  ret = EXIT_SUCCESS;
+  for (; optind < argc; optind++)
+    {
+      if (! sync_arg (mode, argv[optind]))
+        ret = EXIT_FAILURE;
+    }
+  return ret;
 }
-- 
2.1.0

Message #53 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Tue, 27 Jan 2015 14:58:44 +0000

[Message part 1 (text/plain, inline)]

On 27/01/15 10:48, Giuseppe Scrivano wrote:
> Pádraig Brady <P <at> draigbrady.com> writes:
> 
>> thanks!
>> Pádraig.
> 
> Thanks for the review, I've amended the changes you suggested:

There were a few problems:

 - Compile failure without HAVE_SYNCFS (defined)
 - Various errors() used errno where undefined
 - A file that failed to sync was not identified
 - File descriptors were leaked on failure to sync
 - The command would block if presented with a fifo
 - Write only files ere not supported
 - Various references to "paths" rather than "files"
 - The info docs for --file-system were a bit terse
 - No tests

I've fixed up those issues in the attached,
which I'll push soon.

thanks,
Pádraig.

[sync-args.patch (text/x-patch, attachment)]

Message #56 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Wed, 28 Jan 2015 09:17:55 +0100

On 01/27/2015 03:58 PM, Pádraig Brady wrote:
> From 12c6f0fd7f44133a2af8950c69b2bfa46ea5d3a4 Mon Sep 17 00:00:00 2001
> From: Giuseppe Scrivano <gscrivano <at> gnu.org>
> 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.

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?


> 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?

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)

Please note that syncfs() for the /tmp argument succeeded correctly.
For completeness, here is the correct case with a good error diagnostic
for an inaccessible file:

  $ src/sync --file /root/.bashrc
  src/sync: error opening ‘/root/.bashrc’: Permission denied

Thanks & have a nice day,
Berny

Message #59 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>, 
 Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: 19681 <at> debbugs.gnu.org
Subject: Re: bug#19681: [PATCH] sync: use syncfs(2) if any argument is
 specified
Date: Wed, 28 Jan 2015 10:19:59 +0000

[Message part 1 (text/plain, inline)]

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 <gscrivano <at> gnu.org>
>> 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.

[sync-args.patch (text/x-patch, attachment)]

Message #62 received at 19681 <at> debbugs.gnu.org (full text, mbox):

From: Assaf Gordon <assafgordon <at> gmail.com>
To: 19681 <at> debbugs.gnu.org
Date: Wed, 10 Oct 2018 10:11:56 -0600

close 19681
stop

Pushed at 
https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=8b2bf5295f353016d4f5e6a2317d55b6a8e7fd00

closing.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.