From 1b11326ecdd788fccc8e396b7467bbbb591d563f Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Thu, 30 Jul 2020 15:00:42 -0700 Subject: Call lseek(0, SEEK_CUR) unconditionally in runsc fsgofer's Readdir(offset=0). 9P2000.L is silent as to how readdir RPCs interact with directory mutation. The most performant option is for Treaddir with offset=0 to restart iteration, avoiding needing to walk+open+clunk a new directory fid between invocations of getdents64(2), and the VFS2 gofer client assumes this is the case. Make this actually true for the runsc fsgofer. Fixes #3344, #3345, #3355 PiperOrigin-RevId: 324090384 --- images/basic/hostoverlaytest/Dockerfile | 3 +- images/basic/hostoverlaytest/copy_up_testfile.txt | 1 + images/basic/hostoverlaytest/test.c | 88 ----------------------- images/basic/hostoverlaytest/test_copy_up.c | 88 +++++++++++++++++++++++ images/basic/hostoverlaytest/test_rewinddir.c | 78 ++++++++++++++++++++ images/basic/hostoverlaytest/testfile.txt | 1 - runsc/fsgofer/fsgofer.go | 9 ++- test/e2e/integration_test.go | 28 +++++++- 8 files changed, 202 insertions(+), 94 deletions(-) create mode 100644 images/basic/hostoverlaytest/copy_up_testfile.txt delete mode 100644 images/basic/hostoverlaytest/test.c create mode 100644 images/basic/hostoverlaytest/test_copy_up.c create mode 100644 images/basic/hostoverlaytest/test_rewinddir.c delete mode 100644 images/basic/hostoverlaytest/testfile.txt diff --git a/images/basic/hostoverlaytest/Dockerfile b/images/basic/hostoverlaytest/Dockerfile index d83439e9c..6cef1a542 100644 --- a/images/basic/hostoverlaytest/Dockerfile +++ b/images/basic/hostoverlaytest/Dockerfile @@ -4,4 +4,5 @@ WORKDIR /root COPY . . RUN apt-get update && apt-get install -y gcc -RUN gcc -O2 -o test test.c +RUN gcc -O2 -o test_copy_up test_copy_up.c +RUN gcc -O2 -o test_rewinddir test_rewinddir.c diff --git a/images/basic/hostoverlaytest/copy_up_testfile.txt b/images/basic/hostoverlaytest/copy_up_testfile.txt new file mode 100644 index 000000000..e4188c841 --- /dev/null +++ b/images/basic/hostoverlaytest/copy_up_testfile.txt @@ -0,0 +1 @@ +old data diff --git a/images/basic/hostoverlaytest/test.c b/images/basic/hostoverlaytest/test.c deleted file mode 100644 index 088f90746..000000000 --- a/images/basic/hostoverlaytest/test.c +++ /dev/null @@ -1,88 +0,0 @@ -#include -#include -#include -#include -#include -#include - -int main(int argc, char** argv) { - const char kTestFilePath[] = "testfile.txt"; - const char kOldFileData[] = "old data\n"; - const char kNewFileData[] = "new data\n"; - const size_t kPageSize = sysconf(_SC_PAGE_SIZE); - - // Open a file that already exists in a host overlayfs lower layer. - const int fd_rdonly = open(kTestFilePath, O_RDONLY); - if (fd_rdonly < 0) { - err(1, "open(%s, O_RDONLY)", kTestFilePath); - } - - // Check that the file's initial contents are what we expect when read via - // syscall. - char oldbuf[sizeof(kOldFileData)] = {}; - ssize_t n = pread(fd_rdonly, oldbuf, sizeof(oldbuf), 0); - if (n < 0) { - err(1, "initial pread"); - } - if (n != strlen(kOldFileData)) { - errx(1, "short initial pread (%ld/%lu bytes)", n, strlen(kOldFileData)); - } - if (strcmp(oldbuf, kOldFileData) != 0) { - errx(1, "initial pread returned wrong data: %s", oldbuf); - } - - // Check that the file's initial contents are what we expect when read via - // memory mapping. - void* page = mmap(NULL, kPageSize, PROT_READ, MAP_SHARED, fd_rdonly, 0); - if (page == MAP_FAILED) { - err(1, "mmap"); - } - if (strcmp(page, kOldFileData) != 0) { - errx(1, "mapping contains wrong initial data: %s", (const char*)page); - } - - // Open the same file writably, causing host overlayfs to copy it up, and - // replace its contents. - const int fd_rdwr = open(kTestFilePath, O_RDWR); - if (fd_rdwr < 0) { - err(1, "open(%s, O_RDWR)", kTestFilePath); - } - n = write(fd_rdwr, kNewFileData, strlen(kNewFileData)); - if (n < 0) { - err(1, "write"); - } - if (n != strlen(kNewFileData)) { - errx(1, "short write (%ld/%lu bytes)", n, strlen(kNewFileData)); - } - if (ftruncate(fd_rdwr, strlen(kNewFileData)) < 0) { - err(1, "truncate"); - } - - int failed = 0; - - // Check that syscalls on the old FD return updated contents. (Before Linux - // 4.18, this requires that runsc use a post-copy-up FD to service the read.) - char newbuf[sizeof(kNewFileData)] = {}; - n = pread(fd_rdonly, newbuf, sizeof(newbuf), 0); - if (n < 0) { - err(1, "final pread"); - } - if (n != strlen(kNewFileData)) { - warnx("short final pread (%ld/%lu bytes)", n, strlen(kNewFileData)); - failed = 1; - } else if (strcmp(newbuf, kNewFileData) != 0) { - warnx("final pread returned wrong data: %s", newbuf); - failed = 1; - } - - // Check that the memory mapping of the old FD has been updated. (Linux - // overlayfs does not do this, so regardless of kernel version this requires - // that runsc replace existing memory mappings with mappings of a - // post-copy-up FD.) - if (strcmp(page, kNewFileData) != 0) { - warnx("mapping contains wrong final data: %s", (const char*)page); - failed = 1; - } - - return failed; -} diff --git a/images/basic/hostoverlaytest/test_copy_up.c b/images/basic/hostoverlaytest/test_copy_up.c new file mode 100644 index 000000000..010b261dc --- /dev/null +++ b/images/basic/hostoverlaytest/test_copy_up.c @@ -0,0 +1,88 @@ +#include +#include +#include +#include +#include +#include + +int main(int argc, char** argv) { + const char kTestFilePath[] = "copy_up_testfile.txt"; + const char kOldFileData[] = "old data\n"; + const char kNewFileData[] = "new data\n"; + const size_t kPageSize = sysconf(_SC_PAGE_SIZE); + + // Open a file that already exists in a host overlayfs lower layer. + const int fd_rdonly = open(kTestFilePath, O_RDONLY); + if (fd_rdonly < 0) { + err(1, "open(%s, O_RDONLY)", kTestFilePath); + } + + // Check that the file's initial contents are what we expect when read via + // syscall. + char oldbuf[sizeof(kOldFileData)] = {}; + ssize_t n = pread(fd_rdonly, oldbuf, sizeof(oldbuf), 0); + if (n < 0) { + err(1, "initial pread"); + } + if (n != strlen(kOldFileData)) { + errx(1, "short initial pread (%ld/%lu bytes)", n, strlen(kOldFileData)); + } + if (strcmp(oldbuf, kOldFileData) != 0) { + errx(1, "initial pread returned wrong data: %s", oldbuf); + } + + // Check that the file's initial contents are what we expect when read via + // memory mapping. + void* page = mmap(NULL, kPageSize, PROT_READ, MAP_SHARED, fd_rdonly, 0); + if (page == MAP_FAILED) { + err(1, "mmap"); + } + if (strcmp(page, kOldFileData) != 0) { + errx(1, "mapping contains wrong initial data: %s", (const char*)page); + } + + // Open the same file writably, causing host overlayfs to copy it up, and + // replace its contents. + const int fd_rdwr = open(kTestFilePath, O_RDWR); + if (fd_rdwr < 0) { + err(1, "open(%s, O_RDWR)", kTestFilePath); + } + n = write(fd_rdwr, kNewFileData, strlen(kNewFileData)); + if (n < 0) { + err(1, "write"); + } + if (n != strlen(kNewFileData)) { + errx(1, "short write (%ld/%lu bytes)", n, strlen(kNewFileData)); + } + if (ftruncate(fd_rdwr, strlen(kNewFileData)) < 0) { + err(1, "truncate"); + } + + int failed = 0; + + // Check that syscalls on the old FD return updated contents. (Before Linux + // 4.18, this requires that runsc use a post-copy-up FD to service the read.) + char newbuf[sizeof(kNewFileData)] = {}; + n = pread(fd_rdonly, newbuf, sizeof(newbuf), 0); + if (n < 0) { + err(1, "final pread"); + } + if (n != strlen(kNewFileData)) { + warnx("short final pread (%ld/%lu bytes)", n, strlen(kNewFileData)); + failed = 1; + } else if (strcmp(newbuf, kNewFileData) != 0) { + warnx("final pread returned wrong data: %s", newbuf); + failed = 1; + } + + // Check that the memory mapping of the old FD has been updated. (Linux + // overlayfs does not do this, so regardless of kernel version this requires + // that runsc replace existing memory mappings with mappings of a + // post-copy-up FD.) + if (strcmp(page, kNewFileData) != 0) { + warnx("mapping contains wrong final data: %s", (const char*)page); + failed = 1; + } + + return failed; +} diff --git a/images/basic/hostoverlaytest/test_rewinddir.c b/images/basic/hostoverlaytest/test_rewinddir.c new file mode 100644 index 000000000..f1a4085e1 --- /dev/null +++ b/images/basic/hostoverlaytest/test_rewinddir.c @@ -0,0 +1,78 @@ +#include +#include +#include +#include +#include +#include +#include + +int main(int argc, char** argv) { + const char kDirPath[] = "rewinddir_test_dir"; + const char kFileBasename[] = "rewinddir_test_file"; + + // Create the test directory. + if (mkdir(kDirPath, 0755) < 0) { + err(1, "mkdir(%s)", kDirPath); + } + + // The test directory should initially be empty. + DIR* dir = opendir(kDirPath); + if (!dir) { + err(1, "opendir(%s)", kDirPath); + } + int failed = 0; + while (1) { + errno = 0; + struct dirent* d = readdir(dir); + if (!d) { + if (errno != 0) { + err(1, "readdir"); + } + break; + } + if (strcmp(d->d_name, ".") != 0 && strcmp(d->d_name, "..") != 0) { + warnx("unexpected file %s in new directory", d->d_name); + failed = 1; + } + } + + // Create a file in the test directory. + char* file_path = malloc(strlen(kDirPath) + 1 + strlen(kFileBasename)); + if (!file_path) { + errx(1, "malloc"); + } + strcpy(file_path, kDirPath); + file_path[strlen(kDirPath)] = '/'; + strcpy(file_path + strlen(kDirPath) + 1, kFileBasename); + if (mknod(file_path, 0644, 0) < 0) { + err(1, "mknod(%s)", file_path); + } + + // After rewinddir(), re-reading the directory stream should yield the new + // file. + rewinddir(dir); + size_t found_file = 0; + while (1) { + errno = 0; + struct dirent* d = readdir(dir); + if (!d) { + if (errno != 0) { + err(1, "readdir"); + } + break; + } + if (strcmp(d->d_name, kFileBasename) == 0) { + found_file++; + } else if (strcmp(d->d_name, ".") != 0 && strcmp(d->d_name, "..") != 0) { + warnx("unexpected file %s in new directory", d->d_name); + failed = 1; + } + } + if (found_file != 1) { + warnx("readdir returned file %s %zu times, wanted 1", kFileBasename, + found_file); + failed = 1; + } + + return failed; +} diff --git a/images/basic/hostoverlaytest/testfile.txt b/images/basic/hostoverlaytest/testfile.txt deleted file mode 100644 index e4188c841..000000000 --- a/images/basic/hostoverlaytest/testfile.txt +++ /dev/null @@ -1 +0,0 @@ -old data diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go index ebefeacf2..c6694c278 100644 --- a/runsc/fsgofer/fsgofer.go +++ b/runsc/fsgofer/fsgofer.go @@ -979,9 +979,12 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { skip := uint64(0) - // Check if the file is at the correct position already. If not, seek to the - // beginning and read the entire directory again. - if l.lastDirentOffset != offset { + // Check if the file is at the correct position already. If not, seek to + // the beginning and read the entire directory again. We always seek if + // 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 _, err := syscall.Seek(l.file.FD(), 0, 0); err != nil { return nil, extractErrno(err) } diff --git a/test/e2e/integration_test.go b/test/e2e/integration_test.go index 5465dee9b..6fe6d304f 100644 --- a/test/e2e/integration_test.go +++ b/test/e2e/integration_test.go @@ -434,7 +434,33 @@ func TestHostOverlayfsCopyUp(t *testing.T) { if got, err := d.Run(ctx, dockerutil.RunOpts{ Image: "basic/hostoverlaytest", WorkDir: "/root", - }, "./test"); err != nil { + }, "./test_copy_up"); err != nil { + t.Fatalf("docker run failed: %v", err) + } else if got != "" { + t.Errorf("test failed:\n%s", got) + } +} + +// TestHostOverlayfsRewindDir tests that rewinddir() "causes the directory +// stream to refer to the current state of the corresponding directory, as a +// call to opendir() would have done" as required by POSIX, when the directory +// in question is host overlayfs. +// +// This test specifically targets host overlayfs because, per POSIX, "if a file +// is removed from or added to the directory after the most recent call to +// opendir() or rewinddir(), whether a subsequent call to readdir() returns an +// entry for that file is unspecified"; the host filesystems used by other +// automated tests yield newly-added files from readdir() even if the fsgofer +// does not explicitly rewinddir(), but overlayfs does not. +func TestHostOverlayfsRewindDir(t *testing.T) { + ctx := context.Background() + d := dockerutil.MakeContainer(ctx, t) + defer d.CleanUp(ctx) + + if got, err := d.Run(ctx, dockerutil.RunOpts{ + Image: "basic/hostoverlaytest", + WorkDir: "/root", + }, "./test_rewinddir"); err != nil { t.Fatalf("docker run failed: %v", err) } else if got != "" { t.Errorf("test failed:\n%s", got) -- cgit v1.2.3