Merge cockroachdb#64385

64385: pkg/storage: sync file in SafeWriteToFile r=jbowens a=jbowens

Previously, `storage.SafeWriteToFile` did not guarantee that the file it
wrote was synced to the storage media.

I wonder if this could also explain cockroachdb#63847? I did a cursory read
through that famed fsyncgate thread again, and my understanding is
that it is possible, although not typical, for `ENOSPC` to not surface
until the operating system is syncing a dirty page in the background.
At least in some linux versions, the buffer is then marked as clean
and can be thrown away due to memory pressure, losing the
contents.

I'm realizing that we're going to need to give that fsyncgate thread
a thorough reading before building anything for out-of-disk handling.
Entering an out-of-disk mode and avoiding crashing through
detecting `ENOSPC` might not be possible if the `ENOSPC` is
surfaced through a fsync.

Release note (bug fix): Fix a bug where encryption-at-rest metadata was
not synced and might become corrupted during a hard reset.

Informs cockroachlabs/support#945.

Co-authored-by: Jackson Owens <[email protected]>

pkg/storage/BUILD.bazel

@@ -120,6 +120,7 @@ go_test(
         "disk_map_test.go",
         "engine_key_test.go",
         "engine_test.go",
+        "file_util_test.go",
         "intent_interleaving_iter_test.go",
         "intent_reader_writer_test.go",
         "main_test.go",

pkg/storage/file_util.go

@@ -12,26 +12,29 @@ package storage
 
 import (
 	"bytes"
-	"fmt"
 	"io"
 
 	"github.com/cockroachdb/pebble/vfs"
 )
 
-// SafeWriteToFile writes the byte slice to the filename, contained in dir, using the given fs.
-// It returns after both the file and the containing directory are synced.
+// SafeWriteToFile writes the byte slice to the filename, contained in dir,
+// using the given fs.  It returns after both the file and the containing
+// directory are synced.
 func SafeWriteToFile(fs vfs.FS, dir string, filename string, b []byte) error {
 	tempName := filename + ".crdbtmp"
 	f, err := fs.Create(tempName)
 	if err != nil {
-		fmt.Printf("%v\n", err)
 		return err
 	}
 	bReader := bytes.NewReader(b)
 	if _, err = io.Copy(f, bReader); err != nil {
 		f.Close()
 		return err
 	}
+	if err = f.Sync(); err != nil {
+		f.Close()
+		return err
+	}
 	if err = f.Close(); err != nil {
 		return err
 	}

pkg/storage/file_util_test.go

@@ -0,0 +1,66 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package storage
+
+import (
+	"io"
+	"io/ioutil"
+	"os"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/pebble/vfs"
+	"github.com/stretchr/testify/require"
+)
+
+func TestSafeWriteToFile(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	// Use an in-memory FS that strictly enforces syncs.
+	mem := vfs.NewStrictMem()
+	syncDir := func(dir string) {
+		fdir, err := mem.OpenDir(dir)
+		require.NoError(t, err)
+		require.NoError(t, fdir.Sync())
+		require.NoError(t, fdir.Close())
+	}
+	readFile := func(filename string) []byte {
+		f, err := mem.Open("foo/bar")
+		require.NoError(t, err)
+		b, err := ioutil.ReadAll(f)
+		require.NoError(t, err)
+		require.NoError(t, f.Close())
+		return b
+	}
+
+	require.NoError(t, mem.MkdirAll("foo", os.ModePerm))
+	syncDir("")
+	f, err := mem.Create("foo/bar")
+	require.NoError(t, err)
+	_, err = io.WriteString(f, "Hello world")
+	require.NoError(t, err)
+	require.NoError(t, f.Sync())
+	require.NoError(t, f.Close())
+	syncDir("foo")
+
+	// Discard any unsynced writes to make sure we set up the test
+	// preconditions correctly.
+	mem.ResetToSyncedState()
+	require.Equal(t, []byte("Hello world"), readFile("foo/bar"))
+
+	// Use SafeWriteToFile to atomically, durably change the contents of the
+	// file.
+	require.NoError(t, SafeWriteToFile(mem, "foo", "foo/bar", []byte("Hello everyone")))
+
+	// Discard any unsynced writes.
+	mem.ResetToSyncedState()
+	require.Equal(t, []byte("Hello everyone"), readFile("foo/bar"))
+}