Free CachePeer from digest locking/unlocking #241

eduard-bagdasaryan · 2023-12-11T14:58:22Z

This improvement (introduced at Bug 5329 fix) completes the transition
from cbdata abuse for digest refcounting to CachePeer as a digest owner:
An owner does not lock/unlock the object; an owner creates/deletes it.
Establishing an "every digest has a peer owner" relationship simplifies
code and, hopefully, reduces the probability of similar future bugs.

rousskov

It looks like we have to remove more code if we are going to fix/change digest-peer relationship. I left a few suggestions.

Please also sync peerDigestPDFinish() description -- that function no longer destroys the digest.

rousskov · 2023-12-11T17:49:29Z

src/peer_digest.cc

+    if (pd->peer.set() && !pd->peer.valid())
        return;


It is no longer possible for peerDigestCheck() to be called when there is no peer or when the peer became invalid, right? PeerDigest is born with a valid peer. When that peer disappears, the peer destroys the corresponding PeerDigest object, so we will never be here (and would not be able to check anything inside that destroued PeerDigest object if we were!).

Suggested change

if (pd->peer.set() && !pd->peer.valid())

return;

Assure(pd->peer.valid());

Correct, since now CachePeer owns its PeerDigest (and encompasses its lifetime).

rousskov · 2023-12-11T17:53:54Z

src/peer_digest.cc

    debugs(72, 3, "cache_peer " << RawPointer(pd->peer).orNil());
    debugs(72, 3, "peerDigestCheck: time: " << squid_curtime <<


Assuming the new "every digest has a peer" assertion suggested in another change request is correct:

Suggested change

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 <<

src/peer_digest.cc

eduard-bagdasaryan

Done (061be75).

src/peer_digest.cc

eduard-bagdasaryan · 2023-12-14T11:44:44Z

src/peer_digest.cc

+    if (pd->peer.set() && !pd->peer.valid())
        return;


Correct, since now CachePeer owns its PeerDigest (and encompasses its lifetime).

eduard-bagdasaryan · 2023-12-14T12:10:50Z

src/peer_digest.cc

    debugs(72, 3, "cache_peer " << RawPointer(pd->peer).orNil());
    debugs(72, 3, "peerDigestCheck: time: " << squid_curtime <<


rousskov

Great progress, thank you.

rousskov · 2023-12-14T14:51:32Z

src/peer_digest.cc

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

-    assert(fetch->pd);
+    assert(fetch->pd.raw());


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

Suggested change

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

src/peer_digest.cc

This became possible after we had implemented the invariant that each PeerDigest has a valid CachePeer.

src/peer_digest.cc

rousskov

Great progress, thank you.

src/peer_digest.cc

rousskov · 2023-12-15T14:51:13Z

src/peer_digest.cc


+        /* schedule next check if peer is still out there */


The "peer is still out there" condition is always true here since pd is not nil in this context.

Suggested change

/* schedule next check if peer is still out there */

src/peer_digest.cc

rousskov · 2023-12-15T15:03:49Z

src/peer_digest.cc

@@ -733,29 +684,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);


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.

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

src/peer_digest.cc

rousskov · 2023-12-15T15:45:13Z

src/peer_digest.cc

@@ -676,28 +635,24 @@ peerDigestFetchAbort(DigestFetchState * fetch, char *buf, const char *reason)
 {
    assert(reason);
    debugs(72, 2, "peerDigestFetchAbort: peer " << fetch->pd->host << ", reason: " << reason);
-    peerDigestReqFinish(fetch, buf, 1, 1, 1, reason, 1);
+    peerDigestReqFinish(fetch, buf, reason, 1);


Please remove buf argument as well. It is unused AFAICT.

Ok, and following the caller chain I had to remove it in other functions as well.

Some of the functions used in peerDigestHandleReply() switch had this assertion while others did not. We can just rely on peerDigestFetchedEnough() precondition guaranteeing pd existence.

eduard-bagdasaryan

Done (4c5f493).

eduard-bagdasaryan · 2023-12-15T19:53:06Z

src/peer_digest.cc

@@ -676,28 +635,24 @@ peerDigestFetchAbort(DigestFetchState * fetch, char *buf, const char *reason)
 {
    assert(reason);
    debugs(72, 2, "peerDigestFetchAbort: peer " << fetch->pd->host << ", reason: " << reason);
-    peerDigestReqFinish(fetch, buf, 1, 1, 1, reason, 1);
+    peerDigestReqFinish(fetch, buf, reason, 1);


Ok, and following the caller chain I had to remove it in other functions as well.

src/peer_digest.cc

eduard-bagdasaryan · 2023-12-15T20:17:49Z

src/peer_digest.cc


+        /* schedule next check if peer is still out there */


src/peer_digest.cc

eduard-bagdasaryan · 2023-12-15T21:46:41Z

src/peer_digest.cc

@@ -733,29 +684,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);


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

src/peer_digest.cc

rousskov

Great progress, thank you. I hope we are almost done here.

rousskov · 2023-12-18T16:54:10Z

src/peer_digest.cc

-
-    if (fcb_valid)
-        peerDigestFetchFinish(fetch, err);
+    peerDigestFetchFinish(fetch, err);


If possible:

Please move statistics update from peerDigestFetchFinish() to peerDigestFetchSetStats().

Please move StoreEntry updates from peerDigestFetchFinish() to DigestFetchState destructor.

After that, we should be able to remove peerDigestFetchFinish() and simply do this:

Suggested change

peerDigestFetchFinish(fetch, err);

delete fetch;

rousskov · 2023-12-18T17:15:45Z

src/peer_digest.cc

-static void
-peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err)
+void
+PeerDigest::finish(DigestFetchState * const fetch, const int err)


Suggested change

PeerDigest::finish(DigestFetchState * const fetch, const int err)

PeerDigest::noteFetchFinished(DigestFetchState * const fetch, const int err)

src/peer_digest.cc

rousskov · 2023-12-18T17:34:47Z

src/peer_digest.cc

@@ -705,26 +662,16 @@ peerDigestReqFinish(DigestFetchState * fetch, char * /* buf */,
            pd->times.retry_delay = 0;
            peerDigestSetCheck(pd, peerDigestNewDelay(fetch->entry));
        }
+        pd->finish(fetch, err);


Please move the whole if statement code above inside PeerDigest::noteFetchFinished(). We will only check for pd validity in this function and then call noteFetchFinished() to do the rest.

src/peer_digest.cc

eduard-bagdasaryan

Done (4db32d7).

src/peer_digest.cc

eduard-bagdasaryan · 2023-12-19T23:07:29Z

src/peer_digest.cc

-static void
-peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err)
+void
+PeerDigest::finish(DigestFetchState * const fetch, const int err)


