Suspended refs inode #419

GBuella · 2017-10-10T08:54:12Z

This change is

codecov · 2017-10-10T09:13:02Z

Codecov Report

Merging #419 into master will decrease coverage by 0.37%.
The diff coverage is 66.28%.

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
- Coverage   79.46%   79.08%   -0.38%     
==========================================
  Files          71       71              
  Lines        8749     8861     +112     
  Branches     1769     1799      +30     
==========================================
+ Hits         6952     7008      +56     
- Misses       1349     1380      +31     
- Partials      448      473      +25

Flag	Coverage Δ
#ltp_tests	`44.82% <12%> (-0.69%)`	⬇️
#sqlite_tests	`45% <13.14%> (-0.67%)`	⬇️
#tests_antool	`14.97% <0%> (-0.2%)`	⬇️
#tests_posix_multi_threaded	`25.16% <11.42%> (-0.47%)`	⬇️
#tests_posix_single_threaded	`58.17% <61.71%> (+0.67%)`	⬆️
#tests_preload	`45.16% <61.71%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
src/libpmemfile-posix/write.c	`87.12% <ø> (+0.04%)`	⬆️
src/libpmemfile/libpmemfile-posix-wrappers.h	`68.53% <ø> (ø)`	⬆️
src/libpmemfile-posix/read.c	`93.93% <ø> (-0.51%)`	⬇️
src/libpmemfile-posix/inode.c	`87.8% <ø> (-1.27%)`	⬇️
src/libpmemfile-posix/inode.h	`100% <100%> (ø)`	⬆️
src/libpmemfile-posix/pool.c	`60.72% <55.88%> (-2.41%)`	⬇️
src/libpmemfile-posix/unlink.c	`89.91% <74.41%> (-8.81%)`	⬇️
src/libpmemfile/preload.c	`51.53% <77.77%> (+0.26%)`	⬆️
src/libpmemfile-posix/data.c	`94.87% <94.73%> (-0.02%)`	⬇️
src/libpmemfile-posix/inode_array.c	`75% <0%> (-4.77%)`	⬇️
... and 5 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 0aa79fa...c694c15. Read the comment docs.

marcinslusarz · 2017-10-10T09:46:09Z

src/libpmemfile-posix/inode.c

-
-		if (vinode->orphaned.arr)
-			vinode->orphaned.arr =
-					add_off(vinode->orphaned.arr, diff);


Removal of this seems to be accidental.

marcinslusarz · 2017-10-10T09:50:56Z

src/libpmemfile-posix/layout.h

