Avoid cs_main during ReadBlockFromDisk Calls

TheBlueMatt commented at 9:03 pm on December 15, 2017: member

Built on #11281 and one commit from #11824, this removes almost all cs_main holds in ReadBlockFromDisk calls in net_processing/RPC/REST. Only real worry here is if something gets pruned out from under us during reading, so some previously-asserts in net_processing are now LogPrints.

fanquake added the label Refactoring on Dec 15, 2017

TheBlueMatt force-pushed on Dec 15, 2017

laanwj added the label Block storage on Dec 16, 2017

laanwj commented at 4:28 am on December 16, 2017: member

Concept ACK. Good to have less contention on the cs_main lock, it seems a good idea to release it during I/O.

in src/rpc/blockchain.cpp:150 in 571f4ad311 outdated

137@@ -144,6 +138,12 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool tx
138 
139     if (blockindex->pprev)
140         result.push_back(Pair("previousblockhash", blockindex->pprev->GetBlockHash().GetHex()));
141+    LOCK(cs_main);
142+    int confirmations = -1;

promag commented at 4:07 pm on December 19, 2017:

Avoid lock here by receiving the pindex tip? (with the cost of jumping thru pprev).

TheBlueMatt commented at 4:57 pm on December 19, 2017:

That still wouldn’t solve it for the chainActive.Next() call, so you’d need cs_main either way. Less code diff to just do it as-is.

promag commented at 3:30 pm on January 10, 2018:

Something along this should work:

0const CBlockIndex* pindex = tip;
1const CBlockIndex* pnext = nullptr;
2int confirmations = 0;
3while (pindex && pindex != blockindex && pindex->nHeight >= blockindex->nHeight) {
4    pnext = pindex;
5    pindex = pindex->pprev;
6    confirmations++;
7}
8if (pindex != blockindex) confirmations = -1;

This approach avoids accessing chainActive, blockToJSON just needs to receive the tip. This sounds more correct to me because a RPC can take a snapshot of the tip (and then leave cs_main) and all the remaining execution is consistent with that tip.

promag commented at 1:21 pm on January 11, 2018:

Submitted suggestion as #12151.

promag commented at 4:33 pm on December 19, 2017: member

utACK 571f4ad.

jonasschnelli commented at 6:47 pm on January 10, 2018: contributor

Needs rebase on rebased #11281 Concept ACK

promag commented at 1:22 pm on January 11, 2018: member

Needs rebase.

TheBlueMatt commented at 6:00 pm on January 11, 2018: member

Awaiting re-rebase of #11281. Will probably just wait for that to get merged.

in src/net_processing.cpp:1087 in e5b67ef861 outdated

1043@@ -1044,6 +1044,8 @@ bool static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
1044         fWitnessesPresentInARecentCompactBlock = fWitnessesPresentInMostRecentCompactBlock;
1045     }
1046 
1047+    const CBlockIndex *pindex = nullptr;
1048+    {

jimpo commented at 8:08 pm on January 11, 2018:

I find the indentation here confusing. I assume it’s not indented to help reviewers, so maybe indent in a separate commit?

in src/net_processing.cpp:1092 in e5b67ef861 outdated

1088@@ -1085,16 +1089,20 @@ bool static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
1089     }
1090     // Pruned nodes may have deleted the block, so check whether
1091     // it's available before trying to send.
1092-    if (send && (mi->second->nStatus & BLOCK_HAVE_DATA))
1093+    }

jimpo commented at 8:09 pm on January 11, 2018:

Add a // releases cs_main if there is no indentation.

in src/net_processing.cpp:1141 in e5b67ef861 outdated

1088@@ -1085,16 +1089,20 @@ bool static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
1089     }
1090     // Pruned nodes may have deleted the block, so check whether

jimpo commented at 8:09 pm on January 11, 2018:

This comment should get moved up to line 1066.

jimpo commented at 9:43 pm on January 11, 2018: contributor

The concurrent access to data on disk makes me a little nervous. Is it worth having a separate read-write lock around reading/writing block files?

TheBlueMatt commented at 9:45 pm on January 11, 2018: member

@jimpo are you aware of any modern systems that have an issue reading from the middle of a file while it is being appended to? That sounds like it should be considered a critical kernel bug if anything, no?

jimpo commented at 10:05 pm on January 11, 2018: contributor

I say that mostly because of the pruning race.

TheBlueMatt commented at 10:13 pm on January 11, 2018: member

Pruning should be atomic still, no? Either we opened the file, then any unlink operations on the underlying file will result in us being able to finish reading, or we didn’t open the file, so we can return false. I’m skeptical it can ever result in corrupt data, or am I missing some way Windows operates? It’s definitely worth auditing that ReadBlockFromDisk returning false is handled properly everywhere!

promag commented at 2:55 pm on February 3, 2018: member

Needs rebase.

laanwj added this to the "Blockers" column in a project

laanwj removed this from the "Blockers" column in a project

TheBlueMatt force-pushed on Mar 27, 2018

TheBlueMatt commented at 8:22 pm on March 27, 2018: member

Rebased.

TheBlueMatt force-pushed on Mar 28, 2018

TheBlueMatt force-pushed on Apr 28, 2018

TheBlueMatt commented at 1:32 am on April 28, 2018: member

Rebased. In addition to RPC latency in some calls, this should also now make a nontrivial difference to -txindex nodes restart resync after an unclean shutdown (which, at least, should be useful to me).

Significantly reduce GetTransaction cs_main locking 613758d50d

[net_processing] Avoid holding cs_main during ReadBlockFromDisk 62a4d71b7b

TheBlueMatt force-pushed on Apr 28, 2018

Avoid uneccessary cs_main during block reading in rest/rpc/zmq cb14810e6a

TheBlueMatt force-pushed on Apr 28, 2018

promag commented at 1:40 pm on May 3, 2018: member

Needs rebase.

MarcoFalke added the label Needs rebase on Jun 6, 2018

TheBlueMatt closed this on Jun 14, 2018

MarcoFalke referenced this in commit f7e182a973 on Jan 4, 2019

jasonbcox referenced this in commit ec204539c2 on May 31, 2019

jonspock referenced this in commit dd61be2825 on Jun 23, 2019

jtoomim referenced this in commit 86e8de8a80 on Jun 29, 2019

jonspock referenced this in commit a4f40db8d0 on Jul 9, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

Munkybooty referenced this in commit d1d56bd940 on Aug 14, 2021

Munkybooty referenced this in commit 521d1bc0c3 on Aug 24, 2021

Munkybooty referenced this in commit 9ae8784746 on Aug 24, 2021

UdjinM6 referenced this in commit 2234071ec0 on Aug 24, 2021

Munkybooty referenced this in commit 31c5651a7d on Aug 24, 2021

Munkybooty referenced this in commit 9436caf89a on Aug 24, 2021

MarcoFalke locked this on Dec 16, 2021

Avoid cs_main during ReadBlockFromDisk Calls #11913