979900 - Avoid completely filling cache partition on Gonk

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Michael Wu [:mwu]]

Right now, the default size limit for the http cache can run the cache partition out of space, which then interferes with FOTA updates and factory resets. Bug 976450 allows us to wipe the cache when we're out of space, but we should avoid running out of space in the first place.

Comment 1

[Honza Bambas (:mayhemer)]

Considering only the new cache, sounds like something similar to smart size in the old cache.

Comment 2

[Jason Duell]

mwu, is this with the new (cache2, not yet turned on by default) cache, or the old, existing-in-m-c cache?   The existing old cache should be using the smartsize algorithm to never take even a majority of the disk partition that it's on.

Comment 3

[Michael Wu [:mwu]]

(In reply to Jason Duell (:jduell) from comment #2)
> mwu, is this with the new (cache2, not yet turned on by default) cache, or
> the old, existing-in-m-c cache?   The existing old cache should be using the
> smartsize algorithm to never take even a majority of the disk partition that
> it's on.

This is with the old cache. Right now, it's configured to take all of the cache partition minus ~1mb. Unfortunately, this doesn't account for FS overhead, so we end up running out. http://hg.mozilla.org/mozilla-central/file/0086975029c3/b2g/chrome/content/shell.js#l1486

Comment 4

[Jason Duell]

Is this still happening?

Looking at the code we should never grab more than 40% of the partition the cache is on.

  http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsCacheService.cpp#603

Do we need to drop it down to 20% as we do for android?  (See lines just above link)

Comment 5

[Honza Bambas (:mayhemer)]

Ref: bug 996836 - to minimize time we are over the limit.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Jason Duell (:jduell) from comment #4)
> Is this still happening?
> 
> Looking at the code we should never grab more than 40% of the partition the
> cache is on.
> 
>  
> http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsCacheService.
> cpp#603
> 
> Do we need to drop it down to 20% as we do for android?  (See lines just
> above link)

I would have naively guessed that b2g was following the ANDROID preprocessor path and using the 20% as well. I thought the b2g and fennec both set ANDROID.

Comment 7

[[:fabrice] Fabrice Desré]

ANDROID is true on b2g too, but we tweak the cache size here:
https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#1491

I'm all for moving that out of shell.js if the underlying cache is smart enough.

Comment 8

[Jason Duell]

per comment 7, this is currently Gonk code that's being too aggressive with the size. 

https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js?rev=caa1de36d1ad#1300

I propose we remove that code and if someone can tell us the correct algorithm (and preprocessor macro to fork on) for b2g we're happy to stick it in the HTTP cache's smart size code.  But it could also be fixed in the Gonk code if that's preferable.

Comment 9

[Dave Hylands [:dhylands]]

MOZ_WIDGET_GONK is defined for all B2G devices (real devices - like phones and tablets), and the emulator. It isn't defined for B2G desktop or the simulator.

Is that the preprocessor macro you're looking for?

Comment 10

[[:fabrice] Fabrice Desré]

I don't see how to fix that on the gonk side reliably. Each device has a different cache partition size and I don't want to rely on a device-specific pref. So to move it to the HTTP smart size code, use the MOZ_WIDGET_GONK as Dave said, and look for the size of the /cache partition.

Comment 11

[Michal Novotny [:michal]]

HTTP cache's smart size code cannot handle this problem since the reported cache size is not precise and it differs from what the cache on the disk really takes because:

- we internally round to 1kB
- we don't count doomed entries, directories, index files and other cache related files to the cache size

I.e. we cannot avoid filling the partition just by setting the cache size. Instead, we need to check the free space periodically (or maybe on every write) and start eviction or fail to write in case of low free space.

My proposal is to have two limits, soft and hard. After reaching the first we would start eviction and after reaching the hard limit, we would fail to write the data. The reason to have 2 limits is that the eviction process runs always with low priority and we cannot ensure that anything is really evicted synchronously and sometimes even in a reasonable time.

Comment 12

[Michal Novotny [:michal]]

Attachment: fix

Comment 13

[Michal Novotny [:michal]]

Depends on 1013395 because the patch in that bug fixes some issues in error handling in CacheFile, CacheFileChunk and input/output stream.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8448030 [details] [diff] [review]
fix

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

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2525,5 @@
> +  rv = mCacheDirectory->GetDiskSpaceAvailable(&freeSpace);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    // Do not change smart size.
> +    LOG(("CacheFileIOManager::EvictIfOverLimitInternal() - "
> +         "GetDiskSpaceAvailable() failed! [rv=0x%08x]", rv));

you should freeSpace = -1 here, when a method fails, the result is by definition 'undefined'.

::: netwerk/cache2/CacheObserver.cpp
@@ +152,5 @@
>  
>    mozilla::Preferences::AddUintVarCache(
> +    &sDiskFreeSpaceSoftLimit, "browser.cache.disk.free_space_soft_limit", kDefaultDiskFreeSpaceSoftLimit);
> +  mozilla::Preferences::AddUintVarCache(
> +    &sDiskFreeSpaceHardLimit, "browser.cache.disk.free_space_hard_limit", kDefaultDiskFreeSpaceHardLimit);

put these to all.js, I could also imagine diametrically different values for desktop, mobile and b2g.  Personally I'd leave a much more space on desktop (other software may want to swap and there is much more space for us even we leave more free disk space)

Comment 15

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #14)
> I could also imagine diametrically different values for desktop, mobile and
> b2g.  Personally I'd leave a much more space on desktop (other software may
> want to swap and there is much more space for us even we leave more free disk
> space)

You missed the point of this preference. It's intended to handle special cases like the one mentioned in comment 7. It is more or less impossible to fill the partition when smartsizing is turned on which is by default. Even when the smartsizing is disabled the default disk capacity is reasonable small. Just in case somebody sets the cache capacity to a very large value or when somebody places the cache directory to a small partition, we can hit this problem.

Comment 16

[Michal Novotny [:michal]]

Attachment: patch v2

https://tbpl.mozilla.org/?tree=Try&rev=930f6f25d554

Comment 17

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90c8436f8851

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90c8436f8851

Comment 19

[Jason Duell]

So we've fixed this on the http cache side.  Do we need 1) to uplift? (Sounds like no?) and/or 2) open a new bug to get rid of the gonk-side code that's mentioned in comment 7?

Comment 20

[[:fabrice] Fabrice Desré]

(In reply to Jason Duell (:jduell) from comment #19)
> So we've fixed this on the http cache side.  Do we need 1) to uplift?
> (Sounds like no?) and/or 2) open a new bug to get rid of the gonk-side code
> that's mentioned in comment 7?

No need to uplift, and yes, let's remove the b2g hack!

Comment 21

[Jason Duell]

ok, filed bug 1040371 for the gonk code removal.