Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop IO priority to idle while reading blocks for peer requests and startup verification #9245

Open
wants to merge 2 commits into
base: master
from

Conversation

@luke-jr
Copy link
Member

luke-jr commented Nov 30, 2016

No description provided.

src/util.h Outdated Show resolved Hide resolved
@laanwj
Copy link
Member

laanwj commented Nov 30, 2016

Concept ACK. Though not very happy to introduce platform-specific voodoo - we only just got rid of thread priority manipulation. But it may be worth the hassle, I don't know.

Can we quantify whether this works or not somehow?

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 30, 2016

This will also delay other processing, in particular block relay-- at least until the handling is made more concurrent-- no? Not a reason to not do it, but maybe a reason to not do it by default for everyone.

I second the need to quantify this-- I could imagine it making for a big usability improvement. ... or not mattering at all. If the former, I want it... if the latter...

@luke-jr
Copy link
Member Author

luke-jr commented Nov 30, 2016

Whenever I restart my node lately, I find myself eventually manually ioniceing the entire process as it slows down other things monitoring it in iotop. I can't be sure it's sending out old blocks, but I can't imagine what else it'd be spending so much time reading... :/

Added Mac and Windows support for completeness.

@luke-jr luke-jr force-pushed the luke-jr:ionice branch Nov 30, 2016
Copy link
Contributor

ryanofsky left a comment

