New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

librbd: managed lock refactoring #12922

Merged
merged 2 commits into from Jan 17, 2017

Conversation

Projects
None yet
2 participants
@trociny
Contributor

trociny commented Jan 13, 2017

No description provided.

@trociny trociny requested a review from dillaman Jan 13, 2017

@trociny

This comment has been minimized.

Contributor

trociny commented Jan 13, 2017

@dillaman This is still WIP, I have some questions:

  1. I had to add additional param to BreakRequest: blacklist_expire_seconds. In exclusive_lock implementation it was taken from ImageCtx, and now when BreakRequest does not use ImageCtx it should be provided separately.

  2. Still there is issue with AcquireRequest: it has already used cct->_conf->rbd_break_on_blacklist and I have also introduces the usage of cct->_conf->rbd_blacklist_expire_seconds. This is wrong because if params are overriden in ImageCtx, they are ignored. So it looks we need to add these params to managed_lock::AcquireRequest too (and may be define exclusive_lock::AcquireRequest then)?

  3. Althoug GetLockerRequest and BreakRequest do not use ImageCtx, they are still templates, so it is possible to instantiate mock classes. Does it look right for you?

  4. When we implement shared lock mode, we will need to change GetLockerRequest to GetLockersRequest to return a list of lockers. May be do this right now?

  5. I added helper methods to ManagedLock to get locker and break lock. Do you like this?

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 13, 2017

@trociny

  1. Makes sense
  2. I would say they should be passed into ManagedLock as a constructor parameter and then passed to managed_lock::AcquireRequest as needed
  3. Yes, we have other examples of things that don't use librbd::ImageCtx but are templated upon it just for mocking reasons.
  4. I'd say wait for now -- cross that bridge under that shared locker PR
  5. In general, yes -- however, I think in the case of librbd::lock_get_owners, it should work if I open the image as read-only. In that case, ImageCtx::exclusive_lock will be null. In librbd::lock_break, we should probably test for ImageCtx::read_only and return -EROFS before checking the exclusive lock state.
@@ -9,6 +9,10 @@
class Context;
namespace librados {
class IoCtx;

This comment has been minimized.

@dillaman

dillaman Jan 13, 2017

Contributor

Nit: no indentation within namespaces. You could collapse to a single line, though.

Context *on_finish) {
return new BreakRequest(image_ctx, locker, blacklist_locker,
force_break_lock, on_finish);
static BreakRequest* create(librados::IoCtx& ioctx, ContextWQ *work_queue,

This comment has been minimized.

@dillaman

dillaman Jan 13, 2017

Contributor

Nit: forward declare librados::IoCtx in this header as well?

@@ -13,6 +13,7 @@
#include <boost/optional.hpp>
class Context;
class ContextWQ;
namespace librbd {

This comment has been minimized.

@dillaman

dillaman Jan 13, 2017

Contributor

Nit: can forward declare ImageCtx and move include to the CC file

@@ -8,6 +8,7 @@
#include "include/Context.h"
#include "include/rados/librados.hpp"
#include "cls/lock/cls_lock_types.h"
#include "librbd/managed_lock/Types.h"

This comment has been minimized.

@dillaman

dillaman Jan 13, 2017

Contributor

Nit: forward declare managed_lock::Locker

{
RWLock::RLocker l(ictx->owner_lock);
if (ictx->exclusive_lock == nullptr) {

This comment has been minimized.

@dillaman

dillaman Jan 13, 2017

Contributor

We should support getting the lock owner when opened in read-only mode, which implies that the exclusive lock won't be initialized.

This comment has been minimized.

@trociny

trociny Jan 13, 2017

Contributor

Then I suppose I can use ExclusiveLock(*ictx).get_locker(&locker, &get_owner_ctx) for this case, still looks a little better.

@trociny

This comment has been minimized.

Contributor

trociny commented Jan 13, 2017

@dillaman

  1. I would say they should be passed into ManagedLock as a constructor parameter and then passed to managed_lock::AcquireRequest as needed

Agree. There is a potential issue with this though. If a corresponding parameter in ImageCtx is updated by some reason, the exlusive_lock will still use the old value. Currently it looks like we don't update these params during ImageCtx life, this might be a surprise though when we want to do this in some future.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 13, 2017

@trociny There are a lot of parameters that, even if we eliminated the use of their ImageCtx replacement, would not function correctly w/ dynamic config updates. The blacklisting on lock break realistically should never be touched -- another example of way too many Ceph config options just to avoid hard-coding things.

@trociny trociny changed the title from [DNM] librbd: managed lock refactoring to librbd: managed lock refactoring Jan 15, 2017

@trociny

This comment has been minimized.

Contributor

trociny commented Jan 15, 2017

updated: forgot to remove unused headers from internal.cc

librados::IoCtx &m_ioctx;
CephContext *m_cct;
ContextWQ *m_work_queue;
std::string m_oid;
const Locker &m_locker;

This comment has been minimized.

@trociny

trociny Jan 16, 2017

Contributor

@dillaman Don't you think it would be safer to copy the locker in constructor? I just stepped on this in my LeaderWatcher, where I was calling break lock asynchronously and reset the locker not waiting for the request completed. No problem to change my code, but this was unexpected to me -- I had impression (may be wrong) we had a rule to copy input params like this.

This comment has been minimized.

@dillaman

dillaman Jan 16, 2017

Contributor

I'd be fine if you changed it to support your use-case. W/ the original use-case, there was no race since the caller wouldn't be touching the memory while the call was in-progress.

This comment has been minimized.

@trociny

trociny Jan 16, 2017

Contributor

Can I just update this PR to include this change?

This comment has been minimized.

@dillaman

dillaman Jan 16, 2017

Contributor

Sure

@trociny

This comment has been minimized.

Contributor

trociny commented Jan 16, 2017

@dillaman updated

@dillaman

lgtm

const std::string& oid, Watcher *watcher) {
return new ManagedLock(ioctx, work_queue, oid, watcher);
const std::string& oid, Watcher *watcher,
bool blacklist_on_break_lock = true,

This comment has been minimized.

@dillaman

dillaman Jan 16, 2017

Contributor

Nit: can you remove the defaulted params here and below?

Mykola Golub added some commits Jan 13, 2017

Mykola Golub
librbd: managed_lock: make AcquireRequest use GetLockRequest and Brea…
…kRequest

Initially this was implemented for exclusive_lock (03533b9,
23f60fe) but was lost when merging managed_lock refactoring.

Signed-off-by: Mykola Golub <mgolub@mirantis.com>
Mykola Golub
librbd: helper methods to query and break lock
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
@trociny

This comment has been minimized.

Contributor

trociny commented Jan 16, 2017

@dillaman updated

@dillaman dillaman merged commit 06a4e14 into ceph:master Jan 17, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@trociny trociny deleted the trociny:wip-managed_lock_refactoring branch Feb 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment