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

[bcache] Add a new plugin for bcache #2384

Closed
wants to merge 1 commit into from
Closed

[bcache] Add a new plugin for bcache #2384

wants to merge 1 commit into from

Conversation

pponnuvel
Copy link
Contributor

@pponnuvel pponnuvel commented Jan 25, 2021

bcache is used as a caching device (typically an SSD) for
HDDs; bcache stats are useful to identify performance problems.

Closes: #2378
Resolves: #2384

Signed-off-by: Ponnuvel Palaniyappan ponnuvel.palaniyappan@canonical.com


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname email@example.com?
  • If this commit closes an existing issue, is the line Closes: #ISSUENUMBER included in an independent line?
  • If this commit resolves an existing pull request, is the line Resolves: #PRNUMBER included in an independent line?

@pponnuvel
Copy link
Contributor Author

I have tested the changes using a setup that uses 2 x 1:1 mapping (1 bcache device per backing device).

The sos report file (tar.xz) size increase with this setup is ~36KB. The files gathered for bcache plugin are text files, so even if the system has many number of bcache devices, it's not going to massively increase the sos report.

Attaching a sample sos report here:
sosreport-bcache-test.zip

(had to zip a tar.xz file because github doesn't allow tar .xz file ;)

Copy link
Member

@TurboTurtle TurboTurtle left a comment

Choose a reason for hiding this comment

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

General ack overall. Just 2 minor notes.

sos/report/plugins/bcache.py Outdated Show resolved Hide resolved
sos/report/plugins/bcache.py Outdated Show resolved Hide resolved
@slashdd
Copy link

slashdd commented Jan 26, 2021

Would it be best to execute add_copy_spec() iff the bcache module is loaded via SosPredicate' kmods ?

@pponnuvel
Copy link
Contributor Author

Thanks, @slashdd. Gated via a predicate (I believe having files also ensures the same).

Copy link
Contributor

@pmoravec pmoravec left a comment

Choose a reason for hiding this comment

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

ACK, relying on the reporter's knowledge that the collecting files other than priority_stats can't harm a similar way like priority_stats would.

def setup(self):

# Caution: reading /sys/fs/bcache/*/cache0/priority_stats is known
# to performance degradation on some kernels. Needs care if that's
Copy link
Contributor

Choose a reason for hiding this comment

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

A small nit, recommend changing to:
degrade performance on older kernels.

I'm suggesting this change just to be clear that the issue has been fixed in recent kernels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Dan,
My understanding is that performance is much improved with that fix, but it still has some impact. I am referring this message in that thread: https://lkml.org/lkml/2019/8/14/636

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you are right. There is still a minor performance hit due to locking.

bcache is used as a caching device (typically an SSD) for
HDDs; bcache stats are useful to identify performance problems.

Closes: #2378

Signed-off-by: Ponnuvel Palaniyappan <ponnuvel.palaniyappan@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a plugin to collect bcache perf stats
5 participants