fix: race condition on file create

[adammoody]

When more than one process tries to create a given file at the same time with open(O_CREAT), we were returning EEXIST on procs that lost the race. Those procs should still open the file unless O_EXCL is also specified.

Description

Motivation and Context

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Testing (addition of new tests or update to current tests)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the UnifyFS code style requirements.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted.

[adammoody]

@CamStan , I've started to write up some test cases for multi-process open and unlink calls of the same file. This requires an MPI job of at least two procs, though for this, it's sufficient to run all procs on the same node.

Would you mind helping me work that test in on this branch?

In which directory should I place a multi-process test?

The draft of that test is here:
https://github.com/LLNL/UnifyFS/blob/eexist/open_multi.c

Should I add TAP-style ok() statements to report errors?

[CamStan]

I've started to write up some test cases for multi-process open and unlink calls of the same file. This requires an MPI job of at least two procs, though for this, it's sufficient to run all procs on the same node.

Currently we are only set up with single-process unit tests and the integration tests which are based off the example programs (which only just check if they complete correctly; TAP-style tests aren't used in the examples themselves).

We are not set up for multi-process unit tests, but it's something that's been on my mind to attempt at some point, especially now that Travis doesn't limit you as much on this anymore. So I played around with this a bit today to see if I could get something to work and hit a few more bumps than I was expecting.

I got a basic version with your open_multi.c file working locally and pushed it to my fork for now to see if it will work on Travis.
It can be found here CamStan@ecbae08

Alternatively, could possibly write open_multi.c to fail/set a flag whenever return codes/sum/etc are not what is expected and then run this with the integration suite. I haven't thought through this one yet though.

---

In which directory should I place a multi-process test?

I created a new one "t/multi" for now, and named everything associated with it "multi" also. We can change the naming scheme, but just wanted to get it working for now.
Also, if we go this route and end up wanting to add similar tests in the future, we can do something similar to the single-process-unit-test suites and create something like 1100-sysio-gotcha-parallel.tthat will make it easier to extend. For now, I'm treating open_multi.c as it's own test suite called 1000-multi-open.t.

Then to add it to the test harness I created a new .t file, adding everything to the Makefile, and then created a JOB_MULTI_RUN_COMMAND to launch it with more than 1 ppn.

Should I add TAP-style ok() statements to report errors?

This is where most of the issues showed up. The biggest being that libtap (what we're currently using to test C code) doesn't like to be run with multiple processes. Essentially if you call ok(), every process will run it, the first process to complete will succeed, and then the testing harness can't figure out what the additional ones are and crashes. This is essentially why we can't just rerun our single-process tests with more processes.

To work around this, I had to make sure everything libtap related was only called by rank 0, which is ugly and annoying, but works for now. Could always tweak our testing framework in the future to make sure ok() is only called by rank 0, or create wrappers for the libtap tests to make sure they're only called by rank 0 (e.g., wrap ok() with ok_multi()).

---

Changes I made to open_multi.c in my commit to get this to work were:

• include libtap and testutil
• adjust the mountpoint to the one being used by the testing suite
• add plan() and done_testing()
• changed the test file to use a testutil randomly created one
• only added two tests to see if things were working (if mount worked, and if the sum for success is 1 after the first open() call)
  • result of these tests will look something like:

PASS: 1000-multi-open.t 1 - ../../t/multi/open_multi.c:52 unifyfs_mount at /var/tmp/tmp.LGlmjSXd6B/mount (rc=0)
PASS: 1000-multi-open.t 2 - ../../t/multi/open_multi.c:91 1 of 4 ranks opened file /var/tmp/tmp.LGlmjSXd6B/mount/lfV9uIOrbXmwd8lFz7N_AOB59hGpeR+I5

---

Lastly, you'll notice I commented out the the code in mpi_sum() and just have it returning 1 to make my second test above pass.
The program segfaults on me when it hits that first mpi_sum() call, but I didn't look into it beyond that aside from that function being the cause. Was just trying to get it to work with the test framework first.

I can push that commit to this PR if we want to go this direction. Otherwise, if you grab my commit and want to add/adjust things, to test locally:

# get a single node allocation
# after you build
cd build/t
make check TESTS='0001-setup.t 1000-multi-open.t 9010-stop-unifyfsd.t 9999-cleanup.t'

# full log for the test will then be in build/t/1000-multi-open.log

# make adjustments to `open_multi.c`
make clean
make check TESTS='0001-setup.t 1000-multi-open.t 9010-stop-unifyfsd.t 9999-cleanup.t'

[adammoody]

Wow, thanks for all of that work, @CamStan ! The direction you're taking things sounds good to me. In a lot of cases, I think we could get by with an collective all_ok() function that includes an allreduce to check that all ranks are happy and then have just rank 0 call the TAP ok() with a single test result. We could have any failing rank print a message or something. Since this might take more work, we could carve this multi_open test case into its own PR. We can chat about it on our next call.

[MichaelBrim]

Looks OK to me.

[adammoody]

Thanks for reviewing, @MichaelBrim