From 27809081d42e692e01c3047722068f26f89e323e Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Tue, 16 May 2023 11:30:30 -0700 Subject: [PATCH] Read all dirents in directfs. Earlier we were only reading upto 64*1024 bytes and assuming that was all. Lisafs uses that count to make *multiple* Getdents RPCs until it encounters EOF. But directfs was only attempting the 64*1024 read one time. The count limit is not relevant for directfs. Make that lisafs specific. Make directfs read dirents until EOF is encountered. This this change, the nodejs test parallel/test-repl-tab-complete-import passes on ubuntu-1804-lts machine now. PiperOrigin-RevId: 532522376 --- pkg/sentry/fsimpl/gofer/dentry_impl.go | 6 +++--- pkg/sentry/fsimpl/gofer/directfs_dentry.go | 19 +++---------------- pkg/sentry/fsimpl/gofer/directory.go | 3 +-- pkg/sentry/fsimpl/gofer/lisafs_dentry.go | 12 ++++++++---- 4 files changed, 15 insertions(+), 25 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/dentry_impl.go b/pkg/sentry/fsimpl/gofer/dentry_impl.go index 851bc0f43..899de8377 100644 --- a/pkg/sentry/fsimpl/gofer/dentry_impl.go +++ b/pkg/sentry/fsimpl/gofer/dentry_impl.go @@ -379,12 +379,12 @@ func (d *dentry) openCreate(ctx context.Context, name string, accessFlags uint32 // - d.isDir(). // - d.handleMu must be locked. // - !d.isSynthetic(). -func (d *dentry) getDirentsLocked(ctx context.Context, count int, recordDirent func(name string, key inoKey, dType uint8)) error { +func (d *dentry) getDirentsLocked(ctx context.Context, recordDirent func(name string, key inoKey, dType uint8)) error { switch dt := d.impl.(type) { case *lisafsDentry: - return dt.getDirentsLocked(ctx, count, recordDirent) + return dt.getDirentsLocked(ctx, recordDirent) case *directfsDentry: - return dt.getDirentsLocked(count, recordDirent) + return dt.getDirentsLocked(recordDirent) default: panic("unknown dentry implementation") } diff --git a/pkg/sentry/fsimpl/gofer/directfs_dentry.go b/pkg/sentry/fsimpl/gofer/directfs_dentry.go index fdbfee371..a9429cc85 100644 --- a/pkg/sentry/fsimpl/gofer/directfs_dentry.go +++ b/pkg/sentry/fsimpl/gofer/directfs_dentry.go @@ -558,27 +558,16 @@ func (d *directfsDentry) openCreate(name string, accessFlags uint32, mode linux. return child, handle{fd: int32(childHandleFD)}, nil } -func (d *directfsDentry) getDirentsLocked(count int, recordDirent func(name string, key inoKey, dType uint8)) error { +func (d *directfsDentry) getDirentsLocked(recordDirent func(name string, key inoKey, dType uint8)) error { readFD := int(d.readFD.RacyLoad()) if _, err := unix.Seek(readFD, 0, 0); err != nil { return err } var direntsBuf [8192]byte - for bytesRead := 0; bytesRead < count; { - bufEnd := len(direntsBuf) - if remaining := int(count) - bytesRead; remaining < bufEnd { - bufEnd = remaining - } - n, err := unix.Getdents(readFD, direntsBuf[:bufEnd]) + for { + n, err := unix.Getdents(readFD, direntsBuf[:]) if err != nil { - if err == unix.EINVAL && bufEnd < fsutil.UnixDirentMaxSize { - // getdents64(2) returns EINVAL is returned when the result - // buffer is too small. If bufEnd is smaller than the max - // size of unix.Dirent, then just break here to return all - // dirents collected till now. - return nil - } return err } if n <= 0 { @@ -594,12 +583,10 @@ func (d *directfsDentry) getDirentsLocked(count int, recordDirent func(name stri log.Warningf("Getdent64: skipping file %q with failed stat, err: %v", path.Join(genericDebugPathname(&d.dentry), name), err) return true } - bytesRead += int(reclen) recordDirent(name, inoKeyFromStat(&stat), ftype) return true }) } - return nil } // Precondition: fs.renameMu is locked. diff --git a/pkg/sentry/fsimpl/gofer/directory.go b/pkg/sentry/fsimpl/gofer/directory.go index 1324ccb30..acfa9f19a 100644 --- a/pkg/sentry/fsimpl/gofer/directory.go +++ b/pkg/sentry/fsimpl/gofer/directory.go @@ -272,8 +272,7 @@ func (d *dentry) getDirents(ctx context.Context) ([]vfs.Dirent, error) { // have been opened when the calling directoryFD was opened. panic("gofer.dentry.getDirents called without a readable handle") } - const count = 64 * 1024 // for consistency with the vfs1 client - err := d.getDirentsLocked(ctx, count, func(name string, key inoKey, dType uint8) { + err := d.getDirentsLocked(ctx, func(name string, key inoKey, dType uint8) { dirent := vfs.Dirent{ Name: name, Ino: d.fs.inoFromKey(key), diff --git a/pkg/sentry/fsimpl/gofer/lisafs_dentry.go b/pkg/sentry/fsimpl/gofer/lisafs_dentry.go index 0b4c2f831..cc7f63a6e 100644 --- a/pkg/sentry/fsimpl/gofer/lisafs_dentry.go +++ b/pkg/sentry/fsimpl/gofer/lisafs_dentry.go @@ -462,20 +462,24 @@ func (d *lisafsDentry) openCreate(ctx context.Context, name string, flags uint32 return child, h, nil } +// lisafsGetdentsCount is the number of bytes of dirents to read from the +// server in each Getdents RPC. This value is consistent with vfs1 client. +const lisafsGetdentsCount = int32(64 * 1024) + // Preconditions: // - getDirents may not be called concurrently with another getDirents call. -func (d *lisafsDentry) getDirentsLocked(ctx context.Context, count int, recordDirent func(name string, key inoKey, dType uint8)) error { +func (d *lisafsDentry) getDirentsLocked(ctx context.Context, recordDirent func(name string, key inoKey, dType uint8)) error { // shouldSeek0 indicates whether the server should SEEK to 0 before reading // directory entries. shouldSeek0 := true for { - countLisa := int32(count) + count := lisafsGetdentsCount if shouldSeek0 { // See lisafs.Getdents64Req.Count. - countLisa = -countLisa + count = -count shouldSeek0 = false } - dirents, err := d.readFDLisa.Getdents64(ctx, countLisa) + dirents, err := d.readFDLisa.Getdents64(ctx, count) if err != nil { return err }