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

(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.

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

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.

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

sure you can

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

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

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

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

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

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

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.

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

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.

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/776e2f2b53a2 Use fdatasync properly instead of fsync where appropriate. r=glandium