-
Notifications
You must be signed in to change notification settings - Fork 241
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
pre-allocate storage for metadata json files, see containers/podman#13967 #1480
Open
chilikk
wants to merge
1
commit into
containers:main
Choose a base branch
from
chilikk:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,10 @@ package ioutils | |
|
||
import ( | ||
"io" | ||
"math/rand" | ||
"os" | ||
"path/filepath" | ||
"strconv" | ||
"time" | ||
) | ||
|
||
|
@@ -17,6 +19,9 @@ type AtomicFileWriterOptions struct { | |
// On successful return from Close() this is set to the mtime of the | ||
// newly written file. | ||
ModTime time.Time | ||
// Whenever an atomic file is successfully written create a temporary | ||
// file of the same size in order to pre-allocate storage for the next write | ||
PreAllocate bool | ||
} | ||
|
||
var defaultWriterOptions = AtomicFileWriterOptions{} | ||
|
@@ -38,22 +43,33 @@ func NewAtomicFileWriterWithOpts(filename string, perm os.FileMode, opts *Atomic | |
// temporary file and closing it atomically changes the temporary file to | ||
// destination path. Writing and closing concurrently is not allowed. | ||
func newAtomicFileWriter(filename string, perm os.FileMode, opts *AtomicFileWriterOptions) (*atomicFileWriter, error) { | ||
f, err := os.CreateTemp(filepath.Dir(filename), ".tmp-"+filepath.Base(filename)) | ||
if err != nil { | ||
return nil, err | ||
} | ||
dir := filepath.Dir(filename) | ||
base := filepath.Base(filename) | ||
if opts == nil { | ||
opts = &defaultWriterOptions | ||
} | ||
random := "" | ||
// need predictable name when pre-allocated | ||
if !opts.PreAllocate { | ||
random = strconv.FormatUint(rand.Uint64(), 36) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not a sufficient replacement for |
||
} | ||
tmp := filepath.Join(dir, ".tmp"+random+"-"+base) | ||
// if pre-allocated the temporary file exists and contains some data | ||
// do not truncate it here, instead truncate at Close() | ||
f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE, perm) | ||
if err != nil { | ||
return nil, err | ||
} | ||
abspath, err := filepath.Abs(filename) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return &atomicFileWriter{ | ||
f: f, | ||
fn: abspath, | ||
perm: perm, | ||
noSync: opts.NoSync, | ||
f: f, | ||
fn: abspath, | ||
perm: perm, | ||
noSync: opts.NoSync, | ||
preAllocate: opts.PreAllocate, | ||
}, nil | ||
} | ||
|
||
|
@@ -91,12 +107,13 @@ func AtomicWriteFile(filename string, data []byte, perm os.FileMode) error { | |
} | ||
|
||
type atomicFileWriter struct { | ||
f *os.File | ||
fn string | ||
writeErr error | ||
perm os.FileMode | ||
noSync bool | ||
modTime time.Time | ||
f *os.File | ||
fn string | ||
writeErr error | ||
perm os.FileMode | ||
noSync bool | ||
preAllocate bool | ||
modTime time.Time | ||
} | ||
|
||
func (w *atomicFileWriter) Write(dt []byte) (int, error) { | ||
|
@@ -108,22 +125,35 @@ func (w *atomicFileWriter) Write(dt []byte) (int, error) { | |
} | ||
|
||
func (w *atomicFileWriter) Close() (retErr error) { | ||
defer func() { | ||
if retErr != nil || w.writeErr != nil { | ||
os.Remove(w.f.Name()) | ||
if !w.preAllocate { | ||
defer func() { | ||
if retErr != nil || w.writeErr != nil { | ||
os.Remove(w.f.Name()) | ||
} | ||
}() | ||
} else { | ||
truncateAt, err := w.f.Seek(0, io.SeekCurrent) | ||
if err != nil { | ||
return err | ||
} | ||
err = w.f.Truncate(truncateAt) | ||
if err != nil { | ||
return err | ||
} | ||
}() | ||
} | ||
if !w.noSync { | ||
if err := fdatasync(w.f); err != nil { | ||
w.f.Close() | ||
return err | ||
} | ||
} | ||
|
||
var size int64 = 0 | ||
// fstat before closing the fd | ||
info, statErr := w.f.Stat() | ||
if statErr == nil { | ||
w.modTime = info.ModTime() | ||
size = info.Size() | ||
} | ||
// We delay error reporting until after the real call to close() | ||
// to match the traditional linux close() behaviour that an fd | ||
|
@@ -138,11 +168,50 @@ func (w *atomicFileWriter) Close() (retErr error) { | |
return statErr | ||
} | ||
|
||
if err := os.Chmod(w.f.Name(), w.perm); err != nil { | ||
tmpName := w.f.Name() | ||
if err := os.Chmod(tmpName, w.perm); err != nil { | ||
return err | ||
} | ||
if w.writeErr == nil { | ||
return os.Rename(w.f.Name(), w.fn) | ||
if w.preAllocate { | ||
err := swapOrMove(tmpName, w.fn) | ||
if err != nil { | ||
return err | ||
} | ||
// ignore errors, this is a best effort operation | ||
preAllocate(tmpName, size, w.perm) | ||
return nil | ||
} | ||
return os.Rename(tmpName, w.fn) | ||
} | ||
return nil | ||
} | ||
|
||
// ensure that the file is of at least indicated size | ||
func preAllocate(filename string, size int64, perm os.FileMode) error { | ||
f, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_APPEND, perm) | ||
if err != nil { | ||
return err | ||
} | ||
defer f.Close() | ||
info, err := f.Stat() | ||
if err != nil { | ||
return err | ||
} | ||
extendBytes := size - info.Size() | ||
if extendBytes > 0 { | ||
var blockSize int64 = 65536 | ||
block := make([]byte, blockSize) | ||
for extendBytes > 0 { | ||
if blockSize > extendBytes { | ||
blockSize = extendBytes | ||
} | ||
_, err := f.Write(block[:blockSize]) | ||
if err != nil { | ||
return err | ||
} | ||
extendBytes -= blockSize | ||
} | ||
} | ||
return nil | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
PreAllocate
and!PreAllocate
code paths differ so much, and very importantly they have different security implications (PreAllocate
must not be used in directories with hostile writers, like*/tmp
), that I don’t think it makes much sense for them to use the same object at all.The comparatively small parts of the code that can be shared between the two can be shared another way (e.g. using a shared helper function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not just that,
PreAllocate
is also only correct if there is some external-to-pkg/ioutils
locking that ensures serialization of uses ofAtomicFileWriter
. (That’s fine for the container/image/layer stores, we have locks there. But it’s a new responsibility for callers of this code, and it needs to be very clearly documented.)