vfs: clean up Fd functionality

[RaduBerinde]

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 Files 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.

[cockroach-teamcity]

This change is 

[sumeerbhola]

I've always found this confusing about our code, but I can't remember whether there was a deeper reason we went with this approach (Peter or Bilal may remember).

Reviewed all commit messages.
Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @itsbilal and @RaduBerinde)

---

vfs/syncing_file.go line 22 at r1 (raw file):

type syncingFile struct {
	File
	// fd can be 0 if the underlying File does not support it.

0 is a valid file descriptor (stdin).
Should we do the same as the os.File implementation:

	if f == nil {
		return ^(uintptr(0))
	}

[RaduBerinde]

Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @itsbilal and @sumeerbhola)

---

vfs/syncing_file.go line 22 at r1 (raw file):

Previously, sumeerbhola wrote…

0 is a valid file descriptor (stdin).
Should we do the same as the os.File implementation:

	if f == nil {
		return ^(uintptr(0))
	}

Nice catch, switched to that.

[itsbilal]

Thanks for doing this!

I've always found this confusing about our code, but I can't remember whether there was a deeper reason we went with this approach (Peter or Bilal may remember).

I don't believe there's a deeper reason than just inertia. We just wanted to make *os.Files as substitutable as vfs.File as possible, and that's still maintained with this.

You'll also need to make similar changes in Cockroach (eg. with encryptedFile) when we do this.

Reviewed 5 of 11 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RaduBerinde and @sumeerbhola)

---

vfs/fd_test.go line 29 at r2 (raw file):

	f2, err := fs2.Open(filename)
	require.NoError(t, err)
	require.NotZero(t, f2.Fd())

nit: probably also a NotEqual(t, f2.Fd(), InvalidFd) ?

[RaduBerinde]

You'll also need to make similar changes in Cockroach (eg. with encryptedFile) when we do this.

Yeah I plan to bump Pebble and take care of that after I merge this.

Reviewable status: 11 of 12 files reviewed, 2 unresolved discussions (waiting on @itsbilal and @sumeerbhola)

---

vfs/fd_test.go line 29 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

nit: probably also a NotEqual(t, f2.Fd(), InvalidFd) ?

Nice catch, thanks! Done.