Use /proc/mounts instead of statfs(2) for ro state #1002
Conversation
collector/filesystem_linux.go
Outdated
| @@ -59,8 +59,15 @@ func (c *filesystemCollector) GetStats() ([]filesystemStats, error) { | |||
| } | |||
|
|
|||
| var ro float64 | |||
| if (buf.Flags & readOnly) != 0 { | |||
| ro = 1 | |||
| OPTIONS: | |||
There was a problem hiding this comment.
I'm not a fan of label jumps. Is there a better way to do this?
There was a problem hiding this comment.
I dislike them too, but I thought the equivalent conditional was sort of ugly.
var ro float64
for _, option := range strings.Split(labels.options, ",") {
if option == "ro" {
ro = 1
break
}
if option == "rw" {
break
}
}A switch statement seems more idiomatic but I'm happy to change styles for sake of dropping labels.
There was a problem hiding this comment.
Yes, a switch would be better.
Is there really a need to break? The efficiency gain is probably minimal.
There was a problem hiding this comment.
The only reason to break is a minimal efficiency gain ;-)
Dropped the branching since, to your point, we're only scanning tens of bytes.
Cheers.
While the statfs(2) approach is reliable for normally mounted filesystems, the flags returned can be inconsistent when filesystem has been remounted read-only after encountering an error. The returned flags do accurately represent the internal state of the filesystem, but they do not reflect whether the VFS layer will accept writes. Instead, it makes sense to parse the current VFS mount state from the options field in /proc/mounts since it takes precedence. Signed-off-by: Brandon Gilmore <bgilmore@valvesoftware.com>
|
Sorry for squashing your code review @SuperQ. I didn't realize the comments would be lost like tears in the rain. I'd be happy to resolve the build failure if it's a side effect of my change, but I'm unable to see the details. |
|
Yea, buildkite is somewhat annoying that way. The failures are due to openbsd test VMs running out of disk space. 😭 I'll see about fixing those. |
|
The buildkite error is unrelated (OpenBSD build server problems), so I'm going to merge this. |
The pull request #1002 changed the logic used on Linux servers to determine if a filesystem is read-only. As a result of this change, the variable `readOnly` is now unused and can be removed. Signed-off-by: Jerome Froelich <jeromefroelich@hotmail.com>
While the statfs(2) approach is reliable for normally mounted filesystems, the flags returned can be inconsistent when filesystem has been remounted read-only after encountering an error. The returned flags do accurately represent the internal state of the filesystem, but they do not reflect whether the VFS layer will accept writes. Instead, it makes sense to parse the current VFS mount state from the options field in /proc/mounts since it takes precedence. Signed-off-by: Brandon Gilmore <bgilmore@valvesoftware.com>
…#1173) The pull request prometheus#1002 changed the logic used on Linux servers to determine if a filesystem is read-only. As a result of this change, the variable `readOnly` is now unused and can be removed. Signed-off-by: Jerome Froelich <jeromefroelich@hotmail.com>
…#1173) The pull request prometheus#1002 changed the logic used on Linux servers to determine if a filesystem is read-only. As a result of this change, the variable `readOnly` is now unused and can be removed. Signed-off-by: Jerome Froelich <jeromefroelich@hotmail.com>
Fixes #591 with the approach discussed by @SuperQ and @discordianfish
I verified that this exports correctly on a very troubled test machine.