Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Free CachePeer from digest locking/unlocking #241

Closed
wants to merge 19 commits into from
Closed

Free CachePeer from digest locking/unlocking #241

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4fcbe9f
Bug 5329: cbdata.cc:276 "c->locks > 0" assertion on peer reconfiguration
eduard-bagdasaryan Dec 11, 2023
2fc248a
Converted raw DigestFetchState::pd to CbcPointer
eduard-bagdasaryan Dec 13, 2023
6905262
Adjusted the code to reflect the fact that CachePeer owns PeerDigest
eduard-bagdasaryan Dec 14, 2023
061be75
Renamed to avoid foo:foo() notation
eduard-bagdasaryan Dec 14, 2023
c800c6e
Simplified, removing unnecessary flags/arguments
eduard-bagdasaryan Dec 14, 2023
42de021
Do not use void* argument where DigestFetchState* is expected
eduard-bagdasaryan Dec 14, 2023
ccac73f
Removed an unnecessary check in peerDigestFetchedEnough()
eduard-bagdasaryan Dec 14, 2023
2e6c46b
Removed an unused buf argument
eduard-bagdasaryan Dec 15, 2023
a82b88f
Simplified peerDigestReqFinish()
eduard-bagdasaryan Dec 15, 2023
47569b6
Undone a 'host' change in PeerDigest::finish()
eduard-bagdasaryan Dec 15, 2023
a6ae8a8
Removed an out-of-scope change
eduard-bagdasaryan Dec 15, 2023
086ea8a
Adjusted with consts
eduard-bagdasaryan Dec 15, 2023
c750f01
Removed an unused 'buf' argument from peerDigestFetchReply()
eduard-bagdasaryan Dec 15, 2023
de2c1ff
Removed a couple of 'pd' assertions
eduard-bagdasaryan Dec 15, 2023
4c5f493
Polished
eduard-bagdasaryan Dec 15, 2023
4db32d7
Refactored a couple of methods
eduard-bagdasaryan Dec 19, 2023
873e62c
Added a couple of assertions
eduard-bagdasaryan Dec 20, 2023
0b3ffe9
Merged from master
eduard-bagdasaryan Jan 9, 2024
68376c8
Removed CachePeer::digest lock
eduard-bagdasaryan Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/CachePeer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,7 @@ CachePeer::~CachePeer()
aclDestroyAccessList(&access);

#if USE_CACHE_DIGESTS
void *digestTmp = nullptr;
if (cbdataReferenceValidDone(digest, &digestTmp))
peerDigestNotePeerGone(static_cast<PeerDigest *>(digestTmp));
delete digest;
xfree(digest_url);
#endif

Expand Down
6 changes: 4 additions & 2 deletions src/PeerDigest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class DigestFetchState
DigestFetchState(PeerDigest *,HttpRequest *);
~DigestFetchState();

PeerDigest *pd;
CbcPointer<PeerDigest> pd;
StoreEntry *entry;
StoreEntry *old_entry;
store_client *sc;
Expand Down Expand Up @@ -79,6 +79,9 @@ class PeerDigest
PeerDigest(CachePeer *);
~PeerDigest();

/// updates stats when digest transfer is complete
void finish(DigestFetchState *, int err);

CbcPointer<CachePeer> peer; ///< pointer back to peer structure, argh
CacheDigest *cd = nullptr; /**< actual digest structure */
const SBuf host; ///< copy of peer->host
Expand Down Expand Up @@ -116,7 +119,6 @@ class PeerDigest
extern const Version CacheDigestVer;

void peerDigestNeeded(PeerDigest * pd);
void peerDigestNotePeerGone(PeerDigest * pd);
void peerDigestStatsReport(const PeerDigest * pd, StoreEntry * e);

#endif /* USE_CACHE_DIGESTS */
Expand Down
73 changes: 20 additions & 53 deletions src/peer_digest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ static int peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t
static void peerDigestFetchStop(DigestFetchState * fetch, char *buf, const char *reason);
static void peerDigestFetchAbort(DigestFetchState * fetch, char *buf, const char *reason);
static void peerDigestReqFinish(DigestFetchState * fetch, char *buf, int, int, int, const char *reason, int err);
static void peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err);
static void peerDigestFetchFinish(DigestFetchState * fetch, int err);
static void peerDigestFetchSetStats(DigestFetchState * fetch);
static int peerDigestSetCBlock(PeerDigest * pd, const char *buf);
Expand Down Expand Up @@ -77,7 +76,7 @@ CBDATA_CLASS_INIT(PeerDigest);
CBDATA_CLASS_INIT(DigestFetchState);

