netbird/gvisor
0
0
Fork 0
mirror of https://github.com/netbirdio/gvisor.git synced 2026-05-22 17:12:49 -07:00
Code Issues Packages Projects Releases Wiki Activity

CachingInodeOperations was over-dirtying cached attributes

Browse Source 
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
This commit is contained in:
Fabricio Voznika
2019-02-01 17:51:48 -08:00
committed by Shentubot
parent 92e85623a0
commit 2d20b121d7
3 changed files with 48 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/sentry/fs/fsutil/inode_cached.go
+26 -40
View File
@@ -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
pkg/sentry/fs/fsutil/inode_cached_test.go
+11 -28
View File
@@ -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)
}
pkg/sentry/fs/gofer/inode.go
+11 -3
View File
@@ -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
}