eduard-bagdasaryan · 2023-12-19T23:07:41Z

src/peer_digest.cc

-
-    if (fcb_valid)
-        peerDigestFetchFinish(fetch, err);
+    peerDigestFetchFinish(fetch, err);


eduard-bagdasaryan · 2023-12-19T23:13:01Z

src/peer_digest.cc

@@ -705,26 +662,16 @@ peerDigestReqFinish(DigestFetchState * fetch, char * /* buf */,
            pd->times.retry_delay = 0;
            peerDigestSetCheck(pd, peerDigestNewDelay(fetch->entry));
        }
+        pd->finish(fetch, err);


src/peer_digest.cc

rousskov

How would an absolute minimal bug 5329 fix look like? My understanding is that it should be very small -- 10-15 changed lines -- but our confidence in its completeness will be fairly low because we would not be sure that some other locking dependencies are not mistreated. If I am right, please post a separate internal minimal PR with just that fix, without any "confidence boosting" and polishing changes.

PR description: This fix does not restore the lost lock but removes the corresponding unlock from the CachePeer destructor

If restoring that lost lock is the correct minimal fix, then it is OK to restore that lock (instead of removing the corresponding unlock from the CachePeer destructor).

To avoid misunderstanding, we will still propose all the other changes in this PR, even if we propose a minimal bug-fixing PR first.

rousskov · 2023-12-20T04:10:35Z

src/peer_digest.cc


-        assert(pd);


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

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.

rousskov · 2023-12-20T04:11:49Z

