From 3d5201528603693b3383adc656acee0fe460b099 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Mon, 7 Mar 2022 17:25:06 -0800 Subject: [PATCH] Get rid of FSync RPC batching in gofer client with lisafs. This optimization was not used correctly, as described in the change. We could use it correctly to optimize sync(2) and syncfs(2). But that substantially increases code complexity. PiperOrigin-RevId: 433080982 --- pkg/sentry/fsimpl/gofer/filesystem.go | 26 ++++++++++--------------- pkg/sentry/fsimpl/gofer/gofer.go | 16 ++++----------- pkg/sentry/fsimpl/gofer/regular_file.go | 2 +- pkg/sentry/fsimpl/gofer/special_file.go | 9 ++------- 4 files changed, 17 insertions(+), 36 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index a57fa994b..8ebcc8bd3 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -56,15 +56,16 @@ func (fs *filesystem) Sync(ctx context.Context) error { var retErr error if fs.opts.lisaEnabled { - // Try accumulating all FDIDs to fsync and fsync then via one RPC as - // opposed to making an RPC per FDID. Passing a non-nil accFsyncFDIDs to - // dentry.syncCachedFile() and specialFileFD.sync() will cause them to not - // make an RPC, instead accumulate syncable FDIDs in the passed slice. - accFsyncFDIDs := make([]lisafs.FDID, 0, len(ds)+len(sffds)) + // Note that lisafs is capable of batching FSync RPCs. However, we can not + // batch all the FDIDs to be synced from ds and sffds. Because the error + // handling varies based on file type. FSync errors are only considered for + // regular file FDIDs that were opened for writing. We could do individual + // RPCs for such FDIDs and batch the rest, but it increases code complexity + // substantially. We could implement it in the future if need be. // Sync syncable dentries. for _, d := range ds { - if err := d.syncCachedFile(ctx, true /* forFilesystemSync */, &accFsyncFDIDs); err != nil { + if err := d.syncCachedFile(ctx, true /* forFilesystemSync */); err != nil { ctx.Infof("gofer.filesystem.Sync: dentry.syncCachedFile failed: %v", err) if retErr == nil { retErr = err @@ -75,7 +76,7 @@ func (fs *filesystem) Sync(ctx context.Context) error { // Sync special files, which may be writable but do not use dentry shared // handles (so they won't be synced by the above). for _, sffd := range sffds { - if err := sffd.sync(ctx, true /* forFilesystemSync */, &accFsyncFDIDs); err != nil { + if err := sffd.sync(ctx, true /* forFilesystemSync */); err != nil { ctx.Infof("gofer.filesystem.Sync: specialFileFD.sync failed: %v", err) if retErr == nil { retErr = err @@ -83,19 +84,12 @@ func (fs *filesystem) Sync(ctx context.Context) error { } } - if err := fs.clientLisa.SyncFDs(ctx, accFsyncFDIDs); err != nil { - ctx.Infof("gofer.filesystem.Sync: fs.fsyncMultipleFDLisa failed: %v", err) - if retErr == nil { - retErr = err - } - } - return retErr } // Sync syncable dentries. for _, d := range ds { - if err := d.syncCachedFile(ctx, true /* forFilesystemSync */, nil /* accFsyncFDIDsLisa */); err != nil { + if err := d.syncCachedFile(ctx, true /* forFilesystemSync */); err != nil { ctx.Infof("gofer.filesystem.Sync: dentry.syncCachedFile failed: %v", err) if retErr == nil { retErr = err @@ -106,7 +100,7 @@ func (fs *filesystem) Sync(ctx context.Context) error { // Sync special files, which may be writable but do not use dentry shared // handles (so they won't be synced by the above). for _, sffd := range sffds { - if err := sffd.sync(ctx, true /* forFilesystemSync */, nil /* accFsyncFDIDsLisa */); err != nil { + if err := sffd.sync(ctx, true /* forFilesystemSync */); err != nil { ctx.Infof("gofer.filesystem.Sync: specialFileFD.sync failed: %v", err) if retErr == nil { retErr = err diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index 22d356e72..694621237 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/pkg/sentry/fsimpl/gofer/gofer.go @@ -2384,11 +2384,11 @@ func (d *dentry) writeHandleLocked() handle { func (d *dentry) syncRemoteFile(ctx context.Context) error { d.handleMu.RLock() defer d.handleMu.RUnlock() - return d.syncRemoteFileLocked(ctx, nil /* accFsyncFDIDsLisa */) + return d.syncRemoteFileLocked(ctx) } // Preconditions: d.handleMu must be locked. -func (d *dentry) syncRemoteFileLocked(ctx context.Context, accFsyncFDIDsLisa *[]lisafs.FDID) error { +func (d *dentry) syncRemoteFileLocked(ctx context.Context) error { // If we have a host FD, fsyncing it is likely to be faster than an fsync // RPC. Prefer syncing write handles over read handles, since some remote // filesystem implementations may not sync changes made through write @@ -2400,10 +2400,6 @@ func (d *dentry) syncRemoteFileLocked(ctx context.Context, accFsyncFDIDsLisa *[] return err } if d.fs.opts.lisaEnabled && d.writeFDLisa.Ok() { - if accFsyncFDIDsLisa != nil { - *accFsyncFDIDsLisa = append(*accFsyncFDIDsLisa, d.writeFDLisa.ID()) - return nil - } return d.writeFDLisa.Sync(ctx) } else if !d.fs.opts.lisaEnabled && !d.writeFile.isNil() { return d.writeFile.fsync(ctx) @@ -2415,10 +2411,6 @@ func (d *dentry) syncRemoteFileLocked(ctx context.Context, accFsyncFDIDsLisa *[] return err } if d.fs.opts.lisaEnabled && d.readFDLisa.Ok() { - if accFsyncFDIDsLisa != nil { - *accFsyncFDIDsLisa = append(*accFsyncFDIDsLisa, d.readFDLisa.ID()) - return nil - } return d.readFDLisa.Sync(ctx) } else if !d.fs.opts.lisaEnabled && !d.readFile.isNil() { return d.readFile.fsync(ctx) @@ -2426,7 +2418,7 @@ func (d *dentry) syncRemoteFileLocked(ctx context.Context, accFsyncFDIDsLisa *[] return nil } -func (d *dentry) syncCachedFile(ctx context.Context, forFilesystemSync bool, accFsyncFDIDsLisa *[]lisafs.FDID) error { +func (d *dentry) syncCachedFile(ctx context.Context, forFilesystemSync bool) error { d.handleMu.RLock() defer d.handleMu.RUnlock() h := d.writeHandleLocked() @@ -2439,7 +2431,7 @@ func (d *dentry) syncCachedFile(ctx context.Context, forFilesystemSync bool, acc return err } } - if err := d.syncRemoteFileLocked(ctx, accFsyncFDIDsLisa); err != nil { + if err := d.syncRemoteFileLocked(ctx); err != nil { if !forFilesystemSync { return err } diff --git a/pkg/sentry/fsimpl/gofer/regular_file.go b/pkg/sentry/fsimpl/gofer/regular_file.go index 874f9873d..69a44d686 100644 --- a/pkg/sentry/fsimpl/gofer/regular_file.go +++ b/pkg/sentry/fsimpl/gofer/regular_file.go @@ -697,7 +697,7 @@ func regularFileSeekLocked(ctx context.Context, d *dentry, fdOffset, offset int6 // Sync implements vfs.FileDescriptionImpl.Sync. func (fd *regularFileFD) Sync(ctx context.Context) error { - return fd.dentry().syncCachedFile(ctx, false /* forFilesystemSync */, nil /* accFsyncFDIDsLisa */) + return fd.dentry().syncCachedFile(ctx, false /* forFilesystemSync */) } // ConfigureMMap implements vfs.FileDescriptionImpl.ConfigureMMap. diff --git a/pkg/sentry/fsimpl/gofer/special_file.go b/pkg/sentry/fsimpl/gofer/special_file.go index 5c9aa9195..dfcd5acf9 100644 --- a/pkg/sentry/fsimpl/gofer/special_file.go +++ b/pkg/sentry/fsimpl/gofer/special_file.go @@ -24,7 +24,6 @@ import ( "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/fdnotifier" "gvisor.dev/gvisor/pkg/hostarch" - "gvisor.dev/gvisor/pkg/lisafs" "gvisor.dev/gvisor/pkg/metric" "gvisor.dev/gvisor/pkg/p9" "gvisor.dev/gvisor/pkg/safemem" @@ -392,10 +391,10 @@ func (fd *specialFileFD) Seek(ctx context.Context, offset int64, whence int32) ( // Sync implements vfs.FileDescriptionImpl.Sync. func (fd *specialFileFD) Sync(ctx context.Context) error { - return fd.sync(ctx, false /* forFilesystemSync */, nil /* accFsyncFDIDsLisa */) + return fd.sync(ctx, false /* forFilesystemSync */) } -func (fd *specialFileFD) sync(ctx context.Context, forFilesystemSync bool, accFsyncFDIDsLisa *[]lisafs.FDID) error { +func (fd *specialFileFD) sync(ctx context.Context, forFilesystemSync bool) error { // Locks to ensure it didn't race with fd.Release(). fd.releaseMu.RLock() defer fd.releaseMu.RUnlock() @@ -413,10 +412,6 @@ func (fd *specialFileFD) sync(ctx context.Context, forFilesystemSync bool, accFs return err } if fs := fd.filesystem(); fs.opts.lisaEnabled { - if accFsyncFDIDsLisa != nil { - *accFsyncFDIDsLisa = append(*accFsyncFDIDsLisa, fd.handle.fdLisa.ID()) - return nil - } return fd.handle.fdLisa.Sync(ctx) } return fd.handle.file.fsync(ctx)