Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add shared file testing to test_api #487

Merged
merged 13 commits into from
Mar 4, 2022

Conversation

mcfadden8
Copy link
Collaborator

A new option (--shared-file) was added to test_api that will cause all of the participating ranks to share the same file with each process using a unique section of the file for its I/O operations.

A new option (--shared-file) was added to test_api that will cause all
of the participating ranks to share the same file with each process
using a unique section of the file for its I/O operations.
@mcfadden8 mcfadden8 requested a review from adammoody March 3, 2022 16:44
@adammoody

This comment was marked as resolved.

examples/test_api.c Outdated Show resolved Hide resolved
examples/test_common.c Outdated Show resolved Hide resolved
examples/test_api.c Outdated Show resolved Hide resolved
examples/test_api.c Outdated Show resolved Hide resolved
examples/test_api.c Outdated Show resolved Hide resolved
@mcfadden8
Copy link
Collaborator Author

@adammoody - I have marked the comments as resolved. I attempted to simplify file creation logic at the cost of an MPI_Bcast call. Let me know if you think that is ok

@adammoody
Copy link
Contributor

@adammoody - I have marked the comments as resolved. I attempted to simplify file creation logic at the cost of an MPI_Bcast call. Let me know if you think that is ok

Hmm, that's shorter code, but I think it's a bit hard to follow.

Let's keep with using the separate code paths as it's easier to understand. But we could simplify this section of the code by defining a new "create_file" function. That would do the magic to create/open the file (either shared or file per process) and return an opened file descriptor that is ready for writing. This part would thenreduce to something like:

int fd_me = create_file(fname);

while we move your previous code block into the create file function, so something like:

int create_file(fname) {
  if (shared_file) {
    if (rank == 0) {
      open()
      truncate()
      close()
    }
    barrier() or bcast() to wait on rank 0
    return open(fname, ...)
  } else {
    return open(fname, ...)
  }
}

@mcfadden8
Copy link
Collaborator Author

Okay take 3. I like the structure proposed, but it does expand for error handling. Thoughts?

@adammoody
Copy link
Contributor

Looks good. Thanks!


MPI_Bcast(&fd, 1, MPI_INT, 0, MPI_COMM_WORLD); /* Wait for rank 0 to complete creation */

if (fd > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, wait. I think fd==0 also means success, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and if that's true, I have that bug all throughout the test_*.c code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had noticed that too. But it is rare that stdin, stdout, and/or stderr would be closed. But I suppose it could happen. I'll clean this up

@adammoody
Copy link
Contributor

Thanks, @mcfadden8 . This looks good to me. Ready to be merged from your view?

@mcfadden8
Copy link
Collaborator Author

This looks ready to me. I'm finished with my changes.

@adammoody adammoody merged commit 0da3a61 into develop Mar 4, 2022
@adammoody adammoody deleted the feature/single_shared_file_test branch March 4, 2022 18:43
@adammoody
Copy link
Contributor

Thanks @mcfadden8 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants