filestore: fix collection_move guard

We had a sequence like:

 1- write A block 1
 2- write A block 2
 3- write A block 3
 4- write A block 4
 5- move A -> B
     - link B
     - unlink A
     - set guard on B   <crash>
  - replay 3, 4, 5

with the result being B with only half of its content.  The problem is that
we destroyed the old link _and_ didn't guard the new content.  Instead,
set the guard before the link, and replay the unlink step here
unconditionally.

Fixes: #2178
Signed-off-by: Sage Weil <sage.weil@dreamhost.com>

diff --git a/src/os/FileStore.cc b/src/os/FileStore.cc
index 46237fa10882c6bdc7a0da39d35119f32b5d427f..1d9fbeb419f753f4372a527f0ffa843f938184ca 100644 (file)
--- a/src/os/FileStore.cc
+++ b/src/os/FileStore.cc
@@ -4453,19 +4453,40 @@ int FileStore::_collection_move(coll_t c, coll_t oldcid, const hobject_t& o,
 {
   dout(15) << "collection_move " << c << "/" << o << " from " << oldcid << "/" << o << dendl;
 
-  if (!_check_replay_guard(oldcid, o, spos))
-    return 0;
+  /*
+   * Treat this as two distinct steps:
+   *
+   *  1) link to new location, with read from old location
+   *  2) remove old location
+   *
+   * We have to make sure the first part is stable in the new location
+   * before we remove it from the old location.
+   *
+   * Because we set the guard between link and unlink, replay the
+   * unlink even if the guard is set.
+   */
 
-  int r = lfn_link(oldcid, c, o);
-  if (r == 0 || (replaying && r == -EEXIST)) {
-    r = lfn_unlink(oldcid, o);
+  int r = 0;
+  if (_check_replay_guard(c, o, spos)) {
+    r = lfn_link(oldcid, c, o);
+    if (replaying && r == -EEXIST)
+      r = 0;
+    if (r == 0) {
+      // set guard on object so we don't do this again.  set this
+      // _before_ we unlink so that if we crash now we don't unlink old
+      // copy _and_ clobber the new copy due to the lack of a guard.
+      int fd = lfn_open(c, o, 0);
+      assert(fd >= 0);
+      _set_replay_guard(fd, spos);
+      TEMP_FAILURE_RETRY(::close(fd));
+    }
+  }
 
-    // set guard on object so we don't do this again
-    int fd = lfn_open(c, o, 0);
-    assert(fd >= 0);
-    _set_replay_guard(fd, spos);
-    TEMP_FAILURE_RETRY(::close(fd));
+  if (r == 0 &&
+      _check_replay_guard(oldcid, o, spos)) {
+    r = lfn_unlink(oldcid, o);
   }
+
   dout(10) << "collection_move " << c << "/" << o << " from " << oldcid << "/" << o << " = " << r << dendl;
   return r;
 }