summaryrefslogtreecommitdiffhomepage
path: root/pkg
diff options
context:
space:
mode:
Diffstat (limited to 'pkg')
-rw-r--r--pkg/sentry/fs/fsutil/inode_cached.go66
-rw-r--r--pkg/sentry/fs/fsutil/inode_cached_test.go39
-rw-r--r--pkg/sentry/fs/gofer/inode.go14
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
}
generated by cgit v1.2.3 (git 2.39.1) at 2024-11-05 12:30:40 +0000