Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862943002:
SimpleCache: try to recover on CreateEntry on missing cache dir (Closed)
|
Created:
3 years, 7 months ago by Maks Orlovich Modified:
3 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWe seem to be failing a non-trivial portion of creates on Android
in this case, and it's easy enough to just mkdir it --- that will
happen eventually on index saving, anyway, but may take a long time
in the case where the user is actively browsing.
BUG=718900
Review-Url: https://codereview.chromium.org/2862943002
Cr-Commit-Position: refs/heads/master@{#471770}
Committed: https://chromium.googlesource.com/chromium/src/+/2d841835e729da2bef43897070bc83568e783b4c
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add test Tweak code to not have an action after many pure conditionals. #Patch Set 3 : Rebase #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== Prototype for retrying creates that fail due to missing dir. Needs tests (though I am kinda afraid of rm -rf'ing in them), and probably UMA? BUG= ========== to ========== We seem to be failing a non-trivial portion of creates on Android in this case, and it's easy enough to just mkdir it --- that will happen eventually on index saving, anyway. BUG=718900 ==========
Description was changed from ========== We seem to be failing a non-trivial portion of creates on Android in this case, and it's easy enough to just mkdir it --- that will happen eventually on index saving, anyway. BUG=718900 ========== to ========== We seem to be failing a non-trivial portion of creates on Android in this case, and it's easy enough to just mkdir it --- that will happen eventually on index saving, anyway, but may take a long time in the case where the user is actively browsing. BUG=718900 ==========
morlovich@chromium.org changed reviewers: + mmenke@chromium.org, pasko@chromium.org
(Including mmenke as the test would need //net approval, not just simple_cache)
mmenke@chromium.org changed reviewers: + jkarlin@chromium.org
[+jkarlin]: Deferring to jkarlin on this one.
https://codereview.chromium.org/2862943002/diff/1/net/disk_cache/simple/simpl... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/2862943002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_synchronous_entry.cc:879: files_[file_index].Initialize(filename, flags); I worry a bit about the state of the cache being strange here. E.g., we write some entry but don't have an index. How about instead we simply write the empty index out to disk after the user clears the cache?
lgtm, thank you
https://codereview.chromium.org/2862943002/diff/1/net/disk_cache/simple/simpl... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/2862943002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_synchronous_entry.cc:879: files_[file_index].Initialize(filename, flags); On 2017/05/05 17:16:21, jkarlin wrote: > I worry a bit about the state of the cache being strange here. E.g., we write > some entry but don't have an index. How about instead we simply write the empty > index out to disk after the user clears the cache? Writing index here would probably fix the issue, but it looks a bit too tightly coupled this way IMO. The philosophy of the index is that its existence is not necessary for correct functioning of the backend (modulo eviction - adding more states beyond eviction would complicate things like interpreting UMA metrics). Also index file lifecycle depends a lot on OS/environment behavior, better to keep these platform differences small. So I am more with morlovich@ here.
lgtm https://codereview.chromium.org/2862943002/diff/1/net/disk_cache/simple/simpl... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/2862943002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_synchronous_entry.cc:879: files_[file_index].Initialize(filename, flags); On 2017/05/05 17:24:15, pasko wrote: > On 2017/05/05 17:16:21, jkarlin wrote: > > I worry a bit about the state of the cache being strange here. E.g., we write > > some entry but don't have an index. How about instead we simply write the > empty > > index out to disk after the user clears the cache? > > Writing index here would probably fix the issue, but it looks a bit too tightly > coupled this way IMO. The philosophy of the index is that its existence is not > necessary for correct functioning of the backend (modulo eviction - adding more > states beyond eviction would complicate things like interpreting UMA metrics). > Also index file lifecycle depends a lot on OS/environment behavior, better to > keep these platform differences small. So I am more with morlovich@ here. Okay, works with me.
> https://codereview.chromium.org/2862943002/diff/1/net/disk_cache/simple/simpl... > net/disk_cache/simple/simple_synchronous_entry.cc:879: > files_[file_index].Initialize(filename, flags); > I worry a bit about the state of the cache being strange here. E.g., we write > some entry but don't have an index. It's definitely an issue, but the state is different from what you describe: I think we rather have an in-memory index that has entries that are no longer there (as well as an out-of-date disk usage number), so I think we will be (and already are) doing reads that fail on open and incrementally fixing it up, and also doing some "evictions" that kick out entries that aren't even there. Or do you mean the new entry is there but not on an on-disk index? Well, we'll either snapshot an index with it, or do a recovery on next start, so that should be perfectly fine. SimpleCache.Http.SyncOpenResult_WithIndex with result == Platform File Error does look higher on Android as well, BTW (but a few times lower than the create failures, and of course with much gentler outcome). > How about instead we simply write the empty index out to disk after the user clears the cache? The timing of "after the user clears the cache" is kinda funny here, since Android doing it, not us, and it's tricky since one would have to a merge with pending operations (like we do on startup). That seems... substantially more complicated. Like what happens when someone extends an entry that's been opened since before the cache clear? (I didn't actually think to try the chrome UI data clear)
On 2017/05/05 17:44:55, Maks Orlovich wrote: > > > https://codereview.chromium.org/2862943002/diff/1/net/disk_cache/simple/simpl... > > net/disk_cache/simple/simple_synchronous_entry.cc:879: > > files_[file_index].Initialize(filename, flags); > > I worry a bit about the state of the cache being strange here. E.g., we write > > some entry but don't have an index. > > It's definitely an issue, but the state is different from what you describe: I > think we rather > have an in-memory index that has entries that are no longer there (as well as an > out-of-date disk > usage number), so I think we will be (and already are) doing reads that fail on > open and incrementally > fixing it up, and also doing some "evictions" that kick out entries that aren't > even there. Or do you > mean the new entry is there but not on an on-disk index? Well, we'll either > snapshot an index with it, > or do a recovery on next start, so that should be perfectly fine. > > SimpleCache.Http.SyncOpenResult_WithIndex with result == Platform File Error > does look higher on Android > as well, BTW (but a few times lower than the create failures, and of course with > much gentler outcome). > > > How about instead we simply write the empty index out to disk after the user > clears the cache? > > The timing of "after the user clears the cache" is kinda funny here, since > Android doing it, not > us, and it's tricky since one would have to a merge with pending operations > (like we do on startup). > That seems... substantially more complicated. Like what happens when someone > extends an entry that's > been opened since before the cache clear? > > (I didn't actually think to try the chrome UI data clear) Ah, I was thinking about clearing the cache from Chrome, not from Android. Okay, this slgtm.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
On 2017/05/05 17:44:55, Maks Orlovich wrote: > The timing of "after the user clears the cache" is kinda funny here, since > Android doing it, not > us, and it's tricky since one would have to a merge with pending operations > (like we do on startup). > That seems... substantially more complicated. Like what happens when someone > extends an entry that's > been opened since before the cache clear? fwiw, I don't see anything tricky here: we will continue working with cache entries and the in-memory index as usual. Writing to an entry, reading from it, will succeed until the entry is closed even if the file was unlinked (because POSIX specifies refcounting FDs). Occasionally a few things known to the index would not be found on disk, which makes things slower to make a network fallback. Eviction tolerates racing index and disk by design. Any other tricky states?
> fwiw, I don't see anything tricky here: we will continue working with cache > entries and the in-memory index as usual. Writing to an entry, reading from it, > will succeed until the entry is closed even if the file was unlinked (because > POSIX specifies refcounting FDs). Occasionally a few things known to the index > would not be found on disk, which makes things slower to make a network > fallback. Eviction tolerates racing index and disk by design. Any other tricky > states? The tricky thing was mostly about what would happen if we tried to do something different, and yeah, was 50% based on me forgetting unlink's semantics. Anyway, status of this: holding off a bit more, since the CRC CL only made it to Android Canary on Mondayish, meaning I don't have much data on its impact yet (it did have an unexpected bonus of separating it out in time with the SSL change which may have also helped things)
On 2017/05/11 13:09:45, Maks Orlovich wrote: > > fwiw, I don't see anything tricky here: we will continue working with cache > > entries and the in-memory index as usual. Writing to an entry, reading from > it, > > will succeed until the entry is closed even if the file was unlinked (because > > POSIX specifies refcounting FDs). Occasionally a few things known to the index > > would not be found on disk, which makes things slower to make a network > > fallback. Eviction tolerates racing index and disk by design. Any other tricky > > states? > > The tricky thing was mostly about what would happen if we tried to do something > different, > and yeah, was 50% based on me forgetting unlink's semantics. > > Anyway, status of this: holding off a bit more, since the CRC CL only made it to > Android > Canary on Mondayish, meaning I don't have much data on its impact yet (it did > have an unexpected > bonus of separating it out in time with the SSL change which may have also > helped things) OK OK, thanks for the update
The CQ bit was checked by morlovich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by morlovich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2862943002/#ps40001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494861136607490,
"parent_rev": "2421ee1aeb0c71573527fd33145ce1e0b18a9888", "commit_rev":
"2d841835e729da2bef43897070bc83568e783b4c"}
Message was sent while issue was closed.
Description was changed from ========== We seem to be failing a non-trivial portion of creates on Android in this case, and it's easy enough to just mkdir it --- that will happen eventually on index saving, anyway, but may take a long time in the case where the user is actively browsing. BUG=718900 ========== to ========== We seem to be failing a non-trivial portion of creates on Android in this case, and it's easy enough to just mkdir it --- that will happen eventually on index saving, anyway, but may take a long time in the case where the user is actively browsing. BUG=718900 Review-Url: https://codereview.chromium.org/2862943002 Cr-Commit-Position: refs/heads/master@{#471770} Committed: https://chromium.googlesource.com/chromium/src/+/2d841835e729da2bef43897070bc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2d841835e729da2bef43897070bc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
