Only in use on 64-bit systems. Use the upper 28bits of the id of an
index entry as bloom filter. This allows skipping the index entry
traversal most of the time if an id is not stored in the hashmap.
The bloom filter embedded in the index entry id is check each time
before following a reference to an index entry. This further reduces
the risk of false positives. The bloom filter itself is basically for
free on modern CPUs.
The main performance cost of checking for unknown blobs in the index are
the essentially random RAM accesses for the initial bucket lookup as
well as following the next pointer in the index entries. With the bloom
filter most of the time only the initial bucket lookup is necessary.
This speeds up checking for unknown blobs by a factor 5 (!), while
having no effect on the lookup of known blobs:
$ benchstat no-bloom with-bloom
name old time/op new time/op delta
IndexHasUnknown-16 49.0ms ± 2% 9.9ms ± 7% -79.70% (p=0.000 n=10+10)
IndexHasKnown-16 48.0ms ± 3% 47.9ms ± 3% ~ (p=0.968 n=10+9)
This bloom filter parameters m=28 k=1 were derived empirically, while
also leaving sufficient room for very large repositories. Before this
commit, the final merge index step took roughly 1 second per million
index entries. With the chosen bloom filter parameters, it would
currently take 19 hours to just merge such an index. It is safe to
assume that such large repositories don't exist.
Comparison with other parameter sets:
$ m=28 k=1 versus m=32 k=1
name old time/op new time/op delta
IndexHasUnknown-16 49.0ms ± 2% 9.7ms ±16% -80.17% (p=0.000 n=10+10)
IndexHasKnown-16 48.0ms ± 3% 48.4ms ± 3% ~ (p=0.436 n=10+10)
$ m=28 k=1 versus m=24 k=1
name old time/op new time/op delta
IndexHasUnknown-16 49.0ms ± 2% 10.8ms ±13% -77.90% (p=0.000 n=10+10)
IndexHasKnown-16 48.0ms ± 3% 47.9ms ± 3% ~ (p=0.684 n=10+10)
$ m=28 k=1 versus m=28 k=2
name old time/op new time/op delta
IndexHasUnknown-16 49.0ms ± 2% 24.9ms ± 5% -49.27% (p=0.000 n=10+10)
IndexHasKnown-16 48.0ms ± 3% 48.0ms ± 4% ~ (p=1.000 n=10+10)
`k=2` outright wrecks the performance. This is most likely the case as
it performs worse on longer index entry chains, which also happen to be
the expensive ones to process.
`m=32` yields diminishing returns, while getting within an order of
magnitude of the largest known restic repositories.
Design alternatives:
In principle it would be possible to add a single large bloom filter
instead of embedding them in the index entry ids. However, this bloom
filter would necessarily incur additional random memory accesses and
thus slow things down overall.
Do not require a full index reload if only a few additional index files
have been added. This can drastically speed up loading the index in the
mount command.
ui: mention compressed size of added files in `backup -vv`
This is already shown for modified files, but the added files message
wasn't updated when compression was implemented in restic.
Co-authored-by: Ilya Grigoriev <ilyagr@users.noreply.github.com>
cmd/restic/cmd_rewrite.go:
introduction of include filters for this command:
- add include filters, add error checking code
- add new parameter 'keepEmptyDirectoryFunc' to 'walker.NewSnapshotSizeRewriter()',
so empty directories have to be kept to keep the directory structure intact
- add parameter 'keepEmptySnapshot' to 'filterAndReplaceSnapshot()' to keep snapshots
intact when nothing is to be included
- introduce helper function 'gatherIncludeFilters()' and 'gatherExcludeFilters()' to
keep code flow clean
cmd/restic/cmd_rewrite_integration_test.go:
add several new tests around the 'include' functionality
internal/filter/include.go:
this is where is include filter is defined
internal/walker/rewriter.go:
- struct RewriteOpts gains field 'KeepEmtpyDirectory', which is a 'NodeKeepEmptyDirectoryFunc()'
which defaults to nil, so that al subdirectories are kept
- function 'NewSnapshotSizeRewriter()' gains the parameter 'keepEmptyDirecoryFilter' which
controls the management of empty subdirectories in case of include filters active
internal/data/tree.go:
gains a function Count() for checking the number if node elements in a newly built tree
internal/walker/rewriter_test.go:
function 'NewSnapshotSizeRewriter()' gets an additional parameter nil to keeps things happy
cmd/restic/cmd_repair_snapshots.go:
function 'filterAndReplaceSnapshot()' gets an additional parameter 'keepEmptySnapshot=nil'
doc/045_working_with_repos.rst:
gets to mention include filters
changelog/unreleased/issue-4278:
the usual announcement file
git rebase master -i produced this
restic rewrite include - keep linter happy
cmd/restic/cmd_rewrite_integration_test.go:
linter likes strings.Contain() better than my strings.Index() >= 0
While not planned, it's also not completely impossible that a tree node
might get additional top-level fields. As the tree iterator is built
with a strict expectation of the top-level fields, this would result in
a parsing error. Future-proof the code by simply skipping unknown
fields.
The previous implementation stored the whole tree in a map and used it
for checking overlap between trees. This is now replaced with the
DualTreeIterator, which iterates over two trees in parallel and returns
the merge stream in order. In case of overlap between both trees, it
returns both nodes at the same time. Otherwise, only a single node is
returned.
The TreeNodeIterator decodes nodes while iterating over a tree blob.
This should reduce peak memory usage as now only the serialized tree
blob and a single node have to alive at the same time. Using the
iterator has implications for the error handling however. Now it is
necessary that all loops that iterate through a tree check for errors
before using the node returned by the iterator.
The other change is that it is no longer possible to iterate over a tree
multiple times. Instead it must be loaded a second time. This only
affects the tree rewriting code.
The tree.Nodes will be replaced by an iterator to loads and serializes
tree node ondemand. Thus, the processing moves from StreamTrees into the
callback. Schedule them onto the workers used by StreamTrees for proper
load distribution.
Always serialize trees via TreeJSONBuilder. Add a wrapper called
TreeWriter which combines serialization and saving the tree blob in the
repository. In the future, TreeJSONBuilder will have to upload tree
chunks while the tree is still serialized. This will a wrapper like
TreeWriter, so add it right now already.
The archiver.treeSaver still directly uses the TreeJSONBuilder as it
requires special handling.
Calling t.Fatal internally triggers runtime.Goexit . This kills the
current goroutine while only running deferred code. Add an extra context
that gets canceled if the go routine exits while within the user
provided callback.
data.TestCreateSnapshot which is used in particular by TestFindUsedBlobs
and TestFindUsedBlobs could generate trees with duplicate file names.
This is invalid and going forward will result in an error.
* Fix: Correctly restore ACL inheritance state
When restoring a file or directory on Windows, the `IsInherited` property of its Access Control Entries (ACEs) was always being set to `False`, even if the ACEs were inherited in the original backup.
This was caused by the restore process calling the `SetNamedSecurityInfo` API without providing context about the object's inheritance policy. By default, this API applies the provided Discretionary Access Control List (DACL) as an explicit set of permissions, thereby losing the original inheritance state.
This commit fixes the issue by inspecting the `Control` flags of the saved Security Descriptor during restore. Based on whether the `SE_DACL_PROTECTED` flag is present, the code now adds the appropriate `PROTECTED_DACL_SECURITY_INFORMATION` or `UNPROTECTED_DACL_SECURITY_INFORMATION` flag to the `SetNamedSecurityInfo` API call.
By providing this crucial inheritance context, the Windows API can now correctly reconstruct the ACL, ensuring the `IsInherited` status of each ACE is preserved as it was at the time of backup.
* Fix: Correctly restore ACL inheritance flags
This commit resolves an issue where the ACL inheritance state (`IsInherited` property) was not being correctly restored for files and directories on Windows.
The root cause was that the `SECURITY_INFORMATION` flags used in the `SetNamedSecurityInfo` API call contained both the `PROTECTED_DACL_SECURITY_INFORMATION` and `UNPROTECTED_DACL_SECURITY_INFORMATION` flags simultaneously. When faced with this conflicting information, the Windows API defaulted to the more restrictive `PROTECTED` behavior, incorrectly disabling inheritance on restored items.
The fix modifies the `setNamedSecurityInfoHigh` function to first clear all existing inheritance-related flags from the `securityInfo` bitmask. It then adds the single, correct flag (`PROTECTED` or `UNPROTECTED`) based on the `SE_DACL_PROTECTED` control bit from the original, saved Security Descriptor.
This ensures that the API receives unambiguous instructions, allowing it to correctly preserve the inheritance state as it was at the time of backup. The accompanying test case for ACL inheritance now passes with this change.
* Fix inheritance flag handling in low-privilege security descriptor restore
When restoring files without admin privileges, the IsInherited property
of Access Control Entries (ACEs) was not being preserved correctly.
The low-privilege restore path (setNamedSecurityInfoLow) was using a
static PROTECTED_DACL_SECURITY_INFORMATION flag, which always marked
the restored DACL as explicitly set rather than inherited.
This commit updates setNamedSecurityInfoLow to dynamically determine
the correct inheritance flag based on the SE_DACL_PROTECTED control
flag from the original security descriptor, matching the behavior of
the high-privilege path (setNamedSecurityInfoHigh).
Changes:
- Update setNamedSecurityInfoLow to accept control flags parameter
- Add logic to set either PROTECTED_DACL_SECURITY_INFORMATION or
UNPROTECTED_DACL_SECURITY_INFORMATION based on the original SD
- Add TestRestoreSecurityDescriptorInheritanceLowPrivilege to verify
inheritance is correctly restored in low-privilege scenarios
This ensures that both admin and non-admin restore operations correctly
preserve the inheritance state of ACLs, maintaining the original
permissions flow on child objects.
Addresses review feedback on PR for issue #5427
* Refactor security flags into separate backup/restore variants
Split highSecurityFlags into highBackupSecurityFlags and
highRestoreSecurityFlags to avoid runtime bitwise operations.
This makes the code cleaner and more maintainable by using
appropriate flags for GET vs SET operations.
Addresses review feedback on PR for issue #5427
---------
Co-authored-by: Aneesh Nireshwalia <anireshw@akamai.com>
wg.Go() may not be called after wg.Wait(). This prevents connecting two
errgroups such that the errors are propagated between them if the child
errgroup dynamically starts goroutines. Instead use just a single errgroup,
and sequence the shutdown using a sync.WaitGroup. This is far simpler
and does not require any "clever" tricks.
Michael Eischer
Michael Eischer
gunar
Winfried Plappert
Ilya Grigoriev
Aneesh N