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

Feature sync #175

Closed
wants to merge 2 commits into from
Closed

Feature sync #175

wants to merge 2 commits into from

Conversation

JulianKunkel
Copy link
Collaborator

No description provided.

if (barriers) {
MPI_Barrier(testComm);
}
phase_end();
t[1] = GetTimeStamp();

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should phase_end() return GetTimeStamp()? Then you could combine these two lines with:
t[1] = phase_end();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could do that... Personally I find it more verbose to have GetTimeStamp() explicitly here.

@@ -809,3 +809,10 @@ char *HumanReadable(IOR_offset_t value, int base)
}
return valueStr;
}

void call_sync_cmd(){
int ret = system("sync");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the backends should have a sync routine. Isn't it better to call that than to do a system wide sync?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Generally, a good suggestion. Not quite sure if and how any other backend would implement this.
I'd be happy if we proceed for now with MDTest and see if anyone else has another thought on the usefulness.

Copy link

Choose a reason for hiding this comment

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

I would vote for a sync routine (or routines? I haven't looked at the code) for backends. The default provided by the benchmark can print a warning along the lines of "Warning: mdtest configuration requested a sync, but backend XYZ does not implement that operation." so that benchmark users get some indication that they asked for something that the storage system doesn't know how to do. Backends that do support it can populate their own function pointer for that. Backends that already implicitly sync everything without being asked can provide a noop function pointer to hush the warning.

Calling the sync command line utility might produce some odd (or at least inconsistent) results since you don't know what file systems or directories might be affected. You could get a slow benchmark result just because the OS happened to have some changes to flush to a slow local SATA drive.

@JulianKunkel JulianKunkel mentioned this pull request Sep 1, 2019
@JulianKunkel JulianKunkel deleted the feature-sync branch May 30, 2020 17:32
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.

3 participants