-
Notifications
You must be signed in to change notification settings - Fork 510
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
MAP_SYNC support #2594
MAP_SYNC support #2594
Conversation
PMEM_DEV_DAX, /* device dax */ | ||
PMEM_MAP_SYNC, /* mapping with MAP_SYNC flag on dax fs */ | ||
|
||
MAX_PMEM_TYPE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comment here or change the name. Why MAX_*?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are at least two reasons:
1.having a last element of an enum be a MAX_ of something allows you to declare an array with enough space to fit all elements.
2. Every "limit" we declare internally is prefixed with MAX_ so that whenever we need to create an array, iterate over all enum types etc, we can just type MAX_ and let the automatic code completion show us all available options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Reviewed 8 of 30 files at r1. Comments from Reviewable |
src/common/os_deep_persist_linux.c
Outdated
|
||
return 0; | ||
} | ||
case PMEM_MAP_SYNC: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either use braces everywhere or nowhere. This looks particularly jarring because you have 'return ...;' at different indents and at first glance it might look like you are missing a break;
src/common/set.c
Outdated
(os_off_t)offset); | ||
} | ||
|
||
return util_map_sync(addr, len, prot, flags, part->fd, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens when you call mmap /w MAP_SYNC on devdax?
return util_map_sync(addr, len, prot, flags, part->fd, (os_off_t)offset, part->is_dev_dax ? NULL : map_sync);
src/common/set.c
Outdated
for (unsigned p = 1; p < rep->nparts; p++) { | ||
if (map_sync == rep->part[p].map_sync) | ||
continue; | ||
if (rep->part[p].map_sync) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relying on the previous continue;
is confusing.
if (rep->part[p].map_sync != rep->part[p-1].map_sync) {
ERR("replica ... %s mapped with MAP_SYNC ", ..., rep->part[p].map_sync ? "" : "not";);
}
same below
@@ -2213,7 +2272,7 @@ util_header_create(struct pool_set *set, unsigned repidx, unsigned partidx, | |||
1, POOL_HDR_CSUM_END_OFF); | |||
|
|||
/* store pool's header */ | |||
util_persist_auto(rep->part[partidx].is_dev_dax, hdrp, sizeof(*hdrp)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util_persist(is_pmem, ...);
the "auto" function when far away from my original intention...
Reviewed 25 of 30 files at r1. src/common/mmap.c, line 187 at r1 (raw file):
+TAB src/common/mmap_posix.c, line 211 at r1 (raw file):
Might be a good idea to put some debug traces here (i.e. with level 4), so you can easily see in the log if this function mapped with MAP_SYNC or not, and why. src/common/mmap_windows.c, line 145 at r1 (raw file):
I think we could refactor the code somehow and check if the file pointed by "fd" is on a dax-enabled filesystem. On Windows, this is enough to treat it as "pmem" (like for MAP_SYNC). src/common/set.c, line 351 at r1 (raw file):
"memory map into memory" sounds like tautology src/common/set.c, line 355 at r1 (raw file):
Is this needed? Why not to map everything with util_map_sync()? src/common/set.c, line 467 at r1 (raw file):
No check for MAP_FAILED? src/common/set.c, line 1103 at r1 (raw file):
Is it really an error? Why not to treat the entire replica as non-pmem, if any part/hdr was mapped without MAP_SYNC? src/common/set.c, line 2496 at r1 (raw file):
device src/common/set.c, line 2499 at r1 (raw file):
I don't like this one-liner function that is only used twice. "inline" maybe? src/common/set.h, line 104 at r1 (raw file):
Does it make sense to keep this info per part/hdr? I think it's enough to have it per replica. src/test/RUNTESTS, line 445 at r1 (raw file):
What about moving all the PMEM/non-PMEM detection here, instead or running pmemdetect for each and every test in require_fs_type? src/test/testconfig.sh.example, line 45 at r1 (raw file):
It somehow duplicates the sentence above. They could be merged into one, I believe. src/test/tools/pmemdetect/pmemdetect.c, line 155 at r1 (raw file):
If the file does not exists, it's the same case as testing directory with tmpfile. src/test/tools/pmemdetect/pmemdetect.c, line 163 at r1 (raw file):
why not 0 ? src/test/tools/pmemdetect/pmemdetect.c, line 233 at r1 (raw file):
Perhaps I missing something, but it appears to me that checking for MAP_SYNC is actually the same as checking for "is_pmem", isn't it? The only difference is that pmem_map_file is affected by PMEM_IS_PMEM_FORCE, but we can unset it for a moment (saving the previous value), run pmemdetect and restore env vars. src/test/unittest/unittest.ps1, line 1177 at r1 (raw file):
Does it apply to windows? src/test/unittest/unittest.ps1, line 1188 at r1 (raw file):
indent with spaces, pls - it's powershell src/tools/pmempool/info.c, line 500 at r1 (raw file):
it is set to 0 three lines below Comments from Reviewable |
Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed. src/common/set.c, line 364 at r1 (raw file): Previously, pbalcer (Piotr Balcer) wrote…
Good question. I believe it fails, as MAP_SYNC is a feature of a filesystem, not a device. Comments from Reviewable |
Review status: all files reviewed at latest revision, 22 unresolved discussions, some commit checks failed. src/common/mmap.c, line 187 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/mmap_posix.c, line 211 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/mmap_windows.c, line 145 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
I was thinking about this. There is even such function - util_fd_is_device_dax. But this is once again prepending Linux specific feature on windows and I don't like such approach. I'm aware this is not the ideal implementation but I decided to go this way because I believe it is the easiest way to implement it. src/common/os_deep_persist_linux.c, line 104 at r1 (raw file): Previously, pbalcer (Piotr Balcer) wrote…
Done. src/common/set.c, line 351 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/set.c, line 355 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/set.c, line 364 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/set.c, line 467 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/set.c, line 1102 at r1 (raw file): Previously, pbalcer (Piotr Balcer) wrote…
Done. src/common/set.c, line 1103 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
We have two options. Report an error or silently fallback to non-pmem and silently degradate performance significantly. I think we discussed this offline that we should fail in such case - it works the same way right now with device dax. I think this option is better but I'm ok with changing this. src/common/set.c, line 2496 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/set.c, line 2499 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
one-line of code but multiple lines of comment. I believe it is worht keeping this in one place because it is really important line of code. src/common/set.h, line 104 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
The header is mmapped in util_map_hdr function which takes a pool_set_part structure. I found this structure convenient to keep this information - I need to store this flag somewhere. After mapping all headers we check if all values are the same. src/test/RUNTESTS, line 445 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
I don't think I understand your comment. The pmemdetect is executed only once (below). src/test/testconfig.sh.example, line 45 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/test/tools/pmemdetect/pmemdetect.c, line 155 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Yes. But for directory we set PMEM_FILE_TMPFILE. src/test/tools/pmemdetect/pmemdetect.c, line 163 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/test/tools/pmemdetect/pmemdetect.c, line 233 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
yes and no. supports_map_sync returns true only for files on FS which do support MAP_SYNC. is_pmem returns true for MAP_SYNC FSs, device dax and/or PMEM_IS_PMEM_FORCE. I found such functionality useful for testing environment so I implemented it. IMHO it is cleaner to call this function instead of playing with environment variables - this is fragile. src/test/unittest/unittest.ps1, line 1177 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/test/unittest/unittest.ps1, line 1188 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/tools/pmempool/info.c, line 500 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Mistake when resolving confict Comments from Reviewable |
9c5e331
to
e29b39b
Compare
Reviewed 4 of 28 files at r2, 24 of 24 files at r3. src/common/mmap_posix.c, line 193 at r3 (raw file):
BTW, it should be "%lld" IMHO. src/common/mmap_posix.c, line 208 at r3 (raw file):
+errno src/common/mmap_windows.c, line 145 at r1 (raw file): Previously, plebioda (Pawel Lebioda) wrote…
Let's leave it for a post-merge refactoring. src/common/os_deep_persist_linux.c, line 88 at r3 (raw file):
LOG(15, ...) src/common/os_deep_persist_linux.c, line 88 at r3 (raw file):
src/common/os_deep_persist_linux.c, line 92 at r3 (raw file):
pmem_persist() has it's own debug traces. this seems to be unnecessary src/common/set.c, line 467 at r1 (raw file): Previously, plebioda (Pawel Lebioda) wrote…
Nope. Please, revert the original check here. The logic was different. src/common/set.c, line 1103 at r1 (raw file): Previously, plebioda (Pawel Lebioda) wrote…
OK. Just remember to update documentation. src/libpmem/pmem_posix.c, line 70 at r3 (raw file):
LOG(3, ...) src/libpmem/pmem_posix.c, line 72 at r3 (raw file):
I think we could cal util_map() with the same args for any file (including devdax). src/test/RUNTESTS, line 445 at r1 (raw file): Previously, plebioda (Pawel Lebioda) wrote…
See unittest.sh. src/test/tools/pmemdetect/pmemdetect.c, line 155 at r1 (raw file): Previously, plebioda (Pawel Lebioda) wrote…
I know. But if the file does not exists, it is created and unlinked immediately after the check, so... it acts as a temporary file - only the name is different. src/test/tools/pmemdetect/pmemdetect.c, line 233 at r1 (raw file): Previously, plebioda (Pawel Lebioda) wrote…
We never use pmemdetect to check if device dax is pmem, so this is not the case. Comments from Reviewable |
e613eb3
to
7584ff5
Compare
Codecov Report
@@ Coverage Diff @@
## master #2594 +/- ##
==========================================
- Coverage 80.39% 80.32% -0.07%
==========================================
Files 146 146
Lines 22367 22442 +75
==========================================
+ Hits 17981 18027 +46
- Misses 4386 4415 +29 |
If PMEM_FS_DIR_FORCE_PMEM is set to 1 the pmem_is_pmem will not check if the mapping has been created with MAP_SYNC. This patch reports an error if this variable is set to 1 AND the PMEM_FS_DIR supports MAP_SYNC.
Reviewed 7 of 7 files at r4. Comments from Reviewable |
Do not check if PMEM_FS_DIR is pmem and NON_PMEM_FS_DIR is not pmem for evey single require_fs_type execution but once per RUNTESTS[.PS1]. This couse the pmemdetect is invoked less frequently and should positively impact overall test execution time.
Review status: 22 of 30 files reviewed at latest revision, 14 unresolved discussions. src/common/mmap_posix.c, line 193 at r3 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
src/common/mmap_posix.c, line 208 at r3 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/mmap_windows.c, line 145 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/os_deep_persist_linux.c, line 88 at r3 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/os_deep_persist_linux.c, line 88 at r3 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/os_deep_persist_linux.c, line 92 at r3 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/common/set.c, line 467 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/libpmem/pmem_posix.c, line 70 at r3 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/libpmem/pmem_posix.c, line 72 at r3 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/test/RUNTESTS, line 445 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. src/test/tools/pmemdetect/pmemdetect.c, line 155 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
So what do you propose ? Ignore the last part of path and create temporary file ? Current implementation is simple and clear - if you provide a path to a file which doesn't exist you would like to check "if I create such a file will it be a pmem or not ?" src/test/tools/pmemdetect/pmemdetect.c, line 233 at r1 (raw file): Previously, krzycz (Krzysztof Czurylo) wrote…
Done. Comments from Reviewable |
Reviewed 8 of 8 files at r5. Comments from Reviewable |
Review status: all files reviewed at latest revision, 2 unresolved discussions. Comments from Reviewable |
This change is