fix: avoid EEXIST error and truncate if needed when opening an existing file

adammoody commented 1 year ago

Description

This updates unifyfs_fid_open() to handle O_EXCL and EEXIST. Checks from posix wrappers are removed. It splits unifyfs_fid_open() into parts.

a new unifyfs_gfid_create() call is responsible for creating the gfid entry on the server. This does not modify the local fid cache at all.
a new unifyfs_fid_fetch() call retrieves file meta data for the corresponding gfid from the server. It allocates an entry in the fid cache, or if one already exists, it updates the existing cache entry.

The client API library has been updated to call these new functions. unifyfs_create() calls unifyfs_gfid_create() to create the gfid entry, and then it calls unifyfs_open(). unifyfs_open() calls unifyfs_fid_fetch() to allocate an entry in the fid cache and populate it with data about the gfid from the server.

A new UNIFYFS_CLIENT_EXCL_PRIVATE config option is added to toggle whether O_EXCL implies a private file. By default, O_EXCL creates private files. One can create shared files using O_EXCL by setting UNIFYFS_CLIENT_EXCL_PRIVATE=0.

Motivation and Context

When passed the O_CREAT flag, an open call on an existing file should succeed as long as O_EXCL is not also given. We return an EEXIST error from unifyfs_fid_open when the file already exists. In our open and creat wrappers, this is then handled as a special case where we do a second call to unifyfs_fid_open after masking out the O_CREAT flag.

https://github.com/LLNL/UnifyFS/blob/ca3fa9e67efcb32eddc8380161f74fdc6595697d/client/src/unifyfs-sysio.c#L1200-L1209

However, that special case logic is missing from our fopen wrapper, which fails with an error when calling fopen(name, "w") on an existing file. This means that the following case returns an EEXIST error when it should succeed:

// create a file
FILE* fp = fopen(file, "w");
fclose(fp);

// fopen an existing file
// this was incorrectly returning an EEXIST error
fp = fopen(file, "w");

This refactors unifyfs_fid_open to simplify checking when opening laminated files, truncating files, and allocating a new local fid structure when needed.

How Has This Been Tested?

Types of changes

[x] 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)
[x] Testing (addition of new tests or update to current tests)
[x] 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 commented 1 year ago

This PR changes unifyfs_fid_open to be more like POSIX open to that it's successful on existing files when only given O_CREAT, while it fails with EEXIST when given O_CREAT | O_EXCL.

@MichaelBrim , the PR is failing on a test of the client API here:

https://github.com/LLNL/UnifyFS/blob/04fee67d7a2a4111809dd231e19e2c10a5cc0ff0/t/api/create-open-remove.c#L47-L49

I see our implementation only sets the O_CREAT flag.

https://github.com/LLNL/UnifyFS/blob/04fee67d7a2a4111809dd231e19e2c10a5cc0ff0/client/src/unifyfs_api_file.c#L86-L88

We could change this to O_CREAT | O_EXCL. Another option might be to leave this as O_CREAT but modify the test to expect success. What do you think?

MichaelBrim commented 1 year ago

We could change this to O_CREAT | O_EXCL. Another option might be to leave this as O_CREAT but modify the test to expect success. What do you think?

The semantics of unifyfs_create() match that of open(O_CREAT | O_EXCL), so my suggestion is to change the code in unifyfs_create() to pass O_EXCL as well when calling unifyfs_fid_open().

adammoody commented 1 year ago

@MichaelBrim , I added O_EXCL. But now I think it's tripping a test on our "private file" semantic which is triggered with O_EXCL:

https://github.com/LLNL/UnifyFS/blob/04fee67d7a2a4111809dd231e19e2c10a5cc0ff0/t/api/gfid-metadata.c#L178-L179

We indicate that the exclusive bit was set in the open flags and pass that to unifyfs_fid_create.

https://github.com/LLNL/UnifyFS/blob/04fee67d7a2a4111809dd231e19e2c10a5cc0ff0/client/src/unifyfs_fid.c#L413

https://github.com/LLNL/UnifyFS/blob/04fee67d7a2a4111809dd231e19e2c10a5cc0ff0/client/src/unifyfs_fid.c#L443

and then in unifyfs_fid_create, we use that last parameter to mark the file as private (not shared).

https://github.com/LLNL/UnifyFS/blob/04fee67d7a2a4111809dd231e19e2c10a5cc0ff0/client/src/unifyfs_fid.c#L195-L197

https://github.com/LLNL/UnifyFS/blob/04fee67d7a2a4111809dd231e19e2c10a5cc0ff0/client/src/unifyfs_fid.c#L235

So we currently have two meanings for O_EXCL:

mark file as private (not shared)
throw EEXIST if file already exists on O_CREAT

I guess we'd need to either pick a different bit flag to indicate private files or I could revert back to handling the O_EXCL logic up in the sysio/stdio wrappers. What do you think?

MichaelBrim commented 1 year ago

I'm a bit torn on this dilemma. I think it's useful to maintain the idea of a private (non-shared) file, although most of our usefulness to current applications comes from shared files. Using O_EXCL to mean private was always a hack, but I still can't find a better standard flag to substitute for it. If we want to maintain the private notion, the exclusive-create logic would need to live in the POSIX wrappers layer. We could funnel all open() or fopen() calls that should create files through posix_create() and handle exclusive-create there.

adammoody commented 1 year ago

@MichaelBrim , I've got this about halfway done now. As it stands unifyfs_fid_open still does a lot of posix-like processing based on the flags it's given. Since it's pretty posix-specific, it may be better to move this up to the posix wrappers like you suggested. Maybe we could come up with a unifyfs_fid_open that is free from any O_* flags.

To avoid breaking things, I have unifyfs_api call the old unifyfs_fid_open implementation, which I renamed to unifyfs_fid_open2. I also dropped the fid and pos params from that function since the API didn't use them.

Which O_* flags do we want to expose to users in unifyfs_open()?

The unifyfs_fid_open2 implementation currently processes O_DIRECTORY, O_TRUNC, and O_APPEND. Do we want to keep those?

Should we throw an error if a user passes O_CREAT and/or O_EXCL in their call to unifyfs_open() or just ignore them?

MichaelBrim commented 1 year ago

Since the library API assumes stateless file access,O_APPEND has no useful meaning. O_TRUNC is accomplished via an explicit UNIFYFS_IOREQ_OP_TRUNC operation. We don't have directories in the library API yet, so O_DIRECTORY isn't useful either.

I think my conclusion is the only O_XXX flags we should allow in unifyfs_open() are the read/write access ones. It does make sense to check for the other POSIX file-create related flags and throw an error if somebody passes those.

Also, we should define our own flag values for use in unifyfs_create() for all the per-file behaviors we want. But that's a job for me independent of this PR.

adammoody commented 1 year ago

@MichaelBrim , this is ready for another look when you have a chance. The main changes since you last looked at it are in the client API library.

I updated the API calls to use the new gfid_create and fid_fetch calls. I added a check so that unifyfs_open only accepts O_RDONLY/WRONLY/RDWR flags, and there's one test case added to check for O_TRUNC as an example. I dropped the old unifyfs_fid_open() call.

I think it's good to go. Let me know if you have suggestions.

adammoody commented 1 year ago

Thanks, @MichaelBrim !

LLNL / UnifyFS