backupccl: avoid over-shrinking memory monitor#79503
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Apr 6, 2022
Merged
backupccl: avoid over-shrinking memory monitor#79503craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
adityamaru
approved these changes
Apr 6, 2022
| return nil, err | ||
| } | ||
| statsBytes, err = storageccl.DecryptFile(statsBytes, encryptionKey) | ||
| statsBytes, err = storageccl.DecryptFile(ctx, statsBytes, encryptionKey, nil) |
| require.NoError(t, writeBackupManifest(ctx, st, storage, "testmanifest", encOpts, desc)) | ||
| _, sz, err := readBackupManifest(ctx, &mem, storage, "testmanifest", encOpts) | ||
| require.NoError(t, err) | ||
| mem.Shrink(ctx, sz) |
Contributor
There was a problem hiding this comment.
nit: should we also Close() the bound account here?
This change updates DecryptFile to optionally use a memory monitor and
adjusts the relevant calling functions.
Previously, the readManifest function that used DecryptFile could
result in shrinking too many bytes from the bound account:
```
ERROR: a panic has occurred!
root: no bytes in account to release, current 29571, free 30851
(1) attached stack trace
-- stack trace:
| runtime.gopanic
| GOROOT/src/runtime/panic.go:1038
| [...repeated from below...]
Wraps: (2) attached stack trace
-- stack trace:
| github.com/cockroachdb/cockroach/pkg/util/log/logcrash.ReportOrPanic
| github.com/cockroachdb/cockroach/pkg/util/log/logcrash/crash_reporting.go:374
| github.com/cockroachdb/cockroach/pkg/util/mon.(*BoundAccount).Shrink
| github.com/cockroachdb/cockroach/pkg/util/mon/bytes_usage.go:709
| github.com/cockroachdb/cockroach/pkg/ccl/backupccl.(*restoreResumer).doResume.func1
| github.com/cockroachdb/cockroach/pkg/ccl/backupccl/pkg/ccl/backupccl/restore_job.go:1244
```
This was the result of us freeing `cap(descBytes)` despite the fact
that we never accounted for the fact that `descBytes` had been
re-assigned after the decryption without accounting for changes in the
capacity used by the old vs new buffer. That is, we called mem.Grow
with the capcity of the old buffer, but mem.Shrink with the capacity
of the new buffer.
We generally expect that the size of encrypted data to be larger than
the size of the plaintext data, so in most cases, the existing code
would work without error because it was freeing an amount smaller than
the original allocation.
However, while the plaintext data is smaller than the encrypted data,
that doesn't mean that the _capacity of the slice holding the
plaintext data_ is smaller than the _capacity of the slice holding the
encrypted data_.
The slice holding the encrypted data was created with mon.ReadAll
which has slice growth strategy of doubling the size until it reaches
8MB. The slice holding the plaintext data was created by
ioutil.ReadAll that defers to appends slice growth strategy, which
differs from that used in mon.ReadAll.
In the current implementations, for byte sizes around 30k, the
capacity of the buffer create by append's strategy is larger than that
created by mon.ReadAll despite the len of the buffer being smaller:
before decrypt: len(descBytes) = 27898, cap(descBytes) = 32768
after decrypt: len(descBytes) = 27862, cap(descBytes) = 40960
We could have fixed this by simply adjusting the memory account by the
difference. Instead, I've opted to thread the memory monitor into
DecryptFile to better account for the fact that we technically do hold
2 copies of the data during this decryption.
Fixes cockroachdb#79488
Release note: None
d04e281 to
30419a8
Compare
Collaborator
Author
|
bors r=adityamaru |
Contributor
|
Build succeeded: |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This change updates DecryptFile to optionally use a memory monitor and
adjusts the relevant calling functions.
Previously, the readManifest function that used DecryptFile could
result in shrinking too many bytes from the bound account:
This was the result of us freeing
cap(descBytes)despite the factthat we never accounted for the fact that
descByteshad beenre-assigned after the decryption without accounting for changes in the
capacity used by the old vs new buffer. That is, we called mem.Grow
with the capcity of the old buffer, but mem.Shrink with the capacity
of the new buffer.
We generally expect that the size of encrypted data to be larger than
the size of the plaintext data, so in most cases, the existing code
would work without error because it was freeing an amount smaller than
the original allocation.
However, while the plaintext data is smaller than the encrypted data,
that doesn't mean that the capacity of the slice holding the
plaintext data is smaller than the capacity of the slice holding the
encrypted data.
The slice holding the encrypted data was created with mon.ReadAll
which has slice growth strategy of doubling the size until it reaches
8MB. The slice holding the plaintext data was created by
ioutil.ReadAll that defers to appends slice growth strategy, which
differs from that used in mon.ReadAll.
In the current implementations, for byte sizes around 30k, the
capacity of the buffer create by append's strategy is larger than that
created by mon.ReadAll despite the len of the buffer being smaller:
We could have fixed this by simply adjusting the memory account by the
difference. Instead, I've opted to thread the memory monitor into
DecryptFile to better account for the fact that we technically do hold
2 copies of the data during this decryption.
Fixes #79488
Release note: None