mysqlctl: open backup files with fadvise(2) and FADV_SEQUENTIAL #16441
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16441 +/- ##
==========================================
- Coverage 68.66% 68.63% -0.04%
==========================================
Files 1548 1551 +3
Lines 199124 199468 +344
==========================================
+ Hits 136731 136900 +169
- Misses 62393 62568 +175 ☔ View full report in Codecov by Sentry. |
deepthi
removed
NeedsDescriptionUpdate
deepthi
added
Component: Backup and Restore
Type: Performance
and removed
NeedsIssue
| // hint to the kernel that the intent is to read this file sequentially | ||
| if err := fadviseSequential(fd); err != nil { | ||
| // close the open fd since we're not using it anymore | ||
| fd.Close() | ||
| return nil, vterrors.Wrapf(err, "could not fadvise sequential read for %v", name) | ||
| } |
There was a problem hiding this comment.
One alternative here, and not sure if there's a preference, this whole block can swapped out to a openForSequential and have the two implementation that both encapsulate the os.Open call as well.
//go:build linux
func openForSequential(name string) (*os.File, error) {
f, err := os.Open(name)
if err != nil {
return nil, err
}
fstat, err := f.Stat()
if err != nil {
f.Close()
return nil, err
}
if err := unix.Fadvise(int(f.Fd()), 0, fstat.Size(), unix.FADV_SEQUENTIAL); err != nil {
f.Close()
return nil, err
}
return f, nil
}//go:build !linux
func openForSequential(name string) (*os.File, error) {
return os.Open(name)
}I dunno if the project would have a preference for this instead.
There was a problem hiding this comment.
@mattrobenolt Seems like a good approach for this to have those separate implementations then.
There was a problem hiding this comment.
Technically the build tag is likely not linux though (see for example https://man.freebsd.org/cgi/man.cgi?query=posix_fadvise&sektion=2&manpath=FreeBSD+8.3-RELEASE)? But we only really support Vitess properly here and it's purely an optimization so Linux specific also seems fine if it's problematic to have a better build tag.
There was a problem hiding this comment.
From what I can tell, the golang.org/x/sys/unix only exposes unix.Fadvise for linux targets.
Also a GitHub search for this, every usage I can find also explicitly only targets linux: https://github.com/search?q=unix.Fadvise+language%3AGo&type=code&l=Go
I tried to look for a more generic build tag, but I don't think there's anything applicable, unless we directly used syscall.Syscall6 or whatever directly, and I don't think that's worth it. Seems more like if there's freebsd support, that should get into the unix package.
There was a problem hiding this comment.
Will swap this out tho for a generic openForSequential.
On Linux, the fadvise(2) syscall can be leveraged to hint to the kernel to readahead disk pages since the file is intended to be read sequentially. This works well with our use of opening this file, then slamming it into `io.Copy`. In benchmarking, this alone yields around ~50% improvement on the read side without other tunings. In a naive test, the setup was simply an io.Copy with an os.File for src, and io.Discard for dest: no fadvise: 222.41 MB/s with fadvise: 344.69 MB/s This is with no other tunings to the kernel. Refs vitessio#3931 Signed-off-by: Matt Robenolt <[email protected]>
…ssio#16441) Signed-off-by: Matt Robenolt <[email protected]>
On Linux, the fadvise(2) syscall can be leveraged to hint to the kernel to readahead disk pages since the file is intended to be read sequentially.
This works well with our use of opening this file, then slamming it into
io.Copy.In benchmarking, this alone yields around ~50% improvement on the read side without other tunings.
In a naive test, the setup was simply an io.Copy with an os.File for src, and io.Discard for dest:
no fadvise: 222.41 MB/s
with fadvise: 344.69 MB/s
This is with no other tunings to the kernel.
Related Issue(s)
#16442
Checklist