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

[Eduard Bagdasaryan]

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.