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)