Closed Bug 1292623 Opened 3 years ago Closed 3 years ago

Write only HTTP cache metadata after shutdown

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 2 obsolete files)

I think it's worth to bypass data writes immediately after shutdown and only let finish metadata writing.  Writing data only is meaningless w/o writing also the metadata (invalid entries).  If the storage is fast enough (=faster then network), data+meta writes will probably already be done before shutdown anyway.
Note: pending metadata writes are flushed (scheduled immediately) in CacheFileIOManager::Shutdown by ShutdownMetadataWriteScheduling().
Whiteboard: [necko-active]
Attached patch v1 (obsolete) — Splinter Review
- adds killed flag also on a handle
- set only after shutdown when WriteInternal(aValidate == false) is hit on a handle
- this effectively disables any writing on that handle and renders the disk file (if even existing) invalid
- any write on a killed handle is bypassed

This allows to write only metadata (=validate disk files) while when there are data still to be written we don't mess with later adding metadata to the wrong place when data write only were bypassed.
Attachment #8778402 - Flags: review?(michal.novotny)
Comment on attachment 8778402 [details] [diff] [review]
v1

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

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +1958,5 @@
> +  }
> +
> +  if (CacheObserver::ShuttingDown() && !aValidate) {
> +    aHandle->mKilled = true;
> +    aHandle->mInvalid = true;

Is it necessary to set mInvalid flag? If the entry was valid and we failed to write the data it is still valid. The flag should be in sync with the state of the file on the disk, i.e. if mInvalid == false then metadata should fail to parse. The truth is that it's not 100% guaranteed by the code because overwriting some existing data doesn't corrupt metadata, but we don't do this anywhere right now.
Flags: needinfo?(honzab.moz)
(In reply to Michal Novotny (:michal) from comment #4)
> Comment on attachment 8778402 [details] [diff] [review]
> v1
> 
> Review of attachment 8778402 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheFileIOManager.cpp
> @@ +1958,5 @@
> > +  }
> > +
> > +  if (CacheObserver::ShuttingDown() && !aValidate) {
> > +    aHandle->mKilled = true;
> > +    aHandle->mInvalid = true;
> 
> Is it necessary to set mInvalid flag? If the entry was valid and we failed
> to write the data it is still valid. The flag should be in sync with the
> state of the file on the disk, i.e. if mInvalid == false then metadata
> should fail to parse. The truth is that it's not 100% guaranteed by the code
> because overwriting some existing data doesn't corrupt metadata, but we
> don't do this anywhere right now.

And this is exactly the reason why I tell everybody to write comments.  And then not doing it myself!  

Looking again, it doesn't seem necessary, maybe even counterproductive.  But changing it means to retest the whole thing, so I cannot give an answer right now.
Attached patch v2 (obsolete) — Splinter Review
- removed the mInvalid = true piece
- enhanced to also bypass when a file is not currently open ; reason is to bypass closing the least used handle (may take seconds) and opening a new one (may also take time)
- same code added to TruncateSeekSetEOFInternal
Attachment #8778402 - Attachment is obsolete: true
Attachment #8778402 - Flags: review?(michal.novotny)
Flags: needinfo?(honzab.moz)
Attachment #8779791 - Flags: review?(michal.novotny)
Blocks: 1288204
Comment on attachment 8779791 [details] [diff] [review]
v2

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

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +1956,5 @@
> +    LOG(("  handle already killed, nothing written"));
> +    return NS_OK;
> +  }
> +
> +  if (CacheObserver::ShuttingDown() && (!aValidate || !aHandle->mFD)) {

The question is how many entries we will be written with this condition during shutdown. This definitely needs an updated comment about mKilled in the header file, because it's no longer valid. We could also consider increasing kOpenHandlesLimit, what do you think?
Attachment #8779791 - Flags: review?(michal.novotny) → feedback+
(In reply to Michal Novotny (:michal) from comment #7)
> Comment on attachment 8779791 [details] [diff] [review]
> v2
> 
> Review of attachment 8779791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheFileIOManager.cpp
> @@ +1956,5 @@
> > +    LOG(("  handle already killed, nothing written"));
> > +    return NS_OK;
> > +  }
> > +
> > +  if (CacheObserver::ShuttingDown() && (!aValidate || !aHandle->mFD)) {
> 
> The question is how many entries we will be written with this condition
> during shutdown. 

I am not sure I fully understand the question here.

> This definitely needs an updated comment about mKilled in
> the header file, because it's no longer valid. 

Yep, will update after we agree on what to do here.

> We could also consider
> increasing kOpenHandlesLimit, what do you think?

I was thinking of it!  Here you may know the numbers better than me, tho.  IIRC, the limit is caused by NSPR?  AFAIK, on win the OS limit is only how much kernel memory you have and its definitely higher than 64 files.

According the queue length telemetry, we probably very often work with more than 64 files at the same time.
Flags: needinfo?(michal.novotny)
(In reply to Honza Bambas (:mayhemer) from comment #8)
> > ::: netwerk/cache2/CacheFileIOManager.cpp
> > @@ +1956,5 @@
> > > +    LOG(("  handle already killed, nothing written"));
> > > +    return NS_OK;
> > > +  }
> > > +
> > > +  if (CacheObserver::ShuttingDown() && (!aValidate || !aHandle->mFD)) {
> > 
> > The question is how many entries we will be written with this condition
> > during shutdown. 
> 
> I am not sure I fully understand the question here.

I just wanted to say that it's likely that most of the opened handles will be used by entries that are writing data. Entries that want to write metadata only are probably scheduled with CacheFileIOManager::ScheduleMetadataWrite and the nspr handle might be closed due to writing other entries. I don't say this is wrong, we just need to update the comment and possibly change kOpenHandlesLimit to increase the probability that entries with scheduled metadata write will be closed gracefully.

> > We could also consider
> > increasing kOpenHandlesLimit, what do you think?
> 
> I was thinking of it!  Here you may know the numbers better than me, tho. 
> IIRC, the limit is caused by NSPR?  AFAIK, on win the OS limit is only how
> much kernel memory you have and its definitely higher than 64 files.
> 
> According the queue length telemetry, we probably very often work with more
> than 64 files at the same time.

I don't know the limit on Windows. On Linux the default limit is 1024 per process. I don't know how much of this we can dedicate to cache. E.g. my firefox process has 94 opened handles right now (7 of them are used by the cache).
Flags: needinfo?(michal.novotny)
Attached patch v2.1Splinter Review
- comment more detailed
- handles limit 64 -> 128
Attachment #8779791 - Attachment is obsolete: true
Attachment #8785264 - Flags: review?(michal.novotny)
Attachment #8785264 - Flags: review?(michal.novotny) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cf38aa68a54
Only write HTTP cache entries' metadata after shutdown. r=michal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7cf38aa68a54
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.