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>
(cherry picked from commit 479c90993cc80b140af010a72ff9a34ecb7b3b32)

diff --git a/src/cls/lock/cls_lock.cc b/src/cls/lock/cls_lock.cc
index 6e2ae4bbd1d1699570f0fd2745b640d02c55b835..f8081bf7eafd40e9b4b02cbb61c54d1913bc3300 100644 (file)
--- a/src/cls/lock/cls_lock.cc
+++ b/src/cls/lock/cls_lock.cc
@@ -123,9 +123,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;
@@ -133,6 +134,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) {
@@ -160,11 +168,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()) {
@@ -421,7 +431,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;
   }
 
@@ -493,7 +503,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 7aa06238f179d18640dce30fad79df0b65f919de..cf3886eebf20d114d03a2d640c594f10091d974f 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::time_point::min() + 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 10d00590093c1ac15f7d96116cbbd9763d463f6b..96a2b1ae5766dfeb931f373ab84ec0aff6219e87 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 36d39c89014c67c596cf443bf1c7e3e772a7264b..d00d6a3398f5b5dcfcbc217164d3359db5da7032 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 d79188a8d4460d20f43af013c6eb34b4de36c281..47babe661023983dbbee0908d393e0f96b614bf2 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 f123514e470e29f3677cf63d4f394202cabfad7c..75c1cdbc231a91ea5cbf88c489cf3449eb3dcf90 100644 (file)
--- a/src/rgw/rgw_rados.cc
+++ b/src/rgw/rgw_rados.cc
@@ -5501,7 +5501,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 b2f53bf1e4a0b9a6711f223418ea9693c2023c48..1cb8d518ed742050026ef570e5e6da9bcdde530d 100644 (file)
--- a/src/rgw/rgw_reshard.cc
+++ b/src/rgw/rgw_reshard.cc
@@ -245,7 +245,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;
@@ -566,6 +566,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)
 
@@ -780,7 +781,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 :"  <<
@@ -893,7 +894,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 6506f830b28d4647f90faac99eb1a3ac0110177f..f9d3bf0720c4c7b140d143df14ea31f9f8f9b703 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 1a7c58380004646c8fc5192709520bd8a456daf7..3c5b985acc7733194e8dd7af08ef260d97fccb35 100644 (file)
--- a/src/test/librados/lock.cc
+++ b/src/test/librados/lock.cc
@@ -72,16 +72,16 @@ TEST_F(LibRadosLockPP, LockSharedDurPP) {
   ASSERT_EQ(0, ioctx.lock_shared("foo", "TestLock", "Cookie", "Tag", "", NULL, 0));
 }
 
-TEST_F(LibRadosLock, LockRenew) {
+TEST_F(LibRadosLock, LockMayRenew) {
   ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLock", "Cookie", "", NULL, 0));
   ASSERT_EQ(-EEXIST, rados_lock_exclusive(ioctx, "foo", "TestLock", "Cookie", "", NULL, 0));
-  ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLock", "Cookie", "", NULL, LOCK_FLAG_RENEW));
+  ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLock", "Cookie", "", NULL, LOCK_FLAG_MAY_RENEW));
 }
 
-TEST_F(LibRadosLockPP, LockRenewPP) {
+TEST_F(LibRadosLockPP, LockMayRenewPP) {
   ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLock", "Cookie", "", NULL, 0));
   ASSERT_EQ(-EEXIST, ioctx.lock_exclusive("foo", "TestLock", "Cookie", "", NULL, 0));
-  ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLock", "Cookie", "", NULL, LOCK_FLAG_RENEW));
+  ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLock", "Cookie", "", NULL, LOCK_FLAG_MAY_RENEW));
 }
 
 TEST_F(LibRadosLock, Unlock) {
@@ -251,16 +251,16 @@ TEST_F(LibRadosLockECPP, LockSharedDurPP) {
   ASSERT_EQ(0, ioctx.lock_shared("foo", "TestLock", "Cookie", "Tag", "", NULL, 0));
 }
 
-TEST_F(LibRadosLockEC, LockRenew) {
+TEST_F(LibRadosLockEC, LockMayRenew) {
   ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLock", "Cookie", "", NULL, 0));
   ASSERT_EQ(-EEXIST, rados_lock_exclusive(ioctx, "foo", "TestLock", "Cookie", "", NULL, 0));
-  ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLock", "Cookie", "", NULL, LOCK_FLAG_RENEW));
+  ASSERT_EQ(0, rados_lock_exclusive(ioctx, "foo", "TestLock", "Cookie", "", NULL, LOCK_FLAG_MAY_RENEW));
 }
 
-TEST_F(LibRadosLockECPP, LockRenewPP) {
+TEST_F(LibRadosLockECPP, LockMayRenewPP) {
   ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLock", "Cookie", "", NULL, 0));
   ASSERT_EQ(-EEXIST, ioctx.lock_exclusive("foo", "TestLock", "Cookie", "", NULL, 0));
-  ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLock", "Cookie", "", NULL, LOCK_FLAG_RENEW));
+  ASSERT_EQ(0, ioctx.lock_exclusive("foo", "TestLock", "Cookie", "", NULL, LOCK_FLAG_MAY_RENEW));
 }
 
 TEST_F(LibRadosLockEC, Unlock) {