ACK 6430b9232048666996d5d4c3ed154907e3daff67 (after adding missing #includes)

src/utilioprio.h Outdated
#ifdef WIN32
bool ioprio_set_file_idle(FILE *);
#else
#define ioprio_set_file_idle(f) (false)

This comment has been minimized.

@ryanofsky

ryanofsky Nov 30, 2016 Contributor

Maybe change this to ((void)false) to prevent a compiler warning:

main.cpp:1673:12: warning: statement has no effect [-Wunused-value]
     ioprio_set_file_idle(filein.Get());
src/utilioprio.h Outdated
#define IOPRIO_IDLER(actually_idle) ioprio_idler ioprio_idler_(actually_idle)

#else
#define ioprio_get() (-1)

This comment has been minimized.

@ryanofsky

ryanofsky Nov 30, 2016 Contributor

It doesn't seem like it should be necessary to declare these if the ioprio_idler class isn't around to call them.

This comment has been minimized.

@luke-jr

luke-jr Nov 30, 2016 Author Member

We can simplify some of the other stuff (move the logic into the class itself) if low-level access is undesired, but for now it's too early to know if these won't be needed IMO.

src/utilioprio.cpp Outdated
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifdef HAVE_IOPRIO_SYSCALL

This comment has been minimized.

@ryanofsky

ryanofsky Nov 30, 2016 Contributor

Needs #include "config/bitcoin-config.h" to prevent link errors.

src/utilioprio.h Outdated
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_UTIL_IOPRIO_H

This comment has been minimized.

@ryanofsky

ryanofsky Nov 30, 2016 Contributor

Should add #include "config/bitcoin-config.h"

@fanquake
Copy link
Member

fanquake commented Dec 1, 2016

Travis failure:

'../../src/'`utilioprio.cpp
In file included from ../../src/utilioprio.cpp:9:0:
../../src/utilioprio.h: In destructor ‘ioprio_idler::~ioprio_idler()’:
../../src/utilioprio.h:42:51: error: ‘LogPrintf’ was not declared in this scope
             LogPrintf("failed to restore ioprio\n");
@luke-jr
Copy link
Member Author

luke-jr commented Dec 1, 2016

Looks like to make the Windows part work, we need to bump _WIN32_WINNT to 0x0600 which means it will only run on Vista or newer. AFAIK this is okay(?), but I'm going to leave it for a separate PR...

@luke-jr luke-jr force-pushed the luke-jr:ionice branch 2 times, most recently to 3b9eeae Dec 1, 2016
@rebroad
Copy link
Contributor

rebroad commented Dec 19, 2016

I like this (concept ACK) although I wonder what the impact is on the p2p network as a whole if everyone ran this.

@luke-jr luke-jr force-pushed the luke-jr:ionice branch from 3b9eeae to 4073580 Feb 4, 2017
@martinschwarz
Copy link

martinschwarz commented Mar 11, 2017

Looks like to make the Windows part work, we need to bump _WIN32_WINNT to 0x0600 which means it will only run on Vista or newer.

There are win32 and win64 builds. Can't this just be enabled on the win64 build only?

@laanwj
Copy link
Member

laanwj commented Mar 13, 2017

Looks like to make the Windows part work, we need to bump _WIN32_WINNT to 0x0600 which means it will only run on Vista or newer. AFAIK this is okay(?), but I'm going to leave it for a separate PR...

Isn't Vista the version after Windows XP? As we dropped support for Windows XP in 0.13, it seems that requiring Vista for 0.15 is fine.

There are win32 and win64 builds. Can't this just be enabled on the win64 build only?

Could be done, but it'd be confusing to couple those. The low-end systems running 32-bit versions would probably need this more.

@luke-jr
Copy link
Member Author

luke-jr commented Aug 21, 2017

Rebased...

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Aug 21, 2017

Hmm, I dont think this is really the best idea as long as our message processing is still single-threaded. Really we need to refactor stuff so that block reading is async and the network processing can continue for other peers while we're serving blocks for peers in IBD, otherwise we may block receiving a new block longer than required.

@luke-jr
Copy link
Member Author

luke-jr commented Aug 21, 2017

That's somewhat independent from this issue. If users need to shut off their node to use their computer, the delay for processing a new block will be even longer.

@luke-jr luke-jr force-pushed the luke-jr:ionice branch to e673bc7 Aug 21, 2017
@ryanofsky
Copy link
Contributor

ryanofsky commented Oct 12, 2017

@TheBlueMatt @luke-jr, maybe a compromise would be to make this behavior configurable, and perhaps to default to dropping priority if user is running bitcoin-qt on a desktop.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Nov 10, 2017

Another approach which might be simpler would be to have the validation.h-exposed versions of ReadBlockFromDisk drop io priority so that net_processing will use low priority when answering remote-node queries but connecting blocks will not. With 0.15 I/O when doing initial sync is somewhat better, so this may also be less of an issue now unless the user is running with -peerbloomfilters.

@luke-jr
Copy link
Member Author

luke-jr commented Nov 11, 2017

@TheBlueMatt That's exactly what this already does... priority is only dropped when serving peers, not when connecting blocks.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Nov 11, 2017

@luke-jr I was referring to the possibility of not exposing a priority flag in validation.h's API - that seems a bit overkill IMO, as evidenced by the fact that there are now two ReadBlockFromDisk calls in net_processing which dont get the low-priority flag :p. Though that would also result in RPC ReadBlockFromDisk calls getting de-prioritized.

More importantly, I'm curious how much we need this anymore - it seems most of the complaints about I/O usage were primarily due to 0.13.1 preferential peering...On systems where your I/O is severely limited, I both don't know how much this will help (in my experience Linux' ionice is mostly worthless when it comes to desktop latency) and don't know if its not better to direct people towards maxuploadtarget or peerbloomfilters so as to avoid simply slowing down your peers because your I/O is too slow.

@luke-jr
Copy link
Member Author

luke-jr commented Nov 11, 2017

Before writing this, I generally ionice'd the entire bitcoind process to maintain system usability.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Nov 16, 2017

Concept ACK. You need to mark the other ReadBlockFromDisks in net_processing low-priority as well.

@sipa
Copy link
Member

sipa commented Mar 6, 2018

Concept ACK, but needs rebase.

@luke-jr luke-jr force-pushed the luke-jr:ionice branch from e673bc7 to 9e161f0 Mar 6, 2018
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 20, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #19791 ([net processing] Move Misbehaving() to PeerManager by jnewbery)
  • #19522 (build: fix building libconsensus with reduced exports for Darwin targets by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@rebroad
Copy link
Contributor

rebroad commented Apr 23, 2020

This will also delay other processing, in particular block relay-- at least until the handling is made more concurrent-- no? Not a reason to not do it, but maybe a reason to not do it by default for everyone.

I'd have thought it's possible to make the io lowered only for historical block serving and not for the most recent blocks.

@rebroad
Copy link
Contributor

rebroad commented Apr 24, 2020

I've been testing this under WSL and Windows 10 but I can't discern any difference to the build without these changes. According to Process Explorer, all the threads are have I/O Priority of normal.
image

Is there any particular way that this ought to be tested?

@luke-jr
Copy link
Member Author

luke-jr commented Apr 24, 2020

On Linux, which is the only OS this PR supports...

rebroad added a commit to rebroad/bitcoin that referenced this pull request Apr 25, 2020
rebroad added a commit to rebroad/bitcoin that referenced this pull request Apr 26, 2020
rebroad added a commit to rebroad/bitcoin that referenced this pull request Apr 27, 2020
rebroad added a commit to rebroad/bitcoin that referenced this pull request Apr 29, 2020
@luke-jr luke-jr force-pushed the luke-jr:ionice branch from e127695 to 42321f1 Aug 20, 2020
@luke-jr
Copy link
Member Author

luke-jr commented Aug 20, 2020

Rebased again

@ryanofsky
Copy link
Contributor

ryanofsky commented Aug 24, 2020

re: #9245 (review)

Conditional concept and code review ACK e127695 if Evan's comments #9245 (comment) are addressed. Only change since last review is dropping windows support.

Easiest way to address the concerns, I think would be to make this configurable, and maybe only enable it by default for the gui client (assuming UI responsiveness is what motivated this), or only enabled by default on specific platforms where it's been benchmarked.

The change should probably also have release notes.

Same conditional ACK for 42321f1 which just rebases e127695. Hasn't been followup to the comments since then.

@luke-jr
Copy link
Member Author

luke-jr commented Aug 25, 2020

The issue is impacting performance of the rest of the system, whether GUI or daemon is irrelevant.

fanquake added a commit that referenced this pull request Aug 25, 2020
…lobally defined

1ccb9f3 Move Win32 defines to configure.ac to ensure they are globally defined (Luke Dashjr)

Pull request description:

  #9245 no longer needs this, since the main `_WIN32_WINNT` got bumped by something else.

  So rather than just lose it, might as well get it merged in independently.

  I'm not aware of any practical effects, but it seems safer to use the same API versions everywhere.

ACKs for top commit:
  fanquake:
    ACK 1ccb9f3 - checked that the binaries produced are the same.

Tree-SHA512: 273e9186579197be01b443b6968e26b9a8031d356fabc5b73aa967fcdb837df195b7ce0fc4e4529c85d9b86da6f2d7ff1bf56a3ff0cbbcd8cee8a9c2bf70a244
sidhujag added a commit to syscoin/syscoin that referenced this pull request Aug 25, 2020
…y are globally defined

1ccb9f3 Move Win32 defines to configure.ac to ensure they are globally defined (Luke Dashjr)

Pull request description:

  bitcoin#9245 no longer needs this, since the main `_WIN32_WINNT` got bumped by something else.

  So rather than just lose it, might as well get it merged in independently.

  I'm not aware of any practical effects, but it seems safer to use the same API versions everywhere.

ACKs for top commit:
  fanquake:
    ACK 1ccb9f3 - checked that the binaries produced are the same.

Tree-SHA512: 273e9186579197be01b443b6968e26b9a8031d356fabc5b73aa967fcdb837df195b7ce0fc4e4529c85d9b86da6f2d7ff1bf56a3ff0cbbcd8cee8a9c2bf70a244
@luke-jr
Copy link
Member Author

luke-jr commented Sep 11, 2020

@ryanofsky IMO it doesn't make sense to continue giving real-world users a bad experience, because of some theoretical issue nobody has ever observed.

@luke-jr luke-jr force-pushed the luke-jr:ionice branch from 42321f1 to c75bf6f Sep 11, 2020
@DrahtBot DrahtBot removed the Needs rebase label Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.