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

HDF5 file access rights #108

Merged
merged 1 commit into from
Jan 7, 2025
Merged

HDF5 file access rights #108

merged 1 commit into from
Jan 7, 2025

Conversation

7schroet
Copy link

@7schroet 7schroet commented Jan 7, 2025

Some of the HDF5-related functions request more rights than necessary in sections that only read the state. This PR adjusts the rights in the corresponding H5Fopen function calls.

While it is not fundamentally wrong to request RDWR access here, HDF5 applies a file lock in this case, which can lead to errors if multiple of the cloudsc prototypes are run simultaneously, for example in a benchmark setting. The lock-induced errors look like this:

[...]
HDF5-DIAG: Error detected in HDF5 (1.14.3) thread 0:
  #000: [...]/src/H5F.c line 836 in H5Fopen(): unable to synchronously open file
    major: File accessibility
    minor: Unable to open file
[...]

Note that file locking can also be disabled altogether by setting the environment variable HDF5_USE_FILE_LOCKING=FALSE, but users might not be aware of this option.

The changes were tested locally, without any trouble.

@FussyDuck
Copy link

FussyDuck commented Jan 7, 2025

CLA assistant check
All committers have signed the CLA.

@reuterbal reuterbal requested a review from MichaelSt98 January 7, 2025 10:26
@MichaelSt98
Copy link
Contributor

Thanks for this contribution!
I have no objections and it makes sense to open in RDONLY mode.

@7schroet could you please sign the CLA?
Once the CLA is signed and the checks have passed I'll approve and we can merge this.

Copy link
Contributor

@MichaelSt98 MichaelSt98 left a comment

Choose a reason for hiding this comment

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

Clean addition/fix.
Ready to be merged once CLA is signed.

@7schroet
Copy link
Author

7schroet commented Jan 7, 2025

Thanks for the quick feedback, the CLA is now signed!

Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Many thanks for this!

@MichaelSt98 MichaelSt98 merged commit d82a892 into ecmwf-ifs:develop Jan 7, 2025
32 checks passed
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.

4 participants