Avoid lock: Call FlushStateToDisk(…) regardless of fCheckForPruning #11617

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:cs_LastBlockFile-fCheckForPruning changing 1 files +4 −2
  1. practicalswift commented at 10:27 am on November 6, 2017: contributor
    FlushStateToDisk(…) won’t do anything besides check if we need to prune if FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning which is guarded by the mutex cs_LastBlockFile.
  2. fanquake added the label Validation on Nov 6, 2017
  3. practicalswift renamed this:
    Add missing cs_LastBlockFile lock in AcceptBlock
    validation: Add missing cs_LastBlockFile lock in AcceptBlock
    on Nov 6, 2017
  4. promag commented at 2:52 pm on November 6, 2017: member
    From what I can tell it’s missing only one lock (in PruneAndFlush) maybe fix it here too?
  5. practicalswift force-pushed on Nov 6, 2017
  6. practicalswift commented at 3:28 pm on November 6, 2017: contributor
    @promag Good point. Looks good now now?
  7. promag commented at 3:31 pm on November 6, 2017: member
    In that case reword PR and commit, like Add missing cs_LastBlockFile locks
  8. practicalswift renamed this:
    validation: Add missing cs_LastBlockFile lock in AcceptBlock
    validation: Add missing cs_LastBlockFile locks
    on Nov 6, 2017
  9. practicalswift force-pushed on Nov 6, 2017
  10. practicalswift commented at 3:44 pm on November 6, 2017: contributor
    @promag Done :-)
  11. practicalswift force-pushed on Nov 6, 2017
  12. practicalswift commented at 4:23 pm on November 6, 2017: contributor
    @promag Had to revert to initial version. Encountered timeouts when adding locking in PruneAndFlush as suggested and haven’t investigated deeper yet. What is your suggested patch? :-)
  13. in src/validation.cpp:3235 in cc0d685674 outdated 
    3231@@ -3232,6 +3232,7 @@ static bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CValidation
3232         return AbortNode(state, std::string("System error: ") + e.what());
3233     }
3234 
3235+    LOCK(cs_LastBlockFile);
    TheBlueMatt commented at 8:25 pm on November 6, 2017:
    It seems like we could just always call FlushStateToDisk instead? It wont do anything besides check if we need to prune if FLUSH_STATE_NONE is given.
  14. practicalswift force-pushed on Nov 6, 2017
  15. practicalswift commented at 8:33 pm on November 6, 2017: contributor
    @TheBlueMatt Even better! PR updated. Please re-review :-)
  16. promag commented at 8:44 pm on November 6, 2017: member
    utACK 96ff97c. Please reword PR again.
  17. practicalswift renamed this:
    validation: Add missing cs_LastBlockFile locks
    Call FlushStateToDisk(...) regardless of fCheckForPruning
    on Nov 6, 2017
  18. in src/validation.cpp:3235 in 96ff97cc14 outdated 
    3231@@ -3232,8 +3232,7 @@ static bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CValidation
