pmem / pmemfile

Userspace implementation of file APIs using persistent memory.
Other
34 stars 21 forks source link

posix: Multi root support #388

Closed GBuella closed 6 years ago

GBuella commented 6 years ago

This change is Reviewable

krzycz commented 6 years ago

Reviewed 21 of 21 files at r1. Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


src/libpmemfile-posix/pool.c, line 56 at r1 (raw file):

#include "utils.h"

COMPILE_ERROR_ON(PMEMFILE_ROOT_COUNT == 0);

<= 0 ? ;-)


Comments from Reviewable

GBuella commented 6 years ago

Oh, I just "love" our clang-format configuration:

ASSERT_EQ(pmemfile_fstatat(pfp, other_root, "same_name",
                           &other_stat, 0),
          0);
codecov[bot] commented 6 years ago

Codecov Report

Merging #388 into master will increase coverage by 0.04%. The diff coverage is 87.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   77.01%   77.05%   +0.04%     
==========================================
  Files          67       67              
  Lines        8330     8345      +15     
  Branches     1678     1684       +6     
==========================================
+ Hits         6415     6430      +15     
  Misses       1497     1497              
  Partials      418      418
Flag Coverage Δ
#ltp_tests 46.42% <51.28%> (-0.01%) :arrow_down:
#sqlite_tests 45.13% <51.28%> (-0.02%) :arrow_down:
#tests_antool 15.22% <0%> (-0.03%) :arrow_down:
#tests_posix_multi_threaded 25.04% <46.15%> (-0.07%) :arrow_down:
#tests_posix_single_threaded 54.28% <87.17%> (+0.06%) :arrow_up:
#tests_preload 44.77% <51.28%> (-0.21%) :arrow_down:
Impacted Files Coverage Δ
src/libpmemfile/libpmemfile-posix-wrappers.h 68.6% <ø> (ø) :arrow_up:
src/libpmemfile-posix/inode.c 88.65% <100%> (ø) :arrow_up:
src/libpmemfile-posix/inode.h 100% <100%> (ø) :arrow_up:
src/libpmemfile-posix/file.c 75.06% <100%> (+0.34%) :arrow_up:
src/libpmemfile-posix/truncate.c 90.26% <100%> (ø) :arrow_up:
src/libpmemfile-posix/dir.c 85.01% <100%> (-0.25%) :arrow_down:
src/libpmemfile-posix/rename.c 91.26% <100%> (ø) :arrow_up:
src/libpmemfile-posix/rmdir.c 97.02% <100%> (ø) :arrow_up:
src/libpmemfile-posix/pool.c 63.13% <77.27%> (+1.55%) :arrow_up:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5befca3...960fa9a. Read the comment docs.

GBuella commented 6 years ago

Review status: 11 of 22 files reviewed at latest revision, 8 unresolved discussions.


doc/libpmemfile-posix.3.md, line 343 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…
Indeed!

Done.


include/libpmemfile-posix.h, line 208 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…
Left it there by accident.

Done.


src/libpmemfile-posix/layout.h, line 267 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…
Please put it before padding.

Done.


src/libpmemfile-posix/pool.c, line 56 at r1 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…
<= 0 ? ;-)

Done.


tests/posix/pmemfile_test.hpp, line 200 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…
too many empty lines

Done.


tests/posix/basic/basic.cpp, line 138 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…
Good idea

Done.


Comments from Reviewable

marcinslusarz commented 6 years ago

