cls: add semantics for cls locks to require renewal without expiring

Add ability to *require* renewal of an existing lock in addition
toexisting ability to *allow* renewal of an existing lock. The key
difference is that a MUST_RENEW will fail if the lock has expired
(where a MAY_RENEW) will succeed. This provides calling code with the
ability to verify that a lock is held continually and that it was
never lost/expired.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>

diff --git a/src/cls/lock/cls_lock.cc b/src/cls/lock/cls_lock.cc
index 12311b1a3c0e60b8b8414318a529bf4c7be9c719..643eba13666b2d838901ade72b09069dc52dc88a 100644 (file)
--- a/src/cls/lock/cls_lock.cc
+++ b/src/cls/lock/cls_lock.cc
@@ -124,9 +124,10 @@ static int lock_obj(cls_method_context_t hctx,
 {
   bool exclusive = lock_type == LOCK_EXCLUSIVE;
   lock_info_t linfo;
-  bool fail_if_exists = (flags & LOCK_FLAG_RENEW) == 0;
+  bool fail_if_exists = (flags & LOCK_FLAG_MAY_RENEW) == 0;
+  bool fail_if_does_not_exist = flags & LOCK_FLAG_MUST_RENEW;
 
-  CLS_LOG(20, "requested lock_type=%s fail_if_exists=%d", cls_lock_type_str(lock_type), fail_if_exists);
+  CLS_LOG(20, "requested lock_type=%s fail_if_exists=%d fail_if_does_not_exist=%d", cls_lock_type_str(lock_type), fail_if_exists, fail_if_does_not_exist);
   if (lock_type != LOCK_EXCLUSIVE &&
       lock_type != LOCK_SHARED)
     return -EINVAL;
@@ -134,6 +135,13 @@ static int lock_obj(cls_method_context_t hctx,
   if (name.empty())
     return -EINVAL;
 
+  if (!fail_if_exists && fail_if_does_not_exist) {
+    // at most one of LOCK_FLAG_MAY_RENEW and LOCK_FLAG_MUST_RENEW may
+    // be set since they have different implications if the lock does
+    // not already exist
+    return -EINVAL;
+  }
+
   // see if there's already a locker
   int r = read_lock(hctx, name, &linfo);
   if (r < 0 && r != -ENOENT) {
@@ -161,11 +169,13 @@ static int lock_obj(cls_method_context_t hctx,
   CLS_LOG(20, "existing_lock_type=%s", cls_lock_type_str(existing_lock_type));
   iter = lockers.find(id);
   if (iter != lockers.end()) {
-    if (fail_if_exists) {
+    if (fail_if_exists && !fail_if_does_not_exist) {
       return -EEXIST;
     } else {
       lockers.erase(iter); // remove old entry
     }
+  } else if (fail_if_does_not_exist) {
+    return -ENOENT;
   }
 
   if (!lockers.empty()) {
@@ -422,7 +432,7 @@ int assert_locked(cls_method_context_t hctx, bufferlist *in, bufferlist *out)
     return -EINVAL;
   }
 
-  if (op.type != LOCK_EXCLUSIVE && op.type != LOCK_SHARED) {
+  if (!cls_lock_is_valid(op.type)) {
     return -EINVAL;
   }
 
@@ -494,7 +504,7 @@ int set_cookie(cls_method_context_t hctx, bufferlist *in, bufferlist *out)
     return -EINVAL;
   }
 
-  if (op.type != LOCK_EXCLUSIVE && op.type != LOCK_SHARED) {
+  if (!cls_lock_is_valid(op.type)) {
     return -EINVAL;
   }
 

diff --git a/src/cls/lock/cls_lock_client.h b/src/cls/lock/cls_lock_client.h
index 43e14e6a493f7d1767cee397ad53e1fcdb9c9e5e..ae1a238117469568e4b4a632de423ca7d5c23291 100644 (file)
--- a/src/cls/lock/cls_lock_client.h
+++ b/src/cls/lock/cls_lock_client.h
@@ -87,11 +87,24 @@ namespace rados {
        void set_tag(const std::string& t) { tag = t; }
        void set_description(const std::string& desc) { description = desc; }
        void set_duration(const utime_t& e) { duration = e; }
-       void set_renew(bool renew) {
+       void set_duration(const ceph::timespan& d) {
+         duration = utime_t(ceph::real_clock::zero() + d);
+       }
+
+       void set_may_renew(bool renew) {
+         if (renew) {
+           flags |= LOCK_FLAG_MAY_RENEW;
+           flags &= ~LOCK_FLAG_MUST_RENEW; // if may then not must
+         } else {
+           flags &= ~LOCK_FLAG_MAY_RENEW;
+         }
+       }
+       void set_must_renew(bool renew) {
          if (renew) {
-           flags |= LOCK_FLAG_RENEW;
+           flags |= LOCK_FLAG_MUST_RENEW;
+           flags &= ~LOCK_FLAG_MAY_RENEW; // if must then not may
          } else {
-           flags &= ~LOCK_FLAG_RENEW;
+           flags &= ~LOCK_FLAG_MUST_RENEW;
          }
        }
 

diff --git a/src/cls/lock/cls_lock_ops.cc b/src/cls/lock/cls_lock_ops.cc
index 0edeaeac3393aa0a90506789d3057b4a7c1584b8..28c08f344763fb999dbb0081f67690bdf13c8f65 100644 (file)
--- a/src/cls/lock/cls_lock_ops.cc
+++ b/src/cls/lock/cls_lock_ops.cc
@@ -45,7 +45,7 @@ void cls_lock_lock_op::generate_test_instances(list<cls_lock_lock_op*>& o)
   i->tag = "tag";
   i->description = "description";
   i->duration = utime_t(5, 0);
-  i->flags = LOCK_FLAG_RENEW;
+  i->flags = LOCK_FLAG_MAY_RENEW;
   o.push_back(i);
   o.push_back(new cls_lock_lock_op);
 }

diff --git a/src/cls/lock/cls_lock_types.h b/src/cls/lock/cls_lock_types.h
index d88786c6e870c8f4c81308836b51f74f474eee1c..b77bb06ac1d79a7dcdd53fdf2839f5efb30afebe 100644 (file)
--- a/src/cls/lock/cls_lock_types.h
+++ b/src/cls/lock/cls_lock_types.h
@@ -7,7 +7,8 @@
 #include "msg/msg_types.h"
 
 /* lock flags */
-#define LOCK_FLAG_RENEW 0x1        /* idempotent lock acquire */
+#define LOCK_FLAG_MAY_RENEW 0x1    /* idempotent lock acquire */
+#define LOCK_FLAG_MUST_RENEW 0x2   /* lock must already be acquired */
 
 enum ClsLockType {
   LOCK_NONE      = 0,

diff --git a/src/rgw/rgw_cr_rados.cc b/src/rgw/rgw_cr_rados.cc
index a29630677a7fcb07cc62f67fed485846ca2083ae..19caececebad5b25ca4966f0d9066ba6c6ca3ae0 100644 (file)
--- a/src/rgw/rgw_cr_rados.cc
+++ b/src/rgw/rgw_cr_rados.cc
@@ -170,7 +170,7 @@ int RGWAsyncLockSystemObj::_send_request()
   utime_t duration(duration_secs, 0);
   l.set_duration(duration);
   l.set_cookie(cookie);
-  l.set_renew(true);
+  l.set_may_renew(true);
 
   return l.lock_exclusive(&ref.ioctx, ref.oid);
 }

diff --git a/src/rgw/rgw_rados.cc b/src/rgw/rgw_rados.cc
index 770a84334d18203ca98e8ddf95766ebfa4a5f1e7..305acbff814bc2621d961b4243b8ebaebd15c0c8 100644 (file)
--- a/src/rgw/rgw_rados.cc
+++ b/src/rgw/rgw_rados.cc
@@ -5225,7 +5225,7 @@ int RGWRados::lock_exclusive(rgw_pool& pool, const string& oid, timespan& durati
   l.set_duration(ut);
   l.set_cookie(owner_id);
   l.set_tag(zone_id);
-  l.set_renew(true);
+  l.set_may_renew(true);
   
   return l.lock_exclusive(&io_ctx, oid);
 }

diff --git a/src/rgw/rgw_reshard.cc b/src/rgw/rgw_reshard.cc
index 214568c44740a8d9330d584fc6677b9e4ff04d7e..474ea9baf83f96f01dbe30104836cd8af591bd45 100644 (file)
--- a/src/rgw/rgw_reshard.cc
+++ b/src/rgw/rgw_reshard.cc
@@ -244,7 +244,7 @@ int RGWBucketReshard::renew_lock_bucket()
   utime_t now = ceph_clock_now();
  /* do you need to renew lock? */
   if (now > lock_start_time + store->ctx()->_conf->rgw_reshard_bucket_lock_duration/ 2) {
-    reshard_lock.set_renew(true);
+    reshard_lock.set_must_renew(true);
     int ret = reshard_lock.lock_exclusive(&store->reshard_pool_ctx, reshard_oid);
     if (ret == -EBUSY) { /* already locked by another processor */
       ldout(store->ctx(), 5) << __func__ << "(): failed to acquire lock on " << reshard_oid << dendl;
@@ -565,6 +565,7 @@ int RGWBucketReshard::get_status(list<cls_rgw_bucket_instance_entry> *status)
   return 0;
 }
 
+
 int RGWBucketReshard::execute(int num_shards, int max_op_entries,
                               bool verbose, ostream *out, Formatter *formatter, RGWReshard* reshard_log)
 
@@ -779,7 +780,7 @@ int RGWReshardWait::block_while_resharding(RGWRados::BucketShard *bs, string *ne
   int ret = 0;
   cls_rgw_bucket_instance_entry entry;
 
-  for (int i=0; i < num_retries;i++) {
+  for (int i=0; i < num_retries; i++) {
     ret = cls_rgw_get_bucket_resharding(bs->index_ctx, bs->bucket_obj, &entry);
     if (ret < 0) {
       ldout(store->ctx(), 0) << __func__ << " ERROR: failed to get bucket resharding :"  <<
@@ -892,7 +893,7 @@ int RGWReshard::process_single_logshard(int logshard_num)
       utime_t now = ceph_clock_now();
 
       if (now > lock_start_time + max_secs / 2) { /* do you need to renew lock? */
-        l.set_renew(true);
+        l.set_must_renew(true);
         ret = l.lock_exclusive(&store->reshard_pool_ctx, logshard_oid);
         if (ret == -EBUSY) { /* already locked by another processor */
           ldout(store->ctx(), 5) << __func__ << "(): failed to acquire lock on " << logshard_oid << dendl;

diff --git a/src/rgw/rgw_reshard.h b/src/rgw/rgw_reshard.h
index c32f4476a62da0eca697b9147941abd265a6861c..eeaf901f7a2c2832a6187d901c1bc5ef848d93df 100644 (file)
--- a/src/rgw/rgw_reshard.h
+++ b/src/rgw/rgw_reshard.h
@@ -51,7 +51,6 @@ public:
               bool verbose = false, ostream *out = nullptr,
               Formatter *formatter = nullptr,
              RGWReshard *reshard_log = nullptr);
-  int abort();
   int get_status(std::list<cls_rgw_bucket_instance_entry> *status);
   int cancel();
 };

diff --git a/src/test/cls_lock/test_cls_lock.cc b/src/test/cls_lock/test_cls_lock.cc
index 0aeed8245e7d8fdae5d34668eb1bed095cb0e450..b8bc51e01a4143ea7c4a015ea4091d2bacb8e34b 100644 (file)
--- a/src/test/cls_lock/test_cls_lock.cc
+++ b/src/test/cls_lock/test_cls_lock.cc
@@ -94,10 +94,10 @@ TEST(ClsLock, TestMultiLocking) {
   ASSERT_EQ(-EEXIST, l.lock_exclusive(&ioctx, oid));
 
   /* test idempotency */
-  l.set_renew(true);
+  l.set_may_renew(true);
   ASSERT_EQ(0, l.lock_exclusive(&ioctx, oid));
 
-  l.set_renew(false);
+  l.set_may_renew(false);
 
   /* test second client */
   Lock l2(lock_name);
@@ -204,7 +204,7 @@ TEST(ClsLock, TestMeta) {
   /* check new tag */
   string new_tag = "new_tag";
   l.set_tag(new_tag);
-  l.set_renew(true);
+  l.set_may_renew(true);
   ASSERT_EQ(0, l.lock_exclusive(&ioctx, oid));
   lock_info(&ioctx, oid, lock_name, lockers, NULL, &new_tag);
   ASSERT_EQ(1, (int)lockers.size());
@@ -391,3 +391,69 @@ TEST(ClsLock, TestSetCookie) {
 
   ASSERT_EQ(0, destroy_one_pool_pp(pool_name, cluster));
 }
+
+TEST(ClsLock, TestRenew) {
+  Rados cluster;
+  std::string pool_name = get_temp_pool_name();
+  ASSERT_EQ("", create_one_pool_pp(pool_name, cluster));
+  IoCtx ioctx;
+  cluster.ioctx_create(pool_name.c_str(), ioctx);
+
+  bufferlist bl;
+
+  string oid1 = "foo1";
+  string lock_name1 = "mylock1";
+
+  ASSERT_EQ(0, ioctx.write(oid1, bl, bl.length(), 0));
+
+  Lock l1(lock_name1);
+  utime_t lock_duration1(5, 0);
+  l1.set_duration(lock_duration1);
+
+  ASSERT_EQ(0, l1.lock_exclusive(&ioctx, oid1));
+  l1.set_may_renew(true);
+  sleep(2);
+  ASSERT_EQ(0, l1.lock_exclusive(&ioctx, oid1));
+  sleep(7);
+  ASSERT_EQ(0, l1.lock_exclusive(&ioctx, oid1)) <<
+    "when a cls_lock is set to may_renew, a relock after expiration "
+    "should still work";
+  ASSERT_EQ(0, l1.unlock(&ioctx, oid1));
+
+  // ***********************************************
+
+  string oid2 = "foo2";
+  string lock_name2 = "mylock2";
+
+  ASSERT_EQ(0, ioctx.write(oid2, bl, bl.length(), 0));
+
+  Lock l2(lock_name2);
+  utime_t lock_duration2(5, 0);
+  l2.set_duration(lock_duration2);
+
+  ASSERT_EQ(0, l2.lock_exclusive(&ioctx, oid2));
+  l2.set_must_renew(true);
+  sleep(2);
+  ASSERT_EQ(0, l2.lock_exclusive(&ioctx, oid2));
+  sleep(7);
+  ASSERT_EQ(-ENOENT, l2.lock_exclusive(&ioctx, oid2)) <<
+    "when a cls_lock is set to must_renew, a relock after expiration "
+    "should fail";
+  ASSERT_EQ(-ENOENT, l2.unlock(&ioctx, oid2));
+
+  // ***********************************************
+
+  string oid3 = "foo3";
+  string lock_name3 = "mylock3";
+
+  ASSERT_EQ(0, ioctx.write(oid3, bl, bl.length(), 0));
+
+  Lock l3(lock_name3);
+  l3.set_duration(utime_t(5, 0));
+  l3.set_must_renew(true);
+
+  ASSERT_EQ(-ENOENT, l3.lock_exclusive(&ioctx, oid3)) <<
+    "unable to create a lock with must_renew";
+
+  ASSERT_EQ(0, destroy_one_pool_pp(pool_name, cluster));
+}

diff --git a/src/test/librados/lock.cc b/src/test/librados/lock.cc
index 65d3fcdf09f067cb506a3daca5708e59b603879d..ef1033f5e66014ac70eb8bfde8d34b45df77cb55 100644 (file)
--- a/src/test/librados/lock.cc
+++ b/src/test/librados/lock.cc
@@ -109,16 +109,16 @@ TEST_F(LibRadosLockPP, LockSharedDurPP) {
   ASSERT_EQ(expected, wait_until(1.0s, 0.1s, expected, lock_shared, nullptr));
 }
 
-TEST_F(LibRadosLock, LockRenew) {
+TEST_F(LibRadosLock, LockMayRenew) {
   ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLock5", "Cookie", "", NULL, 0));
   ASSERT_EQ(-EEXIST, rados_lock_exclusive(ioctx, "foo", "TestLock5", "Cookie", "", NULL, 0));
-  ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLock5", "Cookie", "", NULL, LOCK_FLAG_RENEW));
+  ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLock5", "Cookie", "", NULL, LOCK_FLAG_MAY_RENEW));
 }
 
-TEST_F(LibRadosLockPP, LockRenewPP) {
+TEST_F(LibRadosLockPP, LockMayRenewPP) {
   ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLockPP5", "Cookie", "", NULL, 0));
   ASSERT_EQ(-EEXIST, ioctx.lock_exclusive("foo", "TestLockPP5", "Cookie", "", NULL, 0));
-  ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLockPP5", "Cookie", "", NULL, LOCK_FLAG_RENEW));
+  ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLockPP5", "Cookie", "", NULL, LOCK_FLAG_MAY_RENEW));
 }
 
 TEST_F(LibRadosLock, Unlock) {
@@ -300,16 +300,16 @@ TEST_F(LibRadosLockECPP, LockSharedDurPP) {
   ASSERT_EQ(expected, wait_until(1.0s, 0.1s, expected, lock_shared, nullptr));
 }
 
-TEST_F(LibRadosLockEC, LockRenew) {
+TEST_F(LibRadosLockEC, LockMayRenew) {
   ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLockEC5", "Cookie", "", NULL, 0));
   ASSERT_EQ(-EEXIST, rados_lock_exclusive(ioctx, "foo", "TestLockEC5", "Cookie", "", NULL, 0));
-  ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLockEC5", "Cookie", "", NULL, LOCK_FLAG_RENEW));
+  ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLockEC5", "Cookie", "", NULL, LOCK_FLAG_MAY_RENEW));
 }
 
-TEST_F(LibRadosLockECPP, LockRenewPP) {
+TEST_F(LibRadosLockECPP, LockMayRenewPP) {
   ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLockECPP5", "Cookie", "", NULL, 0));
   ASSERT_EQ(-EEXIST, ioctx.lock_exclusive("foo", "TestLockECPP5", "Cookie", "", NULL, 0));
-  ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLockECPP5", "Cookie", "", NULL, LOCK_FLAG_RENEW));
+  ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLockECPP5", "Cookie", "", NULL, LOCK_FLAG_MAY_RENEW));
 }
 
 TEST_F(LibRadosLockEC, Unlock) {