From: Jason Dillaman <dillaman@redhat.com>
Date: Wed, 18 Mar 2015 15:51:47 +0000 (-0400)
Subject: librbd: use generic helper for issuing async requests
X-Git-Tag: v9.0.1~149^2~18
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=18fd6ca7f59d5545f0bb0b0e899d0739639ce104;p=ceph.git

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 c79c635aee7a..dd7ed2b46d87 100644
--- 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 900c06ffcdde..9a84db5a082e 100644
--- 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 33107313f1d3..9f71f55da26c 100644
--- 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 1143a1f62101..60abe6a0d23a 100644
--- 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 a6d1bcf529f2..aac4d4dcc719 100644
--- 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) {