ext: Migrate from using fileReader custom interface to using io.Reader.

It gets rid of holding state of the io.Reader offset (which is anyways held by
the vfs.FileDescriptor struct. It is also odd using a io.Reader becuase we
using io.ReaderAt to interact with the device. So making a io.ReaderAt wrapper
makes more sense.

Most importantly, it gets rid of the complexity of extracting the file reader
from a regular file implementation and then using it. Now we can just use the
regular file implementation as a reader which is more intuitive.

PiperOrigin-RevId: 260846927

pkg/sentry/fs/ext/block_map_file.go

@@ -53,27 +53,17 @@ type blockMapFile struct {
 	coverage [4]uint64
 }
 
-// Compiles only if blockMapFile implements fileReader.
-var _ fileReader = (*blockMapFile)(nil)
-
-// Read implements fileReader.getFileReader.
-func (f *blockMapFile) getFileReader(dev io.ReaderAt, blkSize uint64, offset uint64) io.Reader {
-	return &blockMapReader{
-		dev:     dev,
-		file:    f,
-		fileOff: offset,
-		blkSize: blkSize,
-	}
-}
+// Compiles only if blockMapFile implements io.ReaderAt.
+var _ io.ReaderAt = (*blockMapFile)(nil)
 
 // newBlockMapFile is the blockMapFile constructor. It initializes the file to
 // physical blocks map with (at most) the first 12 (direct) blocks.
-func newBlockMapFile(blkSize uint64, regFile regularFile) (*blockMapFile, error) {
+func newBlockMapFile(regFile regularFile) (*blockMapFile, error) {
 	file := &blockMapFile{regFile: regFile}
 	file.regFile.impl = file
 
 	for i := uint(0); i < 4; i++ {
-		file.coverage[i] = getCoverage(blkSize, i)
+		file.coverage[i] = getCoverage(regFile.inode.blkSize, i)
 	}
 
 	blkMap := regFile.inode.diskInode.Data()
@@ -84,40 +74,33 @@ func newBlockMapFile(blkSize uint64, regFile regularFile) (*blockMapFile, error)
 	return file, nil
 }
 
-// blockMapReader implements io.Reader which will fetch fill data from the
-// block maps and build the blockMapFile.fileToPhyBlks array if required.
-type blockMapReader struct {
-	dev     io.ReaderAt
-	file    *blockMapFile
-	fileOff uint64
-	blkSize uint64
-}
-
-// Compiles only if blockMapReader implements io.Reader.
-var _ io.Reader = (*blockMapReader)(nil)
-
-// Read implements io.Reader.Read.
-func (r *blockMapReader) Read(dst []byte) (int, error) {
+// ReadAt implements io.ReaderAt.ReadAt.
+func (f *blockMapFile) ReadAt(dst []byte, off int64) (int, error) {
 	if len(dst) == 0 {
 		return 0, nil
 	}
 
-	if r.fileOff >= r.file.regFile.inode.diskInode.Size() {
+	if off < 0 {
+		return 0, syserror.EINVAL
+	}
+
+	offset := uint64(off)
+	if offset >= f.regFile.inode.diskInode.Size() {
 		return 0, io.EOF
 	}
 
 	// dirBlksEnd is the file offset until which direct blocks cover file data.
 	// Direct blocks cover 0 <= file offset < dirBlksEnd.
-	dirBlksEnd := numDirectBlks * r.file.coverage[0]
+	dirBlksEnd := numDirectBlks * f.coverage[0]
 
 	// indirBlkEnd is the file offset until which the indirect block covers file
 	// data. The indirect block covers dirBlksEnd <= file offset < indirBlkEnd.
-	indirBlkEnd := dirBlksEnd + r.file.coverage[1]
+	indirBlkEnd := dirBlksEnd + f.coverage[1]
 
 	// doubIndirBlkEnd is the file offset until which the double indirect block
 	// covers file data. The double indirect block covers the range
 	// indirBlkEnd <= file offset < doubIndirBlkEnd.
-	doubIndirBlkEnd := indirBlkEnd + r.file.coverage[2]
+	doubIndirBlkEnd := indirBlkEnd + f.coverage[2]
 
 	read := 0
 	toRead := len(dst)
@@ -127,21 +110,22 @@ func (r *blockMapReader) Read(dst []byte) (int, error) {
 
 		// Figure out which block to delegate the read to.
 		switch {
-		case r.fileOff < dirBlksEnd:
+		case offset < dirBlksEnd:
 			// Direct block.
-			curR, err = r.read(r.file.directBlks[r.fileOff/r.blkSize], r.fileOff%r.blkSize, 0, dst[read:])
-		case r.fileOff < indirBlkEnd:
+			curR, err = f.read(f.directBlks[offset/f.regFile.inode.blkSize], offset%f.regFile.inode.blkSize, 0, dst[read:])
+		case offset < indirBlkEnd:
 			// Indirect block.
-			curR, err = r.read(r.file.indirectBlk, r.fileOff-dirBlksEnd, 1, dst[read:])
-		case r.fileOff < doubIndirBlkEnd:
+			curR, err = f.read(f.indirectBlk, offset-dirBlksEnd, 1, dst[read:])
+		case offset < doubIndirBlkEnd:
 			// Doubly indirect block.
-			curR, err = r.read(r.file.doubleIndirectBlk, r.fileOff-indirBlkEnd, 2, dst[read:])
+			curR, err = f.read(f.doubleIndirectBlk, offset-indirBlkEnd, 2, dst[read:])
 		default:
 			// Triply indirect block.
-			curR, err = r.read(r.file.tripleIndirectBlk, r.fileOff-doubIndirBlkEnd, 3, dst[read:])
+			curR, err = f.read(f.tripleIndirectBlk, offset-doubIndirBlkEnd, 3, dst[read:])
 		}
 
 		read += curR
+		offset += uint64(curR)
 		if err != nil {
 			return read, err
 		}
@@ -150,30 +134,29 @@ func (r *blockMapReader) Read(dst []byte) (int, error) {
 	return read, nil
 }
 
-// read is the recursive step of the Read function. It relies on knowing the
+// read is the recursive step of the ReadAt function. It relies on knowing the
 // current node's location on disk (curPhyBlk) and its height in the block map
 // tree. A height of 0 shows that the current node is actually holding file
 // data. relFileOff tells the offset from which we need to start to reading
 // under the current node. It is completely relative to the current node.
-func (r *blockMapReader) read(curPhyBlk uint32, relFileOff uint64, height uint, dst []byte) (int, error) {
-	curPhyBlkOff := int64(curPhyBlk) * int64(r.blkSize)
+func (f *blockMapFile) read(curPhyBlk uint32, relFileOff uint64, height uint, dst []byte) (int, error) {
+	curPhyBlkOff := int64(curPhyBlk) * int64(f.regFile.inode.blkSize)
 	if height == 0 {
-		toRead := int(r.blkSize - relFileOff)
+		toRead := int(f.regFile.inode.blkSize - relFileOff)
 		if len(dst) < toRead {
 			toRead = len(dst)
 		}
 
-		n, _ := r.dev.ReadAt(dst[:toRead], curPhyBlkOff+int64(relFileOff))
-		r.fileOff += uint64(n)
+		n, _ := f.regFile.inode.dev.ReadAt(dst[:toRead], curPhyBlkOff+int64(relFileOff))
 		if n < toRead {
 			return n, syserror.EIO
 		}
 		return n, nil
 	}
 
-	childCov := r.file.coverage[height-1]
+	childCov := f.coverage[height-1]
 	startIdx := relFileOff / childCov
-	endIdx := r.blkSize / 4 // This is exclusive.
+	endIdx := f.regFile.inode.blkSize / 4 // This is exclusive.
 	wantEndIdx := (relFileOff + uint64(len(dst))) / childCov
 	wantEndIdx++ // Make this exclusive.
 	if wantEndIdx < endIdx {
@@ -184,11 +167,12 @@ func (r *blockMapReader) read(curPhyBlk uint32, relFileOff uint64, height uint,
 	curChildOff := relFileOff % childCov
 	for i := startIdx; i < endIdx; i++ {
 		var childPhyBlk uint32
-		if err := readFromDisk(r.dev, curPhyBlkOff+int64(i*4), &childPhyBlk); err != nil {
+		err := readFromDisk(f.regFile.inode.dev, curPhyBlkOff+int64(i*4), &childPhyBlk)
+		if err != nil {
 			return read, err
 		}
 
-		n, err := r.read(childPhyBlk, curChildOff, height-1, dst[read:])
+		n, err := f.read(childPhyBlk, curChildOff, height-1, dst[read:])
 		read += n
 		if err != nil {
 			return read, err

pkg/sentry/fs/ext/block_map_test.go

@@ -16,7 +16,6 @@ package ext
 
 import (
 	"bytes"
-	"io"
 	"math/rand"
 	"testing"
 
@@ -34,14 +33,13 @@ const (
 // TestBlockMapReader stress tests block map reader functionality. It performs
 // random length reads from all possible positions in the block map structure.
 func TestBlockMapReader(t *testing.T) {
-	dev, mockBMFile, want := blockMapSetUp(t)
+	mockBMFile, want := blockMapSetUp(t)
 	n := len(want)
 
 	for from := 0; from < n; from++ {
-		fileReader := mockBMFile.getFileReader(dev, uint64(mockBMBlkSize), uint64(from))
 		got := make([]byte, n-from)
 
-		if read, err := io.ReadFull(fileReader, got); err != nil {
+		if read, err := mockBMFile.ReadAt(got, int64(from)); err != nil {
 			t.Fatalf("file read operation from offset %d to %d only read %d bytes: %v", from, n, read, err)
 		}
 
@@ -85,7 +83,7 @@ func (n *blkNumGen) next() uint32 {
 // block and 1 triple indirect block (basically fill it till the rim). It
 // initializes the disk to reflect the inode. Also returns the file data that
 // the inode covers and that is written to disk.
-func blockMapSetUp(t *testing.T) (io.ReaderAt, *blockMapFile, []byte) {
+func blockMapSetUp(t *testing.T) (*blockMapFile, []byte) {
 	mockDisk := make([]byte, mockBMDiskSize)
 	regFile := regularFile{
 		inode: inode{
@@ -94,6 +92,8 @@ func blockMapSetUp(t *testing.T) (io.ReaderAt, *blockMapFile, []byte) {
 					SizeLo: getMockBMFileFize(),
 				},
 			},
+			dev:     bytes.NewReader(mockDisk),
+			blkSize: uint64(mockBMBlkSize),
 		},
 	}
 
@@ -125,11 +125,11 @@ func blockMapSetUp(t *testing.T) (io.ReaderAt, *blockMapFile, []byte) {
 
 	copy(regFile.inode.diskInode.Data(), data)
 
-	mockFile, err := newBlockMapFile(uint64(mockBMBlkSize), regFile)
+	mockFile, err := newBlockMapFile(regFile)
 	if err != nil {
 		t.Fatalf("newBlockMapFile failed: %v", err)
 	}
-	return bytes.NewReader(mockDisk), mockFile, fileData
+	return mockFile, fileData
 }
 
 // writeFileDataToBlock writes random bytes to the block on disk.

pkg/sentry/fs/ext/extent_file.go

@@ -32,26 +32,16 @@ type extentFile struct {
 	root disklayout.ExtentNode
 }
 
-// Compiles only if extentFile implements fileReader.
-var _ fileReader = (*extentFile)(nil)
-
-// Read implements fileReader.getFileReader.
-func (f *extentFile) getFileReader(dev io.ReaderAt, blkSize uint64, offset uint64) io.Reader {
-	return &extentReader{
-		dev:     dev,
-		file:    f,
-		fileOff: offset,
-		blkSize: blkSize,
-	}
-}
+// Compiles only if extentFile implements io.ReaderAt.
+var _ io.ReaderAt = (*extentFile)(nil)
 
 // newExtentFile is the extent file constructor. It reads the entire extent
 // tree into memory.
 // TODO(b/134676337): Build extent tree on demand to reduce memory usage.
-func newExtentFile(dev io.ReaderAt, blkSize uint64, regFile regularFile) (*extentFile, error) {
+func newExtentFile(regFile regularFile) (*extentFile, error) {
 	file := &extentFile{regFile: regFile}
 	file.regFile.impl = file
-	err := file.buildExtTree(dev, blkSize)
+	err := file.buildExtTree()
 	if err != nil {
 		return nil, err
 	}
@@ -64,7 +54,7 @@ func newExtentFile(dev io.ReaderAt, blkSize uint64, regFile regularFile) (*exten
 // disk.
 //
 // Precondition: inode flag InExtents must be set.
-func (f *extentFile) buildExtTree(dev io.ReaderAt, blkSize uint64) error {
+func (f *extentFile) buildExtTree() error {
 	rootNodeData := f.regFile.inode.diskInode.Data()
 
 	binary.Unmarshal(rootNodeData[:disklayout.ExtentStructsSize], binary.LittleEndian, &f.root.Header)
@@ -94,7 +84,7 @@ func (f *extentFile) buildExtTree(dev io.ReaderAt, blkSize uint64) error {
 	if f.root.Header.Height > 0 {
 		for i := uint16(0); i < f.root.Header.NumEntries; i++ {
 			var err error
-			if f.root.Entries[i].Node, err = buildExtTreeFromDisk(dev, f.root.Entries[i].Entry, blkSize); err != nil {
+			if f.root.Entries[i].Node, err = f.buildExtTreeFromDisk(f.root.Entries[i].Entry); err != nil {
 				return err
 			}
 		}
@@ -106,10 +96,10 @@ func (f *extentFile) buildExtTree(dev io.ReaderAt, blkSize uint64) error {
 // buildExtTreeFromDisk reads the extent tree nodes from disk and recursively
 // builds the tree. Performs a simple DFS. It returns the ExtentNode pointed to
 // by the ExtentEntry.
-func buildExtTreeFromDisk(dev io.ReaderAt, entry disklayout.ExtentEntry, blkSize uint64) (*disklayout.ExtentNode, error) {
+func (f *extentFile) buildExtTreeFromDisk(entry disklayout.ExtentEntry) (*disklayout.ExtentNode, error) {
 	var header disklayout.ExtentHeader
-	off := entry.PhysicalBlock() * blkSize
-	err := readFromDisk(dev, int64(off), &header)
+	off := entry.PhysicalBlock() * f.regFile.inode.blkSize
+	err := readFromDisk(f.regFile.inode.dev, int64(off), &header)
 	if err != nil {
 		return nil, err
 	}
@@ -125,7 +115,7 @@ func buildExtTreeFromDisk(dev io.ReaderAt, entry disklayout.ExtentEntry, blkSize
 			curEntry = &disklayout.ExtentIdx{}
 		}
 
-		err := readFromDisk(dev, int64(off), curEntry)
+		err := readFromDisk(f.regFile.inode.dev, int64(off), curEntry)
 		if err != nil {
 			return nil, err
 		}
@@ -136,7 +126,7 @@ func buildExtTreeFromDisk(dev io.ReaderAt, entry disklayout.ExtentEntry, blkSize
 	if header.Height > 0 {
 		for i := uint16(0); i < header.NumEntries; i++ {
 			var err error
-			entries[i].Node, err = buildExtTreeFromDisk(dev, entries[i].Entry, blkSize)
+			entries[i].Node, err = f.buildExtTreeFromDisk(entries[i].Entry)
 			if err != nil {
 				return nil, err
 			}
@@ -146,38 +136,31 @@ func buildExtTreeFromDisk(dev io.ReaderAt, entry disklayout.ExtentEntry, blkSize
 	return &disklayout.ExtentNode{header, entries}, nil
 }
 
-// extentReader implements io.Reader which can traverse the extent tree and
-// read file data. This is not thread safe.
-type extentReader struct {
-	dev     io.ReaderAt
-	file    *extentFile
-	fileOff uint64 // Represents the current file offset being read from.
-	blkSize uint64
-}
-
-// Compiles only if inlineReader implements io.Reader.
-var _ io.Reader = (*extentReader)(nil)
-
-// Read implements io.Reader.Read.
-func (r *extentReader) Read(dst []byte) (int, error) {
+// ReadAt implements io.ReaderAt.ReadAt.
+func (f *extentFile) ReadAt(dst []byte, off int64) (int, error) {
 	if len(dst) == 0 {
 		return 0, nil
 	}
 
-	if r.fileOff >= r.file.regFile.inode.diskInode.Size() {
+	if off < 0 {
+		return 0, syserror.EINVAL
+	}
+
+	if uint64(off) >= f.regFile.inode.diskInode.Size() {
 		return 0, io.EOF
 	}
 
-	return r.read(&r.file.root, dst)
+	return f.read(&f.root, uint64(off), dst)
 }
 
-// read is a helper which traverses the extent tree and reads data.
-func (r *extentReader) read(node *disklayout.ExtentNode, dst []byte) (int, error) {
+// read is the recursive step of extentFile.ReadAt which traverses the extent
+// tree from the node passed and reads file data.
+func (f *extentFile) read(node *disklayout.ExtentNode, off uint64, dst []byte) (int, error) {
 	// Perform a binary search for the node covering bytes starting at r.fileOff.
 	// A highly fragmented filesystem can have upto 340 entries and so linear
 	// search should be avoided. Finds the first entry which does not cover the
 	// file block we want and subtracts 1 to get the desired index.
-	fileBlk := r.fileBlock()
+	fileBlk := uint32(off / f.regFile.inode.blkSize)
 	n := len(node.Entries)
 	found := sort.Search(n, func(i int) bool {
 		return node.Entries[i].Entry.FileBlock() > fileBlk
@@ -195,12 +178,13 @@ func (r *extentReader) read(node *disklayout.ExtentNode, dst []byte) (int, error
 	var err error
 	for i := found; i < n && read < toRead; i++ {
 		if node.Header.Height == 0 {
-			curR, err = r.readFromExtent(node.Entries[i].Entry.(*disklayout.Extent), dst[read:])
+			curR, err = f.readFromExtent(node.Entries[i].Entry.(*disklayout.Extent), off, dst[read:])
 		} else {
-			curR, err = r.read(node.Entries[i].Node, dst[read:])
+			curR, err = f.read(node.Entries[i].Node, off, dst[read:])
 		}
 
 		read += curR
+		off += uint64(curR)
 		if err != nil {
 			return read, err
 		}
@@ -211,7 +195,7 @@ func (r *extentReader) read(node *disklayout.ExtentNode, dst []byte) (int, error
 
 // readFromExtent reads file data from the extent. It takes advantage of the
 // sequential nature of extents and reads file data from multiple blocks in one
-// call. Also updates the file offset.
+// call.
 //
 // A non-nil error indicates that this is a partial read and there is probably
 // more to read from this extent. The caller should propagate the error upward
@@ -219,8 +203,8 @@ func (r *extentReader) read(node *disklayout.ExtentNode, dst []byte) (int, error
 //
 // A subsequent call to extentReader.Read should continue reading from where we
 // left off as expected.
-func (r *extentReader) readFromExtent(ex *disklayout.Extent, dst []byte) (int, error) {
-	curFileBlk := r.fileBlock()
+func (f *extentFile) readFromExtent(ex *disklayout.Extent, off uint64, dst []byte) (int, error) {
+	curFileBlk := uint32(off / f.regFile.inode.blkSize)
 	exFirstFileBlk := ex.FileBlock()
 	exLastFileBlk := exFirstFileBlk + uint32(ex.Length) // This is exclusive.
 
@@ -231,30 +215,19 @@ func (r *extentReader) readFromExtent(ex *disklayout.Extent, dst []byte) (int, e
 	}
 
 	curPhyBlk := uint64(curFileBlk-exFirstFileBlk) + ex.PhysicalBlock()
-	readStart := curPhyBlk*r.blkSize + r.fileBlockOff()
+	readStart := curPhyBlk*f.regFile.inode.blkSize + (off % f.regFile.inode.blkSize)
 
 	endPhyBlk := ex.PhysicalBlock() + uint64(ex.Length)
-	extentEnd := endPhyBlk * r.blkSize // This is exclusive.
+	extentEnd := endPhyBlk * f.regFile.inode.blkSize // This is exclusive.
 
 	toRead := int(extentEnd - readStart)
 	if len(dst) < toRead {
 		toRead = len(dst)
 	}
 
-	n, _ := r.dev.ReadAt(dst[:toRead], int64(readStart))
-	r.fileOff += uint64(n)
+	n, _ := f.regFile.inode.dev.ReadAt(dst[:toRead], int64(readStart))
 	if n < toRead {
 		return n, syserror.EIO
 	}
 	return n, nil
 }
-
-// fileBlock returns the file block number we are currently reading.
-func (r *extentReader) fileBlock() uint32 {
-	return uint32(r.fileOff / r.blkSize)
-}
-
-// fileBlockOff returns the current offset within the current file block.
-func (r *extentReader) fileBlockOff() uint64 {
-	return r.fileOff % r.blkSize
-}

pkg/sentry/fs/ext/extent_test.go

@@ -16,7 +16,6 @@ package ext
 
 import (
 	"bytes"
-	"io"
 	"math/rand"
 	"testing"
 
@@ -145,14 +144,13 @@ var (
 // TestExtentReader stress tests extentReader functionality. It performs random
 // length reads from all possible positions in the extent tree.
 func TestExtentReader(t *testing.T) {
-	dev, mockExtentFile, want := extentTreeSetUp(t, node0)
+	mockExtentFile, want := extentTreeSetUp(t, node0)
 	n := len(want)
 
 	for from := 0; from < n; from++ {
-		fileReader := mockExtentFile.getFileReader(dev, mockExtentBlkSize, uint64(from))
 		got := make([]byte, n-from)
 
-		if read, err := io.ReadFull(fileReader, got); err != nil {
+		if read, err := mockExtentFile.ReadAt(got, int64(from)); err != nil {
 			t.Fatalf("file read operation from offset %d to %d only read %d bytes: %v", from, n, read, err)
 		}
 
@@ -164,7 +162,7 @@ func TestExtentReader(t *testing.T) {
 
 // TestBuildExtentTree tests the extent tree building logic.
 func TestBuildExtentTree(t *testing.T) {
-	_, mockExtentFile, _ := extentTreeSetUp(t, node0)
+	mockExtentFile, _ := extentTreeSetUp(t, node0)
 
 	opt := cmpopts.IgnoreUnexported(disklayout.ExtentIdx{}, disklayout.ExtentHeader{})
 	if diff := cmp.Diff(&mockExtentFile.root, node0, opt); diff != "" {
@@ -175,7 +173,7 @@ func TestBuildExtentTree(t *testing.T) {
 // extentTreeSetUp writes the passed extent tree to a mock disk as an extent
 // tree. It also constucts a mock extent file with the same tree built in it.
 // It also writes random data file data and returns it.
-func extentTreeSetUp(t *testing.T, root *disklayout.ExtentNode) (io.ReaderAt, *extentFile, []byte) {
+func extentTreeSetUp(t *testing.T, root *disklayout.ExtentNode) (*extentFile, []byte) {
 	t.Helper()
 
 	mockDisk := make([]byte, mockExtentBlkSize*10)
@@ -187,17 +185,18 @@ func extentTreeSetUp(t *testing.T, root *disklayout.ExtentNode) (io.ReaderAt, *e
 						SizeLo: uint32(mockExtentBlkSize) * getNumPhyBlks(root),
 					},
 				},
+				blkSize: mockExtentBlkSize,
+				dev:     bytes.NewReader(mockDisk),
 			},
 		},
 	}
 
 	fileData := writeTree(&mockExtentFile.regFile.inode, mockDisk, node0, mockExtentBlkSize)
 
-	r := bytes.NewReader(mockDisk)
-	if err := mockExtentFile.buildExtTree(r, mockExtentBlkSize); err != nil {
+	if err := mockExtentFile.buildExtTree(); err != nil {
 		t.Fatalf("inode.buildExtTree failed: %v", err)
 	}
-	return r, mockExtentFile, fileData
+	return mockExtentFile, fileData
 }
 
 // writeTree writes the tree represented by `root` to the inode and disk. It

pkg/sentry/fs/ext/filesystem.go

@@ -34,12 +34,11 @@ type filesystem struct {
 	// mu serializes changes to the Dentry tree.
 	mu sync.RWMutex
 
-	// dev is the io.ReaderAt for the underlying fs device. It does not require
-	// protection because io.ReaderAt permits concurrent read calls to it. It
-	// translates to the pread syscall which passes on the read request directly
-	// to the device driver. Device drivers are intelligent in serving multiple
-	// concurrent read requests in the optimal order (taking locality into
-	// consideration).
+	// dev represents the underlying fs device. It does not require protection
+	// because io.ReaderAt permits concurrent read calls to it. It translates to
+	// the pread syscall which passes on the read request directly to the device
+	// driver. Device drivers are intelligent in serving multiple concurrent read
+	// requests in the optimal order (taking locality into consideration).
 	dev io.ReaderAt
 
 	// inodeCache maps absolute inode numbers to the corresponding Inode struct.
@@ -69,7 +68,7 @@ func (fs *filesystem) getOrCreateInode(ctx context.Context, inodeNum uint32) (*i
 		return in, nil
 	}
 
-	in, err := newInode(ctx, fs.dev, fs.sb, fs.bgs, inodeNum)
+	in, err := newInode(ctx, fs, inodeNum)
 	if err != nil {
 		return nil, err
 	}

pkg/sentry/fs/ext/inline_file.go

@@ -24,14 +24,8 @@ type inlineFile struct {
 	regFile regularFile
 }
 
-// Compiles only if inlineFile implements fileReader.
-var _ fileReader = (*inlineFile)(nil)
-
-// getFileReader implements fileReader.getFileReader.
-func (f *inlineFile) getFileReader(_ io.ReaderAt, _ uint64, offset uint64) io.Reader {
-	diskInode := f.regFile.inode.diskInode
-	return &inlineReader{offset: offset, data: diskInode.Data()[:diskInode.Size()]}
-}
+// Compiles only if inlineFile implements io.ReaderAt.
+var _ io.ReaderAt = (*inlineFile)(nil)
 
 // newInlineFile is the inlineFile constructor.
 func newInlineFile(regFile regularFile) *inlineFile {
@@ -40,27 +34,22 @@ func newInlineFile(regFile regularFile) *inlineFile {
 	return file
 }
 
-// inlineReader implements io.Reader which can read the underlying data. This
-// is not thread safe.
-type inlineReader struct {
-	offset uint64
-	data   []byte
-}
-
-// Compiles only if inlineReader implements io.Reader.
-var _ io.Reader = (*inlineReader)(nil)
-
-// Read implements io.Reader.Read.
-func (r *inlineReader) Read(dst []byte) (int, error) {
+// ReadAt implements io.ReaderAt.ReadAt.
+func (f *inlineFile) ReadAt(dst []byte, off int64) (int, error) {
 	if len(dst) == 0 {
 		return 0, nil
 	}
 
-	if int(r.offset) >= len(r.data) {
+	size := f.regFile.inode.diskInode.Size()
+	if uint64(off) >= size {
 		return 0, io.EOF
 	}
 
-	n := copy(dst, r.data[r.offset:])
-	r.offset += uint64(n)
+	to := uint64(off) + uint64(len(dst))
+	if to > size {
+		to = size
+	}
+
+	n := copy(dst, f.regFile.inode.diskInode.Data()[off:to])
 	return n, nil
 }

pkg/sentry/fs/ext/inode.go

@@ -46,6 +46,12 @@ type inode struct {
 	// identify inodes within the ext filesystem.
 	inodeNum uint32
 
+	// dev represents the underlying device. Same as filesystem.dev.
+	dev io.ReaderAt
+
+	// blkSize is the fs data block size. Same as filesystem.sb.BlockSize().
+	blkSize uint64
+
 	// diskInode gives us access to the inode struct on disk. Immutable.
 	diskInode disklayout.Inode
 
@@ -86,12 +92,12 @@ func (in *inode) decRef(fs *filesystem) {
 
 // newInode is the inode constructor. Reads the inode off disk. Identifies
 // inodes based on the absolute inode number on disk.
-func newInode(ctx context.Context, dev io.ReaderAt, sb disklayout.SuperBlock, bgs []disklayout.BlockGroup, inodeNum uint32) (*inode, error) {
+func newInode(ctx context.Context, fs *filesystem, inodeNum uint32) (*inode, error) {
 	if inodeNum == 0 {
 		panic("inode number 0 on ext filesystems is not possible")
 	}
 
-	inodeRecordSize := sb.InodeSize()
+	inodeRecordSize := fs.sb.InodeSize()
 	var diskInode disklayout.Inode
 	if inodeRecordSize == disklayout.OldInodeSize {
 		diskInode = &disklayout.InodeOld{}
@@ -100,12 +106,12 @@ func newInode(ctx context.Context, dev io.ReaderAt, sb disklayout.SuperBlock, bg
 	}
 
 	// Calculate where the inode is actually placed.
-	inodesPerGrp := sb.InodesPerGroup()
-	blkSize := sb.BlockSize()
-	inodeTableOff := bgs[getBGNum(inodeNum, inodesPerGrp)].InodeTable() * blkSize
+	inodesPerGrp := fs.sb.InodesPerGroup()
+	blkSize := fs.sb.BlockSize()
+	inodeTableOff := fs.bgs[getBGNum(inodeNum, inodesPerGrp)].InodeTable() * blkSize
 	inodeOff := inodeTableOff + uint64(uint32(inodeRecordSize)*getBGOff(inodeNum, inodesPerGrp))
 
-	if err := readFromDisk(dev, int64(inodeOff), diskInode); err != nil {
+	if err := readFromDisk(fs.dev, int64(inodeOff), diskInode); err != nil {
 		return nil, err
 	}
 
@@ -113,18 +119,20 @@ func newInode(ctx context.Context, dev io.ReaderAt, sb disklayout.SuperBlock, bg
 	inode := inode{
 		refs:      1,
 		inodeNum:  inodeNum,
+		dev:       fs.dev,
+		blkSize:   blkSize,
 		diskInode: diskInode,
 	}
 
 	switch diskInode.Mode().FileType() {
 	case linux.ModeSymlink:
-		f, err := newSymlink(dev, blkSize, inode)
+		f, err := newSymlink(inode)
 		if err != nil {
 			return nil, err
 		}
 		return &f.inode, nil
 	case linux.ModeRegular:
-		f, err := newRegularFile(dev, blkSize, inode)
+		f, err := newRegularFile(inode)
 		if err != nil {
 			return nil, err
 		}

pkg/sentry/fs/ext/regular_file.go

@@ -18,20 +18,6 @@ import (
 	"io"
 )
 
-// fileReader is used to abstact away the complexity of how the file data is
-// stored under the hood. Provides a method to get a file reader which can be
-// used to read file data without worrying about how it is organized on disk.
-type fileReader interface {
-
-	// getFileReader returns a Reader implementation which can be used to read a
-	// file. It abstracts away the complexity of how the file is actually
-	// organized on disk. The reader is initialized with the passed offset.
-	//
-	// This reader is not meant to be retained across Read operations as it needs
-	// to be reinitialized with the correct offset for every Read.
-	getFileReader(dev io.ReaderAt, blkSize uint64, offset uint64) io.Reader
-}
-
 // regularFile represents a regular file's inode. This too follows the
 // inheritance pattern prevelant in the vfs layer described in
 // pkg/sentry/vfs/README.md.
@@ -40,12 +26,12 @@ type regularFile struct {
 
 	// This is immutable. The first field of fileReader implementations must be
 	// regularFile to ensure temporality.
-	impl fileReader
+	impl io.ReaderAt
 }
 
 // newRegularFile is the regularFile constructor. It figures out what kind of
 // file this is and initializes the fileReader.
-func newRegularFile(dev io.ReaderAt, blkSize uint64, inode inode) (*regularFile, error) {
+func newRegularFile(inode inode) (*regularFile, error) {
 	regFile := regularFile{
 		inode: inode,
 	}
@@ -53,7 +39,7 @@ func newRegularFile(dev io.ReaderAt, blkSize uint64, inode inode) (*regularFile,
 	inodeFlags := inode.diskInode.Flags()
 
 	if inodeFlags.Extents {
-		file, err := newExtentFile(dev, blkSize, regFile)
+		file, err := newExtentFile(regFile)
 		if err != nil {
 			return nil, err
 		}
@@ -72,7 +58,7 @@ func newRegularFile(dev io.ReaderAt, blkSize uint64, inode inode) (*regularFile,
 		return &file.regFile, nil
 	}
 
-	file, err := newBlockMapFile(blkSize, regFile)
+	file, err := newBlockMapFile(regFile)
 	if err != nil {
 		return nil, err
 	}

pkg/sentry/fs/ext/symlink.go

@@ -15,8 +15,6 @@
 package ext
 
 import (
-	"io"
-
 	"gvisor.dev/gvisor/pkg/syserror"
 )
 
@@ -28,7 +26,7 @@ type symlink struct {
 
 // newSymlink is the symlink constructor. It reads out the symlink target from
 // the inode (however it might have been stored).
-func newSymlink(dev io.ReaderAt, blkSize uint64, inode inode) (*symlink, error) {
+func newSymlink(inode inode) (*symlink, error) {
 	var file *symlink
 	var link []byte
 
@@ -39,14 +37,13 @@ func newSymlink(dev io.ReaderAt, blkSize uint64, inode inode) (*symlink, error)
 		link = inode.diskInode.Data()[:size]
 	} else {
 		// Create a regular file out of this inode and read out the target.
-		regFile, err := newRegularFile(dev, blkSize, inode)
+		regFile, err := newRegularFile(inode)
 		if err != nil {
 			return nil, err
 		}
 
 		link = make([]byte, size)
-		reader := regFile.impl.getFileReader(dev, blkSize, 0)
-		if _, err := io.ReadFull(reader, link); err != nil {
+		if n, _ := regFile.impl.ReadAt(link, 0); uint64(n) < size {
 			return nil, syserror.EIO
 		}
 	}