src/peer_digest.cc

 {
-    DigestFetchState *fetch = (DigestFetchState *)data;
-    PeerDigest *pd;
+    const auto pd = fetch->pd.get();



Suggested change

assert(pd);

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.

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()¹.

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

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

Done (873e62).

eduard-bagdasaryan · 2023-12-20T22:22:42Z

If restoring that lost lock is the correct minimal fix, then it is OK to restore that lock

Yes, is the smallest possible fix: done in PR242. I tested it with the scenario described at Bug 5329 report.

The PR code already removed the corresponding unlock - the lock was brought in by the recent 4657405 after merging.

eduard-bagdasaryan · 2024-01-22T23:43:30Z

Extracted minimal changes into PR244. The initial bug was fixed at 465740.

Bug 5329: cbdata.cc:276 "c->locks > 0" assertion on peer reconfiguration

4fcbe9f

rousskov requested changes Dec 11, 2023

View reviewed changes

rousskov changed the title ~~Bug 5329: cbdata.cc:276 "c->locks > 0" assertion on peer reconfiguration~~ Bug 5329: cbdata.cc:276 "c->locks > 0" assertion on reconfigure Dec 11, 2023

eduard-bagdasaryan added 3 commits December 14, 2023 01:24

Converted raw DigestFetchState::pd to CbcPointer

2fc248a

Adjusted the code to reflect the fact that CachePeer owns PeerDigest

6905262

Renamed to avoid foo:foo() notation

061be75

eduard-bagdasaryan commented Dec 14, 2023

View reviewed changes

rousskov requested changes Dec 14, 2023

View reviewed changes

eduard-bagdasaryan added 3 commits December 15, 2023 01:26

Simplified, removing unnecessary flags/arguments

c800c6e

This became possible after we had implemented the invariant that each PeerDigest has a valid CachePeer.

Do not use void* argument where DigestFetchState* is expected

42de021

Removed an unnecessary check in peerDigestFetchedEnough()

ccac73f

eduard-bagdasaryan commented Dec 15, 2023

View reviewed changes

src/peer_digest.cc Show resolved Hide resolved

rousskov requested changes Dec 15, 2023

View reviewed changes

eduard-bagdasaryan added 8 commits December 15, 2023 23:42

Removed an unused buf argument

2e6c46b

Simplified peerDigestReqFinish()

a82b88f

Undone a 'host' change in PeerDigest::finish()

47569b6

Removed an out-of-scope change

a6ae8a8

Adjusted with consts

086ea8a

Removed an unused 'buf' argument from peerDigestFetchReply()

c750f01

Removed a couple of 'pd' assertions

de2c1ff

Some of the functions used in peerDigestHandleReply() switch had this assertion while others did not. We can just rely on peerDigestFetchedEnough() precondition guaranteeing pd existence.

Polished

4c5f493

eduard-bagdasaryan commented Dec 15, 2023

View reviewed changes

rousskov requested changes Dec 18, 2023

View reviewed changes

Refactored a couple of methods

4db32d7

eduard-bagdasaryan commented Dec 19, 2023

View reviewed changes

rousskov reviewed Dec 20, 2023

View reviewed changes

Added a couple of assertions

873e62c

eduard-bagdasaryan added 2 commits January 9, 2024 17:19

Merged from master

0b3ffe9

Removed CachePeer::digest lock

68376c8

The PR code already removed the corresponding unlock - the lock was brought in by the recent 4657405 after merging.

eduard-bagdasaryan changed the title ~~Bug 5329: cbdata.cc:276 "c->locks > 0" assertion on reconfigure~~ Free CachePeer from digest locking/unlocking Jan 11, 2024

eduard-bagdasaryan closed this Jan 22, 2024

	if (pd->peer.set() && !pd->peer.valid())
	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 " << RawPointer(pd->peer).orNil());
	debugs(72, 3, "peerDigestCheck: time: " << squid_curtime <<
	debugs(72, 3, "cache_peer " << *pd->peer << " now: " << squid_curtime <<

	PeerDigest::finish(DigestFetchState * const fetch, const int err)
	PeerDigest::noteFetchFinished(DigestFetchState * const fetch, const int err)

Free CachePeer from digest locking/unlocking #241

Free CachePeer from digest locking/unlocking #241

Conversation

eduard-bagdasaryan commented Dec 11, 2023 • edited Loading

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

eduard-bagdasaryan commented Dec 20, 2023

eduard-bagdasaryan commented Jan 22, 2024

eduard-bagdasaryan commented Dec 11, 2023 •

edited

Loading

rousskov Dec 20, 2023 •

edited

Loading