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

Read I/O rate limit for background and foreground read request. #2440

Merged
merged 27 commits into from
Aug 5, 2021
Merged

Read I/O rate limit for background and foreground read request. #2440

merged 27 commits into from
Aug 5, 2021

Conversation

JinheLin
Copy link
Contributor

@JinheLin JinheLin commented Jul 20, 2021

What problem does this PR solve?

Problem Summary:

This PR is a sub-task of issue: #1720. It implements configurable I/O rate limit for background and foreground read request base on the statistic data of /proc/<pid>/io and /proc/<pid>/tasks/<tid>/io. You can refer the design document (Chinese) for more detail.

What is changed and how it works?

Proposal: TiFlash IO Rate Limiter

What's Changed:

  • New Configurations:

    storage.io-rate-limit.max-bytes-per-sec: 0
    storage.io-rate-limit.max-read-bytes-per-sec: 0
    storage.io-rate-limit.max-write-bytes-per-sec: 0
    

    max-bytes-per-sec is the maximum bandwidth for I/O limiter.

    max-read-bytes-per-sec and max-write-bytes-per-sec is the maximum bandwidth for read and write I/O limiter.

    Also, you can refer the document (Chinese) for more detail.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch:

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

Release note

  • Make I/O rate limiter more comprehensive.

Testing results

Limit the foreground read to 1MB/s, 2MB/s, 3MB/s, 60MB/s.Below is the screenshot of the testing results: yellow line is the bandwidth of the disk, red line is the statistic data of the limiter.

Before each test, we need to drop the page cache by the commands:

  • sync
  • echo 3 > /proc/sys/vm/drop_caches

Limit the foreground read to 1MB/s

image-20210720134319408

image-20210720141844598

Limit the foreground read to 2MB/s

image-20210720134404473

image-20210720142036154

Limit the foreground read to 3MB/s

image-20210720134222386

image-20210720142128927

Limit the foreground read to 60MB/s

image-20210720173816410

image-20210720173927498

Grafana I/O limiter example link: http://172.16.5.59:19169/d/SVbh2xUWk/jinhe-test-bench-tiflash-summary?orgId=1&from=1626766214135&to=1626774127373&viewPanel=84

@JinheLin
Copy link
Contributor Author

/run-all-tests

1 similar comment
@JinheLin
Copy link
Contributor Author

/run-all-tests

@JinheLin
Copy link
Contributor Author

/run-all-tests

@@ -0,0 +1,219 @@
#include <Encryption/RateLimiter.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, why RateLimiter.h is put in Encryption?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was in Encryption before.

@lidezhu Do you know why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment it in https://github.com/pingcap/tics/blob/master/dbms/src/Encryption/RateLimiter.h#L11. Put it simply, just to avoid the dependency of the io module on encryption module which makes the compile of some clickhouse unit test fail.

@flowbehappy flowbehappy self-requested a review August 5, 2021 03:14
Copy link
Contributor

@flowbehappy flowbehappy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a comment

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 5, 2021
@JinheLin
Copy link
Contributor Author

JinheLin commented Aug 5, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 5, 2021
@ti-srebot
Copy link
Collaborator

/run-all-tests

@ti-srebot
Copy link
Collaborator

@JinheLin merge failed.

@JinheLin
Copy link
Contributor Author

JinheLin commented Aug 5, 2021

/merge

@ti-srebot
Copy link
Collaborator

Your auto merge job has been accepted, waiting for:

  • 2577
  • 2579

@ti-srebot
Copy link
Collaborator

/run-all-tests

@ti-srebot
Copy link
Collaborator

@JinheLin merge failed.

@JinheLin
Copy link
Contributor Author

JinheLin commented Aug 5, 2021

/run-all-tests

@JinheLin
Copy link
Contributor Author

JinheLin commented Aug 5, 2021

/merge

@ti-srebot
Copy link
Collaborator

Your auto merge job has been accepted, waiting for:

  • 2573
  • 2571
  • 2454

@ti-srebot
Copy link
Collaborator

/run-all-tests

@ti-srebot ti-srebot merged commit 1ecef6c into pingcap:master Aug 5, 2021
JaySon-Huang pushed a commit to JaySon-Huang/tiflash that referenced this pull request Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants