Last Comment Bug 681085 - sync cache map file after writing the header
: sync cache map file after writing the header
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Michal Novotny (:michal)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-22 15:01 PDT by Michal Novotny (:michal)
Modified: 2011-08-29 10:48 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (825 bytes, patch)
2011-08-22 15:01 PDT, Michal Novotny (:michal)
jduell.mcbugs: review+
Details | Diff | Review

Description Michal Novotny (:michal) 2011-08-22 15:01:00 PDT
Created attachment 554975 [details] [diff] [review]
fix

There is missing PR_Sync() after writing the cache map header in nsDiskCacheMap::FlushHeader(). IMO it could happen that the dirty flag isn't written to the disk in case of an early crash. I guess that this could be the reason why Firefox is sometimes crashing at every startup as described in bug #529733 (e.g. comments #2 and #38).
Comment 1 Jason Duell [:jduell] (needinfo? me) 2011-08-22 15:12:42 PDT
Comment on attachment 554975 [details] [diff] [review]
fix

Review of attachment 554975 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, this sounds fine in theory, and it has no downside (other than the cost of an extra PR_Sync), so let's land it and see if it fixes things.

But I'm a little confused--we also have a patch +r'd for bug 529733 that's been sitting around for quite a while.  Shouldn't we land that too?
Comment 2 Michal Novotny (:michal) 2011-08-22 15:33:47 PDT
http://hg.mozilla.org/mozilla-central/rev/212179627888
Comment 3 Boris Zbarsky [:bz] 2011-08-22 23:09:25 PDT
How often is FlushHeader() called?  sync() calls are something we've generally been trying to avoid....
Comment 4 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-22 23:41:27 PDT
I agree with Boris. I would go further and say that the cache should *never* be syncing to disk and any such synching disk should be considered a bug.

IRRC, most OSs will still complete a successful write even if the app crashes without syncing it. Syncs are helpful primarily for operating-system crashes, not application-level crashes.
Comment 5 Michal Novotny (:michal) 2011-08-23 01:55:35 PDT
(In reply to Boris Zbarsky (:bz) from comment #3)
> How often is FlushHeader() called?  sync() calls are something we've
> generally been trying to avoid....

It is called only when opening and closing the cache map. This means it is called when we initialize the disk device, close the disk device and when we clear the whole cache.
Comment 6 Jason Duell [:jduell] (needinfo? me) 2011-08-23 15:21:06 PDT
So is the better way to do this to write a hash of the cache map to disk at shutdown, and compare it at startup?  That was Brian's suggestion for avoiding fsync on the phone call today.

Backout this patch in the meantime?

FWIW: while Chromium has a FlushFile function that's a wrapper around fsync, I don't see it used anywhere in their disk_cache directory.  They have an "EntryImpl::Flush" function (EntryImpl is a single cache entry), but all it does is write from memory to disk storage, w/o any fsync AFAICT.
Comment 7 Michal Novotny (:michal) 2011-08-29 10:48:02 PDT
(In reply to Jason Duell (:jduell) from comment #6)
> So is the better way to do this to write a hash of the cache map to disk at
> shutdown, and compare it at startup?  That was Brian's suggestion for
> avoiding fsync on the phone call today.

This won't work. We don't write cache map entries continuously. We flush them all at shutdown, so the hash would be invalid only for a while during the flush at shutdown.

Note You need to log in before you can comment on or make changes to this bug.