Linux-only fdatasync() in 3c13bc

Linux-only fdatasync() in 3c13bc

[Xīcò]

[-- Attachment #1: Type: text/plain, Size: 149 bytes --]

Also, commit 3c13bc introduced a call to fdatasync() which is not
available on FreeBSD, and probably not either on MacOS at least.

Best,

--
Xīcò

[-- Attachment #2: 0002-Compatibility-for-Linux-s-fdatasync.patch --]
[-- Type: text/x-diff, Size: 2826 bytes --]

From e2b9520e69e52b35348d07eb53a6a88d1397fa3f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?X=C4=ABc=C3=B2?= <xico@atelo.org>
Date: Tue, 6 May 2014 12:56:03 -0700
Subject: [PATCH 1/1] =?UTF-8?q?Compatibility=20for=20Linux=E2=80=99s=20fda?=
 =?UTF-8?q?tasync().?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 configure      | 7 +++++++
 notmuch-dump.c | 4 ++++
 2 files changed, 11 insertions(+)

diff --git a/configure b/configure
index 7204812..7bc602d 100755
--- a/configure
+++ b/configure
@@ -271,6 +271,7 @@ EOF
 errors=0
 
 libdir_in_ldconfig=0
+have_fdatasync=0
 
 printf "Checking which platform we are on... "
 uname=`uname`
@@ -294,6 +295,7 @@ elif [ $uname = "Linux" ] || [ $uname = "GNU" ] ; then
     printf "$uname\n"
     platform="$uname"
     linker_resolves_library_dependencies=1
+    have_fdatasync=1
 
     printf "Checking for $libdir_expanded in ldconfig... "
     ldconfig_paths=$(/sbin/ldconfig -N -X -v 2>/dev/null | sed -n -e 's,^\(/.*\):\( (.*)\)\?$,\1,p')
@@ -859,6 +861,9 @@ HAVE_D_TYPE = ${have_d_type}
 # Whether the Xapian version in use supports compaction
 HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
 
+# Optimized fsync() on Linux
+HAVE_FDATASYNC = ${have_fdatasync}
+
 # Whether the getpwuid_r function is standards-compliant
 # (if not, then notmuch will #define _POSIX_PTHREAD_SEMANTICS
 # to enable the standards-compliant version -- needed for Solaris)
@@ -926,6 +931,7 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
 		   -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\
 		   -DSTD_ASCTIME=\$(STD_ASCTIME)                         \\
 		   -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)	 \\
+		   -DHAVE_FDATASYNC=\$(HAVE_FDATASYNC)	                 \\
 		   -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER)
 
 CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
@@ -939,6 +945,7 @@ CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
 		     -DSTD_ASCTIME=\$(STD_ASCTIME)                       \\
 		     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)       \\
+		     -DHAVE_FDATASYNC=\$(HAVE_FDATASYNC)	         \\
 		     -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER)
 
 CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(ZLIB_LDFLAGS) \$(XAPIAN_LDFLAGS)
