[squid-dev] [PATCH] Purge cache entries in SMP-aware caches

[Alex Rousskov]

On 08/04/2017 01:52 PM, Eduard Bagdasaryan wrote:
> To avoid this 'going around releaseRequest() API' I would like
> to suggest renaming 'RELEASE_REQUEST' flag to something
> more suitable, e.g., 'ENTRY_REUSABLE' and adding a helper
> API avoiding direct flag manipulation. This would not contradict
> with the existing setReleaseFlag() meaning (i.e., "marking the
> corresponding entry for eventual removal"), adding an additional
> meaning (i.e., since then, the entry is not suggested to be re-used
> by others).

Let's focus on the flag meaning. We can always rename it later (or even
in this project). It is the flag meaning that should drive our primary
decisions, not the letters spelling flag's name.

My response has three parts. Most readers may want to skip the first two
parts. I left them to document my thought process in case I missed
something important in my reasoning; the surprising (to me) final
conclusion ended up being rather far from where I started!


== Part I. Simplify by replacing "A or B" with "B" ==

I like the overall direction of your proposal, but I wonder whether we
really have to _add_ the new/proposed "not reusable" meaning to the
old/implied "marked for removal" meaning. Can we just define the flag to
mean "not usable for new Store clients"?

1. The old code that removes idle RELEASE_REQUEST entries from Store
would still work fine: It will be removing idle entries that cannot be
used by anybody. Clearly, removing such entries is logical/natural.

2. Old StoreEntry::release() would still work fine: If it cannot remove
the entry immediately, it will mark it unusable for new clients, knowing
very well that code in #1 will remove such entries (when they become
idle), eventually achieving the result desired by the release() caller.

3. The old StoreEntry::releaseRequest() hack continues to be nothing
more than a temporary hack for callers that are (possibly justifiably!)
afraid of calling release(). No need to discuss it at this high level.

4. The old storeCreatePureEntry() would be able to use the new
StoreEntry::banFutureReuse() or a similar treat-as-private flag-setting
method without raising the same red flag I raised in my review (I will
still argue for moving/changing that code for other reasons, but at
least that it would not look so out of place anymore).

Is there any code that really needs to interpret the RELEASE_REQUEST
flag itself as something other than "unusable for new Store clients"?

If there is no such code, then I have another question: Why do we need
this flag at all? It is answered in Part II.


== Part II. Simplify further by replacing big "B" with small "b" ==

We already have private keys that mean almost the same thing. You have
touched upon this conflict in your email (thank you for carefully
thinking about this!):

> This intersects a little with the existing 'private entries'
> definition. However, private entries may eventually become public,
> while an entry with ENTRY_REUSABLE unset, would stay 'private'
> until it is removed.

I would replace "a little" with something like "98%", but we are
otherwise in agreement :-).

AFAICT, the only difference between a private key and an "unusable for
new Store clients" flag is reversibility of the former. Moreover, IIRC,
all current code paths that set the flag also make (or should make) the
key private (which was a part of my original concern that started this
discussion), tying the two concepts together (naturally).

If this reasoning is correct, then we do not need a "unusable for new
Store clients" flag. We just need a "cannot become public" flag! The
rest of the required functionality/meaning is already covered by the
private key.


== Part III. The new "b" is the old "A"! ==

Part II concluded that we do not need a "unusable for new Store clients"
flag because private keys already address that need. We just need a
"cannot become public" flag. Which reminds me of this old code:

> void
> StoreEntry::makePublic(const KeyScope scope)
> {
>     if (!EBIT_TEST(flags, RELEASE_REQUEST))
>         setPublicKey(scope);
> }

The above code suggests that we may already have that "cannot become
public" flag. It is named differently, it may be set in slightly the
wrong place (separated from the corresponding makePrivate() call), and
some of its tests might need to be adjusted to look for the key status,
but ultimately it can be used correctly as the minimal flag we need to
make a private key permanent!

If you agree with the above reasoning, then it should be easy to adjust
the code to match the newly developed understanding of the
true/desirable RELEASE_REQUEST flag meaning as the "making private keys
permanent" marker. I can suggest specific changes if you prefer, but you
should probably be the one driving this. To reduce noise, I suggest
keeping RELEASE_REQUEST name (at least for now) unless we already have
to change most RELEASE_REQUEST lines for other reasons.


HTH,

Alex.


> On 24.07.2017 02:00, Eduard Bagdasaryan wrote:
>> On 23.07.2017 02:04, Alex Rousskov wrote:
>>>> +    if (!flags.cachable)
>>>> +        EBIT_SET(e->flags, RELEASE_REQUEST);
>>> This release request feels out of place and direct flags setting goes
>>> around the existing releaseRequest() API. Please check all callers --
>>> perhaps we do not need the above because all callers already do an
>>> equivalent action (e.g., makePrivate()) for "uncachable" requests?
>>
>> I don't think this lines are 'out of place': storeCreatePureEntry() just
>> initializes the just created StoreEntry fields (including
>> StoreEntry::flags) with correct values.  If we definitely know a
>> this moment that 'flags' should have RELEASE_REQUEST set, why do we need
>> to postpone this to many callers, hoping that all of them will do that
>> work correctly?  There are lots of storeCreateEntry() calls and it is
>> hardly possible to track that all of them end up with
>> 'releaseRequest()', when flags.cachable is false.  BTW, at the time of
>> StoreEntry initialization we do not need to do most of the work
>> releaseRequest() does. E.g., there are no connected storages to
>> disconnect from, no public keys to make them private, etc. The only
>> thing to do is RELEASE_REQUEST flag setting.
> 
> 
> 
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>