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

crl-release-23.1: vfs: include size of write in DiskSlowInfo #2503

Merged
merged 2 commits into from
Apr 28, 2023

Conversation

nicktrav
Copy link
Contributor

This is a backport of #2281 and #2499 to 23.1.


vfs: include size of write in DiskSlowInfo

This commit adds the size of a write to DiskSlowInfo, in cases where a write is
sized. A small write stalling out points at file system / disk issues, while
a large write taking time to complete may indicate CRDB issues with a certain
workload, etc.

vfs: mark file basename as safe to avoid log redaction

When a disk stall is detected, the operation and path of the file are
logged. Currently, in Cockroach, the path is redacted as it is not
marked as safe.

As a path in a production environment could contain sensitive
information (we have no control over how a user configures the directory
in which they use for the Pebble store), we want to avoid leaking too
much information. Only take the basename of the file, and mark this as
safe. The filenames that Pebble uses are well-formed, and should not
contain sensitive information.

joshimhoff and others added 2 commits April 27, 2023 13:41
This commit adds the size of a write to DiskSlowInfo, in cases where a write is
sized. A small write stalling out points at file system / disk issues, while
a large write taking time to complete may indicate CRDB issues with a certain
workload, etc.
When a disk stall is detected, the operation and path of the file are
logged. Currently, in Cockroach, the path is redacted as it is not
marked as safe.

As a path in a production environment could contain sensitive
information (we have no control over how a user configures the directory
in which they use for the Pebble store), we want to avoid leaking too
much information. Only take the basename of the file, and mark this as
safe. The filenames that Pebble uses are well-formed, and should not
contain sensitive information.
@nicktrav nicktrav requested review from jbowens, joshimhoff and a team April 27, 2023 20:47
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nicktrav
Copy link
Contributor Author

Figured these would be useful in 23.1.

Copy link
Contributor

@joshimhoff joshimhoff left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for backporting the change I wrote earlier.

@nicktrav
Copy link
Contributor Author

TFTRs!

@nicktrav nicktrav merged commit 11c9b6d into cockroachdb:crl-release-23.1 Apr 28, 2023
@nicktrav nicktrav deleted the nickt.23.1-disk-info branch April 28, 2023 17:33
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.

4 participants