diff --git a/notmuch-dump.c b/notmuch-dump.c
index 2849eab..86b275a 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -169,7 +169,11 @@ notmuch_database_dump (notmuch_database_t *notmuch,
     }
 
     if (output_file_name) {
+#if HAVE_FDATASYNC
 	ret = fdatasync (outfd);
+#else
+	ret = fsync (outfd);
+#endif
 	if (ret) {
 	    fprintf (stderr, "Error syncing %s to disk: %s\n",
 		     name_for_error, strerror (errno));
-- 
1.9.2

Re: Linux-only fdatasync() in 3c13bc

[Kushal Kumaran]

[-- Attachment #1: Type: text/plain, Size: 296 bytes --]

Xīcò <xico@atelo.org> writes:

> Also, commit 3c13bc introduced a call to fdatasync() which is not
> available on FreeBSD, and probably not either on MacOS at least.
>

fdatasync is POSIX:
http://pubs.opengroup.org/onlinepubs/009695399/functions/fdatasync.html

-- 
regards,
kushal

[-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --]

Re: Linux-only fdatasync() in 3c13bc

[Tomi Ollila]

On Wed, May 07 2014, Kushal Kumaran <kushal.kumaran+notmuch@gmail.com> wrote:

> Xīcò <xico@atelo.org> writes:
>
>> Also, commit 3c13bc introduced a call to fdatasync() which is not
>> available on FreeBSD, and probably not either on MacOS at least.
>>
>
> fdatasync is POSIX:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/fdatasync.html

No wonder it is problematic, then >;)

> -- 
> regards,
> kushal

Tomi

Re: Linux-only fdatasync() in 3c13bc

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Wed, May 07 2014, Kushal Kumaran <kushal.kumaran+notmuch@gmail.com> wrote:
>
>> Xīcò <xico@atelo.org> writes:
>>
>>> Also, commit 3c13bc introduced a call to fdatasync() which is not
>>> available on FreeBSD, and probably not either on MacOS at least.
>>>
>>
>> fdatasync is POSIX:
>> http://pubs.opengroup.org/onlinepubs/009695399/functions/fdatasync.html
>
> No wonder it is problematic, then >;)
>

I seem to recall Austin saying on IRC that this usage was guaranteed to
call fsync anyway. Comments Austin?

d

Re: Linux-only fdatasync() in 3c13bc

[Austin Clements]

Quoth David Bremner on May 07 at 10:17 pm:
> Tomi Ollila <tomi.ollila@iki.fi> writes:
> 
> > On Wed, May 07 2014, Kushal Kumaran <kushal.kumaran+notmuch@gmail.com> wrote:
> >
> >> Xīcò <xico@atelo.org> writes:
> >>
> >>> Also, commit 3c13bc introduced a call to fdatasync() which is not
> >>> available on FreeBSD, and probably not either on MacOS at least.
> >>>
> >>
> >> fdatasync is POSIX:
> >> http://pubs.opengroup.org/onlinepubs/009695399/functions/fdatasync.html
> >
> > No wonder it is problematic, then >;)
> >
> 
> I seem to recall Austin saying on IRC that this usage was guaranteed to
> call fsync anyway. Comments Austin?

Yes, since the size of the file will have definitely changed, the
metadata will have to be flushed anyway, so using fdatasync here has
no advantage over using fsync.

[PATCH] notmuch-dump: use fsync instead of fdatasync

[David Bremner]

Since the file size will have changed, there is no performance benefit
to calling fdatasync.  Somewhat surprisingly, using fdatasync
apparently causes portability problems on FreeBSD.
---
 notmuch-dump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/notmuch-dump.c b/notmuch-dump.c
index 2849eab..887a208 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -169,7 +169,7 @@ notmuch_database_dump (notmuch_database_t *notmuch,
     }
 
     if (output_file_name) {
-	ret = fdatasync (outfd);
+	ret = fsync (outfd);
 	if (ret) {
 	    fprintf (stderr, "Error syncing %s to disk: %s\n",
 		     name_for_error, strerror (errno));
-- 
1.9.2

Re: [PATCH] notmuch-dump: use fsync instead of fdatasync

[Tomi Ollila]

On Thu, May 08 2014, David Bremner <david@tethera.net> wrote:

> Since the file size will have changed, there is no performance benefit
> to calling fdatasync.  Somewhat surprisingly, using fdatasync
> apparently causes portability problems on FreeBSD.
> ---

Well, this is easy to review! LGTM.

Tomi


>  notmuch-dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/notmuch-dump.c b/notmuch-dump.c
> index 2849eab..887a208 100644
> --- a/notmuch-dump.c
> +++ b/notmuch-dump.c
> @@ -169,7 +169,7 @@ notmuch_database_dump (notmuch_database_t *notmuch,
>      }
>  
>      if (output_file_name) {
> -	ret = fdatasync (outfd);
> +	ret = fsync (outfd);
>  	if (ret) {
>  	    fprintf (stderr, "Error syncing %s to disk: %s\n",
>  		     name_for_error, strerror (errno));
> -- 
> 1.9.2

Re: [PATCH] notmuch-dump: use fsync instead of fdatasync

[David Bremner]

David Bremner <david@tethera.net> writes:

> Since the file size will have changed, there is no performance benefit
> to calling fdatasync.  Somewhat surprisingly, using fdatasync
> apparently causes portability problems on FreeBSD.
> ---

pushed to release and master.

d