Reviewed 9 of 21 files at r1, 11 of 11 files at r2. Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/libpmemfile-posix/inode.c, line 235 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…
As it is now, the `vinode_is_root` function first checks if it is a directory (if it is not a dir, it can't be root), and for that, it uses vinode->inode->flags or whatnot. Which is a problem if vinode->inode is free'd already -- valgrind complains.

Ah, right. Please add comment explaining that.


src/libpmemfile-posix/mkdir.c, line 114 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…
I wasn't sure if this is meant to assert that the parent is the default root (the path "/"), or any root. There was no difference before...

I think you could add test for that. mkdir on root should return EEXIST independent of which root it's called on.


Comments from Reviewable

marcinslusarz commented 6 years ago

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

GBuella commented 6 years ago

Review status: 22 of 23 files reviewed at latest revision, 3 unresolved discussions.


src/libpmemfile-posix/inode.c, line 235 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…
Ah, right. Please add comment explaining that.

Done.


src/libpmemfile-posix/mkdir.c, line 114 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…
I think you could add test for that. mkdir on root should return EEXIST independent of which root it's called on.

Is that related to this PR? I mean to root directories? I could add something like

ASSERT_NE(pmemfile_mkdirat(pfp, root, "/", 0700), 0);
EXPECT_EQ(errno, EEXISTS);

But that should hold for any dir anyways, not just for root directories, right? You can't mkdir "/" at any directory.


Comments from Reviewable

GBuella commented 6 years ago

Review status: 21 of 23 files reviewed at latest revision, 2 unresolved discussions.


src/libpmemfile-posix/mkdir.c, line 114 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…
Is that related to this PR? I mean to root directories? I could add something like ``` ASSERT_NE(pmemfile_mkdirat(pfp, root, "/", 0700), 0); EXPECT_EQ(errno, EEXISTS); ``` But that should hold for any dir anyways, not just for root directories, right? You can't mkdir "/" at any directory.

Got it now!


Comments from Reviewable

GBuella commented 6 years ago

Review status: 21 of 23 files reviewed at latest revision, 2 unresolved discussions.


src/libpmemfile-posix/mkdir.c, line 114 at r1 (raw file):

Previously, GBuella (Gabor Buella) wrote…
Got it now!

Well, I realized, the check needed in relation to multiple roots about absolute paths, e.g. open(pfp, root, "/dummy") should open dummy at the default root, regardless of the dir supplied to it. So, mkdirat(pfp, root, "/", mode) should ignore the second argument, return EEXIST because the default root exists already.


Comments from Reviewable

GBuella commented 6 years ago

Wrzeszcztool coverage decreased by 0.15 percent? Do we have an explanation for that?

marcinslusarz commented 6 years ago

It seems CodeCov is buggy and compares coverage between master and PR instead of master and merge-of(master, PR) (which is what is tested on Travis). If there are coverage improvements on master which you don't have in your PR, then it will show decreased coverage. I guess I can report it as a bug. If you want to "fix" it right now you can rebase your branch.

krzycz commented 6 years ago

Reviewed 10 of 11 files at r2, 1 of 1 files at r3, 3 of 3 files at r4. Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

sarahjelinek commented 6 years ago

Reviewed 1 of 21 files at r1. Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/libpmemfile-posix/layout.h, line 252 at r4 (raw file):

 * constant. But the client is required to get this by calling
 * pmemfile_root_count(), therefore it can be a dynamic number later in later
 * implementations.

s/can be a dynamic number later in later implementations/can be a dynamic value in future implementations.


Comments from Reviewable

GBuella commented 6 years ago

Review status: 19 of 22 files reviewed at latest revision, 2 unresolved discussions.


src/libpmemfile-posix/layout.h, line 252 at r4 (raw file):

Previously, sarahjelinek (Sarah Jelinek) wrote…
s/can be a dynamic number later in later implementations/can be a dynamic value in future implementations.

Done.


Comments from Reviewable

marcinslusarz commented 6 years ago
:lgtm:

Reviewed 2 of 3 files at r4, 3 of 3 files at r5. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

GBuella commented 6 years ago

While merging be aware: this bumps the version number, so does #393 One of these two will need to change it again to 0.4

krzycz commented 6 years ago

Reviewed 3 of 3 files at r5. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

sarahjelinek commented 6 years ago

Reviewed 1 of 3 files at r5. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

sarahjelinek commented 6 years ago
:lgtm:

Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable