Use fdatasync properly instead of fsync where appropriate

RESOLVED FIXED in Firefox 57

Status

()

defect
P3
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: ishikawa, Assigned: liangwei012, Mentored)

Tracking

(Blocks 2 bugs, {perf})

unspecified
mozilla57
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

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
Summary: Uset fdatasync properly instead of fdata where appropriate → Uset fdatasync properly instead of fsync where appropriate
Assignee: Jan.Varga → nobody
Component: SQL → Storage
Product: Core → Toolkit
QA Contact: owen.marshall+bmo
(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
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.
Depends on: SQLite3.8.8.1
Keywords: perf
Blocks: 487375
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
Mentor: mak77
Whiteboard: [good first bug][lang=cpp]
Summary: Uset fdatasync properly instead of fsync where appropriate → Use fdatasync properly instead of fsync where appropriate
I would like to work on this bug (its my first can you give me some tips)
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.
Priority: -- → P3
Whiteboard: [good first bug][lang=cpp] → [good first bug][lang=c++]
Hi ! I am new developer. Can I work on this bug ?
sure you can
Attachment #8902107 - Flags: review?(mak77)
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
Attachment #8902107 - Flags: review?(mak77)
Attachment #8902107 - Flags: review?(mh+mozilla)
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.
Assignee: nobody → liangwei012
Status: NEW → ASSIGNED
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.
Attachment #8902107 - Flags: review?(mh+mozilla)
Ah right, I had missed the if test -n "$MOZ_SYSTEM_SQLITE".
Attachment #8903472 - Flags: review?(mak77)
Attachment #8902107 - Attachment is obsolete: true
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...
Attachment #8903472 - Flags: review?(mak77)
Mike, the latest patch, apart from the patching issues, looks correct, right?
Flags: needinfo?(mh+mozilla)
Attachment #8903472 - Flags: review?(mak77)
Thanks for your help. Can you patch tehse two commits on landing? I will use 'hg commit --amend" in future.
(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.
Flags: needinfo?(mh+mozilla)
let's see if I can fix the mozreview...
Attachment #8903472 - Attachment is obsolete: true
Attachment #8903472 - Flags: review?(mak77)
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.
Attachment #8907239 - Flags: review?(mh+mozilla) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/776e2f2b53a2
Use fdatasync properly instead of fsync where appropriate. r=glandium
https://hg.mozilla.org/mozilla-central/rev/776e2f2b53a2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 484119
You need to log in before you can comment on or make changes to this bug.