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

os/filestore: fix journal logger #12099

Merged
merged 1 commit into from Nov 28, 2016
Merged

Conversation

wjin
Copy link
Contributor

@wjin wjin commented Nov 21, 2016

counter type (value 10) shouldn't be decreased in that it may generate
negative value when monitoring.

Signed-off-by: Wei Jin wjin.cn@gmail.com

@liewegas
Copy link
Member

@athanatos this was introduced by f54e563 ... it does look like the dec shouldn't be there; can you confirm?

@liewegas liewegas added this to the kraken milestone Nov 21, 2016
@liewegas liewegas changed the title fix filestore perf logger os/filestore: fix journal logger Nov 21, 2016
@athanatos
Copy link
Contributor

No, the decrements should be right. Those counters are supposed to be at any moment equal to throttle values, thus, we decrement them when we release the throttle. If you like, you could restructure it to a pair of _taken and _released throttles, but I think it would be harder to read the code. In the current code, it should never go negative. If you saw it underflow, that's a bug and you should find where we are taking throttle without incrementing the counters.

@wjin
Copy link
Contributor Author

wjin commented Nov 22, 2016

These two items (journal_ops/bytes) were used to count the journal total value according to description.

And from the document about perf counters: If bit 8 is set (counter), the reader may want to subtract off the previously read value to get the delta during the previous interval.
Here it conflicts with the document.

We may have two ways to fix it?

  1. change it from value 10 to 2
  2. If we want to track 'active journal data' (submitted to memory or committed to journal disk, but not applied to store), we may add another two items.

@wjin
Copy link
Contributor Author

wjin commented Nov 22, 2016

No, the decrements should be right. Those counters are supposed to be at any moment equal to throttle values, thus, we decrement them when we release the throttle.

Actually, the decrements are still there. Flush is called and I only remove the logger dec.

@liewegas
Copy link
Member

liewegas commented Nov 22, 2016 via email

Using u64 intsead of u64_counter for journal_ops/bytes.

These two items are used to metric active journal entry/size,
active means data in journal queue and journal disk, which will be
applied soon and then become inactive or useless.

Signed-off-by: Wei Jin <wjin.cn@gmail.com>
@wjin
Copy link
Contributor Author

wjin commented Nov 22, 2016

Updated. As for dynamic throttling, I think people are care more about the active journal data instead of total data written to journal for monitoring.

@athanatos
Copy link
Contributor

lgtm

@liewegas liewegas merged commit ccf6da4 into ceph:master Nov 28, 2016
@wjin wjin deleted the fix_filestore_perfcounter branch November 29, 2016 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants