Suspended refs inode

[GBuella]

This change is 

[codecov[bot]]

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%)	:arrow_down:
#sqlite_tests	45% <13.14%> (-0.67%)	:arrow_down:
#tests_antool	14.97% <0%> (-0.2%)	:arrow_down:
#tests_posix_multi_threaded	25.16% <11.42%> (-0.47%)	:arrow_down:
#tests_posix_single_threaded	58.17% <61.71%> (+0.67%)	:arrow_up:
#tests_preload	45.16% <61.71%> (+0.02%)	:arrow_up:

Impacted Files	Coverage Δ
src/libpmemfile-posix/write.c	87.12% <ø> (+0.04%)	:arrow_up:
src/libpmemfile/libpmemfile-posix-wrappers.h	68.53% <ø> (ø)	:arrow_up:
src/libpmemfile-posix/read.c	93.93% <ø> (-0.51%)	:arrow_down:
src/libpmemfile-posix/inode.c	87.8% <ø> (-1.27%)	:arrow_down:
src/libpmemfile-posix/inode.h	100% <100%> (ø)	:arrow_up:
src/libpmemfile-posix/pool.c	60.72% <55.88%> (-2.41%)	:arrow_down:
src/libpmemfile-posix/unlink.c	89.91% <74.41%> (-8.81%)	:arrow_down:
src/libpmemfile/preload.c	51.53% <77.77%> (+0.26%)	:arrow_up:
src/libpmemfile-posix/data.c	94.87% <94.73%> (-0.02%)	:arrow_down:
src/libpmemfile-posix/inode_array.c	75% <0%> (-4.77%)	:arrow_down:
... 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]

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