posix: fix harmless race between exchange and unref

[marcinslusarz]

vinode->parent was compared to vinode (as a "is_root" check) without holding vinode lock. However exchange won't ever set vinode->parent to parent, so this race is harmless. Fixing this race actually simplifies vinode_unref code.

Introduced by 3e3782e41f4f13836a033328130ff1a3b3ac8701.

==3433== Conflicting store by thread 13 at 0x06a3b3a0 size 8
==3433==    at 0x4E5A17C: vinode_exchange (rename.c:152)
==3433==    by 0x4E5AE1D: _pmemfile_renameat2 (rename.c:449)
==3433==    by 0x4E5B2E9: pmemfile_renameat2 (rename.c:559)
==3433==    by 0x4789FC: test_exchange(char const*, char const*) (mt.cpp:204)
==3433==    by 0x478A5E: test_exchange_loop(char const*, char const*) (mt.cpp:213)
==3433==    by 0x478B00: rename_worker6() (mt.cpp:261)
==3433==    by 0x485F0A: void std::_Bind_simple<void (*())()>::_M_invoke<>(std::_Index_tuple<>) (functional:1531)
==3433==    by 0x485C3D: std::_Bind_simple<void (*())()>::operator()() (functional:1520)
==3433==    by 0x48591F: std::thread::_Impl<std::_Bind_simple<void (*())()> >::_M_run() (thread:115)
==3433==    by 0x5762C7F: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==3433==    by 0x4C33CAB: vgDrd_thread_wrapper (drd_pthread_intercepts.c:367)
==3433==    by 0x54946B9: start_thread (pthread_create.c:333)
==3433== Address 0x6a3b3a0 is at offset 112 from 0x6a3b330. Allocation context:
==3433==    at 0x4C32D27: calloc (vg_replace_malloc.c:623)
==3433==    by 0x4E614EA: _pf_calloc (alloc.c:70)
==3433==    by 0x4E50E9B: inode_ref (inode.c:118)
==3433==    by 0x4E4B0C3: vinode_lookup_vinode_by_name_locked (dir.c:312)
==3433==    by 0x4E4C0C7: lock_parents_and_children (dir.c:726)
==3433==    by 0x4E5AB45: _pmemfile_renameat2 (rename.c:363)
==3433==    by 0x4E5B2E9: pmemfile_renameat2 (rename.c:559)
==3433==    by 0x4789FC: test_exchange(char const*, char const*) (mt.cpp:204)
==3433==    by 0x478A5E: test_exchange_loop(char const*, char const*) (mt.cpp:213)
==3433==    by 0x478B00: rename_worker6() (mt.cpp:261)
==3433==    by 0x485F0A: void std::_Bind_simple<void (*())()>::_M_invoke<>(std::_Index_tuple<>) (functional:1531)
==3433==    by 0x485C3D: std::_Bind_simple<void (*())()>::operator()() (functional:1520)
==3433== Other segment start (thread 7)
==3433==    at 0x4C37E74: pthread_rwlock_trywrlock_intercept (drd_pthread_intercepts.c:1242)
==3433==    by 0x4C37E74: pthread_rwlock_trywrlock$* (drd_pthread_intercepts.c:1247)
==3433==    by 0x4E5553D: os_rwlock_wrlock (os_thread_pthread.c:109)
==3433==    by 0x4E512EA: vinode_unref (inode.c:215)
==3433==    by 0x4E5AF6D: _pmemfile_renameat2 (rename.c:475)
==3433==    by 0x4E5B2E9: pmemfile_renameat2 (rename.c:559)
==3433==    by 0x4789FC: test_exchange(char const*, char const*) (mt.cpp:204)
==3433==    by 0x478A5E: test_exchange_loop(char const*, char const*) (mt.cpp:213)
==3433==    by 0x478B00: rename_worker6() (mt.cpp:261)
==3433==    by 0x485F0A: void std::_Bind_simple<void (*())()>::_M_invoke<>(std::_Index_tuple<>) (functional:1531)
==3433==    by 0x485C3D: std::_Bind_simple<void (*())()>::operator()() (functional:1520)
==3433==    by 0x48591F: std::thread::_Impl<std::_Bind_simple<void (*())()> >::_M_run() (thread:115)
==3433==    by 0x5762C7F: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==3433== Other segment end (thread 7)
==3433==    at 0x4C37F75: pthread_rwlock_unlock_intercept (drd_pthread_intercepts.c:1257)
==3433==    by 0x4C37F75: pthread_rwlock_unlock$* (drd_pthread_intercepts.c:1265)
==3433==    by 0x4E55592: os_rwlock_unlock (os_thread_pthread.c:119)
==3433==    by 0x4E51538: vinode_unref (inode.c:276)
==3433==    by 0x4E4BDBE: path_info_cleanup (dir.c:626)
==3433==    by 0x4E5AFA8: _pmemfile_renameat2 (rename.c:481)
==3433==    by 0x4E5B2E9: pmemfile_renameat2 (rename.c:559)
==3433==    by 0x4789FC: test_exchange(char const*, char const*) (mt.cpp:204)
==3433==    by 0x478A5E: test_exchange_loop(char const*, char const*) (mt.cpp:213)
==3433==    by 0x478B00: rename_worker6() (mt.cpp:261)
==3433==    by 0x485F0A: void std::_Bind_simple<void (*())()>::_M_invoke<>(std::_Index_tuple<>) (functional:1531)
==3433==    by 0x485C3D: std::_Bind_simple<void (*())()>::operator()() (functional:1520)
==3433==    by 0x48591F: std::thread::_Impl<std::_Bind_simple<void (*())()> >::_M_run() (thread:115)

---

This change is 

[codecov[bot]]

Codecov Report

Merging #422 into master will decrease coverage by 0.13%. The diff coverage is 88%.

@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
- Coverage   76.02%   75.88%   -0.14%     
==========================================
  Files          68       68              
  Lines        8767     8763       -4     
  Branches     1776     1775       -1     
==========================================
- Hits         6665     6650      -15     
- Misses       1608     1624      +16     
+ Partials      494      489       -5

Flag	Coverage Δ
#ltp_tests	44.77% <84%> (-0.31%)	:arrow_down:
#sqlite_tests	44.94% <84%> (-0.06%)	:arrow_down:
#tests_antool	14.65% <0%> (ø)	:arrow_up:
#tests_posix_multi_threaded	25.51% <84%> (-0.04%)	:arrow_down:
#tests_posix_single_threaded	53.48% <84%> (ø)	:arrow_up:
#tests_preload	44.75% <88%> (+0.22%)	:arrow_up:

Impacted Files	Coverage Δ
src/libpmemfile-posix/inode.c	89.07% <88%> (+0.51%)	:arrow_up:
src/libpmemfile-posix/fcntl.c	49.38% <0%> (-13.59%)	:arrow_down:
src/libpmemfile-posix/dir.c	85.78% <0%> (-0.5%)	:arrow_down:

---

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 b7e34b3...d4e8bfd. Read the comment docs.

[GBuella]

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.

---

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

          inode = vinode->inode = NULL;
      } else if (vinode->atime_dirty) {
          bool atime_slot = inode_next_atime_slot(inode);

Perhaps this is a good opportunity to fix the type name here.

---

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

      }

      to_unregister = vinode;

This to_unregister variable always throws me off track when I try to read this code. In fact, it is the same as the vinode pointer, we just have two variables referring to the same thing now. Can't we get rid of it? We could have this in the beginning of the loop:

struct pmemfile_vinode *parent = NULL;
if (vinode->parent != vinode)
    parent = vinode->parent;

and this at the end of the loop:

vinode = parent;

Or we could call it next instead of parent.

---

Comments from Reviewable

[GBuella]

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

---

Comments from Reviewable

[marcinslusarz]

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.

---

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

Previously, GBuella (Gabor Buella) wrote…

Perhaps this is a good opportunity to fix the type name here.

Done.

---

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

Previously, GBuella (Gabor Buella) wrote…

This to_unregister variable always throws me off track when I try to read this code. In fact, it is the same as the vinode pointer, we just have two variables referring to the same thing now. Can't we get rid of it? We could have this in the beginning of the loop: ``` struct pmemfile_vinode *parent = NULL; if (vinode->parent != vinode) parent = vinode->parent; ``` and this at the end of the loop: ``` vinode = parent; ``` Or we could call it `next` instead of `parent`.

Done.

---

Comments from Reviewable

[GBuella]

---

Reviewed 1 of 1 files at r2. Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable