Suspended refs inode

[GBuella]

This change is 

[codecov]

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]

Removal of this seems to be accidental.

[GBuella]

Done.

[marcinslusarz]

Please bump file system version.

[GBuella]

Oops, forgot about that.

[GBuella]

Done.

[marcinslusarz]

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

[GBuella]

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