From 2d20b121d710fda3ad3382b66cd6c936e20a1119 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Fri, 1 Feb 2019 17:50:32 -0800 Subject: 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 --- pkg/sentry/fs/fsutil/inode_cached.go | 66 ++++++++++++------------------- pkg/sentry/fs/fsutil/inode_cached_test.go | 39 ++++++------------ pkg/sentry/fs/gofer/inode.go | 14 +++++-- 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 } -- cgit v1.2.3