Re: [squid-dev] [PATCH] Purge cache entries in SMP-aware caches
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
Re: [squid-dev] [PATCH] Purge cache entries in SMP-aware caches
On 05/08/17 07:52, 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. I don't see how renaming would help much. Any API control over use of a flag can be added regardless of its name. Also, the 'RELEASE' flag value being true indicates REUSABLE false value. So the rename implies a careful and complete inversion of all logic conditions relying on or otherwise touching the flag. Which risks many new bug additions. If done at all I think the rename should be left out-of-scope for this PR / patching. Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] Purge cache entries in SMP-aware caches
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. +SWAPOUT_NONE, ///< the store entry has not been stored on a disk This definition seems to contradict the "else" usage in Rock::SwapDir::disconnect(). What does SWAPOUT_NONE state correspond to in that "else" clause? A readable disk entry? It may be tempting to use SWAPOUT_DONE in that "else" clause inside disconnect() but I suspect that another worker may still be writing that disk entry (i.e., there is a SWAPOUT_WITING in another worker). We may need to either * broaden SWAPOUT_NONE definition to cover that "else" clause or * call anchor.complete() and use SWAPOUT_DONE/SWAPOUT_WITING in that "else" clause, similar to what Rock::SwapDir::anchorEntry() does. Please investigate and suggest any necessary changes. This patch introduces an invariant StoreEntry::checkDisk() for disk fields. According to this rule, a disconnected/unlinked entry (swap_dirn < 0) must have SWAPOUT_NONE. So I assume we should correct SWAPOUT_NONE definition, e.g.,: /// a store entry which is either unlinked the disk Store or /// has not been stored on a disk. Thanks, Eduard. ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] Purge cache entries in SMP-aware caches
On 07/13/2017 07:26 AM, Eduard Bagdasaryan wrote:
> Hello,
>
> I am attaching an improved version of the patch posted before.
> It is based on v4 r15081. What was fixed:
I do not see any high-level problems with this code, and it does not
look like others are going to comment on the v4 patch that have been
available for a while. Please sync to master and post a pull request!
A few improvement suggestions:
Please see if you can either
* make updateCollapsed() calls consistent with respect to requiring (via
assert) the given entry presence in the index or always returning either
true or false when the given entry is not present
or
* preserve the old checks meaning (while using new/improved interfaces)
Currently, the removal of some checks left the code in an arguably more
inconsistent state than it was before the changes.
> +MemStore::unlinkByKeyIfFound(const cache_key *key)
> +{
> +map->freeEntryByKey(key);
> }
Do nothing if map is nil.
> +/// whether there is a corresponding disk store entry
> +bool hasDisk(const sdirno dirn = -1, const sfileno filen = -1) const;
The description is misleading because the disk entry may exist without
StoreEntry knowing anything about it. I do not have a good solution for
this problem, but can suggest:
/// whether one of this StoreEntry owners has locked the corresponding
/// disk entry (at the specified disk entry coordinates, if any)
> +/// Makes hasDisk() false. The caller should have deleted
> +/// the corresponding disk store entry.
> +void detachFromDisk();
Should we relax the "should have deleted" precondition from "deleted" to
"unlocked"?
Do callers normally unlock the entry before calling this method? After
calling this method? Depends on the caller?
> // private entry or loading failure
> map->closeForReading(index);
> return NULL;
The comment became stale after the changes:
// (private or completed) entry or loading failure
> StoreEntry *
> storeCreatePureEntry(const char *url, const char *log_url, const
> RequestFlags , const HttpRequestMethod& method)
> {
...
> +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?
> +SWAPOUT_NONE, ///< the store entry has not been stored on a disk
This definition seems to contradict the "else" usage in
Rock::SwapDir::disconnect(). What does SWAPOUT_NONE state correspond to
in that "else" clause? A readable disk entry? It may be tempting to use
SWAPOUT_DONE in that "else" clause inside disconnect() but I suspect
that another worker may still be writing that disk entry (i.e., there is
a SWAPOUT_WITING in another worker). We may need to either
* broaden SWAPOUT_NONE definition to cover that "else" clause or
* call anchor.complete() and use SWAPOUT_DONE/SWAPOUT_WITING in that
"else" clause, similar to what Rock::SwapDir::anchorEntry() does.
Please investigate and suggest any necessary changes.
Thank you,
Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
