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

fstime.c - Seperate r/w files for each parallel #85

Merged
merged 2 commits into from
Feb 27, 2023

Conversation

pdeng6
Copy link
Contributor

@pdeng6 pdeng6 commented Feb 16, 2023

Existing workload is using 1 read file and 1 write file for file read/write/copy test. In multi-parallel scenario, it leads to high file lock contention, while read/write/copy is not stressed.

This change seperates r/w files for each parallel to satisfy the multi-parallel test purpose.

Existing workload is using 1 read file and 1 write file for file
read/write/copy test. In multi-parallel scenario, it leads to
high file lock contention, while read/write/copy is not stressed.

This change seperates r/w files for each parallel to satisfy the
multi-parallel test purpose.
@pdeng6
Copy link
Contributor Author

pdeng6 commented Feb 17, 2023

Test on my 160 core machine:

Without the change, 160P score is even lower than 1P.

  1P 160P 160P/1P
File Copy 1024 bufsize 2000 maxblocks 4,151 2,477 59.68%
File Copy 256 bufsize 500 maxblocks 2,816 1,554 55.16%
File Copy 4096 bufsize 8000 maxblocks 7,746 5,265 67.96%

With the change, 160P score is ~26X - 79X of 1P:

  1P 160P 160P/1P
File Copy 1024 bufsize 2000 maxblocks 4,311 249,846 57.95
File Copy 256 bufsize 500 maxblocks 2,817 222,938 79.14
File Copy 4096 bufsize 8000 maxblocks 8,224 213,497 25.96

@gstrauss
Copy link
Collaborator

PATH_MAX is a bit more portable on unix than is MAX_PATH (hi Windoze), but we do not need it. See below.

Let's reduce the number of lines changed in the patch:

-#define FNAME0  "dummy0"
-#define FNAME1  "dummy1"
+char FNAME0[] = "dummy0-XXXXXXXXXX";
+char FNAME1[] = "dummy1-XXXXXXXXXX";

later:

snprintf(FNAME0+sizeof("dummy0"), sizeof(FNAME0)-sizeof("dummy0"), "%d", pid);
snprintf(FNAME1+sizeof("dummy1"), sizeof(FNAME1)-sizeof("dummy1"), "%d", pid);

Note: there may still be some filesystem contention on the directory file since these are all created in the same directory.


meta: benchmarking, like statistics, can be manipulated. Comparing numbers generated from different code can be less accurate and even misleading. I made similar comments many years ago here:
#23 (comment)

That said, where the implementation is obviously detrimental to what the code is trying to exercise and test, I am happy to evaluate proposed patches.

@pdeng6
Copy link
Contributor Author

pdeng6 commented Feb 27, 2023

Thanks @gstrauss , quite agree with the comments.
For the concern of remained contention, according to my profiling data on both ext4 and xfs, with this change, it is not obvious anymore, maybe we can address it in future if detected anywhere.

Cheers
Pan

@gstrauss gstrauss merged commit 81e9de5 into kdlucas:master Feb 27, 2023
@gstrauss
Copy link
Collaborator

Thank you for your contribution.

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