Skip to content

Commit

Permalink
vfs: add SyncData,SyncTo,Preallocate to vfs.File
Browse files Browse the repository at this point in the history
Expand the vfs.File interface to expose SyncData, SyncTo and Preallocate as
first-class methods. Previously, the file created through vfs.NewSyncingFile
would perform analagous I/O operations (eg, `Fdatasync`, `sync_file_range`,
`Fallocate`) outside the context of the interface. This easily allowed the
accidental loss of the disk-health checking over these operations by minor
tweaks to the interface of the disk-health checking implementation or by adding
intermediary VFS wrappers between the disk-health checking FS and the syncing
file.

See cockroachdb/cockroach#94373 where the introduction of an additional VFS
wrapper resulted in these I/O operations being uncovered by disk-stall
detection.
  • Loading branch information
jbowens committed Jan 31, 2023
1 parent a5c9053 commit 7540fdf
Show file tree
Hide file tree
Showing 22 changed files with 573 additions and 327 deletions.
10 changes: 10 additions & 0 deletions event_listener_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,16 @@ func (f loggingFile) Sync() error {
return f.File.Sync()
}

func (f loggingFile) SyncData() error {
fmt.Fprintf(f.w, "sync-data: %s\n", f.name)
return f.File.SyncData()
}

func (f loggingFile) SyncTo(length int64) (fullSync bool, err error) {
fmt.Fprintf(f.w, "sync-to(%d): %s\n", length, f.name)
return f.File.SyncTo(length)
}

// Verify event listener actions, as well as expected filesystem operations.
func TestEventListener(t *testing.T) {
var d *DB
Expand Down
21 changes: 20 additions & 1 deletion internal/errorfs/errorfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ const (
OpLock
// OpList describes a list directory operation.
OpList
// OpFilePreallocate describes a file preallocate operation.
OpFilePreallocate
// OpStat describes a path-based stat operation.
OpStat
// OpGetDiskUsage describes a disk usage operation.
Expand All @@ -72,7 +74,7 @@ func (o Op) OpKind() OpKind {
switch o {
case OpOpen, OpOpenDir, OpList, OpStat, OpGetDiskUsage, OpFileRead, OpFileReadAt, OpFileStat:
return OpKindRead
case OpCreate, OpLink, OpRemove, OpRemoveAll, OpRename, OpReuseForRewrite, OpMkdirAll, OpLock, OpFileClose, OpFileWrite, OpFileSync, OpFileFlush:
case OpCreate, OpLink, OpRemove, OpRemoveAll, OpRename, OpReuseForRewrite, OpMkdirAll, OpLock, OpFileClose, OpFileWrite, OpFileSync, OpFileFlush, OpFilePreallocate:
return OpKindWrite
default:
panic(fmt.Sprintf("unrecognized op %v\n", o))
Expand Down Expand Up @@ -363,13 +365,30 @@ func (f *errorFile) Stat() (os.FileInfo, error) {
return f.file.Stat()
}

func (f *errorFile) Preallocate(offset, length int64) error {
if err := f.inj.MaybeError(OpFilePreallocate, f.path); err != nil {
return err
}
return f.file.Preallocate(offset, length)
}

func (f *errorFile) Sync() error {
if err := f.inj.MaybeError(OpFileSync, f.path); err != nil {
return err
}
return f.file.Sync()
}

func (f *errorFile) SyncData() error {
// TODO(jackson): Consider error injection.
return f.file.SyncData()
}

func (f *errorFile) SyncTo(length int64) (fullSync bool, err error) {
// TODO(jackson): Consider error injection.
return f.file.SyncTo(length)
}

func (f *errorFile) Fd() uintptr {
return f.file.Fd()
}
2 changes: 1 addition & 1 deletion sstable/reader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ func TestInjectedErrors(t *testing.T) {

for _, prebuiltSST := range prebuiltSSTs {
run := func(i int) (reterr error) {
f, err := os.Open(filepath.FromSlash(prebuiltSST))
f, err := vfs.Default.Open(filepath.FromSlash(prebuiltSST))
require.NoError(t, err)
r, err := NewReader(errorfs.WrapFile(f, errorfs.OnIndex(int32(i))), ReaderOptions{})
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion sstable/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ func build(

func testReader(t *testing.T, filename string, comparer *Comparer, fp FilterPolicy) {
// Check that we can read a pre-made table.
f, err := os.Open(filepath.FromSlash("testdata/" + filename))
f, err := vfs.Default.Open(filepath.FromSlash("testdata/" + filename))
if err != nil {
t.Error(err)
return
Expand Down
46 changes: 23 additions & 23 deletions testdata/checkpoint
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,16 @@ set a 1
set b 2
set c 3
----
sync: db/000002.log
sync-data: db/000002.log

flush db
----
sync: db/000002.log
sync-data: db/000002.log
close: db/000002.log
create: db/000004.log
sync: db
create: db/000005.sst
sync: db/000005.sst
sync-data: db/000005.sst
close: db/000005.sst
sync: db
sync: db/MANIFEST-000001
Expand All @@ -85,16 +85,16 @@ set b 5
set d 7
set e 8
----
sync: db/000004.log
sync-data: db/000004.log

flush db
----
sync: db/000004.log
sync-data: db/000004.log
close: db/000004.log
reuseForWrite: db/000002.log -> db/000006.log
sync: db
create: db/000007.sst
sync: db/000007.sst
sync-data: db/000007.sst
close: db/000007.sst
sync: db
sync: db/MANIFEST-000001
Expand All @@ -103,7 +103,7 @@ batch db
set f 9
set g 10
----
sync: db/000006.log
sync-data: db/000006.log

checkpoint db checkpoints/checkpoint1
----
Expand All @@ -118,23 +118,23 @@ open-dir: checkpoints/checkpoint1
link: db/OPTIONS-000003 -> checkpoints/checkpoint1/OPTIONS-000003
open-dir: checkpoints/checkpoint1
create: checkpoints/checkpoint1/marker.format-version.000001.012
sync: checkpoints/checkpoint1/marker.format-version.000001.012
sync-data: checkpoints/checkpoint1/marker.format-version.000001.012
close: checkpoints/checkpoint1/marker.format-version.000001.012
sync: checkpoints/checkpoint1
close: checkpoints/checkpoint1
link: db/000005.sst -> checkpoints/checkpoint1/000005.sst
link: db/000007.sst -> checkpoints/checkpoint1/000007.sst
create: checkpoints/checkpoint1/MANIFEST-000001
sync: checkpoints/checkpoint1/MANIFEST-000001
sync-data: checkpoints/checkpoint1/MANIFEST-000001
close: checkpoints/checkpoint1/MANIFEST-000001
open-dir: checkpoints/checkpoint1
create: checkpoints/checkpoint1/marker.manifest.000001.MANIFEST-000001
sync: checkpoints/checkpoint1/marker.manifest.000001.MANIFEST-000001
sync-data: checkpoints/checkpoint1/marker.manifest.000001.MANIFEST-000001
close: checkpoints/checkpoint1/marker.manifest.000001.MANIFEST-000001
sync: checkpoints/checkpoint1
close: checkpoints/checkpoint1
create: checkpoints/checkpoint1/000006.log
sync: checkpoints/checkpoint1/000006.log
sync-data: checkpoints/checkpoint1/000006.log
close: checkpoints/checkpoint1/000006.log
sync: checkpoints/checkpoint1
close: checkpoints/checkpoint1
Expand All @@ -154,22 +154,22 @@ open-dir: checkpoints/checkpoint2
link: db/OPTIONS-000003 -> checkpoints/checkpoint2/OPTIONS-000003
open-dir: checkpoints/checkpoint2
create: checkpoints/checkpoint2/marker.format-version.000001.012
sync: checkpoints/checkpoint2/marker.format-version.000001.012
sync-data: checkpoints/checkpoint2/marker.format-version.000001.012
close: checkpoints/checkpoint2/marker.format-version.000001.012
sync: checkpoints/checkpoint2
close: checkpoints/checkpoint2
link: db/000007.sst -> checkpoints/checkpoint2/000007.sst
create: checkpoints/checkpoint2/MANIFEST-000001
sync: checkpoints/checkpoint2/MANIFEST-000001
sync-data: checkpoints/checkpoint2/MANIFEST-000001
close: checkpoints/checkpoint2/MANIFEST-000001
open-dir: checkpoints/checkpoint2
create: checkpoints/checkpoint2/marker.manifest.000001.MANIFEST-000001
sync: checkpoints/checkpoint2/marker.manifest.000001.MANIFEST-000001
sync-data: checkpoints/checkpoint2/marker.manifest.000001.MANIFEST-000001
close: checkpoints/checkpoint2/marker.manifest.000001.MANIFEST-000001
sync: checkpoints/checkpoint2
close: checkpoints/checkpoint2
create: checkpoints/checkpoint2/000006.log
sync: checkpoints/checkpoint2/000006.log
sync-data: checkpoints/checkpoint2/000006.log
close: checkpoints/checkpoint2/000006.log
sync: checkpoints/checkpoint2
close: checkpoints/checkpoint2
Expand All @@ -185,48 +185,48 @@ open-dir: checkpoints/checkpoint3
link: db/OPTIONS-000003 -> checkpoints/checkpoint3/OPTIONS-000003
open-dir: checkpoints/checkpoint3
create: checkpoints/checkpoint3/marker.format-version.000001.012
sync: checkpoints/checkpoint3/marker.format-version.000001.012
sync-data: checkpoints/checkpoint3/marker.format-version.000001.012
close: checkpoints/checkpoint3/marker.format-version.000001.012
sync: checkpoints/checkpoint3
close: checkpoints/checkpoint3
link: db/000005.sst -> checkpoints/checkpoint3/000005.sst
link: db/000007.sst -> checkpoints/checkpoint3/000007.sst
create: checkpoints/checkpoint3/MANIFEST-000001
sync: checkpoints/checkpoint3/MANIFEST-000001
sync-data: checkpoints/checkpoint3/MANIFEST-000001
close: checkpoints/checkpoint3/MANIFEST-000001
open-dir: checkpoints/checkpoint3
create: checkpoints/checkpoint3/marker.manifest.000001.MANIFEST-000001
sync: checkpoints/checkpoint3/marker.manifest.000001.MANIFEST-000001
sync-data: checkpoints/checkpoint3/marker.manifest.000001.MANIFEST-000001
close: checkpoints/checkpoint3/marker.manifest.000001.MANIFEST-000001
sync: checkpoints/checkpoint3
close: checkpoints/checkpoint3
create: checkpoints/checkpoint3/000006.log
sync: checkpoints/checkpoint3/000006.log
sync-data: checkpoints/checkpoint3/000006.log
close: checkpoints/checkpoint3/000006.log
sync: checkpoints/checkpoint3
close: checkpoints/checkpoint3

compact db
----
sync: db/000006.log
sync-data: db/000006.log
close: db/000006.log
reuseForWrite: db/000004.log -> db/000008.log
sync: db
create: db/000009.sst
sync: db/000009.sst
sync-data: db/000009.sst
close: db/000009.sst
sync: db
sync: db/MANIFEST-000001
create: db/000010.sst
sync: db/000010.sst
sync-data: db/000010.sst
close: db/000010.sst
sync: db
sync: db/MANIFEST-000001

batch db
set h 11
----
sync: db/000008.log
sync-data: db/000008.log

list db
----
Expand Down
14 changes: 7 additions & 7 deletions testdata/cleaner
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,16 @@ set a 1
set b 2
set c 3
----
sync: wal/000002.log
sync-data: wal/000002.log

flush db
----
sync: wal/000002.log
sync-data: wal/000002.log
close: wal/000002.log
create: wal/000004.log
sync: wal
create: db/000005.sst
sync: db/000005.sst
sync-data: db/000005.sst
close: db/000005.sst
sync: db
sync: db/MANIFEST-000001
Expand All @@ -47,23 +47,23 @@ rename: wal/000002.log -> wal/archive/000002.log
batch db
set d 4
----
sync: wal/000004.log
sync-data: wal/000004.log

compact db
----
sync: wal/000004.log
sync-data: wal/000004.log
close: wal/000004.log
create: wal/000006.log
sync: wal
create: db/000007.sst
sync: db/000007.sst
sync-data: db/000007.sst
close: db/000007.sst
sync: db
sync: db/MANIFEST-000001
mkdir-all: wal/archive 0755
rename: wal/000004.log -> wal/archive/000004.log
create: db/000008.sst
sync: db/000008.sst
sync-data: db/000008.sst
close: db/000008.sst
sync: db
sync: db/MANIFEST-000001
Expand Down
30 changes: 15 additions & 15 deletions testdata/event_listener
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,16 @@ sync: db

flush
----
sync: wal/000002.log
sync: wal/000002.log
sync-data: wal/000002.log
sync-data: wal/000002.log
close: wal/000002.log
create: wal/000004.log
sync: wal
[JOB 4] WAL created 000004
[JOB 5] flushing 1 memtable to L0
create: db/000005.sst
[JOB 5] flushing: sstable created 000005
sync: db/000005.sst
sync-data: db/000005.sst
close: db/000005.sst
sync: db
create: db/MANIFEST-000006
Expand All @@ -101,16 +101,16 @@ sync: db

compact
----
sync: wal/000004.log
sync: wal/000004.log
sync-data: wal/000004.log
sync-data: wal/000004.log
close: wal/000004.log
reuseForWrite: wal/000002.log -> wal/000007.log
sync: wal
[JOB 6] WAL created 000007 (recycled 000002)
[JOB 7] flushing 1 memtable to L0
create: db/000008.sst
[JOB 7] flushing: sstable created 000008
sync: db/000008.sst
sync-data: db/000008.sst
close: db/000008.sst
sync: db
create: db/MANIFEST-000009
Expand All @@ -125,7 +125,7 @@ sync: db
[JOB 8] compacting(default) L0 [000005 000008] (1.5 K) + L6 [] (0 B)
create: db/000010.sst
[JOB 8] compacting: sstable created 000010
sync: db/000010.sst
sync-data: db/000010.sst
close: db/000010.sst
sync: db
create: db/MANIFEST-000011
Expand All @@ -145,16 +145,16 @@ disable-file-deletions

flush
----
sync: wal/000007.log
sync: wal/000007.log
sync-data: wal/000007.log
sync-data: wal/000007.log
close: wal/000007.log
reuseForWrite: wal/000004.log -> wal/000012.log
sync: wal
[JOB 9] WAL created 000012 (recycled 000004)
[JOB 10] flushing 1 memtable to L0
create: db/000013.sst
[JOB 10] flushing: sstable created 000013
sync: db/000013.sst
sync-data: db/000013.sst
close: db/000013.sst
sync: db
create: db/MANIFEST-000014
Expand Down Expand Up @@ -227,24 +227,24 @@ open-dir: checkpoint
link: db/OPTIONS-000003 -> checkpoint/OPTIONS-000003
open-dir: checkpoint
create: checkpoint/marker.format-version.000001.012
sync: checkpoint/marker.format-version.000001.012
sync-data: checkpoint/marker.format-version.000001.012
close: checkpoint/marker.format-version.000001.012
sync: checkpoint
close: checkpoint
link: db/000013.sst -> checkpoint/000013.sst
link: db/000015.sst -> checkpoint/000015.sst
link: db/000010.sst -> checkpoint/000010.sst
create: checkpoint/MANIFEST-000016
sync: checkpoint/MANIFEST-000016
sync-data: checkpoint/MANIFEST-000016
close: checkpoint/MANIFEST-000016
open-dir: checkpoint
create: checkpoint/marker.manifest.000001.MANIFEST-000016
sync: checkpoint/marker.manifest.000001.MANIFEST-000016
sync-data: checkpoint/marker.manifest.000001.MANIFEST-000016
close: checkpoint/marker.manifest.000001.MANIFEST-000016
sync: checkpoint
close: checkpoint
create: checkpoint/000012.log
sync: checkpoint/000012.log
sync-data: checkpoint/000012.log
close: checkpoint/000012.log
sync: checkpoint
close: checkpoint
Expand All @@ -256,7 +256,7 @@ pebble: file deletion disablement invariant violated
close
----
close: db
sync: wal/000012.log
sync-data: wal/000012.log
close: wal/000012.log
close: db/MANIFEST-000016
close: db
Expand Down
Loading

0 comments on commit 7540fdf

Please sign in to comment.