From de54e60f57e4dca090253716bfecf6ba1c91cdba Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Tue, 23 Aug 2022 13:48:34 -0700 Subject: [PATCH] Make P9 faster for large directories. As of right now, runsc's fsgofer Readdir implementation is awfully slow for large directories that have >2000 entries. So slow that is deserves a description. This happens because fsgofer confuses the unit of `Count` as "number of dirents" rather than "number of bytes". lisafs does not suffer from this. Lets say there is a large directory with 100,000 files. When the application does `ls`, the gofer reads all 100,000 entries from the host and populates a huge slice with all these dirents. p9/messages.go:Rreaddir.encode() silently discards 98,000 of those dirents because only ~2,000 of them fit in `Count` bytes. Then the gofer client again makes a Readdir RPC with offset 2,000. The gofer reads all 100,000 files, skips first 2,000, returns next 2,000 and discards 96,000. This repeats until all files are returned. Updated fsgofer to realize `Count` as number of bytes to read. fsgofer only reads upto 80% of the count limit from the host to take into account the fact that p9.Dirent takes more bytes to be encoded than unix.Dirent. Added warning logging in encode() when it is discarding dirents. Before: ``` $ docker run --runtime=runsc --rm -v /host/test:/test ubuntu bash -c 'time ls test > /dev/null' real 0m7.826s user 0m0.120s sys 0m0.030s ``` After: ``` $ docker run --runtime=runsc --rm -v /host/test:/test ubuntu bash -c 'time ls test > /dev/null' real 0m0.635s user 0m0.130s sys 0m0.040s ``` Updates #6665 PiperOrigin-RevId: 469546979 --- pkg/p9/client_file.go | 4 +- pkg/p9/file.go | 13 ++++++- pkg/p9/handlers.go | 2 +- pkg/p9/messages.go | 14 ++++--- pkg/p9/messages_test.go | 6 +-- pkg/p9/p9.go | 4 ++ runsc/fsgofer/fsgofer.go | 69 ++++++++++++++++++--------------- runsc/fsgofer/fsgofer_test.go | 2 +- runsc/fsgofer/fsgofer_unsafe.go | 20 ++++++---- runsc/fsgofer/lisafs.go | 6 +-- 10 files changed, 83 insertions(+), 57 deletions(-) diff --git a/pkg/p9/client_file.go b/pkg/p9/client_file.go index 236b230ac..3446c2db4 100644 --- a/pkg/p9/client_file.go +++ b/pkg/p9/client_file.go @@ -763,13 +763,13 @@ func (c *clientFile) UnlinkAt(name string, flags uint32) error { } // Readdir implements File.Readdir. -func (c *clientFile) Readdir(offset uint64, count uint32) ([]Dirent, error) { +func (c *clientFile) Readdir(direntOffset uint64, count uint32) ([]Dirent, error) { if c.closed.Load() != 0 { return nil, unix.EBADF } rreaddir := Rreaddir{} - if err := c.client.sendRecv(&Treaddir{Directory: c.fid, Offset: offset, Count: count}, &rreaddir); err != nil { + if err := c.client.sendRecv(&Treaddir{Directory: c.fid, DirentOffset: direntOffset, Count: count}, &rreaddir); err != nil { return nil, err } diff --git a/pkg/p9/file.go b/pkg/p9/file.go index b209f4ce7..c4a79b4c2 100644 --- a/pkg/p9/file.go +++ b/pkg/p9/file.go @@ -275,10 +275,19 @@ type File interface { // Readdir reads directory entries. // - // This may return io.EOF in addition to unix.Errno values. + // This may return io.EOF in addition to unix.Errno values. count is the + // number of bytes to read. + // + // direntOffset is the directory offset at which the read should happen. + // direntOffset can be set to 0 to start reading the directory from start. + // direntOffset is used more like a cookie. The unit of direntOffset is + // unspecified. Gofers can choose their own unit. The client must set it + // to one of the values returned in Dirent.Offset, preferably the last offset + // returned, which should cause the readdir to continue from where it was + // left off. // // On the server, Readdir has a read concurrency guarantee. - Readdir(offset uint64, count uint32) ([]Dirent, error) + Readdir(direntOffset uint64, count uint32) ([]Dirent, error) // Readlink reads the link target. // diff --git a/pkg/p9/handlers.go b/pkg/p9/handlers.go index a4f1c6f74..c0f5f8b3f 100644 --- a/pkg/p9/handlers.go +++ b/pkg/p9/handlers.go @@ -1093,7 +1093,7 @@ func (t *Treaddir) handle(cs *connState) message { } // Read the entries. - entries, err = ref.file.Readdir(t.Offset, t.Count) + entries, err = ref.file.Readdir(t.DirentOffset, t.Count) if err != nil && err != io.EOF { return err } diff --git a/pkg/p9/messages.go b/pkg/p9/messages.go index 2e7b714b3..044b57a92 100644 --- a/pkg/p9/messages.go +++ b/pkg/p9/messages.go @@ -19,6 +19,7 @@ import ( "math" "gvisor.dev/gvisor/pkg/fd" + "gvisor.dev/gvisor/pkg/log" ) // ErrInvalidMsgType is returned when an unsupported message type is found. @@ -1916,8 +1917,8 @@ type Treaddir struct { // Directory is the directory FID to read. Directory FID - // Offset is the offset to read at. - Offset uint64 + // DirentOffset is the dirent offset to read at. + DirentOffset uint64 // Count is the number of bytes to read. Count uint32 @@ -1926,14 +1927,14 @@ type Treaddir struct { // decode implements encoder.decode. func (t *Treaddir) decode(b *buffer) { t.Directory = b.ReadFID() - t.Offset = b.Read64() + t.DirentOffset = b.Read64() t.Count = b.Read32() } // encode implements encoder.encode. func (t *Treaddir) encode(b *buffer) { b.WriteFID(t.Directory) - b.Write64(t.Offset) + b.Write64(t.DirentOffset) b.Write32(t.Count) } @@ -1944,7 +1945,7 @@ func (*Treaddir) Type() MsgType { // String implements fmt.Stringer. func (t *Treaddir) String() string { - return fmt.Sprintf("Treaddir{DirectoryFID: %d, Offset: %d, Count: %d}", t.Directory, t.Offset, t.Count) + return fmt.Sprintf("Treaddir{DirectoryFID: %d, DirentOffset: %d, Count: %d}", t.Directory, t.DirentOffset, t.Count) } // Rreaddir is a readdir response. @@ -1985,9 +1986,10 @@ func (r *Rreaddir) decode(b *buffer) { func (r *Rreaddir) encode(b *buffer) { entriesBuf := buffer{} payloadSize := 0 - for _, d := range r.Entries { + for i, d := range r.Entries { d.encode(&entriesBuf) if len(entriesBuf.data) > int(r.Count) { + log.Warningf("hit Rreaddir.Count limit while encoding dirents, discarding %d dirents", len(r.Entries)-i) break } payloadSize = len(entriesBuf.data) diff --git a/pkg/p9/messages_test.go b/pkg/p9/messages_test.go index 8da664bf2..858a98c5e 100644 --- a/pkg/p9/messages_test.go +++ b/pkg/p9/messages_test.go @@ -221,9 +221,9 @@ func TestEncodeDecode(t *testing.T) { }, &Rsetxattr{}, &Treaddir{ - Directory: 1, - Offset: 2, - Count: 3, + Directory: 1, + DirentOffset: 2, + Count: 3, }, &Rreaddir{ // Count must be sufficient to encode a dirent. diff --git a/pkg/p9/p9.go b/pkg/p9/p9.go index 34fbf9beb..5db73179a 100644 --- a/pkg/p9/p9.go +++ b/pkg/p9/p9.go @@ -1084,6 +1084,10 @@ func (a *Attr) Apply(mask SetAttrMask, attr SetAttr) { } } +// DirentSizeStatic is the number of bytes required to encode a p9.Dirent +// with an empty name. In other words, it is the static part of its size. +const DirentSizeStatic = 24 + // Dirent is used for readdir. type Dirent struct { // QID is the entry QID. diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go index 6a40b9bd6..bf8c73d05 100644 --- a/runsc/fsgofer/fsgofer.go +++ b/runsc/fsgofer/fsgofer.go @@ -989,7 +989,7 @@ func (l *localFile) UnlinkAt(name string, flags uint32) error { } // Readdir implements p9.File. -func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { +func (l *localFile) Readdir(direntOffset uint64, count uint32) ([]p9.Dirent, error) { if l.mode != p9.ReadOnly && l.mode != p9.ReadWrite { return nil, unix.EBADF } @@ -1010,18 +1010,18 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { // offset is 0, since this is side-effectual (equivalent to rewinddir(3), // which causes the directory stream to resynchronize with the directory's // current contents). - if l.lastDirentOffset != offset || offset == 0 { + if l.lastDirentOffset != direntOffset || direntOffset == 0 { if _, err := unix.Seek(l.file.FD(), 0, 0); err != nil { return nil, extractErrno(err) } - skip = offset + skip = direntOffset } - dirents, err := l.readDirent(l.file.FD(), offset, count, skip) + dirents, err := l.readDirent(l.file.FD(), direntOffset, count, skip) if err == nil { // On success, remember the offset that was returned at the current // position. - l.lastDirentOffset = offset + uint64(len(dirents)) + l.lastDirentOffset = direntOffset + uint64(len(dirents)) } else { // On failure, the state is unknown, force call to seek() next time. l.lastDirentOffset = math.MaxUint64 @@ -1029,57 +1029,62 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { return dirents, err } -func (l *localFile) readDirent(f int, offset uint64, count uint32, skip uint64) ([]p9.Dirent, error) { +func (l *localFile) readDirent(f int, direntOffset uint64, count uint32, skip uint64) ([]p9.Dirent, error) { var dirents []p9.Dirent - // Limit 'count' to cap the slice size that is returned. - const maxCount = 100000 + // p9.Dirent takes 5 extra bytes to be encoded than a unix.Dirent. + // count will be used to count against the number of bytes read from the + // host. So scale it down, so that when encoding the same dirents in p9, we + // don't hit count limit. Scale down to 80%. + count = (count * 8) / 10 + // Limit 'count' to cap the amount of data that is returned. + const maxCount = 102_400 if count > maxCount { count = maxCount } // Pre-allocate buffers that will be reused to get partial results. direntsBuf := make([]byte, 8192) - names := make([]string, 0, 100) - - end := offset + uint64(count) - for offset < end { - dirSize, err := unix.ReadDirent(f, direntsBuf) + for bytesRead := 0; bytesRead < int(count); { + bufEnd := len(direntsBuf) + if remaining := int(count) - bytesRead; remaining < bufEnd { + bufEnd = remaining + } + n, err := unix.Getdents(f, direntsBuf[:bufEnd]) if err != nil { + if err == unix.EINVAL && bufEnd < unixDirentMaxSize { + // getdents64(2) returns EINVAL 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 dirents, nil + } return dirents, err } - if dirSize <= 0 { + if n <= 0 { return dirents, nil } - names := names[:0] - _, _, names = unix.ParseDirent(direntsBuf[:dirSize], -1, names) - - // Skip over entries that the caller is not interested in. - if skip > 0 { - if skip > uint64(len(names)) { - skip -= uint64(len(names)) - names = names[:0] - } else { - names = names[skip:] - skip = 0 - } - } - for _, name := range names { + parseDirents(direntsBuf[:n], func(ino uint64, off int64, ftype uint8, name string, reclen uint16) bool { stat, err := statAt(l.file.FD(), name) if err != nil { log.Warningf("Readdir is skipping file %q with failed stat, err: %v", path.Join(l.hostPath, name), err) - continue + return true + } + if skip > 0 { + skip-- + return true } qid := l.attachPoint.makeQID(&stat) - offset++ + direntOffset++ dirents = append(dirents, p9.Dirent{ QID: qid, Type: qid.Type, Name: name, - Offset: offset, + Offset: direntOffset, }) - } + bytesRead += int(reclen) + return true + }) } return dirents, nil } diff --git a/runsc/fsgofer/fsgofer_test.go b/runsc/fsgofer/fsgofer_test.go index 69c5ee0df..9889807f0 100644 --- a/runsc/fsgofer/fsgofer_test.go +++ b/runsc/fsgofer/fsgofer_test.go @@ -738,7 +738,7 @@ func TestReaddir(t *testing.T) { t.Fatalf("%v: Open(ReadOnly) failed, err: %v", s, err) } - dirents, err := s.file.Readdir(0, 10) + dirents, err := s.file.Readdir(0, 4096) if err != nil { t.Fatalf("%v: Readdir(0, 10) failed, err: %v", s, err) } diff --git a/runsc/fsgofer/fsgofer_unsafe.go b/runsc/fsgofer/fsgofer_unsafe.go index fb4fbe0d2..5af99b5b3 100644 --- a/runsc/fsgofer/fsgofer_unsafe.go +++ b/runsc/fsgofer/fsgofer_unsafe.go @@ -21,7 +21,7 @@ import ( "gvisor.dev/gvisor/pkg/syserr" ) -var unixDirentMaxSize uint32 = uint32(unsafe.Sizeof(unix.Dirent{})) +var unixDirentMaxSize = int(unsafe.Sizeof(unix.Dirent{})) func utimensat(dirFd int, name string, times [2]unix.Timespec, flags int) error { // utimensat(2) doesn't accept empty name, instead name must be nil to make it @@ -83,11 +83,14 @@ func renameat(oldDirFD int, oldName string, newDirFD int, newName string) error return nil } -func parseDirents(buf []byte, handleDirent func(ino uint64, off int64, ftype uint8, name string) bool) { +func parseDirents(buf []byte, handleDirent func(ino uint64, off int64, ftype uint8, name string, reclen uint16) bool) { for len(buf) > 0 { // Interpret the buf populated by unix.Getdents as unix.Dirent. dirent := *(*unix.Dirent)(unsafe.Pointer(&buf[0])) + // Advance buf for the next dirent. + buf = buf[dirent.Reclen:] + // Extracting the name is pretty tedious... var nameBuf [unix.NAME_MAX]byte var nameLen int @@ -101,12 +104,15 @@ func parseDirents(buf []byte, handleDirent func(ino uint64, off int64, ftype uin } name := string(nameBuf[:nameLen]) - // Deliver results to caller. - if !handleDirent(dirent.Ino, dirent.Off, dirent.Type, name) { - return + // Skip `.` and `..` entries. It is anyways ignored by the client. We also + // don't want to leak information about `..`. + if name == "." || name == ".." { + continue } - // Advance buf for the next dirent. - buf = buf[dirent.Reclen:] + // Deliver results to caller. + if !handleDirent(dirent.Ino, dirent.Off, dirent.Type, name, dirent.Reclen) { + return + } } } diff --git a/runsc/fsgofer/lisafs.go b/runsc/fsgofer/lisafs.go index fc96ddbdc..5767dba2c 100644 --- a/runsc/fsgofer/lisafs.go +++ b/runsc/fsgofer/lisafs.go @@ -842,7 +842,7 @@ func (fd *openFDLisa) Getdent64(count uint32, seek0 bool, recordDirent func(lisa } n, err := unix.Getdents(fd.hostFD, direntsBuf[:bufEnd]) if err != nil { - if err == unix.EINVAL && bufEnd < 268 { + if err == unix.EINVAL && bufEnd < 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 @@ -854,9 +854,8 @@ func (fd *openFDLisa) Getdent64(count uint32, seek0 bool, recordDirent func(lisa if n <= 0 { break } - bytesRead += n - parseDirents(direntsBuf[:n], func(ino uint64, off int64, ftype uint8, name string) bool { + parseDirents(direntsBuf[:n], func(ino uint64, off int64, ftype uint8, name string, reclen uint16) bool { dirent := lisafs.Dirent64{ Ino: primitive.Uint64(ino), Off: primitive.Uint64(off), @@ -874,6 +873,7 @@ func (fd *openFDLisa) Getdent64(count uint32, seek0 bool, recordDirent func(lisa dirent.DevMinor = primitive.Uint32(unix.Minor(stat.Dev)) dirent.DevMajor = primitive.Uint32(unix.Major(stat.Dev)) recordDirent(dirent) + bytesRead += int(reclen) return true }) }