From 195a5cf99600218b0e3fb2bbea3ed8325422b9b5 Mon Sep 17 00:00:00 2001 From: greatroar <@> Date: Mon, 2 Nov 2020 10:34:21 +0100 Subject: [PATCH 1/3] Save files under temporary name in local backend Fixes #3435. --- internal/backend/local/local.go | 50 +++++++++++-------- internal/backend/local/local_internal_test.go | 11 ++-- 2 files changed, 35 insertions(+), 26 deletions(-) diff --git a/internal/backend/local/local.go b/internal/backend/local/local.go index 3d9fbd374..bc7ea354d 100644 --- a/internal/backend/local/local.go +++ b/internal/backend/local/local.go @@ -3,6 +3,7 @@ package local import ( "context" "io" + "io/ioutil" "os" "path/filepath" "syscall" @@ -88,7 +89,8 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade return backoff.Permanent(err) } - filename := b.Filename(h) + finalname := b.Filename(h) + dir := filepath.Dir(finalname) defer func() { // Mark non-retriable errors as such @@ -97,19 +99,20 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade } }() - // create new file - f, err := openFile(filename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, backend.Modes.File) + // Create new file with a temporary name. + tmpname := filepath.Base(finalname) + "-tmp-" + f, err := tempFile(dir, tmpname) if b.IsNotExist(err) { debug.Log("error %v: creating dir", err) // error is caused by a missing directory, try to create it - mkdirErr := os.MkdirAll(filepath.Dir(filename), backend.Modes.Dir) + mkdirErr := fs.MkdirAll(dir, backend.Modes.Dir) if mkdirErr != nil { - debug.Log("error creating dir %v: %v", filepath.Dir(filename), mkdirErr) + debug.Log("error creating dir %v: %v", dir, mkdirErr) } else { // try again - f, err = openFile(filename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, backend.Modes.File) + f, err = tempFile(dir, tmpname) } } @@ -117,37 +120,44 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade return errors.WithStack(err) } + defer func(f *os.File) { + if err != nil { + _ = f.Close() // Double Close is harmless. + // Remove after Rename is harmless: we embed the final name in the + // temporary's name and no other goroutine will get the same data to + // Save, so the temporary name should never be reused by another + // goroutine. + _ = fs.Remove(f.Name()) + } + }(f) + // save data, then sync wbytes, err := io.Copy(f, rd) if err != nil { - _ = f.Close() return errors.WithStack(err) } // sanity check if wbytes != rd.Length() { - _ = f.Close() return errors.Errorf("wrote %d bytes instead of the expected %d bytes", wbytes, rd.Length()) } - if err = f.Sync(); err != nil { - pathErr, ok := err.(*os.PathError) - isNotSupported := ok && pathErr.Op == "sync" && pathErr.Err == syscall.ENOTSUP - // ignore error if filesystem does not support the sync operation - if !isNotSupported { - _ = f.Close() - return errors.WithStack(err) - } + // Ignore error if filesystem does not support fsync. + if err = f.Sync(); err != nil && !errors.Is(err, syscall.ENOTSUP) { + return errors.WithStack(err) } - err = f.Close() - if err != nil { + // Close, then rename. Windows doesn't like the reverse order. + if err = f.Close(); err != nil { + return errors.WithStack(err) + } + if err = os.Rename(f.Name(), finalname); err != nil { return errors.WithStack(err) } // try to mark file as read-only to avoid accidential modifications // ignore if the operation fails as some filesystems don't allow the chmod call // e.g. exfat and network file systems with certain mount options - err = setFileReadonly(filename, backend.Modes.File) + err = setFileReadonly(finalname, backend.Modes.File) if err != nil && !os.IsPermission(err) { return errors.WithStack(err) } @@ -155,7 +165,7 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade return nil } -var openFile = fs.OpenFile // Overridden by test. +var tempFile = ioutil.TempFile // Overridden by test. // Load runs fn with a reader that yields the contents of the file at h at the // given offset. diff --git a/internal/backend/local/local_internal_test.go b/internal/backend/local/local_internal_test.go index 030099488..8d2ec08c3 100644 --- a/internal/backend/local/local_internal_test.go +++ b/internal/backend/local/local_internal_test.go @@ -3,6 +3,7 @@ package local import ( "context" "errors" + "fmt" "os" "syscall" "testing" @@ -14,15 +15,13 @@ import ( ) func TestNoSpacePermanent(t *testing.T) { - oldOpenFile := openFile + oldTempFile := tempFile defer func() { - openFile = oldOpenFile + tempFile = oldTempFile }() - openFile = func(name string, flags int, mode os.FileMode) (*os.File, error) { - // The actual error from os.OpenFile is *os.PathError. - // Other functions called inside Save may return *os.SyscallError. - return nil, os.NewSyscallError("open", syscall.ENOSPC) + tempFile = func(_, _ string) (*os.File, error) { + return nil, fmt.Errorf("not creating tempfile, %w", syscall.ENOSPC) } dir, cleanup := rtest.TempDir(t) From 81e2499d19c7c2e360ebc8e972aaf64b2622e497 Mon Sep 17 00:00:00 2001 From: greatroar <@> Date: Fri, 9 Jul 2021 17:11:39 +0200 Subject: [PATCH 2/3] Sync directory to get durable writes in local backend --- internal/backend/local/local.go | 12 +++++++++++- internal/backend/local/local_unix.go | 22 ++++++++++++++++++++++ internal/backend/local/local_windows.go | 3 +++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/internal/backend/local/local.go b/internal/backend/local/local.go index bc7ea354d..33f81f182 100644 --- a/internal/backend/local/local.go +++ b/internal/backend/local/local.go @@ -142,7 +142,9 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade } // Ignore error if filesystem does not support fsync. - if err = f.Sync(); err != nil && !errors.Is(err, syscall.ENOTSUP) { + err = f.Sync() + syncNotSup := errors.Is(err, syscall.ENOTSUP) + if err != nil && !syncNotSup { return errors.WithStack(err) } @@ -154,6 +156,14 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade return errors.WithStack(err) } + // Now sync the directory to commit the Rename. + if !syncNotSup { + err = fsyncDir(dir) + if err != nil { + return errors.WithStack(err) + } + } + // try to mark file as read-only to avoid accidential modifications // ignore if the operation fails as some filesystems don't allow the chmod call // e.g. exfat and network file systems with certain mount options diff --git a/internal/backend/local/local_unix.go b/internal/backend/local/local_unix.go index cc99d4a0b..81250a550 100644 --- a/internal/backend/local/local_unix.go +++ b/internal/backend/local/local_unix.go @@ -3,11 +3,33 @@ package local import ( + "errors" "os" + "syscall" "github.com/restic/restic/internal/fs" ) +// fsyncDir flushes changes to the directory dir. +func fsyncDir(dir string) error { + d, err := os.Open(dir) + if err != nil { + return err + } + + err = d.Sync() + if errors.Is(err, syscall.ENOTSUP) { + err = nil + } + + cerr := d.Close() + if err == nil { + err = cerr + } + + return err +} + // set file to readonly func setFileReadonly(f string, mode os.FileMode) error { return fs.Chmod(f, mode&^0222) diff --git a/internal/backend/local/local_windows.go b/internal/backend/local/local_windows.go index ccf788072..72ced630c 100644 --- a/internal/backend/local/local_windows.go +++ b/internal/backend/local/local_windows.go @@ -4,6 +4,9 @@ import ( "os" ) +// Can't explicitly flush directory changes on Windows. +func fsyncDir(dir string) error { return nil } + // We don't modify read-only on windows, // since it will make us unable to delete the file, // and this isn't common practice on this platform. From 65908647e3090f5a4b0d0b9a89b1f2be747443e9 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 16 Jul 2021 22:11:01 +0200 Subject: [PATCH 3/3] Add changelog --- changelog/unreleased/pull-3436 | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog/unreleased/pull-3436 diff --git a/changelog/unreleased/pull-3436 b/changelog/unreleased/pull-3436 new file mode 100644 index 000000000..ffe1e5841 --- /dev/null +++ b/changelog/unreleased/pull-3436 @@ -0,0 +1,12 @@ +Enhancement: Improve local backend's resilience to (system) crashes + +Restic now ensures that files stored using the `local` backend are created +atomically (that is, files are either stored completely or not at all). This +ensures that no incomplete files are left behind even if restic is terminated +while writing a file. + +In addition, restic now tries to ensure that the directory in the repository +which contains a newly uploaded file is also written to disk. This can prevent +missing files if the system crashes or the disk is not properly unmounted. + +https://github.com/restic/restic/pull/3436