DigestFetchState::DigestFetchState(PeerDigest *aPd, HttpRequest *req) :
pd(cbdataReference(aPd)),
pd(aPd),
entry(nullptr),
old_entry(nullptr),
sc(nullptr),
Expand Down Expand Up @@ -111,8 +110,6 @@ DigestFetchState::~DigestFetchState()
entry = nullptr;

HTTPMSGUNLOCK(request);

assert(pd == nullptr);
}

PeerDigest::~PeerDigest()
Expand Down Expand Up @@ -168,21 +165,6 @@ peerDigestSetCheck(PeerDigest * pd, time_t delay)
debugs(72, 3, "peerDigestSetCheck: will check peer " << pd->host << " in " << delay << " secs");
}

/*
* called when peer is about to disappear or have already disappeared
*/
void
peerDigestNotePeerGone(PeerDigest * pd)
{
if (pd->flags.requested) {
debugs(72, 2, "peerDigest: peer " << pd->host << " gone, will destroy after fetch.");
/* do nothing now, the fetching chain will notice and take action */
} else {
debugs(72, 2, "peerDigest: peer " << pd->host << " is gone, destroying now.");
delete pd;
}
}

/* callback for eventAdd() (with peer digest locked)
* request new digest if our copy is too old or if we lack one;
* schedule next check otherwise */
Expand All @@ -196,13 +178,9 @@ peerDigestCheck(void *data)

pd->times.next_check = 0; /* unknown */

if (pd->peer.set() && !pd->peer.valid()) {
peerDigestNotePeerGone(pd);
return;
}
Assure(pd->peer.valid());

debugs(72, 3, "cache_peer " << RawPointer(pd->peer).orNil());
debugs(72, 3, "peerDigestCheck: time: " << squid_curtime <<
debugs(72, 3, "cache_peer " << *pd->peer << " now: " << squid_curtime <<
", last received: " << (long int) pd->times.received << " (" <<
std::showpos << (int) (squid_curtime - pd->times.received) << ")");

Expand Down Expand Up @@ -349,7 +327,7 @@ peerDigestHandleReply(void *data, StoreIOBuffer receivedData)
return;
}

assert(fetch->pd);
assert(fetch->pd.raw());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove because this function has no code dereferencing fetch->pd?

Suggested change
assert(fetch->pd.raw());

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

