librbd: use generic helper for issuing async requests

resize, flatten, and rebuild object map now use the same
bootstrap code for sending the request to the remote lock owner
or executing the request locally.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/ImageWatcher.cc b/src/librbd/ImageWatcher.cc
index c79c635aee7a81dd37f8625a6bcabc3a9c6a934e..dd7ed2b46d875867ea95901850de324bf66f30d1 100644 (file)
--- a/src/librbd/ImageWatcher.cc
+++ b/src/librbd/ImageWatcher.cc
@@ -898,7 +898,8 @@ void ImageWatcher::handle_payload(const SnapCreatePayload &payload,
   if (m_lock_owner_state == LOCK_OWNER_STATE_LOCKED) {
     ldout(m_image_ctx.cct, 10) << "remote snap_create request: "
                               << payload.snap_name << dendl;
-    int r = librbd::snap_create(&m_image_ctx, payload.snap_name.c_str(), false);
+    int r = librbd::snap_create_helper(&m_image_ctx, NULL,
+                                       payload.snap_name.c_str());
 
     ::encode(ResponseMessage(r), *out);
   }

diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index 900c06ffcdde59e28fa7f29a74ae25554fdc80d2..9a84db5a082e74ce19d025de47093896c80d3802 100644 (file)
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -81,6 +81,81 @@ int remove_object_map(ImageCtx *ictx) {
   return 0;
 }
 
+int prepare_image_update(ImageCtx *ictx) {
+  assert(ictx->owner_lock.is_locked() && !ictx->owner_lock.is_wlocked());
+  if (ictx->image_watcher == NULL) {
+    return -EROFS;
+  } else if (!ictx->image_watcher->is_lock_supported() ||
+             ictx->image_watcher->is_lock_owner()) {
+    return 0;
+  }
+
+  // need to upgrade to a write lock
+  int r = 0;
+  bool acquired_lock = false;
+  ictx->owner_lock.put_read();
+  {
+    RWLock::WLocker l(ictx->owner_lock);
+    if (!ictx->image_watcher->is_lock_owner()) {
+      r = ictx->image_watcher->try_lock();
+      acquired_lock = ictx->image_watcher->is_lock_owner();
+    }
+  }
+  if (acquired_lock) {
+    // finish any AIO that was previously waiting on acquiring the
+    // exclusive lock
+    ictx->flush_async_operations();
+  }
+  ictx->owner_lock.get_read();
+  return r;
+}
+
+int invoke_async_request(ImageCtx *ictx, const std::string& request_type,
+                         const boost::function<int(Context*)>& local_request,
+                         const boost::function<int()>& remote_request) {
+  int r;
+  do {
+    C_SaferCond ctx;
+    {
+      RWLock::RLocker l(ictx->owner_lock);
+      {
+        RWLock::RLocker snap_locker(ictx->snap_lock);
+        if (ictx->read_only || ictx->snap_id != CEPH_NOSNAP) {
+          return -EROFS;
+        }
+      }
+
+      while (ictx->image_watcher->is_lock_supported()) {
+        r = prepare_image_update(ictx);
+        if (r < 0) {
+          return -EROFS;
+        } else if (ictx->image_watcher->is_lock_owner()) {
+          break;
+        }
+
+        r = remote_request();
+        if (r != -ETIMEDOUT && r != -ERESTART) {
+          return r;
+        }
+        ldout(ictx->cct, 5) << request_type << " timed out notifying lock owner"
+                            << dendl;
+      }
+
+      r = local_request(&ctx);
+      if (r < 0) {
+        return r;
+      }
+    }
+
+    r = ctx.wait();
+    if (r == -ERESTART) {
+      ldout(ictx->cct, 5) << request_type << " interrupted: restarting"
+                          << dendl;
+    }
+  } while (r == -ERESTART);
+  return r;
+}
+
 } // anonymous namespace
 
   const string id_obj_name(const string &name)
@@ -471,43 +546,13 @@ int remove_object_map(ImageCtx *ictx) {
     return 0;
   }
 
-  static int prepare_image_update(ImageCtx *ictx)
+  int snap_create(ImageCtx *ictx, const char *snap_name)
   {
-    assert(ictx->owner_lock.is_locked() && !ictx->owner_lock.is_wlocked());
-    if (ictx->image_watcher == NULL) {
-      return -EROFS;
-    } else if (!ictx->image_watcher->is_lock_supported() ||
-              ictx->image_watcher->is_lock_owner()) {
-      return 0;
-    }
-
-    // need to upgrade to a write lock
-    int r = 0;
-    bool acquired_lock = false;
-    ictx->owner_lock.put_read();
-    {
-      RWLock::WLocker l(ictx->owner_lock);
-      if (!ictx->image_watcher->is_lock_owner()) {
-       r = ictx->image_watcher->try_lock();
-        acquired_lock = ictx->image_watcher->is_lock_owner();
-      }
-    }
-    if (acquired_lock) {
-      // finish any AIO that was previously waiting on acquiring the
-      // exclusive lock
-      ictx->flush_async_operations();
-    }
-    ictx->owner_lock.get_read();
-    return r;
-  }
-
-  int snap_create(ImageCtx *ictx, const char *snap_name, bool notify)
-  {
-    assert(ictx->owner_lock.is_locked());
     ldout(ictx->cct, 20) << "snap_create " << ictx << " " << snap_name << dendl;
 
-    if (ictx->read_only)
+    if (ictx->read_only) {
       return -EROFS;
+    }
 
     int r = ictx_check(ictx);
     if (r < 0)
@@ -520,45 +565,51 @@ int remove_object_map(ImageCtx *ictx) {
       }
     }
 
-    bool lock_owner = false;
-    while (ictx->image_watcher->is_lock_supported()) {
-      r = prepare_image_update(ictx);
-      if (r < 0) {
-       return -EROFS;
-      } else if (ictx->image_watcher->is_lock_owner()) {
-       lock_owner = true;
-       break;
-      }
+    r = invoke_async_request(ictx, "snap_create",
+                             boost::bind(&snap_create_helper, ictx, _1,
+                                         snap_name),
+                             boost::bind(&ImageWatcher::notify_snap_create,
+                                         ictx->image_watcher, snap_name));
+    if (r < 0 && r != -EEXIST) {
+      return r;
+    }
 
-      r = ictx->image_watcher->notify_snap_create(snap_name);
-      if (r == 0 || r == -EEXIST) {
-        notify_change(ictx->md_ctx, ictx->header_oid, ictx);
-        return 0;
-      } else if (r != -ETIMEDOUT) {
-       return r;
-      }
-      ldout(ictx->cct, 5) << "snap_create timed out notifying lock owner" << dendl;
+    ictx->perfcounter->inc(l_librbd_snap_create);
+    notify_change(ictx->md_ctx, ictx->header_oid, ictx);
+    return 0;
+  }
+
+  int snap_create_helper(ImageCtx* ictx, Context* ctx,
+                         const char* snap_name) {
+    assert(ictx->owner_lock.is_locked());
+    assert(!ictx->image_watcher->is_lock_supported() ||
+          ictx->image_watcher->is_lock_owner());
+
+    ldout(ictx->cct, 20) << "snap_create_helper " << ictx << " " << snap_name
+                         << dendl;
+
+    int r = ictx_check(ictx);
+    if (r < 0) {
+      return r;
     }
 
-    RWLock::WLocker l2(ictx->md_lock);
+    RWLock::WLocker md_locker(ictx->md_lock);
     r = _flush(ictx);
     if (r < 0) {
       return r;
     }
 
     do {
-      r = add_snap(ictx, snap_name, lock_owner);
+      r = add_snap(ictx, snap_name);
     } while (r == -ESTALE);
 
     if (r < 0) {
       return r;
     }
 
-    if (notify) {
-      notify_change(ictx->md_ctx, ictx->header_oid, ictx);
+    if (ctx != NULL) {
+      ctx->complete(0);
     }
-
-    ictx->perfcounter->inc(l_librbd_snap_create);
     return 0;
   }
 
@@ -1758,42 +1809,12 @@ reprotect_and_return_err:
     }
 
     uint64_t request_id = ictx->async_request_seq.inc();
