crl-release-22.1: vfs: add SyncData,SyncTo,Preallocate to vfs.File #2284
Conversation
We currently sidestep the `vfs.File` interface to obtain the raw file
descriptor when the `File` is backed by an underlying `*os.File`.
This mechanism (where we cast to `interface{ Fd() }`) is fragile and
requires extra "fd wrappers" to plumb the `Fd` method through disk full and disk health wrappers.
Each fd wrapper introduces an extra virtual table indirection on every
File interface call.
This change cleans this up by making all `File`s implement `Fd()`; we
allow returning InvalidFd when there is no raw file descriptor. It is
up to the (very few) callers to check if the result is InvalidFd.
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.
|
This change is |
There was a problem hiding this comment.
Looks very good overall, some minor comments.
Reviewable status: 0 of 26 files reviewed, 3 unresolved discussions (waiting on @jbowens)
vfs/default_linux.go line 64 at r2 (raw file):
func (f *linuxFile) SyncTo(offset int64) (fullSync bool, err error) { if !f.useSyncRange { // Use fdatasync, which does provide persistence guarantees but won't
[nit] Maybe this belongs inside SyncData and this can call f.SyncData()
vfs/syncing_file.go line 22 at r2 (raw file):
File // fd can be InvalidFd if the underlying File does not support it. fd uintptr
Do we still need a fd here?
vfs/syncing_file.go line 151 at r2 (raw file):
} if fullSync { f.ratchetSyncOffset(offset)
We go through some trouble to return the flag just for this, which seems like a minor optimization (for configurations we don't care that much about)
|
@jbowens is the plan to forward-port this to v22.2 and master when it's ready? |
|
@RaduBerinde yeah, this is actually already the backport of #2276. and the plan is to update #2271 to incorporate the bit of feedback that was applied to #2276 but not yet #2271. |
There was a problem hiding this comment.
Reviewable status: 0 of 26 files reviewed, 3 unresolved discussions (waiting on @RaduBerinde)
vfs/syncing_file.go line 22 at r2 (raw file):
Previously, RaduBerinde wrote…
Do we still need a
fdhere?
i suppose we could call file.Fd()
vfs/syncing_file.go line 151 at r2 (raw file):
Previously, RaduBerinde wrote…
We go through some trouble to return the flag just for this, which seems like a minor optimization (for configurations we don't care that much about)
it's not just configurations we don't care about, because sync_file_range isn't universally available on linux. we only even attempt to use it on ext{2,3,4} filesystems. XFS is also a recommended CockroachDB filesystem, and despite not being recommended we have customers that use ZFS.
There was a problem hiding this comment.
Ah, I didn't realize that merged to 22.2. Then please ignore any comments here, we don't want the backport to be different
Reviewable status: 0 of 26 files reviewed, 3 unresolved discussions (waiting on @jbowens)
vfs/syncing_file.go line 22 at r2 (raw file):
Previously, jbowens (Jackson Owens) wrote…
i suppose we could call
file.Fd()
I meant do we still need it at all? I guess we are still comparing it to InvalidFd so never mind.. (though in the future we might want to switch to some get-capabilities functionality in vfs.File, or at least rename the field here to supportsPreallocate bool)
|
TFTR! |
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 theaccidental 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.