[LibOS/shim] Add tests for multiple writers

[pwmarcz]

Test if concurrent writes to the same file descriptor are handled correctly (for example, if they do not overlap each other). Right now, this is the case for multiple threads, but not multiple processes, so the multi-process tests are disabled.

---

This change is 

[pwmarcz]

Looks like the problem of test classes inheriting from each other bit me :(

For now, I defined a separate test class to add new tests, as the (hopefully) least hacky solution.

Reviewable status: 0 of 7 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners

[dimakuv]

Reviewed 4 of 5 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners

[mkow]

Reviewed 4 of 5 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

LibOS/shim/test/fs/multiple_writers.c, line 32 at r2 (raw file):

        size_t size = snprintf(buf, sizeof(buf), "%04d %04d %04d: proc %d thread %d line %d\n",
                               g_proc_id, thread_id, i, g_proc_id, thread_id, i);
        write_fd(g_path, g_fd, buf, size);

Could you loop write_fd? (to be EINTR/partial-write-safe)

---

LibOS/shim/test/fs/multiple_writers.c, line 48 at r2 (raw file):

    for (int i = 1; i < n_processes; i++) {
        int ret = fork();
        if (ret < 0)

Please add braces (we either put them on both clauses or none, including if-else case)

---

LibOS/shim/test/fs/test_fs.py, line 317 at r2 (raw file):

class TC_01_Sync(RegressionTestCase):

This test collection seems to not be executed on Jenkins, or at least I can't find it in the logs. Is this intended? Or maybe I missed it?

---

LibOS/shim/test/fs/test_fs.py, line 353 at r2 (raw file):

    @unittest.skip('file handle sync is not supported yet')
    def test_003_multiple_writers_many_processes_and_threads(self):
        self._test_multiple_writers(20, 3, 3)

3 seems pretty low to reliably detect problems, could we fire a few more? I'd feel more confident with at least 5 :)

[pwmarcz]

Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

---

LibOS/shim/test/fs/multiple_writers.c, line 32 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you loop write_fd? (to be EINTR/partial-write-safe)

write_fd is a helper function in these tests that already does that.

---

LibOS/shim/test/fs/multiple_writers.c, line 48 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please add braces (we either put them on both clauses or none, including if-else case)

Done.

---

LibOS/shim/test/fs/test_fs.py, line 317 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This test collection seems to not be executed on Jenkins, or at least I can't find it in the logs. Is this intended? Or maybe I missed it?

Oh, this is again about the tests inheriting from each other :( We just run pytest tests_pf.py, and the test class from test_fs.py is also included because we import it there. And of course other classes from test_fs.py do not get run this way. Horrible. (This was also discussed in #2124).

I think I fixed it. The classes still inherit from each other, but now there are separate, independent targets (fs-test and pf-test).

---

LibOS/shim/test/fs/test_fs.py, line 353 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

3 seems pretty low to reliably detect problems, could we fire a few more? I'd feel more confident with at least 5 :)

Sure. I bumped it up to 5 processes, 5 threads.

(Right now I'm seeing problems with even 2 processes, because the sync simply isn't there yet, and all processes start from position 0. But yeah, it will help test sync better).

[mkow]

Reviewed 5 of 5 files at r3.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners

---

LibOS/shim/test/fs/multiple_writers.c, line 32 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

write_fd is a helper function in these tests that already does that.

Oh, sorry, brainlag on my side, it's not write() :)

[dimakuv]

Reviewed 5 of 5 files at r3.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

[mkow]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

[dimakuv]

Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[mkow]

Jenkins, retest Jenkins-Debug please (apps.LTP.abort01 failed, unrelated to this PR, but worrying)

[boryspoplawski]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @pwmarcz)

a discussion (no related file):
Seems I was late with review, it got merged in the meantime. Comments for the future then :)

---

a discussion (no related file):
Please swap commits, if the 2nd fixes and issue discovered by the first.

---

---

LibOS/shim/test/fs/multiple_writers.c, line 17 at r4 (raw file):

#include "common.h"

pthread_barrier_t g_barrier;

Please add static to all of these variables.

---

LibOS/shim/test/fs/multiple_writers.c, line 24 at r4 (raw file):

static void* writer(void* arg) {
    int thread_id = *(int*)arg;

You don't need that ids table, you could pass the thread id directly in the argument.

---

LibOS/shim/test/fs/multiple_writers.c, line 30 at r4 (raw file):

    for (int i = 0; i < g_n_lines; i++) {
        char buf[100];
        size_t size = snprintf(buf, sizeof(buf), "%04d %04d %04d: proc %d thread %d line %d\n",

I guess it cannot fail in practice, but I still don't like the lack of error checks...

---

LibOS/shim/test/fs/multiple_writers.c, line 40 at r4 (raw file):

    g_fd = open_output_fd(path, /*rdwr=*/false);
    if (ftruncate(g_fd, 0))
        fatal_error("truncate(%s) failed: %d\n", path, path, errno);

path is doubled...

---

LibOS/shim/test/fs/multiple_writers.c, line 47 at r4 (raw file):

    g_proc_id = 0;
    for (int i = 1; i < n_processes; i++) {
        int ret = fork();

pid_t might be bigger than int...

---

LibOS/shim/test/fs/multiple_writers.c, line 72 at r4 (raw file):

    if (g_proc_id == 0) {
        for (int i = 1; i < n_processes; i++)
            wait(NULL);

Lack of error checking.