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

zrepl(perf) adding cur and dispatched IO counter. #190

Merged
merged 2 commits into from Feb 8, 2019

Conversation

Projects
None yet
4 participants
@pawanpraka1
Copy link

pawanpraka1 commented Feb 7, 2019

root@cstor-disk-1-jgo9-754f596bb5-jdfbt:/# zfs stats | jq
{
"stats": [
{
"name": "cstor-5ac49b1b-2ab4-11e9-a02a-42010a80014b/pvc-5fef263d-2ab4-11e9-a02a-42010a80014b",
"status": "Offline",
"rebuildStatus": "INIT",
"isIOAckSenderCreated": 0,
"isIOReceiverCreated": 0,
"runningIONum": 0,
"checkpointedIONum": 0,
"degradedCheckpointedIONum": 0,
"checkpointedTime": 0,
"rebuildBytes": 0,
"rebuildCnt": 0,
"rebuildDoneCnt": 0,
"rebuildFailedCnt": 0,
"readCount": 0,
"readLatency": 0,
"readByte": 0,
"writeCount": 0,
"writeLatency": 0,
"writeByte": 0,
"syncCount": 0,
"syncLatency": 0,
"curIOCnt": 0,
"dispatchedIOCnt": 0,
"ruio": {},
"wuio": {}
},
{
"pool": {
"cstor-5ac49b1b-2ab4-11e9-a02a-42010a80014b": {
"rzio": {
"32KB": "24576/3"
},
"wzio": {
"32KB": "1249280/161",
"64KB": "176128/4",
"128KB": "917504/8",
"160KB": "524288/4"
}
}
}
}
]
}

zrepl(perf) adding cur and dispatched IO counter.
Signed-off-by: Pawan <pawanprakash101@gmail.com>

@pawanpraka1 pawanpraka1 requested a review from vishnuitta Feb 7, 2019

@@ -197,6 +197,8 @@ typedef struct zvol_info_s {
uint64_t write_byte;
uint64_t sync_req_ack_cnt;
uint64_t sync_latency;
uint64_t cur_io_cnt;

This comment has been minimized.

Copy link
@vishnuitta

vishnuitta Feb 7, 2019

Member

how about changing this to inflight_io_cnt?
Also, please add description for these two variables, for ex., 'dispatched_io_cnt' includes the 'inflight_io_cnt' as well.

adding comment
Signed-off-by: Pawan <pawanprakash101@gmail.com>
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 7, 2019

Codecov Report

Merging #190 into zfs-0.7-release will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           zfs-0.7-release     #190      +/-   ##
===================================================
- Coverage             52.5%   52.45%   -0.06%     
===================================================
  Files                  240      240              
  Lines                78163    78168       +5     
===================================================
- Hits                 41040    41001      -39     
- Misses               37123    37167      +44

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81e5e95...a4651e8. Read the comment docs.

@vishnuitta
Copy link
Member

vishnuitta left a comment

changes are good

@vishnuitta vishnuitta merged commit b781be2 into openebs:zfs-0.7-release Feb 8, 2019

5 of 6 checks passed

Better Code Hub ✋ This code needs to be refactored
Details
ci/circleci: build_tag_0 Your tests passed on CircleCI!
Details
ci/circleci: build_tag_1 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 52.5%)
Details
codecov/project Absolute coverage decreased by -0.05% but relative coverage increased by +47.49% compared to 81e5e95
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pawanpraka1 pawanpraka1 deleted the pawanpraka1:stats branch Feb 8, 2019

pawanpraka1 added a commit to pawanpraka1/zfs that referenced this pull request Feb 8, 2019

zrepl(perf): adding inflight and dispatched IO counter (openebs#190)
Signed-off-by: Pawan <pawanprakash101@gmail.com>

vishnuitta added a commit that referenced this pull request Feb 8, 2019

cherrypick(v0.8.x): inflight IO stats and zvol worker tunable from zf…
…s-0.7-release (#192)

* zrepl(perf): adding inflight and dispatched IO counter (#190)

Signed-off-by: Pawan <pawanprakash101@gmail.com>

* zrepl(perf): adding tunable uzfs worker threads via env (#191)

Signed-off-by: Pawan <pawanprakash101@gmail.com>
@@ -434,6 +436,9 @@ uzfs_zvol_worker(void *arg)
(void) pthread_mutex_unlock(&zinfo->zinfo_mutex);

drop_refcount:
atomic_add_64(&zinfo->inflight_io_cnt, -1);
atomic_add_64(&zinfo->dispatched_io_cnt, -1);

This comment has been minimized.

Copy link
@richardelling

richardelling Feb 13, 2019

Apologies for the late comment, but in general we prefer to only have incrementing counters. If we need to know the difference between inflight and dispatched, then that logic can be succinctly described in an alert monitor.

Also, when the values are gauges (can decrement) vs counters (never decrement) then it is more difficult to downsample data. As it stands, we can only know the current difference between inflight and dispatched and downsampling this data be lossy making it more difficult to see trends.

This comment has been minimized.

Copy link
@vishnuitta

vishnuitta Feb 14, 2019

Member

Thanks @richardelling and glad to see your comments.
That makes sense.
I see that we may need three counters (which never decrements):
One at the place where dispatch is happening,
another at the place where thread started executing the IO
and, another at the place where thread completed executing the IO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.