1120444 - Use fdatasync properly instead of fsync where appropriate

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect, P3)

Description

[ISHIKAWA, Chiaki (I will be tied up until Dec 14 with day job events.)]

It was advised to move the discussion to a new bugzilla entry in
Bug 421482 - Firefox 3 uses fsync excessively

So here it is.

I found that 
|fdatasync| was defined to be macro for |fsync| in
the current mozilla sqlite source code. (well at least until two days ago in the upstream according to [2].)

This causes enough additional disk activity to be noticed by testing [1].

Sqlite seems to have been modified to properly use fdatasync where applicable in the last few days.
[2]

So I propose that mozilla source code use fdatasync appropriately where it can be used (under modern linux for example).

At the same time, we may have to think about old linux distributions where fdatasync was buggy [3].
It may be even necessary for 
making a run-time decision which system call to invoke.
But please note the added overhead of run-time decision performed only once  when fdatasync is to be called for the first time is
peanut compared with the added overhead of calling fsync many times unnecessarily in place of more appropriate fdatasync calls.

TIA

[1] https://lists.mozilla.org/pipermail/dev-platform/2014-May/004762.html

[2] https://bugzilla.mozilla.org/show_bug.cgi?id=421482#c197
--- begin quote ---
sqlite used to use fdatasync on linux before 3.7.10, and made the conscious choice not to use it in 3.7.10, three years ago. OTOH, it happens that it was reenabled on platforms that pass the AC_CHECK_FUNCS(fdatasync) test two days ago[1], so in fact, next version of sqlite will have it... if we add an AC_CHECK_FUNCS to our configure. And now looking further, I see the reason it was disabled in the first place is because Android's libc doesn't have it[2].

1. https://github.com/mackyle/sqlite/commit/55e2a047b98fac03643753d07d06c3bf157d77ff
2. https://github.com/mackyle/sqlite/commit/e29af45dcfbe5e00fc0a947a6a5009917783115e
--- end quote

[3] https://bugzilla.mozilla.org/show_bug.cgi?id=487375#c39

Comment 1

[Howard Chu]

(In reply to ISHIKAWA, Chiaki from comment #0)

> At the same time, we may have to think about old linux distributions where
> fdatasync was buggy [3].
> It may be even necessary for 
> making a run-time decision which system call to invoke.

As an example, we've added this runtime check to LMDB. It occurs once when a DB env is opened.
http://www.openldap.org/devel/gitweb.cgi?p=openldap.git;a=blob;f=libraries/liblmdb/mdb.c;h=84cd8a35a0bfdcd9253f7f3cd12be23e83c782e3;hb=HEAD#l3962

Comment 2

[Marco Bonardo [:mak]]

I think we should try to use fdatasync on Linux, when we'll get HAVE_FDATASYNC support in Sqlite. We'll have to properly skip Android.

We could even do before by using fdatasync=fdatasync, but I feel like we can wait next Sqlite version, since it's affecting Linux only (I'm not understating the importance of Linux, but since the benefit doesn't affect most of the population, it's worth to have a better fix rather than a workaround).

On the other side, I don't think it's worth to do acrobatics to skip broken kernels, it was more than 2 years ago when the kernel issue was fixed. But thanks for pointing out the code for a dynamic check.

Comment 3

[Marco Bonardo [:mak]]

If anyone wants to work on this, should just be matter of using AC_CHECK_FUNCS(fdatasync) in configure.in (in the Sqlite section) and set HAVE_FDATASYNC if it succeeds (something like AC_CHECK_FUNCS(fdatasync, HAVE_FDATASYNC=1, HAVE_FDATASYNC=) (please correct me if I'm wrong)

Mike Hommey can review the change to configure.in

Comment 4

[harsh shah]

I would like to work on this bug (its my first can you give me some tips)

Comment 5

[Marco Bonardo [:mak]]

There is a section of old-configure.in dedicated to Sqlite checks
http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/old-configure.in#4138

There we need to add a check that sets HAVE_FDATASYNC=1 if fdatasync is defined in libc
I suggested adding
AC_CHECK_FUNCS(fdatasync, HAVE_FDATASYNC=1, HAVE_FDATASYNC=)
plus some dnl lines to describe what's up. You can see the other checks there for examples, even if those are far more complex than what we need here.

Comment 6

[manish.patel.mnnit]

Hi ! I am new developer. Can I work on this bug ?

Comment 7

[Marco Bonardo [:mak]]

sure you can

Comment 8

[Marco Bonardo [:mak]]

this document should help with the process:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

Comment 9

[Wei Liang]

Attachment: Bug 1120444 - Use fdatasync properly instead of fsync where appropriate

Review commit: https://reviewboard.mozilla.org/r/173546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173546/

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8902107 [details]
Bug 1120444 - Use fdatasync properly instead of fsync where appropriate

https://reviewboard.mozilla.org/r/173546/#review179518

::: old-configure.in:3561
(Diff revision 1)
> +    dnl ==============================
> +    dnl === SQLite fdatasync check ===
> +    dnl ==============================
> +    dnl Check to see if fdatasync is available
> +    AC_CHECK_FUNCS(fdatasync, HAVE_FDATASYNC=1, HAVE_FDATASYNC=)
> +    

there are some trailing spaces here

Comment 11

[Marco Bonardo [:mak]]

I'm forwarding the review to someone who has far mor e knowledge of the configure system than me, I don't know if we have new ways to modernize this code or it's fine to have good old configure syntax. Off-hand it looks ok.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8902107 [details]
Bug 1120444 - Use fdatasync properly instead of fsync where appropriate

https://reviewboard.mozilla.org/r/173546/#review179886

::: old-configure.in:3560
(Diff revision 1)
>  
> +    dnl ==============================
> +    dnl === SQLite fdatasync check ===
> +    dnl ==============================
> +    dnl Check to see if fdatasync is available
> +    AC_CHECK_FUNCS(fdatasync, HAVE_FDATASYNC=1, HAVE_FDATASYNC=)

AC_CHECK_FUNCS(fdatasync) is actually enough (setting HAVE_FDATASYNC appropriately is implied)

However, this is the wrong place to put this, as this applies only to building against system sqlite3, which makes us *not* built the in-tree version, making HAVE_FDATASYNC useless.

Comment 13

[Marco Bonardo [:mak]]

Ah right, I had missed the if test -n "$MOZ_SYSTEM_SQLITE".

Comment 14

[Wei Liang]

Attachment: Bug 1120444 - Use fdatasync properly instead of fsync where appropriate

Review commit: https://reviewboard.mozilla.org/r/175308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175308/

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8903472 [details]
Bug 1120444 - Use fdatasync properly instead of fsync where appropriate

https://reviewboard.mozilla.org/r/175308/#review180358

You should merge the patches before pushing to mozreview, or alternatively, enable the Mercurial evolve extension and use hg amend after making changes, that will squash your changes to the previous commit.
In any case, if you're having troubles with that, I can fix the patch on landing for you.

::: old-configure.in:3560
(Diff revision 1)
>  then
>      dnl ============================
>      dnl === SQLite Version check ===
>      dnl ============================
>      dnl Check to see if the system SQLite package is new enough.
> -    PKG_CHECK_MODULES(SQLITE, sqlite3 >= $SQLITE_VERSION)
> +    PKG_CHECK_MODULES(SQLITE, sqlite3 >= $SQLITE_VERSION)  

you introduced trailing spaces here

::: old-configure.in:3586
(Diff revision 1)
>              ac_cv_sqlite_secure_delete=no
>          )
>      ])
>      AC_MSG_RESULT($ac_cv_sqlite_secure_delete)
>      CFLAGS="$_SAVE_CFLAGS"
> -    LIBS="$_SAVE_LIBS"
> +    LIBS="$_SAVhg pull -r 5c627956860516c0144b41e7a9c9004024901742 https://reviewboard-hg.mozilla.org/geckoE_LIBS"

Looks like you typed a command while having the editor focused...

Comment 16

[Marco Bonardo [:mak]]

Mike, the latest patch, apart from the patching issues, looks correct, right?

Comment 17

[Wei Liang]

Comment on attachment 8903472 [details]
Bug 1120444 - Use fdatasync properly instead of fsync where appropriate

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175308/diff/1-2/

Comment 18

[Wei Liang]

Thanks for your help. Can you patch tehse two commits on landing? I will use 'hg commit --amend" in future.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Marco Bonardo [::mak] from comment #16)
> Mike, the latest patch, apart from the patching issues, looks correct, right?

More than the previous one, but it would be even better if the check wasn't performed when we're building against system sqlite.

Comment 20

[Marco Bonardo [:mak]]

let's see if I can fix the mozreview...

Comment 21

[Marco Bonardo [:mak]]

Attachment: Bug 1120444 - Use fdatasync properly instead of fsync where appropriate.

Review commit: https://reviewboard.mozilla.org/r/178908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178908/

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8907239 [details]
Bug 1120444 - Use fdatasync properly instead of fsync where appropriate.

https://reviewboard.mozilla.org/r/178908/#review184822

::: old-configure.in:3705
(Diff revision 1)
> +else
> +    dnl ==============================
> +    dnl === SQLite fdatasync check ===
> +    dnl ==============================
> +    dnl Check to see if fdatasync is available
> +    AC_CHECK_FUNCS(fdatasync)

Note you can also use AC_CHECK_FUNC, since you're only checking one function. That makes not practical difference.

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 8907239 [details]
Bug 1120444 - Use fdatasync properly instead of fsync where appropriate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178908/diff/1-2/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/776e2f2b53a2
Use fdatasync properly instead of fsync where appropriate. r=glandium

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/776e2f2b53a2