sync cache map file after writing the header

RESOLVED FIXED in mozilla9

Status

()

Core
Networking: Cache
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: michal, Assigned: michal)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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).
Attachment #554975 - Flags: review?(bzbarsky)
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?
Attachment #554975 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 2

6 years ago
http://hg.mozilla.org/mozilla-central/rev/212179627888
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Comment 3

6 years ago
How often is FlushHeader() called?  sync() calls are something we've generally been trying to avoid....
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.
(Assignee)

Comment 5

6 years ago
(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.
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.
(Assignee)

Comment 7

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.