Hold PeerState in an RwLock rather than a Mutex

[tnull]

Previously, we would hold the PeerState in a Mutex, which disallows concurrent read-only operations. Here we switch to an RwLock making this possible.

To this end, we first switch all instances of Mutex::lock to RwLock::write, and then selectively adapt some usages permitting for it to RwLock::read.

[TheBlueMatt]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.34884% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 89.37%. Comparing base (1d9e541) to head (cf2d418).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	95.13%	1 Missing and 6 partials ⚠️
lightning/src/ln/functional_test_utils.rs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2968      +/-   ##
==========================================
+ Coverage   89.31%   89.37%   +0.05%     
==========================================
  Files         117      117              
  Lines       95618    95618              
  Branches    95618    95618              
==========================================
+ Hits        85404    85456      +52     
+ Misses       7974     7926      -48     
+ Partials     2240     2236       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

Right, the users who reported this should def. investigate why it was blocking for so long. I now went through all usages and found several places where we could also just us a RwLockReadGuard (forward_intercepted_htlcs, compute_inflight_htlcs, get_relevant_txids), a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

IMO, the other cases make this a worthwhile improvement, independently of the concrete reported issue.

[TheBlueMatt]

a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

Writing the manager basically halts the world no matter what due to the consistency write lock.

I'm not entirely convinced by the remaining changes that they're worth it.

[tnull]

Writing the manager basically halts the world no matter what due to the consistency write lock.

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

[TheBlueMatt]

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

Ah, fair point, duh. Note that write currently takes the top-level per_peer_state write lock, but that's an easy swap in this PR.

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

[tnull]

Ah, fair point, duh. Note that write currently takes the top-level per_peer_state write lock, but that's an easy swap in this PR.

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

So, IIUC, we can have during access during parts of the serialization, but not for the full write call?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

Ah, okay, the doc's explanation around 'different branches' had me confused whether we're allowed to skip the root or not. Logically it makes sense, the docs aren't fully clear on that though, IMO.

[TheBlueMatt]

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Yea, I guess I was figuring that loop would be pretty quick, but I guess it does write all the channels so its not free, even if pretty quick.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

Yea, its...delicate. IIRC this is the only place we actually take per-peer locks recursively, so the total_consistency_guard saves us, but of course it is a little less fragile to keep relying on the per_peer write lock for that.

[tnull]

Rebased on main to resolve minor conflicts.

[TheBlueMatt]

Still not entirely convinced this improves parallelism in any practical cases, honestly.

[TheBlueMatt]

Is there a race condition here where we decide whether or not to include the channel at this step but then the channel gets updated and we write the wrong number of channels? Doesn't seem like it was introduced in this PR but still worth fixing.

[tnull]

Yeah, I noticed that as well and created a TODO for it but so far intended to fix it elsewhere. Looking at it again it seems that this could be fixed by a) only iterating the peer states once and persisting to a temporary buffer while keeping track of serializable_peer_count and number_of_funded_channels or b) taking the per_peer_state.write() lock earlier and holding it for the remainder of the method.

Seems a) is rather inefficient and b) would mean dropping any chance of concurrent access during at least parts of write. I think switching to an RwLock might still be nice in general and going forward, but if you don't agree, possibly we should just do b) in a separate PR and close this one?

[TheBlueMatt]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

[tnull]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

Now opened to this effect: #2998

Closing this as the benefits of switching to RwLock are not immediately clear, and we can always do so if we find a compelling reason in the future.