-    do {
-      C_SaferCond ctx;
-      {
-       RWLock::RLocker l(ictx->owner_lock);
-       while (ictx->image_watcher->is_lock_supported()) {
-         r = prepare_image_update(ictx);
-         if (r < 0) {
-           return -EROFS;
-         } else if (ictx->image_watcher->is_lock_owner()) {
-           break;
-         }
-
-          RWLock::RLocker snap_locker(ictx->snap_lock);
-          if (ictx->snap_id != CEPH_NOSNAP || ictx->read_only) {
-            return -EROFS;
-          }
-
-         r = ictx->image_watcher->notify_resize(request_id, size, prog_ctx);
-         if (r != -ETIMEDOUT && r != -ERESTART) {
-           return r;
-         }
-         ldout(ictx->cct, 5) << "resize timed out notifying lock owner"
-                              << dendl;
-       }
-
-       r = async_resize(ictx, &ctx, size, prog_ctx);
-       if (r < 0) {
-         return r;
-       }
-      }
-
-      r = ctx.wait();
-      if (r == -ERESTART) {
-       ldout(ictx->cct, 5) << "resize interrupted: restarting" << dendl;
-      }
-    } while (r == -ERESTART);
+    r = invoke_async_request(ictx, "resize",
+                             boost::bind(&async_resize, ictx, _1, size,
+                                         boost::ref(prog_ctx)),
+                             boost::bind(&ImageWatcher::notify_resize,
+                                         ictx->image_watcher, request_id, size,
+                                         boost::ref(prog_ctx)));
 
     ictx->perfcounter->inc(l_librbd_resize);
     notify_change(ictx->md_ctx, ictx->header_oid, ictx);
@@ -1874,12 +1895,17 @@ reprotect_and_return_err:
   }
 
 
-  int add_snap(ImageCtx *ictx, const char *snap_name, bool lock_owner)
+  int add_snap(ImageCtx *ictx, const char *snap_name)
   {
     assert(ictx->owner_lock.is_locked());
     assert(ictx->md_lock.is_wlocked());
-    uint64_t snap_id;
 
+    bool lock_owner = ictx->image_watcher->is_lock_owner();
+    if (ictx->image_watcher->is_lock_supported()) {
+      assert(lock_owner);
+    }
+
+    uint64_t snap_id;
     int r = ictx->md_ctx.selfmanaged_snap_create(&snap_id);
     if (r < 0) {
       lderr(ictx->cct) << "failed to create snap id: " << cpp_strerror(-r)
@@ -1892,7 +1918,7 @@ reprotect_and_return_err:
                                       snap_id, snap_name);
     } else {
       librados::ObjectWriteOperation op;
-      if (ictx->image_watcher->is_lock_supported()) {
+      if (lock_owner) {
        ictx->image_watcher->assert_header_locked(&op);
       }
       cls_client::snapshot_add(&op, snap_id, snap_name);
@@ -2602,45 +2628,13 @@ reprotect_and_return_err:
       return r;
     }
 
-    {
-      RWLock::RLocker snap_locker(ictx->snap_lock);
-      if (ictx->read_only || ictx->snap_id != CEPH_NOSNAP) {
-        return -EROFS;
-      }
-    }
-
     uint64_t request_id = ictx->async_request_seq.inc();
-    do {
-      C_SaferCond ctx;
-      {
-        RWLock::RLocker l(ictx->owner_lock);
-        while (ictx->image_watcher->is_lock_supported()) {
-          r = prepare_image_update(ictx);
-          if (r < 0) {
-            return -EROFS;
-          } else if (ictx->image_watcher->is_lock_owner()) {
-           break;
-         }
-
-          r = ictx->image_watcher->notify_flatten(request_id, prog_ctx);
-          if (r != -ETIMEDOUT && r != -ERESTART) {
-            return r;
-          }
-          ldout(ictx->cct, 5) << "flatten timed out notifying lock owner"
-                              << dendl;
-        }
-
-        r = async_flatten(ictx, &ctx, prog_ctx);
-        if (r < 0) {
-         return r;
-        }
-      }
-
-      r = ctx.wait();
-      if (r == -ERESTART) {
-       ldout(ictx->cct, 5) << "flatten interrupted: restarting" << dendl;
-      }
-    } while (r == -ERESTART);
+    r = invoke_async_request(ictx, "flatten",
+                             boost::bind(&async_flatten, ictx, _1,
+                                         boost::ref(prog_ctx)),
+                             boost::bind(&ImageWatcher::notify_flatten,
+                                         ictx->image_watcher, request_id,
+                                         boost::ref(prog_ctx)));
 
     notify_change(ictx->md_ctx, ictx->header_oid, ictx);
     ldout(cct, 20) << "flatten finished" << dendl;
@@ -2712,49 +2706,30 @@ reprotect_and_return_err:
       return r;
     }
 
