From 74d7c8e625457a6429a7a51550888c06f5ce9ca6 Mon Sep 17 00:00:00 2001 From: Audrius Butkevicius Date: Fri, 19 Dec 2014 23:12:12 +0000 Subject: [PATCH 1/2] Efficient renames (fixes #1217) --- internal/model/puller.go | 100 ++++++++++++++++++++++++++++++++------ internal/osutil/osutil.go | 95 +++++++++++++++++++++++++++--------- 2 files changed, 159 insertions(+), 36 deletions(-) diff --git a/internal/model/puller.go b/internal/model/puller.go index fc736d67c..23727c8a0 100644 --- a/internal/model/puller.go +++ b/internal/model/puller.go @@ -297,7 +297,9 @@ func (p *Puller) pullerIteration(ignores *ignore.Matcher) int { changed := 0 - var deletions []protocol.FileInfo + fileDeletions := map[string]protocol.FileInfo{} + dirDeletions := []protocol.FileInfo{} + buckets := map[string][]protocol.FileInfo{} folderFiles.WithNeed(protocol.LocalDeviceID, func(intf db.FileIntf) bool { @@ -327,7 +329,20 @@ func (p *Puller) pullerIteration(ignores *ignore.Matcher) int { switch { case file.IsDeleted(): // A deleted file, directory or symlink - deletions = append(deletions, file) + if file.IsDirectory() { + dirDeletions = append(dirDeletions, file) + } else { + fileDeletions[file.Name] = file + df, ok := p.model.CurrentFolderFile(p.folder, file.Name) + // Local file can be already deleted, but with a lower version + // number, hence the deletion coming in again as part of + // WithNeed + if ok && !df.IsDeleted() { + // Put files into buckets per first hash + key := string(df.Blocks[0].Hash) + buckets[key] = append(buckets[key], df) + } + } case file.IsDirectory() && !file.IsSymlink(): // A new or changed directory p.handleDir(file) @@ -341,17 +356,41 @@ func (p *Puller) pullerIteration(ignores *ignore.Matcher) int { return true }) +nextFile: for { fileName, ok := p.queue.Pop() if !ok { break } - if f, ok := p.model.CurrentGlobalFile(p.folder, fileName); ok { - p.handleFile(f, copyChan, finisherChan) - } else { + + f, ok := p.model.CurrentGlobalFile(p.folder, fileName) + if !ok { // File is no longer in the index. Mark it as done and drop it. p.queue.Done(fileName) + continue } + + if !f.IsSymlink() { + key := string(f.Blocks[0].Hash) + for i, candidate := range buckets[key] { + if scanner.BlocksEqual(candidate.Blocks, f.Blocks) { + // Remove the candidate from the bucket + l := len(buckets[key]) - 1 + buckets[key][i] = buckets[key][l] + buckets[key] = buckets[key][:l] + // Remove the pending deletion (as we perform it by renaming) + delete(fileDeletions, candidate.Name) + + p.renameFile(candidate, f) + + p.queue.Done(fileName) + continue nextFile + } + } + } + + // Not a rename or a symlink, deal with it. + p.handleFile(f, copyChan, finisherChan) } // Signal copy and puller routines that we are done with the in data for @@ -367,13 +406,12 @@ func (p *Puller) pullerIteration(ignores *ignore.Matcher) int { // Wait for the finisherChan to finish. doneWg.Wait() - for i := range deletions { - deletion := deletions[len(deletions)-i-1] - if deletion.IsDirectory() { - p.deleteDir(deletion) - } else { - p.deleteFile(deletion) - } + for _, file := range fileDeletions { + p.deleteFile(file) + } + + for i := range dirDeletions { + p.deleteDir(dirDeletions[len(dirDeletions)-i-1]) } return changed @@ -479,6 +517,40 @@ func (p *Puller) deleteFile(file protocol.FileInfo) { } } +// renameFile attempts to rename an existing file to a destination +// and set the right attributes on it. +func (p *Puller) renameFile(source, target protocol.FileInfo) { + if debug { + l.Debugln(p, "taking rename shortcut", source.Name, "->", target.Name) + } + + from := filepath.Join(p.dir, source.Name) + to := filepath.Join(p.dir, target.Name) + + var err error + if p.versioner != nil { + err = osutil.Copy(from, to) + if err == nil { + err = osutil.InWritableDir(p.versioner.Archive, from) + } + } else { + err = osutil.TryRename(from, to) + } + + if err != nil { + l.Infof("Puller (folder %q, file %q): rename from %q: %v", p.folder, target.Name, source.Name, err) + return + } + + // Fix-up the metadata, and update the local index of the target file + p.shortcutFile(target) + + // Source file already has the delete bit set. + // Because we got rid of the file (by renaming it), we just need to update + // the index, and we're done with it. + p.model.updateLocal(p.folder, source) +} + // handleFile queues the copies and pulls as necessary for a single new or // changed file. func (p *Puller) handleFile(file protocol.FileInfo, copyChan chan<- copyBlocksState, finisherChan chan<- *sharedPullerState) { @@ -493,7 +565,7 @@ func (p *Puller) handleFile(file protocol.FileInfo, copyChan chan<- copyBlocksSt } p.queue.Done(file.Name) if file.IsSymlink() { - p.shortcutSymlink(curFile, file) + p.shortcutSymlink(file) } else { p.shortcutFile(file) } @@ -595,7 +667,7 @@ func (p *Puller) shortcutFile(file protocol.FileInfo) { } // shortcutSymlink changes the symlinks type if necessery. -func (p *Puller) shortcutSymlink(curFile, file protocol.FileInfo) { +func (p *Puller) shortcutSymlink(file protocol.FileInfo) { err := symlinks.ChangeType(filepath.Join(p.dir, file.Name), file.Flags) if err != nil { l.Infof("Puller (folder %q, file %q): symlink shortcut: %v", p.folder, file.Name, err) diff --git a/internal/osutil/osutil.go b/internal/osutil/osutil.go index ebd55942d..5ce2704a4 100644 --- a/internal/osutil/osutil.go +++ b/internal/osutil/osutil.go @@ -19,6 +19,7 @@ package osutil import ( "errors" "fmt" + "io" "os" "path/filepath" "runtime" @@ -32,34 +33,36 @@ var ErrNoHome = errors.New("No home directory found - set $HOME (or the platform // often enough that there is any contention on this lock. var renameLock sync.Mutex -// Rename renames a file, while trying hard to succeed on various systems by -// temporarily tweaking directory permissions and removing the destination -// file when necessary. Will make sure to delete the from file if the -// operation fails, so use only for situations like committing a temp file to -// it's final location. -func Rename(from, to string) error { +// TryRename renames a file, leaving source file intact in case of failure. +// Tries hard to succeed on various systems by temporarily tweaking directory +// permissions and removing the destination file when necessary. +func TryRename(from, to string) error { renameLock.Lock() defer renameLock.Unlock() - // Make sure the destination directory is writeable - toDir := filepath.Dir(to) - if info, err := os.Stat(toDir); err == nil && info.IsDir() && info.Mode()&0200 == 0 { - os.Chmod(toDir, 0755) - defer os.Chmod(toDir, info.Mode()) - } - - // On Windows, make sure the destination file is writeable (or we can't delete it) - if runtime.GOOS == "windows" { - os.Chmod(to, 0666) - err := os.Remove(to) - if err != nil && !os.IsNotExist(err) { - return err - } - } + return withPreparedTarget(to, func() error { + return os.Rename(from, to) + }) +} +// Rename moves a temporary file to it's final place. +// Will make sure to delete the from file if the operation fails, so use only +// for situations like committing a temp file to it's final location. +// Tries hard to succeed on various systems by temporarily tweaking directory +// permissions and removing the destination file when necessary. +func Rename(from, to string) error { // Don't leave a dangling temp file in case of rename error defer os.Remove(from) - return os.Rename(from, to) + return TryRename(from, to) +} + +// Copy copies the file content from source to destination. +// Tries hard to succeed on various systems by temporarily tweaking directory +// permissions and removing the destination file when necessary. +func Copy(from, to string) (err error) { + return withPreparedTarget(to, func() error { + return copyFileContents(from, to) + }) } // InWritableDir calls fn(path), while making sure that the directory @@ -123,3 +126,51 @@ func getHomeDir() (string, error) { return home, nil } + +// Tries hard to succeed on various systems by temporarily tweaking directory +// permissions and removing the destination file when necessary. +func withPreparedTarget(to string, f func() error) error { + // Make sure the destination directory is writeable + toDir := filepath.Dir(to) + if info, err := os.Stat(toDir); err == nil && info.IsDir() && info.Mode()&0200 == 0 { + os.Chmod(toDir, 0755) + defer os.Chmod(toDir, info.Mode()) + } + + // On Windows, make sure the destination file is writeable (or we can't delete it) + if runtime.GOOS == "windows" { + os.Chmod(to, 0666) + err := os.Remove(to) + if err != nil && !os.IsNotExist(err) { + return err + } + } + return f() +} + +// copyFileContents copies the contents of the file named src to the file named +// by dst. The file will be created if it does not already exist. If the +// destination file exists, all it's contents will be replaced by the contents +// of the source file. +func copyFileContents(src, dst string) (err error) { + in, err := os.Open(src) + if err != nil { + return + } + defer in.Close() + out, err := os.Create(dst) + if err != nil { + return + } + defer func() { + cerr := out.Close() + if err == nil { + err = cerr + } + }() + if _, err = io.Copy(out, in); err != nil { + return + } + err = out.Sync() + return +} From e50a8917ece67050e3b72d487eece7421cf99b6d Mon Sep 17 00:00:00 2001 From: Audrius Butkevicius Date: Sat, 20 Dec 2014 23:12:12 +0000 Subject: [PATCH 2/2] Add renames to integration tests --- test/h2/config.xml | 6 +++--- test/sync_test.go | 10 +++++----- test/util.go | 15 +++++++++++++++ 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/test/h2/config.xml b/test/h2/config.xml index 1a456eec1..0b6f68586 100644 --- a/test/h2/config.xml +++ b/test/h2/config.xml @@ -7,7 +7,7 @@ false 1 16 - 1 + 0 @@ -16,7 +16,7 @@ false 1 16 - 1 + 0 @@ -25,7 +25,7 @@ false 1 16 - 1 + 0
127.0.0.1:22001
diff --git a/test/sync_test.go b/test/sync_test.go index 941df00b1..85dc97fa7 100644 --- a/test/sync_test.go +++ b/test/sync_test.go @@ -105,7 +105,7 @@ func testSyncCluster(t *testing.T) { } // We'll use this file for appending data without modifying the time stamp. - fd, err := os.Create("s1/appendfile") + fd, err := os.Create("s1/test-appendfile") if err != nil { t.Fatal(err) } @@ -207,12 +207,12 @@ func testSyncCluster(t *testing.T) { break } - // Alter the "appendfile" without changing it's modification time. Sneaky! - fi, err := os.Stat("s1/appendfile") + // Alter the "test-appendfile" without changing it's modification time. Sneaky! + fi, err := os.Stat("s1/test-appendfile") if err != nil { t.Fatal(err) } - fd, err := os.OpenFile("s1/appendfile", os.O_APPEND|os.O_WRONLY, 0644) + fd, err := os.OpenFile("s1/test-appendfile", os.O_APPEND|os.O_WRONLY, 0644) if err != nil { t.Fatal(err) } @@ -228,7 +228,7 @@ func testSyncCluster(t *testing.T) { if err != nil { t.Fatal(err) } - err = os.Chtimes("s1/appendfile", fi.ModTime(), fi.ModTime()) + err = os.Chtimes("s1/test-appendfile", fi.ModTime(), fi.ModTime()) if err != nil { t.Fatal(err) } diff --git a/test/util.go b/test/util.go index 4342ceba3..76edf694e 100644 --- a/test/util.go +++ b/test/util.go @@ -116,6 +116,10 @@ func alterFiles(dir string) error { return err } + if strings.HasPrefix(filepath.Base(path), "test-") { + return nil + } + switch filepath.Base(path) { case ".stfolder": return nil @@ -161,6 +165,17 @@ func alterFiles(dir string) error { if err != nil { return err } + case r < 0.3 && comps > 1 && (info.Mode().IsRegular() || rand.Float64() < 0.2): + rpath := filepath.Dir(path) + if rand.Float64() < 0.2 { + for move := rand.Intn(comps - 1); move > 0; move-- { + rpath = filepath.Join(rpath, "..") + } + } + err = os.Rename(path, filepath.Join(rpath, randomName())) + if err != nil { + return err + } } return nil })