<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Hello,<br>
</p>
I am attaching an improved version of the patch posted before.<br>
It is based on v4 r15081. What was fixed:<br>
<br>
1. Correctly purge StoreEntries, 'disconnected' from the Store<br>
(unlinking by a valid cache_key in these cases).<br>
2. Do not release just created keyless StoreEntries. This caused<br>
several problems, such as calling key-dependent methods with<br>
nil key, setting the private key twice and setting the private
key<br>
to be overwritten by the public key later in some other callers.<br>
3. Fixed tests/testRock test case, to conform to the<br>
"do not overwrite an existing (and identical) disk entry"<br>
requirement.<br>
4. Documented several methods and eliminated some code<br>
duplication (hasMemStore() and hasTransients() helper<br>
methods). More work needed in this direction.<br>
<br>
<br>
Eduard.<br>
<br>
<div class="moz-cite-prefix">On 01.06.2017 19:27, Eduard Bagdasaryan
wrote:<br>
</div>
<blockquote
cite="mid:68a0d7fb-6e14-195c-b167-fc9bdbea4643@measurement-factory.com"
type="cite">Hello,
<br>
<br>
This patch fixes Squid bug 4505.
<br>
<br>
Before this change, SMP Squid with SMP-aware caches sometimes
<br>
does not purge cache entries.
<br>
<br>
When Squid finds a requested entry in the memory cache, it does
not
<br>
check whether the same entry is also stored in a cache_dir. The
<br>
StoreEntry object may become associated with its store entry in
the
<br>
memory cache but not with its store entry on disk. This
inconsistency
<br>
causes two known problems:
<br>
<br>
1. Squid may needlessly swap out the memory hit to disk, either
<br>
overwriting an existing (and identical) disk entry or, worse,
<br>
creating a duplicate entry on another disk. In the second case,
the
<br>
two disk entries are not synchronized and may eventually start
to
<br>
differ if one of them is removed or updated.
<br>
<br>
2. Squid may not delete a stale disk entry when needed, violating
<br>
various HTTP MUSTs, and eventually serving stale [disk] cache
entries
<br>
to clients.
<br>
<br>
Another purging problem is not caused by the above inconsistency:
<br>
<br>
3. A DELETE request or equivalent may come for the entry which is
still
<br>
locked for writing. Squid fails to get a lock for such an entry
(in
<br>
order to purge it) and the entry remains in disk and/or memory
cache.
<br>
<br>
To solve the first two problems:
<br>
<br>
* StoreEntry::mayStartSwapout() now avoids needless swapouts by
checking
<br>
whether StoreEntry was fully loaded, is being loaded, or could
have
<br>
been loaded from disk.
<br>
<br>
To fix problem #3:
<br>
<br>
* A new Store::Controller::unlinkByKey() method purges (or marks
for
<br>
deletion if purging is impossible) all the matching store
entries,
<br>
without loading the StoreEntry information from stores. The lack
of
<br>
StoreEntry creation reduces waste of resources (the StoreEntry
object
<br>
would have to be deleted anyway) _and_ allows us to mark
being-created
<br>
entries (that are locked for writing and, hence, cannot be
loaded into
<br>
a StoreEntry object).
<br>
<br>
Note that even if Squid properly avoids storing duplicate disk
entries,
<br>
some cache_dir manipulations by humans and Squid crashes may lead
to
<br>
such duplicates being present. This patch leaves dealing with
potential
<br>
duplicates out of scope except it guarantees that if an entry is
<br>
deleted, then all [possible] duplicates are deleted as well.
<br>
<br>
Also added StoreEntry helper methods to prevent direct
manipulation of
<br>
individual disk-related data members (swap_dirn, swap_filen, and
<br>
swap_status). These methods help keep these related data members
in a
<br>
coherent state and minimize code duplication.
<br>
<br>
Also tightened rules around several Storage methods: A storage
method
<br>
must not be given a StoreEntry object unless that object belongs
to that
<br>
storage.
<br>
<br>
Also do not mark Transients entry as "waitingToBeFreed" when the
entry
<br>
is simply closed for writing. This flag is used to mark locked
entries
<br>
for deletion and should not be used for "completed" transient
entries to
<br>
avoid confusion.
<br>
<br>
XXX: SMP cache purges may continue to malfunction when the
Transients
<br>
table is missing. Currently, Transients are created only when the
<br>
collapsed_forwarding is on. After Squid bug 4579 is fixed, every
public
<br>
StoreEntry will have the corresponding Transients entry and vice
versa,
<br>
extending these fixes to all SMP environments.
<br>
<br>
Since this work started long ago and is v4-based, I am attaching
the
<br>
existing v4 patch for review now and start converting it to v5
version.
<br>
I plan to post final v5 version after the review is over.
<br>
<br>
<br>
Thanks,
<br>
Eduard.
<br>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
squid-dev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:squid-dev@lists.squid-cache.org">squid-dev@lists.squid-cache.org</a>
<a class="moz-txt-link-freetext" href="http://lists.squid-cache.org/listinfo/squid-dev">http://lists.squid-cache.org/listinfo/squid-dev</a>
</pre>
</blockquote>
<br>
</body>
</html>