Merge pull request #1237 from syncthing/renamer

Efficient renames (fixes #1217)

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)
+		f, ok := p.model.CurrentGlobalFile(p.folder, fileName)
-		} else {
+		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 {
+	for _, file := range fileDeletions {
-		deletion := deletions[len(deletions)-i-1]
+		p.deleteFile(file)
-		if deletion.IsDirectory() {
-			p.deleteDir(deletion)
-		} else {
-			p.deleteFile(deletion)
 	}
+
+	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)

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
+// TryRename renames a file, leaving source file intact in case of failure.
-// temporarily tweaking directory permissions and removing the destination
+// Tries hard to succeed on various systems by temporarily tweaking directory
-// file when necessary. Will make sure to delete the from file if the
+// permissions and removing the destination file when necessary.
-// operation fails, so use only for situations like committing a temp file to
+func TryRename(from, to string) error {
-// it's final location.
-func Rename(from, to string) error {
 	renameLock.Lock()
 	defer renameLock.Unlock()
 
-	// Make sure the destination directory is writeable
+	return withPreparedTarget(to, func() error {
-	toDir := filepath.Dir(to)
+		return os.Rename(from, 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
-		}
-	}
 
+// 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
+}

test/h2/config.xml

@@ -7,7 +7,7 @@
         <lenientMtimes>false</lenientMtimes>
         <copiers>1</copiers>
         <pullers>16</pullers>
-        <finishers>1</finishers>
+        <hashers>0</hashers>
     </folder>
     <folder id="s12" path="s12-2" ro="false" rescanIntervalS="15" ignorePerms="false">
         <device id="I6KAH76-66SLLLB-5PFXSOA-UFJCDZC-YAOMLEK-CP2GB32-BV5RQST-3PSROAU"></device>
@@ -16,7 +16,7 @@
         <lenientMtimes>false</lenientMtimes>
         <copiers>1</copiers>
         <pullers>16</pullers>
-        <finishers>1</finishers>
+        <hashers>0</hashers>
     </folder>
     <folder id="s23" path="s23-2" ro="false" rescanIntervalS="15" ignorePerms="false">
         <device id="JMFJCXB-GZDE4BN-OCJE3VF-65GYZNU-AIVJRET-3J6HMRQ-AUQIGJO-FKNHMQU"></device>
@@ -25,7 +25,7 @@
         <lenientMtimes>false</lenientMtimes>
         <copiers>1</copiers>
         <pullers>16</pullers>
-        <finishers>1</finishers>
+        <hashers>0</hashers>
     </folder>
     <device id="I6KAH76-66SLLLB-5PFXSOA-UFJCDZC-YAOMLEK-CP2GB32-BV5RQST-3PSROAU" name="s1" compression="true" introducer="false">
         <address>127.0.0.1:22001</address>

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!
+		// Alter the "test-appendfile" without changing it's modification time. Sneaky!
-		fi, err := os.Stat("s1/appendfile")
+		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)
 		}

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
 	})