3232         return AbortNode(state, std::string("System error: ") + e.what());
3233     }
3234 
3235-    if (fCheckForPruning)
3236-        FlushStateToDisk(chainparams, state, FLUSH_STATE_NONE); // we just allocated more disk space for block files
3237+    FlushStateToDisk(chainparams, state, FLUSH_STATE_NONE); // we just allocated more disk space for block files
    TheBlueMatt commented at 9:51 pm on November 6, 2017:
    Comment now needs updated.
  19. TheBlueMatt commented at 9:56 pm on November 6, 2017: member
    utACK 96ff97cc14b483643020df2497ab06511f5d2ae8 modulo now-incorrect comment.
  20. practicalswift force-pushed on Nov 6, 2017
  21. practicalswift commented at 10:16 pm on November 6, 2017: contributor
    @TheBlueMatt Comment removed :-)
  22. TheBlueMatt commented at 11:46 pm on November 6, 2017: member
    utACK 932dcd49486c6616ee5bfab9588d59fd75c4f0d4
  23. promag commented at 0:54 am on November 7, 2017: member

    This PR is a bit messy now. Regarding the original intent, I think it should do:

    0@@ -2012,6 +2012,7 @@ void FlushStateToDisk() {
1 }
2
3 void PruneAndFlush() {
4+    LOCK2(cs_main, cs_LastBlockFile);
5     CValidationState state;
6     fCheckForPruning = true;
7     const CChainParams& chainparams = Params();

    An alternative is to add an argument to FlushStateToDisk, like bool force_check_for_pruning to avoid touching fCheckForPruning.

  24. TheBlueMatt commented at 0:58 am on November 7, 2017: member

    FlushStateToDisk already checks fCheckForPruning and doesn’t do anything if it’s not set and NONE is passed as the second argument. Despite the complication in FlushStateToDisk, I do not believe this PR has any effect except resolving the (probably not actually a) missing lock bug.

    On November 6, 2017 7:55:04 PM EST, “João Barbosa” notifications@github.com wrote:

    This PR is a bit messy now. Regarding the original intent, I think it should do:

    0@@ -2012,6 +2012,7 @@ void FlushStateToDisk() {
1}
2
3void PruneAndFlush() {
4+    LOCK2(cs_main, cs_LastBlockFile);
5    CValidationState state;
6    fCheckForPruning = true;
7    const CChainParams& chainparams = Params();

    An alternative is to add an argument to FlushStateToDisk, like bool force_check_for_pruning to avoid touching fCheckForPruning.

    – You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11617#issuecomment-342339354

  25. promag commented at 1:00 am on November 7, 2017: member
    @TheBlueMatt IMO the best approach to avoid long/recursive/shared locks is to create a snapshot of the chain and use that downstream. The only lock required is when the chain snapshot is created. A slight more advanced solution is copy-on-write, which compared to shared lock, doesn’t make the writer wait for readers to finish.
  26. promag commented at 1:04 am on November 7, 2017: member

    @TheBlueMatt PruneAndFlush changes fCheckForPruning without the lock.

    With:

    0@@ -2012,6 +2012,7 @@ void FlushStateToDisk() {
1 }
2
3 void PruneAndFlush() {
4+    AssertLockHeld(cs_LastBlockFile);
5     CValidationState state;
6     fCheckForPruning = true;
7     const CChainParams& chainparams = Params();

    The following fails:

    0./test/functional/pruning.py
12017-11-07 01:02:25.034000 TestFramework (INFO): Initializing test directory /var/folders/1v/8_69hby54nj2k3n6fywt44x80000gn/T/testgwrg340t
2Assertion failed: lock cs_LastBlockFile not held in validation.cpp:2015; locks held:

    Note ./configure CPPFLAGS=-DDEBUG_LOCKORDER.

  27. TheBlueMatt commented at 1:58 am on November 7, 2017: member

    PruneAndFlush() would change behavior in an otherwise-simple lock-fix PR, please, no.

    The other part of my context is #10279, in which I want to more clearly separate out the disk operations from validation logic - thus, I’m strongly averse to adding a disk-management lock in validation functions directly.

    On November 6, 2017 8:04:20 PM EST, “João Barbosa” notifications@github.com wrote:

    @TheBlueMatt PruneAndFlush changes fCheckForPruning without the lock.

    With:

    0@@ -2012,6 +2012,7 @@ void FlushStateToDisk() {
1}
2
3void PruneAndFlush() {
4+    AssertLockHeld(cs_LastBlockFile);
5    CValidationState state;
6    fCheckForPruning = true;
7    const CChainParams& chainparams = Params();

    The following fails:

    0./test/functional/pruning.py
12017-11-07 01:02:25.034000 TestFramework (INFO): Initializing test
2directory /var/folders/1v/8_69hby54nj2k3n6fywt44x80000gn/T/testgwrg340t
3Assertion failed: lock cs_LastBlockFile not held in
4validation.cpp:2015; locks held:

    Note ./configure CPPFLAGS=-DDEBUG_LOCKORDER.

    – You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11617#issuecomment-342341040

  28. practicalswift commented at 10:11 am on November 8, 2017: contributor
    We have one utACK from @TheBlueMatt. Anyone else willing to review? :-)
  29. promag commented at 1:26 pm on November 8, 2017: member

    Testing fCheckForPruning is not needed in AcceptBlock (which BTW asserts cs_main is held) since the check is done in FlushStateToDisk.

    ACK 932dcd4.

  30. practicalswift commented at 7:11 pm on November 21, 2017: contributor
    Ready for merge? :-)
  31. practicalswift commented at 10:48 am on January 28, 2018: contributor
    Any chance of this getting merged or should I close?
  32. practicalswift force-pushed on Jan 29, 2018
  33. practicalswift commented at 3:13 pm on January 29, 2018: contributor
    Rebased! Please re-review :-)
  34. practicalswift commented at 3:00 pm on March 14, 2018: contributor

    This trivial fix is blocking my other locking annotations work since I have to re-include this fix in all other PR:s to get correct locking. Having this merged into master would help the locking annotations effort :-)

    Ping @MarcoFalke, @TheBlueMatt and @laanwj :-)

  35. TheBlueMatt commented at 4:10 pm on March 15, 2018: member
    utACK 48c8610a9c42df6de5dead0f98dc6ced214345ba
  36. practicalswift force-pushed on Mar 29, 2018
  37. practicalswift commented at 2:11 pm on March 29, 2018: contributor
    Rebased! Please re-review :-)
  38. practicalswift renamed this:
    Call FlushStateToDisk(...) regardless of fCheckForPruning
    Avoid (previously missing) lock: Call FlushStateToDisk(...) regardless of fCheckForPruning
    on Apr 9, 2018
  39. practicalswift renamed this:
    Avoid (previously missing) lock: Call FlushStateToDisk(...) regardless of fCheckForPruning
    Avoid lock: Call FlushStateToDisk(...) regardless of fCheckForPruning
    on Apr 9, 2018
  40. practicalswift commented at 11:02 pm on April 9, 2018: contributor
    Any chance of getting this merged or re-reviewed?
  41. sdaftuar commented at 6:14 pm on April 10, 2018: member
    utACK. I’d prefer if we also added clearer documentation that FlushStateMode::NONE behaves in the way that we’re relying on here (perhaps an extra comment in FlushStateToDisk?).
  42. Call FlushStateToDisk(...) regardless of fCheckForPruning 
    FlushStateToDisk(...) won't do anything besides check if we need to prune if
FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning
which is guarded by the mutex cs_LastBlockFile.
    2311c7cc86
  43. Document how FlushStateMode::NONE is handled 0000d8f727
  44. practicalswift force-pushed on Apr 11, 2018
  45. practicalswift commented at 10:47 am on April 11, 2018: contributor
    @sdaftuar Good point! Documentation added. Please re-review :-)
  47. laanwj merged this on Apr 11, 2018
  48. laanwj closed this on Apr 11, 2018
  49. laanwj referenced this in commit 1b5723ee57 on Apr 11, 2018
  50. jasonbcox referenced this in commit 61b72926bc on Aug 16, 2019
  51. jonspock referenced this in commit d20295dd59 on Nov 4, 2019
  52. proteanx referenced this in commit 356a11e6ac on Nov 4, 2019
  53. PastaPastaPasta referenced this in commit da0e1360b2 on Nov 1, 2020
  54. LarryRuane referenced this in commit c930122e34 on Mar 22, 2021
  55. LarryRuane referenced this in commit a3890c9c9c on Mar 22, 2021
  56. practicalswift deleted the branch on Apr 10, 2021
  57. LarryRuane referenced this in commit 4e5622cc30 on Apr 26, 2021
  58. LarryRuane referenced this in commit b9b6303974 on Apr 26, 2021
  59. LarryRuane referenced this in commit 892e5ec8dd on May 27, 2021
  60. LarryRuane referenced this in commit a78deb8ac8 on May 27, 2021
  61. str4d referenced this in commit f1d48e6023 on Sep 23, 2021
  62. str4d referenced this in commit 261dbe731d on Sep 23, 2021
  63. gades referenced this in commit 7299e642e3 on Mar 28, 2022
  64. str4d referenced this in commit 0410057c07 on May 14, 2022
  65. str4d referenced this in commit a7101db8d3 on May 14, 2022
  66. DrahtBot locked this on Aug 16, 2022


practicalswift promag TheBlueMatt sdaftuar

Labels
Validation

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me