CachingInodeOperations was over-dirtying cached attributes

Dirty should be set only when the attribute is changed in the cache
only. Instances where the change was also sent to the backing file
doesn't need to dirty the attribute.

Also remove size update during WriteOut as writing dirty page would
naturaly grow the file if needed.

RELNOTES: relnotes is needed for the parent CL.
PiperOrigin-RevId: 232068978
Change-Id: I00ba54693a2c7adc06efa9e030faf8f2e8e7f188

3 files changed, 48 insertions, 71 deletions

diff --git a/pkg/sentry/fs/fsutil/inode_cached.go b/pkg/sentry/fs/fsutil/inode_cached.go
index 707ca76d2..5e7e861d2 100644
--- a/pkg/sentry/fs/fsutil/inode_cached.go
+++ b/pkg/sentry/fs/fsutil/inode_cached.go
@@ -22,6 +22,7 @@ import (
 	"gvisor.googlesource.com/gvisor/pkg/log"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/context"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/fs"
+	"gvisor.googlesource.com/gvisor/pkg/sentry/kernel/time"
 	ktime "gvisor.googlesource.com/gvisor/pkg/sentry/kernel/time"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/memmap"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/platform"
@@ -141,10 +142,6 @@ type CachedFileObject interface {
 	//
 	// FD is called iff the file has been memory mapped. This implies that
 	// the file was opened (see fs.InodeOperations.GetFile).
-	//
-	// FIXME: This interface seems to be
-	// fundamentally broken.  We should clarify CachingInodeOperation's
-	// behavior with metadata.
 	FD() int
 }
 
@@ -190,16 +187,14 @@ func (c *CachingInodeOperations) SetPermissions(ctx context.Context, inode *fs.I
 	c.attrMu.Lock()
 	defer c.attrMu.Unlock()
 
+	now := ktime.NowFromContext(ctx)
 	masked := fs.AttrMask{Perms: true}
 	if err := c.backingFile.SetMaskedAttributes(ctx, masked, fs.UnstableAttr{Perms: perms}); err != nil {
 		return false
 	}
 	c.attr.Perms = perms
-	// FIXME: Clarify CachingInodeOperations behavior with metadata.
-	c.dirtyAttr.Perms = true
-	c.touchStatusChangeTimeLocked(ctx)
+	c.touchStatusChangeTimeLocked(now)
 	return true
-
 }
 
 // SetOwner implements fs.InodeOperations.SetOwner.
@@ -211,6 +206,7 @@ func (c *CachingInodeOperations) SetOwner(ctx context.Context, inode *fs.Inode,
 	c.attrMu.Lock()
 	defer c.attrMu.Unlock()
 
+	now := ktime.NowFromContext(ctx)
 	masked := fs.AttrMask{
 		UID: owner.UID.Ok(),
 		GID: owner.GID.Ok(),
@@ -220,15 +216,11 @@ func (c *CachingInodeOperations) SetOwner(ctx context.Context, inode *fs.Inode,
 	}
 	if owner.UID.Ok() {
 		c.attr.Owner.UID = owner.UID
-		// FIXME: Clarify CachingInodeOperations behavior with metadata.
-		c.dirtyAttr.UID = true
 	}
 	if owner.GID.Ok() {
 		c.attr.Owner.GID = owner.GID
-		// FIXME: Clarify CachingInodeOperations behavior with metadata.
-		c.dirtyAttr.GID = true
 	}
-	c.touchStatusChangeTimeLocked(ctx)
+	c.touchStatusChangeTimeLocked(now)
 	return nil
 }
 
@@ -260,15 +252,11 @@ func (c *CachingInodeOperations) SetTimestamps(ctx context.Context, inode *fs.In
 	}
 	if !ts.ATimeOmit {
 		c.attr.AccessTime = ts.ATime
-		// FIXME: Clarify CachingInodeOperations behavior with metadata.
-		c.dirtyAttr.AccessTime = true
 	}
 	if !ts.MTimeOmit {
 		c.attr.ModificationTime = ts.MTime
-		// FIXME: Clarify CachingInodeOperations behavior with metadata.
-		c.dirtyAttr.ModificationTime = true
 	}
-	c.touchStatusChangeTimeLocked(ctx)
+	c.touchStatusChangeTimeLocked(now)
 	return nil
 }
 
@@ -279,21 +267,17 @@ func (c *CachingInodeOperations) Truncate(ctx context.Context, inode *fs.Inode,
 
 	// c.attr.Size is protected by both c.attrMu and c.dataMu.
 	c.dataMu.Lock()
-	if err := c.backingFile.SetMaskedAttributes(ctx, fs.AttrMask{
-		Size: true,
-	}, fs.UnstableAttr{
-		Size: size,
-	}); err != nil {
+	now := ktime.NowFromContext(ctx)
+	masked := fs.AttrMask{Size: true}
+	attr := fs.UnstableAttr{Size: size}
+	if err := c.backingFile.SetMaskedAttributes(ctx, masked, attr); err != nil {
 		c.dataMu.Unlock()
 		return err
 	}
 	oldSize := c.attr.Size
-	if oldSize != size {
-		c.attr.Size = size
-		// FIXME: Clarify CachingInodeOperations behavior with metadata.
-		c.dirtyAttr.Size = true
-		c.touchModificationTimeLocked(ctx)
-	}
+	c.attr.Size = size
+	c.touchModificationTimeLocked(now)
+
 	// We drop c.dataMu here so that we can lock c.mapsMu and invalidate
 	// mappings below. This allows concurrent calls to Read/Translate/etc.
 	// These functions synchronize with an in-progress Truncate by refusing to
@@ -346,6 +330,10 @@ func (c *CachingInodeOperations) WriteOut(ctx context.Context, inode *fs.Inode)
 		return err
 	}
 
+	// SyncDirtyAll above would have grown if needed. On shrinks, the backing
+	// file is called directly, so size is never needs to be updated.
+	c.dirtyAttr.Size = false
+
 	// Write out cached attributes.
 	if err := c.backingFile.SetMaskedAttributes(ctx, c.dirtyAttr, c.attr); err != nil {
 		c.attrMu.Unlock()
@@ -363,7 +351,7 @@ func (c *CachingInodeOperations) WriteOut(ctx context.Context, inode *fs.Inode)
 func (c *CachingInodeOperations) IncLinks(ctx context.Context) {
 	c.attrMu.Lock()
 	c.attr.Links++
-	c.touchModificationTimeLocked(ctx)
+	c.touchModificationTimeLocked(ktime.NowFromContext(ctx))
 	c.attrMu.Unlock()
 }
 
@@ -371,7 +359,7 @@ func (c *CachingInodeOperations) IncLinks(ctx context.Context) {
 func (c *CachingInodeOperations) DecLinks(ctx context.Context) {
 	c.attrMu.Lock()
 	c.attr.Links--
-	c.touchModificationTimeLocked(ctx)
+	c.touchModificationTimeLocked(ktime.NowFromContext(ctx))
 	c.attrMu.Unlock()
 }
 
@@ -384,7 +372,7 @@ func (c *CachingInodeOperations) TouchAccessTime(ctx context.Context, inode *fs.
 	}
 
 	c.attrMu.Lock()
-	c.touchAccessTimeLocked(ctx)
+	c.touchAccessTimeLocked(ktime.NowFromContext(ctx))
 	c.attrMu.Unlock()
 }
 
@@ -392,8 +380,8 @@ func (c *CachingInodeOperations) TouchAccessTime(ctx context.Context, inode *fs.
 // time.
 //
 // Preconditions: c.attrMu is locked for writing.
-func (c *CachingInodeOperations) touchAccessTimeLocked(ctx context.Context) {
-	c.attr.AccessTime = ktime.NowFromContext(ctx)
+func (c *CachingInodeOperations) touchAccessTimeLocked(now time.Time) {
+	c.attr.AccessTime = now
 	c.dirtyAttr.AccessTime = true
 }
 
@@ -401,7 +389,7 @@ func (c *CachingInodeOperations) touchAccessTimeLocked(ctx context.Context) {
 // in-place to the current time.
 func (c *CachingInodeOperations) TouchModificationTime(ctx context.Context) {
 	c.attrMu.Lock()
-	c.touchModificationTimeLocked(ctx)
+	c.touchModificationTimeLocked(ktime.NowFromContext(ctx))
 	c.attrMu.Unlock()
 }
 
@@ -409,8 +397,7 @@ func (c *CachingInodeOperations) TouchModificationTime(ctx context.Context) {
 // change time in-place to the current time.
 //
 // Preconditions: c.attrMu is locked for writing.
-func (c *CachingInodeOperations) touchModificationTimeLocked(ctx context.Context) {
-	now := ktime.NowFromContext(ctx)
+func (c *CachingInodeOperations) touchModificationTimeLocked(now time.Time) {
 	c.attr.ModificationTime = now
 	c.dirtyAttr.ModificationTime = true
 	c.attr.StatusChangeTime = now
@@ -421,8 +408,7 @@ func (c *CachingInodeOperations) touchModificationTimeLocked(ctx context.Context
 // in-place to the current time.
 //
 // Preconditions: c.attrMu is locked for writing.
-func (c *CachingInodeOperations) touchStatusChangeTimeLocked(ctx context.Context) {
-	now := ktime.NowFromContext(ctx)
+func (c *CachingInodeOperations) touchStatusChangeTimeLocked(now time.Time) {
 	c.attr.StatusChangeTime = now
 	c.dirtyAttr.StatusChangeTime = true
 }
@@ -513,7 +499,7 @@ func (c *CachingInodeOperations) Write(ctx context.Context, src usermem.IOSequen
 
 	c.attrMu.Lock()
 	// Compare Linux's mm/filemap.c:__generic_file_write_iter() => file_update_time().
-	c.touchModificationTimeLocked(ctx)
+	c.touchModificationTimeLocked(ktime.NowFromContext(ctx))
 	n, err := src.CopyInTo(ctx, &inodeReadWriter{ctx, c, offset})
 	c.attrMu.Unlock()
 	return n, err
diff --git a/pkg/sentry/fs/fsutil/inode_cached_test.go b/pkg/sentry/fs/fsutil/inode_cached_test.go
index 9c9391511..2a8a1639c 100644
--- a/pkg/sentry/fs/fsutil/inode_cached_test.go
+++ b/pkg/sentry/fs/fsutil/inode_cached_test.go
@@ -17,7 +17,6 @@ package fsutil
 import (
 	"bytes"
 	"io"
-	"reflect"
 	"testing"
 
 	"gvisor.googlesource.com/gvisor/pkg/sentry/context"
@@ -66,9 +65,6 @@ func TestSetPermissions(t *testing.T) {
 	}
 
 	// Did permissions change?
-	if !iops.dirtyAttr.Perms {
-		t.Fatalf("got perms not dirty, want dirty")
-	}
 	if iops.attr.Perms != perms {
 		t.Fatalf("got perms +%v, want +%v", iops.attr.Perms, perms)
 	}
@@ -85,9 +81,9 @@ func TestSetPermissions(t *testing.T) {
 func TestSetTimestamps(t *testing.T) {
 	ctx := contexttest.Context(t)
 	for _, test := range []struct {
-		desc      string
-		ts        fs.TimeSpec
-		wantDirty fs.AttrMask
+		desc        string
+		ts          fs.TimeSpec
+		wantChanged fs.AttrMask
 	}{
 		{
 			desc: "noop",
@@ -95,7 +91,7 @@ func TestSetTimestamps(t *testing.T) {
 				ATimeOmit: true,
 				MTimeOmit: true,
 			},
-			wantDirty: fs.AttrMask{},
+			wantChanged: fs.AttrMask{},
 		},
 		{
 			desc: "access time only",
@@ -103,9 +99,8 @@ func TestSetTimestamps(t *testing.T) {
 				ATime:     ktime.NowFromContext(ctx),
 				MTimeOmit: true,
 			},
-			wantDirty: fs.AttrMask{
-				AccessTime:       true,
-				StatusChangeTime: true,
+			wantChanged: fs.AttrMask{
+				AccessTime: true,
 			},
 		},
 		{
@@ -114,9 +109,8 @@ func TestSetTimestamps(t *testing.T) {
 				ATimeOmit: true,
 				MTime:     ktime.NowFromContext(ctx),
 			},
-			wantDirty: fs.AttrMask{
+			wantChanged: fs.AttrMask{
 				ModificationTime: true,
-				StatusChangeTime: true,
 			},
 		},
 		{
@@ -125,10 +119,9 @@ func TestSetTimestamps(t *testing.T) {
 				ATime: ktime.NowFromContext(ctx),
 				MTime: ktime.NowFromContext(ctx),
 			},
-			wantDirty: fs.AttrMask{
+			wantChanged: fs.AttrMask{
 				AccessTime:       true,
 				ModificationTime: true,
-				StatusChangeTime: true,
 			},
 		},
 		{
@@ -137,10 +130,9 @@ func TestSetTimestamps(t *testing.T) {
 				ATimeSetSystemTime: true,
 				MTimeSetSystemTime: true,
 			},
-			wantDirty: fs.AttrMask{
+			wantChanged: fs.AttrMask{
 				AccessTime:       true,
 				ModificationTime: true,
-				StatusChangeTime: true,
 			},
 		},
 	} {
@@ -159,10 +151,7 @@ func TestSetTimestamps(t *testing.T) {
 			if err := iops.SetTimestamps(ctx, nil, test.ts); err != nil {
 				t.Fatalf("SetTimestamps got error %v, want nil", err)
 			}
-			if !reflect.DeepEqual(iops.dirtyAttr, test.wantDirty) {
-				t.Fatalf("dirty got %+v, want %+v", iops.dirtyAttr, test.wantDirty)
-			}
-			if iops.dirtyAttr.AccessTime {
+			if test.wantChanged.AccessTime {
 				if !iops.attr.AccessTime.After(uattr.AccessTime) {
 					t.Fatalf("diritied access time did not advance, want %v > %v", iops.attr.AccessTime, uattr.AccessTime)
 				}
@@ -173,7 +162,7 @@ func TestSetTimestamps(t *testing.T) {
 					t.Fatalf("dirtied status change time did not advance")
 				}
 			}
-			if iops.dirtyAttr.ModificationTime {
+			if test.wantChanged.ModificationTime {
 				if !iops.attr.ModificationTime.After(uattr.ModificationTime) {
 					t.Fatalf("diritied modification time did not advance")
 				}
@@ -200,16 +189,10 @@ func TestTruncate(t *testing.T) {
 	if err := iops.Truncate(ctx, nil, uattr.Size); err != nil {
 		t.Fatalf("Truncate got error %v, want nil", err)
 	}
-	if iops.dirtyAttr.Size {
-		t.Fatalf("Truncate caused size to be dirtied")
-	}
 	var size int64 = 4096
 	if err := iops.Truncate(ctx, nil, size); err != nil {
 		t.Fatalf("Truncate got error %v, want nil", err)
 	}
-	if !iops.dirtyAttr.Size {
-		t.Fatalf("Truncate caused size to not be dirtied")
-	}
 	if iops.attr.Size != size {
 		t.Fatalf("Truncate got %d, want %d", iops.attr.Size, size)
 	}
diff --git a/pkg/sentry/fs/gofer/inode.go b/pkg/sentry/fs/gofer/inode.go
index 1dc0ca0db..16435169a 100644
--- a/pkg/sentry/fs/gofer/inode.go
+++ b/pkg/sentry/fs/gofer/inode.go
@@ -247,6 +247,7 @@ func (i *inodeFileState) SetMaskedAttributes(ctx context.Context, mask fs.AttrMa
 // skipSetAttr checks if attribute change can be skipped. It can be skipped
 // when:
 //   - Mask is empty
+//   - Mask contains only attributes that cannot be set in the gofer
 //   - Mask contains only atime and/or mtime, and host FD exists
 //
 // Updates to atime and mtime can be skipped because cached value will be
@@ -254,15 +255,22 @@ func (i *inodeFileState) SetMaskedAttributes(ctx context.Context, mask fs.AttrMa
 // Skipping atime updates is particularly important to reduce the number of
 // operations sent to the Gofer for readonly files.
 func (i *inodeFileState) skipSetAttr(mask fs.AttrMask) bool {
-	if mask.Empty() {
+	// First remove attributes that cannot be updated.
+	cpy := mask
+	cpy.Type = false
+	cpy.DeviceID = false
+	cpy.InodeID = false
+	cpy.BlockSize = false
+	cpy.Usage = false
+	cpy.Links = false
+	if cpy.Empty() {
 		return true
 	}
 
-	cpy := mask
+	// Then check if more than just atime and mtime is being set.
 	cpy.AccessTime = false
 	cpy.ModificationTime = false
 	if !cpy.Empty() {
-		// More than just atime and mtime is being set.
 		return false
 	}