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 &flags, 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