+    uint64_t snap_id;
     {
       RWLock::RLocker snap_locker(ictx->snap_lock);
-      if (ictx->read_only || ictx->snap_id != CEPH_NOSNAP) {
-        return -EROFS;
-      }
+      snap_id = ictx->snap_id;
     }
 
-    uint64_t request_id = ictx->async_request_seq.inc();
-    do {
+    if (snap_id == CEPH_NOSNAP) {
+      uint64_t request_id = ictx->async_request_seq.inc();
+      r = invoke_async_request(ictx, "rebuild object map",
+                               boost::bind(&async_rebuild_object_map, ictx, _1,
+                                           boost::ref(prog_ctx)),
+                               boost::bind(&ImageWatcher::notify_rebuild_object_map,
+                                           ictx->image_watcher, request_id,
+                                           boost::ref(prog_ctx)));
+    } else {
       C_SaferCond ctx;
       {
-        RWLock::RLocker l(ictx->owner_lock);
-        while (ictx->image_watcher->is_lock_supported()) {
-          r = prepare_image_update(ictx);
-          if (r < 0) {
-            return -EROFS;
-          } else if (ictx->image_watcher->is_lock_owner()) {
-           break;
-         }
-
-          r = ictx->image_watcher->notify_rebuild_object_map(request_id,
-                                                             prog_ctx);
-          if (r != -ETIMEDOUT && r != -ERESTART) {
-            return r;
-          }
-          ldout(ictx->cct, 5) << "rebuild object map timed out notifying lock "
-                              << "owner" << dendl;
-        }
-
+        RWLock::RLocker owner_locker(ictx->owner_lock);
         r = async_rebuild_object_map(ictx, &ctx, prog_ctx);
-        if (r < 0) {
-         return r;
-        }
       }
-
-      r = ctx.wait();
-      if (r == -ERESTART) {
-       ldout(ictx->cct, 5) << "rebuild object map interrupted: restarting"
-                            << dendl;
+      if (r == 0) {
+        r = ctx.wait();
       }
-    } while (r == -ERESTART);
-
-    // TODO rebuild snapshots
+    }
 
     ldout(cct, 10) << "rebuild object map finished" << dendl;
     return r;

diff --git a/src/librbd/internal.h b/src/librbd/internal.h
index 33107313f1d372623fb503e955265263dc196d9b..9f71f55da26cc5f9176ca95ce3801e943c71ab17 100644 (file)
--- a/src/librbd/internal.h
+++ b/src/librbd/internal.h
@@ -110,7 +110,8 @@ namespace librbd {
   int remove(librados::IoCtx& io_ctx, const char *imgname,
             ProgressContext& prog_ctx);
   int resize(ImageCtx *ictx, uint64_t size, ProgressContext& prog_ctx);
-  int snap_create(ImageCtx *ictx, const char *snap_name, bool notify);
+  int snap_create(ImageCtx *ictx, const char *snap_name);
+  int snap_create_helper(ImageCtx *ictx, Context* ctx, const char *snap_name);
   int snap_list(ImageCtx *ictx, std::vector<snap_info_t>& snaps);
   bool snap_exists(ImageCtx *ictx, const char *snap_name);
   int snap_rollback(ImageCtx *ictx, const char *snap_name,
@@ -120,7 +121,7 @@ namespace librbd {
   int snap_unprotect(ImageCtx *ictx, const char *snap_name);
   int snap_is_protected(ImageCtx *ictx, const char *snap_name,
                        bool *is_protected);
-  int add_snap(ImageCtx *ictx, const char *snap_name, bool lock_owner);
+  int add_snap(ImageCtx *ictx, const char *snap_name);
   int rm_snap(ImageCtx *ictx, const char *snap_name);
   int refresh_parent(ImageCtx *ictx);
   int ictx_check(ImageCtx *ictx);

diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc
index 1143a1f6210142a08ecd5f1d0af253e19e67bd43..60abe6a0d23a9673c035451f60a3c165f077fbbf 100644 (file)
--- a/src/librbd/librbd.cc
+++ b/src/librbd/librbd.cc
@@ -518,8 +518,7 @@ namespace librbd {
   {
     ImageCtx *ictx = (ImageCtx *)ctx;
     tracepoint(librbd, snap_create_enter, ictx, ictx->name.c_str(), ictx->snap_name.c_str(), ictx->read_only, snap_name);
-    RWLock::RLocker owner_locker(ictx->owner_lock);
-    int r = librbd::snap_create(ictx, snap_name, true);
+    int r = librbd::snap_create(ictx, snap_name);
     tracepoint(librbd, snap_create_exit, r);
     return r;
   }
@@ -1274,8 +1273,7 @@ extern "C" int rbd_snap_create(rbd_image_t image, const char *snap_name)
 {
   librbd::ImageCtx *ictx = (librbd::ImageCtx *)image;
   tracepoint(librbd, snap_create_enter, ictx, ictx->name.c_str(), ictx->snap_name.c_str(), ictx->read_only, snap_name);
-  RWLock::RLocker owner_locker(ictx->owner_lock);
-  int r = librbd::snap_create(ictx, snap_name, true);
+  int r = librbd::snap_create(ictx, snap_name);
   tracepoint(librbd, snap_create_exit, r);
   return r;
 }

diff --git a/src/test/librbd/test_internal.cc b/src/test/librbd/test_internal.cc
index a6d1bcf529f21443b6f59c71d9bad1a6e5d444dd..aac4d4dcc7194c6bf87ff05675d6e8710b6eee03 100644 (file)
--- a/src/test/librbd/test_internal.cc
+++ b/src/test/librbd/test_internal.cc
@@ -38,10 +38,7 @@ public:
       return r;
     }
 
-    {
-      RWLock::RLocker l(ictx->owner_lock);
-      r = librbd::snap_create(ictx, snap_name, true);
-    }
+    r = librbd::snap_create(ictx, snap_name);
     if (r < 0) {
       return r;
     }
@@ -116,10 +113,7 @@ TEST_F(TestInternal, SnapCreateLocksImage) {
   librbd::ImageCtx *ictx;
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
 
-  {
-    RWLock::RLocker l(ictx->owner_lock);
-    ASSERT_EQ(0, librbd::snap_create(ictx, "snap1", true));
-  }
+  ASSERT_EQ(0, librbd::snap_create(ictx, "snap1"));
   BOOST_SCOPE_EXIT( (ictx) ) {
     ASSERT_EQ(0, librbd::snap_remove(ictx, "snap1"));
   } BOOST_SCOPE_EXIT_END;
@@ -136,8 +130,7 @@ TEST_F(TestInternal, SnapCreateFailsToLockImage) {
   ASSERT_EQ(0, open_image(m_image_name, &ictx));
   ASSERT_EQ(0, lock_image(*ictx, LOCK_EXCLUSIVE, "manually locked"));
 
-  RWLock::RLocker l(ictx->owner_lock);
-  ASSERT_EQ(-EROFS, librbd::snap_create(ictx, "snap1", true));
+  ASSERT_EQ(-EROFS, librbd::snap_create(ictx, "snap1"));
 }
 
 TEST_F(TestInternal, SnapRollbackLocksImage) {