repair: Release permit earlier when the repair_reader is done

[asias]

Consider

• 10 repair instances take all the 10 _streaming_concurrency_sem

• repair readers are done but the permits are not released since they are waiting for view update _registration_sem

• view updates trying to take the _streaming_concurrency_sem to make progress of view update so it could release _registration_sem, but it could not take _streaming_concurrency_sem since the 10 repair instances have taken them

• dead lock happens

To fix, release the permits as soon as the repair readers are done even if the repair job is waiting for _registration_sem.

Fixes #14676

[denesb]

I think we should further clarify that this is caused by an edge-case of having switched to empty reader, see #14676 (comment).
In general, the evictable reader used by repair should take care of this deadlock, but we defeated that mechanism by replacing EOF-d evictable readers with empty ones.

[scylladb-promoter]

CI state SUCCESS - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2456/

[asias]

I think we should further clarify that this is caused by an edge-case of having switched to empty reader, see #14676 (comment). In general, the evictable reader used by repair should take care of this deadlock, but we defeated that mechanism by replacing EOF-d evictable readers with empty ones.

Sure. I updated the commit log.

[dorlaor]

Is there a way (not in this fix, but later), to add a watchdog to identify such deadlocks and automatically release pending permits if there is no progress?

…

On Thu, Jul 13, 2023 at 9:28 AM Asias He ***@***.***> wrote: I think we should further clarify that this is caused by an edge-case of having switched to empty reader, see #14676 (comment) <#14676 (comment)>. In general, the evictable reader used by repair should take care of this deadlock, but we defeated that mechanism by replacing EOF-d evictable readers with empty ones. Sure. I updated the commit log. — Reply to this email directly, view it on GitHub <#14677 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANHURKRYZ4WXRMUH3ASU7TXP6IRLANCNFSM6AAAAAA2ILYBAE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[asias]

Is there a way (not in this fix, but later), to add a watchdog to identify such deadlocks and automatically release pending permits if there is no progress?

We can start with adding a big timeout (like we do for the repair reader to read fragment, timeout in 30minutes). This way we know something wrong with the lock. It is too hard to fix the deadlock just with a watchdog.

However, I have an idea of getting rid of view registration lock completely, so we would not play with two locks to avoid deadlock.

[asias]

In addition, Botond has the idea of getting rid of the _streaming_concurrency_sem lock.

[denesb]

In addition, Botond has the idea of getting rid of the _streaming_concurrency_sem lock.

I want to get rid of the (currently very limited) count resources, which is only 10 for streaming. Note that deadlocks can still happen based on memory resources, but that is much-much less likely, because these reads don't use that much memory.

[denesb]

Is there a way (not in this fix, but later), to add a watchdog to identify such deadlocks and automatically release pending permits if there is no progress?

I think the best we could do with such a watchdog is to forcibly break the cycle by injecting an exception. This can already be achieved by timeouts, which we do use.
Knowing which permit to release would require intricate understanding the purpose and lifecycle of each reader involved. The amount of heuristics this would require, would be just as error prone, if not more, as the current code.

[scylladb-promoter]

CI state SUCCESS - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2458/