From d64d9547212723ec7e890256143c6d022969d867 Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Thu, 17 May 2018 09:26:40 +0200 Subject: [PATCH] lib/db: Fix prefixed walks (fixes #4925) (#4940) --- lib/db/benchmark_test.go | 56 ++++++++++---- lib/db/leveldb_dbinstance.go | 145 +++++++++++++++++------------------ lib/db/set.go | 8 +- lib/db/set_test.go | 45 +++++++++++ lib/model/model.go | 3 +- 5 files changed, 165 insertions(+), 92 deletions(-) diff --git a/lib/db/benchmark_test.go b/lib/db/benchmark_test.go index b331d352..144b6670 100644 --- a/lib/db/benchmark_test.go +++ b/lib/db/benchmark_test.go @@ -19,9 +19,13 @@ import ( ) var files, oneFile, firstHalf, secondHalf []protocol.FileInfo -var s *db.FileSet +var benchS *db.FileSet + +func lazyInitBenchFileSet() { + if benchS != nil { + return + } -func init() { for i := 0; i < 1000; i++ { files = append(files, protocol.FileInfo{ Name: fmt.Sprintf("file%d", i), @@ -36,9 +40,9 @@ func init() { oneFile = firstHalf[middle-1 : middle] ldb, _ := tempDB() - s = db.NewFileSet("test)", fs.NewFilesystem(fs.FilesystemTypeBasic, "."), ldb) - replace(s, remoteDevice0, files) - replace(s, protocol.LocalDeviceID, firstHalf) + benchS = db.NewFileSet("test)", fs.NewFilesystem(fs.FilesystemTypeBasic, "."), ldb) + replace(benchS, remoteDevice0, files) + replace(benchS, protocol.LocalDeviceID, firstHalf) } func tempDB() (*db.Instance, string) { @@ -70,16 +74,19 @@ func BenchmarkReplaceAll(b *testing.B) { } func BenchmarkUpdateOneChanged(b *testing.B) { + lazyInitBenchFileSet() + changed := make([]protocol.FileInfo, 1) changed[0] = oneFile[0] changed[0].Version = changed[0].Version.Update(myID) changed[0].Blocks = genBlocks(len(changed[0].Blocks)) + b.ResetTimer() for i := 0; i < b.N; i++ { if i%1 == 0 { - s.Update(protocol.LocalDeviceID, changed) + benchS.Update(protocol.LocalDeviceID, changed) } else { - s.Update(protocol.LocalDeviceID, oneFile) + benchS.Update(protocol.LocalDeviceID, oneFile) } } @@ -87,17 +94,23 @@ func BenchmarkUpdateOneChanged(b *testing.B) { } func BenchmarkUpdateOneUnchanged(b *testing.B) { + lazyInitBenchFileSet() + + b.ResetTimer() for i := 0; i < b.N; i++ { - s.Update(protocol.LocalDeviceID, oneFile) + benchS.Update(protocol.LocalDeviceID, oneFile) } b.ReportAllocs() } func BenchmarkNeedHalf(b *testing.B) { + lazyInitBenchFileSet() + + b.ResetTimer() for i := 0; i < b.N; i++ { count := 0 - s.WithNeed(protocol.LocalDeviceID, func(fi db.FileIntf) bool { + benchS.WithNeed(protocol.LocalDeviceID, func(fi db.FileIntf) bool { count++ return true }) @@ -110,9 +123,12 @@ func BenchmarkNeedHalf(b *testing.B) { } func BenchmarkHave(b *testing.B) { + lazyInitBenchFileSet() + + b.ResetTimer() for i := 0; i < b.N; i++ { count := 0 - s.WithHave(protocol.LocalDeviceID, func(fi db.FileIntf) bool { + benchS.WithHave(protocol.LocalDeviceID, func(fi db.FileIntf) bool { count++ return true }) @@ -125,9 +141,12 @@ func BenchmarkHave(b *testing.B) { } func BenchmarkGlobal(b *testing.B) { + lazyInitBenchFileSet() + + b.ResetTimer() for i := 0; i < b.N; i++ { count := 0 - s.WithGlobal(func(fi db.FileIntf) bool { + benchS.WithGlobal(func(fi db.FileIntf) bool { count++ return true }) @@ -140,9 +159,12 @@ func BenchmarkGlobal(b *testing.B) { } func BenchmarkNeedHalfTruncated(b *testing.B) { + lazyInitBenchFileSet() + + b.ResetTimer() for i := 0; i < b.N; i++ { count := 0 - s.WithNeedTruncated(protocol.LocalDeviceID, func(fi db.FileIntf) bool { + benchS.WithNeedTruncated(protocol.LocalDeviceID, func(fi db.FileIntf) bool { count++ return true }) @@ -155,9 +177,12 @@ func BenchmarkNeedHalfTruncated(b *testing.B) { } func BenchmarkHaveTruncated(b *testing.B) { + lazyInitBenchFileSet() + + b.ResetTimer() for i := 0; i < b.N; i++ { count := 0 - s.WithHaveTruncated(protocol.LocalDeviceID, func(fi db.FileIntf) bool { + benchS.WithHaveTruncated(protocol.LocalDeviceID, func(fi db.FileIntf) bool { count++ return true }) @@ -170,9 +195,12 @@ func BenchmarkHaveTruncated(b *testing.B) { } func BenchmarkGlobalTruncated(b *testing.B) { + lazyInitBenchFileSet() + + b.ResetTimer() for i := 0; i < b.N; i++ { count := 0 - s.WithGlobalTruncated(func(fi db.FileIntf) bool { + benchS.WithGlobalTruncated(func(fi db.FileIntf) bool { count++ return true }) diff --git a/lib/db/leveldb_dbinstance.go b/lib/db/leveldb_dbinstance.go index e95805b3..cd8f02cf 100644 --- a/lib/db/leveldb_dbinstance.go +++ b/lib/db/leveldb_dbinstance.go @@ -186,20 +186,28 @@ func (db *Instance) removeSequences(folder []byte, fs []protocol.FileInfo) { } func (db *Instance) withHave(folder, device, prefix []byte, truncate bool, fn Iterator) { + if len(prefix) > 0 { + unslashedPrefix := prefix + if bytes.HasSuffix(prefix, []byte{'/'}) { + unslashedPrefix = unslashedPrefix[:len(unslashedPrefix)-1] + } else { + prefix = append(prefix, '/') + } + + if f, ok := db.getFileTrunc(db.deviceKey(folder, device, unslashedPrefix), true); ok && !fn(f) { + return + } + } + t := db.newReadOnlyTransaction() defer t.close() dbi := t.NewIterator(util.BytesPrefix(db.deviceKey(folder, device, prefix)[:keyPrefixLen+keyFolderLen+keyDeviceLen+len(prefix)]), nil) defer dbi.Release() - slashedPrefix := prefix - if !bytes.HasSuffix(prefix, []byte{'/'}) { - slashedPrefix = append(slashedPrefix, '/') - } - for dbi.Next() { name := db.deviceKeyName(dbi.Key()) - if len(prefix) > 0 && !bytes.Equal(name, prefix) && !bytes.HasPrefix(name, slashedPrefix) { + if len(prefix) > 0 && !bytes.HasPrefix(name, prefix) { return } @@ -277,20 +285,26 @@ func (db *Instance) withAllFolderTruncated(folder []byte, fn func(device []byte, } func (db *Instance) getFile(key []byte) (protocol.FileInfo, bool) { + if f, ok := db.getFileTrunc(key, false); ok { + return f.(protocol.FileInfo), true + } + return protocol.FileInfo{}, false +} + +func (db *Instance) getFileTrunc(key []byte, trunc bool) (FileIntf, bool) { bs, err := db.Get(key, nil) if err == leveldb.ErrNotFound { - return protocol.FileInfo{}, false + return nil, false } if err != nil { l.Debugln("surprise error:", err) - return protocol.FileInfo{}, false + return nil, false } - var f protocol.FileInfo - err = f.Unmarshal(bs) + f, err := unmarshalTrunc(bs, trunc) if err != nil { l.Debugln("unmarshal error:", err) - return protocol.FileInfo{}, false + return nil, false } return f, true } @@ -306,75 +320,54 @@ func (db *Instance) getGlobal(folder, file []byte, truncate bool) (FileIntf, boo return nil, false } - var vl VersionList - err = vl.Unmarshal(bs) - if err == leveldb.ErrNotFound { - return nil, false - } - if err != nil { - l.Debugln("unmarshal error:", k, err) - return nil, false - } - if len(vl.Versions) == 0 { - l.Debugln("no versions:", k) + vl, ok := unmarshalVersionList(bs) + if !ok { return nil, false } - k = db.deviceKey(folder, vl.Versions[0].Device, file) - bs, err = t.Get(k, nil) - if err != nil { - l.Debugln("surprise error:", k, err) - return nil, false + if fi, ok := db.getFileTrunc(db.deviceKey(folder, vl.Versions[0].Device, file), truncate); ok { + return fi, true } - fi, err := unmarshalTrunc(bs, truncate) - if err != nil { - l.Debugln("unmarshal error:", k, err) - return nil, false - } - return fi, true + return nil, false } func (db *Instance) withGlobal(folder, prefix []byte, truncate bool, fn Iterator) { + if len(prefix) > 0 { + unslashedPrefix := prefix + if bytes.HasSuffix(prefix, []byte{'/'}) { + unslashedPrefix = unslashedPrefix[:len(unslashedPrefix)-1] + } else { + prefix = append(prefix, '/') + } + + if f, ok := db.getGlobal(folder, unslashedPrefix, truncate); ok && !fn(f) { + return + } + } + t := db.newReadOnlyTransaction() defer t.close() dbi := t.NewIterator(util.BytesPrefix(db.globalKey(folder, prefix)), nil) defer dbi.Release() - slashedPrefix := prefix - if !bytes.HasSuffix(prefix, []byte{'/'}) { - slashedPrefix = append(slashedPrefix, '/') - } - var fk []byte for dbi.Next() { - var vl VersionList - err := vl.Unmarshal(dbi.Value()) - if err != nil { - l.Debugln("unmarshal error:", err) - continue - } - if len(vl.Versions) == 0 { - l.Debugln("no versions:", dbi.Key()) - continue - } - name := db.globalKeyName(dbi.Key()) - if len(prefix) > 0 && !bytes.Equal(name, prefix) && !bytes.HasPrefix(name, slashedPrefix) { + if len(prefix) > 0 && !bytes.HasPrefix(name, prefix) { return } - fk = db.deviceKeyInto(fk, folder, vl.Versions[0].Device, name) - bs, err := t.Get(fk, nil) - if err != nil { - l.Debugln("surprise error:", err) + vl, ok := unmarshalVersionList(dbi.Value()) + if !ok { continue } - f, err := unmarshalTrunc(bs, truncate) - if err != nil { - l.Debugln("unmarshal error:", err) + fk = db.deviceKeyInto(fk, folder, vl.Versions[0].Device, name) + + f, ok := db.getFileTrunc(fk, truncate) + if !ok { continue } @@ -395,10 +388,8 @@ func (db *Instance) availability(folder, file []byte) []protocol.DeviceID { return nil } - var vl VersionList - err = vl.Unmarshal(bs) - if err != nil { - l.Debugln("unmarshal error:", err) + vl, ok := unmarshalVersionList(bs) + if !ok { return nil } @@ -426,14 +417,8 @@ func (db *Instance) withNeed(folder, device []byte, truncate bool, fn Iterator) var fk []byte for dbi.Next() { - var vl VersionList - err := vl.Unmarshal(dbi.Value()) - if err != nil { - l.Debugln("unmarshal error:", err) - continue - } - if len(vl.Versions) == 0 { - l.Debugln("no versions:", dbi.Key()) + vl, ok := unmarshalVersionList(dbi.Value()) + if !ok { continue } @@ -586,11 +571,8 @@ func (db *Instance) checkGlobals(folder []byte, meta *metadataTracker) { var fk []byte for dbi.Next() { - gk := dbi.Key() - var vl VersionList - err := vl.Unmarshal(dbi.Value()) - if err != nil { - l.Debugln("unmarshal error:", err) + vl, ok := unmarshalVersionList(dbi.Value()) + if !ok { continue } @@ -599,7 +581,7 @@ func (db *Instance) checkGlobals(folder []byte, meta *metadataTracker) { // there are global entries pointing to no longer existing files. Here // we find those and clear them out. - name := db.globalKeyName(gk) + name := db.globalKeyName(dbi.Key()) var newVL VersionList for i, version := range vl.Versions { fk = db.deviceKeyInto(fk, folder, version.Device, name) @@ -923,6 +905,19 @@ func unmarshalTrunc(bs []byte, truncate bool) (FileIntf, error) { return tf, err } +func unmarshalVersionList(data []byte) (VersionList, bool) { + var vl VersionList + if err := vl.Unmarshal(data); err != nil { + l.Debugln("unmarshal error:", err) + return VersionList{}, false + } + if len(vl.Versions) == 0 { + l.Debugln("empty version list") + return VersionList{}, false + } + return vl, true +} + // A "better" version of leveldb's errors.IsCorrupted. func leveldbIsCorrupted(err error) bool { switch { diff --git a/lib/db/set.go b/lib/db/set.go index 61c58386..b6e55707 100644 --- a/lib/db/set.go +++ b/lib/db/set.go @@ -193,8 +193,10 @@ func (s *FileSet) WithHaveSequence(startSeq int64, fn Iterator) { s.db.withHaveSequence([]byte(s.folder), startSeq, nativeFileIterator(fn)) } +// Except for an item with a path equal to prefix, only children of prefix are iterated. +// E.g. for prefix "dir", "dir/file" is iterated, but "dir.file" is not. func (s *FileSet) WithPrefixedHaveTruncated(device protocol.DeviceID, prefix string, fn Iterator) { - l.Debugf("%s WithPrefixedHaveTruncated(%v)", s.folder, device) + l.Debugf(`%s WithPrefixedHaveTruncated(%v, "%v")`, s.folder, device, prefix) s.db.withHave([]byte(s.folder), device[:], []byte(osutil.NormalizedFilename(prefix)), true, nativeFileIterator(fn)) } func (s *FileSet) WithGlobal(fn Iterator) { @@ -207,8 +209,10 @@ func (s *FileSet) WithGlobalTruncated(fn Iterator) { s.db.withGlobal([]byte(s.folder), nil, true, nativeFileIterator(fn)) } +// Except for an item with a path equal to prefix, only children of prefix are iterated. +// E.g. for prefix "dir", "dir/file" is iterated, but "dir.file" is not. func (s *FileSet) WithPrefixedGlobalTruncated(prefix string, fn Iterator) { - l.Debugf("%s WithPrefixedGlobalTruncated()", s.folder, prefix) + l.Debugf(`%s WithPrefixedGlobalTruncated("%v")`, s.folder, prefix) s.db.withGlobal([]byte(s.folder), []byte(osutil.NormalizedFilename(prefix)), true, nativeFileIterator(fn)) } diff --git a/lib/db/set_test.go b/lib/db/set_test.go index fd64e5e1..7f8da0de 100644 --- a/lib/db/set_test.go +++ b/lib/db/set_test.go @@ -50,6 +50,15 @@ func globalList(s *db.FileSet) []protocol.FileInfo { }) return fs } +func globalListPrefixed(s *db.FileSet, prefix string) []db.FileInfoTruncated { + var fs []db.FileInfoTruncated + s.WithPrefixedGlobalTruncated(prefix, func(fi db.FileIntf) bool { + f := fi.(db.FileInfoTruncated) + fs = append(fs, f) + return true + }) + return fs +} func haveList(s *db.FileSet, n protocol.DeviceID) []protocol.FileInfo { var fs []protocol.FileInfo @@ -61,6 +70,16 @@ func haveList(s *db.FileSet, n protocol.DeviceID) []protocol.FileInfo { return fs } +func haveListPrefixed(s *db.FileSet, n protocol.DeviceID, prefix string) []db.FileInfoTruncated { + var fs []db.FileInfoTruncated + s.WithPrefixedHaveTruncated(n, prefix, func(fi db.FileIntf) bool { + f := fi.(db.FileInfoTruncated) + fs = append(fs, f) + return true + }) + return fs +} + func needList(s *db.FileSet, n protocol.DeviceID) []protocol.FileInfo { var fs []protocol.FileInfo s.WithNeed(n, func(fi db.FileIntf) bool { @@ -892,6 +911,32 @@ func TestWithHaveSequence(t *testing.T) { }) } +func TestIssue4925(t *testing.T) { + ldb := db.OpenMemory() + + folder := "test)" + s := db.NewFileSet(folder, fs.NewFilesystem(fs.FilesystemTypeBasic, "."), ldb) + + localHave := fileList{ + protocol.FileInfo{Name: "dir"}, + protocol.FileInfo{Name: "dir.file"}, + protocol.FileInfo{Name: "dir/file"}, + } + + replace(s, protocol.LocalDeviceID, localHave) + + for _, prefix := range []string{"dir", "dir/"} { + pl := haveListPrefixed(s, protocol.LocalDeviceID, prefix) + if l := len(pl); l != 2 { + t.Errorf("Expected 2, got %v local items below %v", l, prefix) + } + pl = globalListPrefixed(s, prefix) + if l := len(pl); l != 2 { + t.Errorf("Expected 2, got %v global items below %v", l, prefix) + } + } +} + func replace(fs *db.FileSet, device protocol.DeviceID, files []protocol.FileInfo) { fs.Drop(device) fs.Update(device, files) diff --git a/lib/model/model.go b/lib/model/model.go index ebc77e44..15861347 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -2403,7 +2403,8 @@ func (m *Model) GlobalDirectoryTree(folder, prefix string, levels int, dirsonly files.WithPrefixedGlobalTruncated(prefix, func(fi db.FileIntf) bool { f := fi.(db.FileInfoTruncated) - if f.IsInvalid() || f.IsDeleted() || f.Name == prefix { + // Don't include the prefix itself. + if f.IsInvalid() || f.IsDeleted() || strings.HasPrefix(prefix, f.Name) { return true }