/* The existing code assumes that the received pointer is
* where we asked the data to be put
*/
Expand Down Expand Up @@ -448,7 +426,7 @@ static int
peerDigestFetchReply(void *data, char *buf, ssize_t size)
{
DigestFetchState *fetch = (DigestFetchState *)data;
PeerDigest *pd = fetch->pd;
const auto pd = fetch->pd.get();
assert(pd && buf);
assert(!fetch->offset);

Expand Down Expand Up @@ -535,7 +513,7 @@ peerDigestSwapInCBlock(void *data, char *buf, ssize_t size)
return -1;

if (size >= (ssize_t)StoreDigestCBlockSize) {
PeerDigest *pd = fetch->pd;
const auto pd = fetch->pd.get();

assert(pd);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we can restore this assertion because peerDigestFetchedEnough() above guarantees that the PeerDigest object is still valid.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's restore it. However I thought that we decided minimize these 'assert(pd)' throughout the code, relying on peerDigestFetchedEnough() guarantees of existing pd.

assert(fetch->entry->mem_obj);
Expand Down Expand Up @@ -566,9 +544,8 @@ int
peerDigestSwapInMask(void *data, char *buf, ssize_t size)
{
DigestFetchState *fetch = (DigestFetchState *)data;
PeerDigest *pd;
const auto pd = fetch->pd.get();

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert(pd);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Note that there is also another case in peerDigestFetchReply(), where fetch->pd goes unchecked... Before our changes there was an assertion in peerDigestHandleReply() (peerDigestFetchReply() caller), but we removed that assertion.

Copy link

@rousskov rousskov Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That peerDigestFetchReply() use case is fine, I think: We are allowed to dereference fetch->pd without checking right after a peerDigestFetchedEnough() call because we know that peerDigestFetchedEnough() will protect us. This peerDigestSwapInMask() check is different because we dereference fetch->pd before calling peerDigestFetchedEnough()1.

Needless to say, the whole thing is a big mess. We can probably simplify by checking for pd validity at the beginning of each fetching callback, never destroying pd in any fetching callbacks, and never checking pd anywhere else in fetching callbacks. However, that simplification spills into splitting peerDigestFetchedEnough() from "a universal check that we do not really know when to call because our state always changes as we make progress through callback code" to a set of change-specific checks. For now, let's not make those changes. We will revisit this when we know how the minimal fix looks like.

Footnotes

  1. A peerDigestFetchedEnough() call in our caller does not count, especially when our caller is looping and may reach "fetched enough" state inside that loop. ↩

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (873e62).

pd = fetch->pd;
assert(pd->cd && pd->cd->mask);

/*
Expand Down Expand Up @@ -602,17 +579,17 @@ peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const
static const SBuf hostUnknown("<unknown>"); // peer host (if any)
SBuf host = hostUnknown;

PeerDigest *pd = nullptr;
const auto pd = fetch->pd.get();
const char *reason = nullptr; /* reason for completion */
const char *no_bug = nullptr; /* successful completion if set */
const int pdcb_valid = cbdataReferenceValid(fetch->pd);
const int pdcb_valid = fetch->pd.valid() ? 1 : 0;
const int pcb_valid = pdcb_valid && fetch->pd->peer.valid();

/* test possible exiting conditions (the same for most steps!)
* cases marked with '?!' should not happen */

if (!reason) {
if (!pdcb_valid || !(pd = fetch->pd))
if (!pd)
reason = "peer digest disappeared?!";
else
host = pd->host;
Expand Down Expand Up @@ -687,7 +664,7 @@ peerDigestReqFinish(DigestFetchState * fetch, char * /* buf */,
{
assert(reason);

/* must go before peerDigestPDFinish */
/* must go before PeerDigest::finish() */

if (pdcb_valid) {
fetch->pd->flags.requested = false;
Expand All @@ -696,7 +673,7 @@ peerDigestReqFinish(DigestFetchState * fetch, char * /* buf */,

/* schedule next check if peer is still out there */
if (pcb_valid) {
PeerDigest *pd = fetch->pd;
const auto pd = fetch->pd.get();

if (err) {
pd->times.retry_delay = peerDigestIncDelay(pd);
Expand All @@ -711,20 +688,17 @@ peerDigestReqFinish(DigestFetchState * fetch, char * /* buf */,
if (fcb_valid)
peerDigestFetchSetStats(fetch);

if (pdcb_valid)
peerDigestPDFinish(fetch, pcb_valid, err);
if (const auto pd = fetch->pd.get())
pd->finish(fetch, err);

if (fcb_valid)
peerDigestFetchFinish(fetch, err);
}

/* destroys digest if peer disappeared
* must be called only when fetch and pd cbdata are valid */
static void
peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err)
void
PeerDigest::finish(DigestFetchState * fetch, int err)
{
PeerDigest *pd = fetch->pd;
const auto host = pd->host;
const auto pd = this; // TODO: remove this diff reducer
pd->times.received = squid_curtime;
pd->times.req_delay = fetch->resp_time;
pd->stats.sent.kbytes += fetch->sent.bytes;
Expand All @@ -733,29 +707,22 @@ peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err)
pd->stats.recv.msgs += fetch->recv.msg;

if (err) {
debugs(72, DBG_IMPORTANT, "" << (pcb_valid ? "temporary " : "" ) << "disabling (" << pd->req_result << ") digest from " << host);
debugs(72, DBG_IMPORTANT, "disabling (" << pd->req_result << ") digest from " << pd->host);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere in this method, if possible, please undo pd-> addition for host. AFAICT, we do not need to add pd-> to preserve the old functionality here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I got confused with this 'pd' diff reducer...


delete pd->cd;
pd->cd = nullptr;

pd->flags.usable = false;

if (!pcb_valid)
peerDigestNotePeerGone(pd);
} else {
assert(pcb_valid);

pd->flags.usable = true;

/* XXX: ugly condition, but how? */

if (fetch->entry->store_status == STORE_OK)
debugs(72, 2, "re-used old digest from " << host);
debugs(72, 2, "re-used old digest from " << pd->host);
else
debugs(72, 2, "received valid digest from " << host);
debugs(72, 2, "received valid digest from " << pd->host);
}

cbdataReferenceDone(fetch->pd);
}

/* free fetch state structures
Expand Down