Re: [PATCH v2] Do not release entries that may be kept in local memory cache.

On 07/05/2012 09:02 PM, Amos Jeffries wrote:

> It sounds like another analysis of the places calling trimMemory() is
> needed.

StoreEntry::trimMemory() is only called from StoreEntry::swapOut(),
before we are starting to swap data out (or skipping swap because the
entry is not swappable). Ideally, it should also be called from places
where we unlock entry's memory as well, but that is the way it was
written. Changing that is outside this bug scope and may be difficult.

> So that trimMemory is isolated to just trim like its name
> suggests. The maybeTrim() decisions being outside functionality (in
> StoreController?) and which is the only place calling e.trimMemory().

First of all, I would ignore the "maybe" aspect. The caller does not
rely on any trimming actually happening and some computations of how
much to trim (if anything!) happen deeper inside MemObject. We can add
"maybe" to a bunch of methods so that their names reflect the
possibility that nothing would be trimmed. Those old methods are present
in all Squid versions. I think renaming them just to polish the names
would cause more porting headaches than it would solve problems.

If I get your primary idea correctly, then instead of
StoreEntry::swapOut() calling e.trimMemory() it will call new
StoreController::trimMemory(e) and the controller would decide whether
to proceed with trimming and, if yes, will call e.trimMemory().
StoreEntry::trimMemory() will not have any new conditions added then.

If that is what you are suggesting, it sounds good to me, and I can do
that during commit of v3 patch (it would not change the functionality or
the order of the checks; just move a couple of checks to a different
class/method).

> Looking at this v2 patch the Bug comment and IN_MEMORY test looks like
> it should be inside the keepInLocalMemory() instead. Since things
> already *in* local memory are obviously allowed there somehow. That is
> probably wrong, but is clearly implied by the symbol names around it.

They were allowed into local memory because Squid cannot do I/O without
using local memory :-). Every byte read is placed in local memory first.

The issue here is whether those local memory bytes can be forgotten
(i.e., the entry memory can be trimmed). The answer depends on the entry
state and on the memory caching module state/functionality. The latter
aspect was ignored for a while, until this patch.

HTH,

Alex.
Received on Fri Jul 06 2012 - 14:36:44 MDT