From 479c90993cc80b140af010a72ff9a34ecb7b3b32 Mon Sep 17 00:00:00 2001 From: "J. Eric Ivancich" Date: Mon, 1 Oct 2018 15:18:39 -0400 Subject: [PATCH] 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 --- src/cls/lock/cls_lock.cc | 20 ++++++--- src/cls/lock/cls_lock_client.h | 19 ++++++-- src/cls/lock/cls_lock_ops.cc | 2 +- src/cls/lock/cls_lock_types.h | 3 +- src/rgw/rgw_cr_rados.cc | 2 +- src/rgw/rgw_rados.cc | 2 +- src/rgw/rgw_reshard.cc | 7 +-- src/rgw/rgw_reshard.h | 1 - src/test/cls_lock/test_cls_lock.cc | 72 ++++++++++++++++++++++++++++-- src/test/librados/lock.cc | 16 +++---- 10 files changed, 117 insertions(+), 27 deletions(-) diff --git a/src/cls/lock/cls_lock.cc b/src/cls/lock/cls_lock.cc index 12311b1a3c0..643eba13666 100644 --- 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 43e14e6a493..ae1a2381174 100644 --- 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 0edeaeac339..28c08f34476 100644 --- 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& 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 d88786c6e87..b77bb06ac1d 100644 --- 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 a29630677a7..19caececeba 100644 --- 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 770a84334d1..305acbff814 100644 --- 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 214568c4474..474ea9baf83 100644 --- 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 *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 c32f4476a62..eeaf901f7a2 100644 --- 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 *status); int cancel(); }; diff --git a/src/test/cls_lock/test_cls_lock.cc b/src/test/cls_lock/test_cls_lock.cc index 0aeed8245e7..b8bc51e01a4 100644 --- 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 65d3fcdf09f..ef1033f5e66 100644 --- 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) { -- 2.39.5