@@ -294,9 +303,6 @@ struct pmemfile_super {
 	/* list of arrays of inodes that were deleted, but are still opened */
 	TOID(struct pmemfile_inode_array) orphaned_inodes;

-	/* list of arrays of inodes that are suspended */
-	TOID(struct pmemfile_inode_array) suspended_inodes;
-


Please bump file system version.

Oops, forgot about that.

marcinslusarz · 2017-10-10T15:47:35Z

Reviewed 13 of 18 files at r1.
Review status: 13 of 18 files reviewed at latest revision, 10 unresolved discussions.

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

	return (size_t)snprintf(buf, buf_size,
			"0x%016" PRIx64 ":0x%016" PRIx64 "\n", raw[0], raw[1]);

Just use tinode->oid.pool_uuid_lo and tinode->oid.off. There's no need for memcpy.

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

		if (file == NULL)
			return -1;
		off = (uintptr_t)file->vinode->inode - (uintptr_t)pfp->pop;

file->vinode->inode.oid.off

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

		if (off != pfp->suspense)
			return -1;

Please set errno.

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

		pmemfile_close(pfp, file_at);

	int oerrno = errno;

Please move this line to before "if (file_at != NULL)"

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

	if (!file->vinode->blocks)
		error = vinode_rebuild_block_tree(pfp, file->vinode);
	os_rwlock_unlock(&file->vinode->rwlock);

Can we do it just before the transaction and hold a lock for the duration of transaction?

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

		goto err;

	pfp->suspense = (uintptr_t)file->vinode->inode - (uintptr_t)pfp->pop;

file->vinode->inode.oid.off

... or actually store file->vinode->inode.
... and rename "suspense" to "suspend_inode"

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

		if (error)
			goto end_vinode;
		os_rwlock_wrlock(&pfp->super_rwlock);

This is racy. Take the lock before looking at vinode->blocks.

tests/posix/suspend_resume/suspend_resume.cpp, line 73 at r1 (raw file):

	errno = 0;
	r = pmemfile_pool_suspend(pfp, 1, paths, 1);
	ASSERT_NE(r, 0);

Please use ASSERT_EQ.

tests/posix/suspend_resume/suspend_resume.cpp, line 126 at r1 (raw file):

	r = pmemfile_read(pfp2, dummy1, buf, sizeof(buf));
	ASSERT_GE(r, 16) << strerror(errno);

We know how many bytes are going to be read (because we pad those values with zeroes), so why not use ASSERT_EQ?

tests/posix/suspend_resume/suspend_resume.cpp, line 134 at r1 (raw file):

	ASSERT_NE(strchr(strchr(buf, '\n') + 1, '\n'), nullptr);
	ASSERT_TRUE(contains_two_ints(strchr(buf, '\n') + 1));

Please verify there are no other lines.

Comments from Reviewable

They are not only available inside read.c or write.c anymore.

GBuella · 2017-10-26T15:54:36Z

Review status: 4 of 18 files reviewed at latest revision, 10 unresolved discussions.

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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Please set errno.

Done.

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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Please move this line to before "if (file_at != NULL)"

Done.

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

Previously, marcinslusarz (Marcin Ślusarz) wrote…

This is racy. Take the lock before looking at vinode->blocks.

Done.

tests/posix/suspend_resume/suspend_resume.cpp, line 73 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Please use ASSERT_EQ.

Done.

Comments from Reviewable

pmem deleted a comment from codecov bot Oct 10, 2017

marcinslusarz reviewed Oct 10, 2017

View reviewed changes

GBuella force-pushed the suspended_refs_inode2 branch from d685acc to dfc8705 Compare October 10, 2017 11:49

GBuella added 3 commits October 26, 2017 17:42

posix: relocate vinode_read and vinode_write

a77df9d

They are not only available inside read.c or write.c anymore.

posix: special inode: suspended refs

08c7adf

tests: suspend_resume

c694c15

GBuella force-pushed the suspended_refs_inode2 branch from dfc8705 to c694c15 Compare October 26, 2017 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suspended refs inode #419

Suspended refs inode #419

GBuella commented Oct 10, 2017 •

edited by krzycz

codecov bot commented Oct 10, 2017 •

edited

marcinslusarz Oct 10, 2017

GBuella Oct 10, 2017

marcinslusarz Oct 10, 2017

GBuella Oct 10, 2017

GBuella Oct 10, 2017

marcinslusarz commented Oct 10, 2017

GBuella commented Oct 26, 2017

Suspended refs inode #419

Are you sure you want to change the base?

Suspended refs inode #419

Conversation

GBuella commented Oct 10, 2017 • edited by krzycz

codecov bot commented Oct 10, 2017 • edited

Codecov Report

marcinslusarz Oct 10, 2017

Choose a reason for hiding this comment

GBuella Oct 10, 2017

Choose a reason for hiding this comment

marcinslusarz Oct 10, 2017

Choose a reason for hiding this comment

GBuella Oct 10, 2017

Choose a reason for hiding this comment

GBuella Oct 10, 2017

Choose a reason for hiding this comment

marcinslusarz commented Oct 10, 2017

GBuella commented Oct 26, 2017

GBuella commented Oct 10, 2017 •

edited by krzycz

codecov bot commented Oct 10, 2017 •

edited