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: API methods to directly acquire and release the exclusive lock #9592

Merged
merged 1 commit into from Sep 5, 2016

Conversation

Projects
None yet
2 participants
@trociny
Contributor

trociny commented Jun 8, 2016

No description provided.

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Jun 8, 2016

Contributor

@dillaman It is still WIP and serves more as an illustration to the questions I have.

I am not sure how it should coexist with exclusive-lock image feature. If exclusive-lock is not enabled, then it looks straightforward. But if the feature is enabled and the lock is requested, how safe it is to change the policy, and the back to StandardPolicy, when the lock is released?

E.g. consider the situation: exclusive-lock feature is enabled, the image is locked after write is started. Then the user acquires exclusive lock via API, I suppose it should return success. Then the user releases the lock via API, the image is left unlocked while initially it was locked.

Another issue, in theory when the exclusive lock is acquired via API, the policy may differ from Standard. When releasing the lock via API, I suppose the old policy is should be restored?

Contributor

trociny commented Jun 8, 2016

@dillaman It is still WIP and serves more as an illustration to the questions I have.

I am not sure how it should coexist with exclusive-lock image feature. If exclusive-lock is not enabled, then it looks straightforward. But if the feature is enabled and the lock is requested, how safe it is to change the policy, and the back to StandardPolicy, when the lock is released?

E.g. consider the situation: exclusive-lock feature is enabled, the image is locked after write is started. Then the user acquires exclusive lock via API, I suppose it should return success. Then the user releases the lock via API, the image is left unlocked while initially it was locked.

Another issue, in theory when the exclusive lock is acquired via API, the policy may differ from Standard. When releasing the lock via API, I suppose the old policy is should be restored?

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Jun 9, 2016

Contributor

@dillaman So it looks to me it would be (at least) confusing to use exclusive-lock API together with exclusive-lock feature enabled. Still exclusive-lock feature is needed to enable other useful features.

May be we should add a new feature, something like manual-exclusive-lock? It should be used instead (or together with) exclusive-lock, and when it is enabled we don't activate/transfer exclusive lock automatically, and any write operation fails with EPERM (EROFS, EINVAL?) until the lock is explicitly acquired by client via API?

Contributor

trociny commented Jun 9, 2016

@dillaman So it looks to me it would be (at least) confusing to use exclusive-lock API together with exclusive-lock feature enabled. Still exclusive-lock feature is needed to enable other useful features.

May be we should add a new feature, something like manual-exclusive-lock? It should be used instead (or together with) exclusive-lock, and when it is enabled we don't activate/transfer exclusive lock automatically, and any write operation fails with EPERM (EROFS, EINVAL?) until the lock is explicitly acquired by client via API?

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman Jun 15, 2016

Contributor

@trociny Sorry for the delay getting back to PRs.

I am not sure how it should coexist with exclusive-lock image feature. If exclusive-lock is not enabled, then it looks straightforward. But if the feature is enabled and the lock is requested, how safe it is to change the policy, and the back to StandardPolicy, when the lock is released?

With the proper locking of owner_lock, it shouldn't be an issue to swap out the policy. As an alternative, perhaps we don't swap out StandardPolicy but instead add block_release and unblock_release methods to that interface?

E.g. consider the situation: exclusive-lock feature is enabled, the image is locked after write is started. Then the user acquires exclusive lock via API, I suppose it should return success. Then the user releases the lock via API, the image is left unlocked while initially it was locked.

Well the good thing is that the IO will force to to automatically re-acquire the lock if needed. Long term, I'd actually like to see the automatic lock grabbing removed since it was always a workaround for QEMU's lack of locking and the need to support the live-migration case.

Another issue, in theory when the exclusive lock is acquired via API, the policy may differ from Standard. When releasing the lock via API, I suppose the old policy is should be restored?

For librbd, it should always be StandardPolicy -- but for safety we could add the block/unblock_release methods.

Contributor

dillaman commented Jun 15, 2016

@trociny Sorry for the delay getting back to PRs.

I am not sure how it should coexist with exclusive-lock image feature. If exclusive-lock is not enabled, then it looks straightforward. But if the feature is enabled and the lock is requested, how safe it is to change the policy, and the back to StandardPolicy, when the lock is released?

With the proper locking of owner_lock, it shouldn't be an issue to swap out the policy. As an alternative, perhaps we don't swap out StandardPolicy but instead add block_release and unblock_release methods to that interface?

E.g. consider the situation: exclusive-lock feature is enabled, the image is locked after write is started. Then the user acquires exclusive lock via API, I suppose it should return success. Then the user releases the lock via API, the image is left unlocked while initially it was locked.

Well the good thing is that the IO will force to to automatically re-acquire the lock if needed. Long term, I'd actually like to see the automatic lock grabbing removed since it was always a workaround for QEMU's lack of locking and the need to support the live-migration case.

Another issue, in theory when the exclusive lock is acquired via API, the policy may differ from Standard. When releasing the lock via API, I suppose the old policy is should be restored?

For librbd, it should always be StandardPolicy -- but for safety we could add the block/unblock_release methods.

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman Jun 15, 2016

Contributor

So it looks to me it would be (at least) confusing to use exclusive-lock API together with exclusive-lock feature enabled. Still exclusive-lock feature is needed to enable other useful features.

Hmm -- I guess I just see this as essentially exclusive-lock w/ the auto-lock acquire disabled. Perhaps I see it in the same vain as our writethrough-until-flush backend tweaks. Before we know that you are properly using the lock API, we treat it as auto-lock. Maybe even a new config option to disable auto-lock requests?

Contributor

dillaman commented Jun 15, 2016

So it looks to me it would be (at least) confusing to use exclusive-lock API together with exclusive-lock feature enabled. Still exclusive-lock feature is needed to enable other useful features.

Hmm -- I guess I just see this as essentially exclusive-lock w/ the auto-lock acquire disabled. Perhaps I see it in the same vain as our writethrough-until-flush backend tweaks. Before we know that you are properly using the lock API, we treat it as auto-lock. Maybe even a new config option to disable auto-lock requests?

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Jun 15, 2016

Contributor

@dillaman I like your analogy with writethrough-until-flush. I think we don't need (at least for now) new block_release/unblock_release methods then, because we will change the behavior only once, on the first API request, and just changing the Policy should work.

So, the plan could be:

  1. Rename Standard policy to AutoPolicy (AutomaticTransferPolicy, AutomaticReleasePolicy?).

  2. Provide Standard policy with lock_requested() method being noop (ignore the lock request).

  3. Add config option rbd_exclusive_lock_auto_until_requested (default is true).

  4. On initialization, if rbd_exclusive_lock_auto_until_requested is false, set policy to Standard. If it is true set to Auto, any update operation will automatically request the lock.

  5. When the lock is requested via API and rbd_exclusive_lock_auto_until_requested is true (current policy AutoLock), change the policy to Standard and disable automatic lock request/release on update operations.

What do you think?

Contributor

trociny commented Jun 15, 2016

@dillaman I like your analogy with writethrough-until-flush. I think we don't need (at least for now) new block_release/unblock_release methods then, because we will change the behavior only once, on the first API request, and just changing the Policy should work.

So, the plan could be:

  1. Rename Standard policy to AutoPolicy (AutomaticTransferPolicy, AutomaticReleasePolicy?).

  2. Provide Standard policy with lock_requested() method being noop (ignore the lock request).

  3. Add config option rbd_exclusive_lock_auto_until_requested (default is true).

  4. On initialization, if rbd_exclusive_lock_auto_until_requested is false, set policy to Standard. If it is true set to Auto, any update operation will automatically request the lock.

  5. When the lock is requested via API and rbd_exclusive_lock_auto_until_requested is true (current policy AutoLock), change the policy to Standard and disable automatic lock request/release on update operations.

What do you think?

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Jun 29, 2016

Contributor

@dillaman here some working implementation (not sure I found all (and the best) places to hook the checks.

A new configuration option rbd_exclusive_lock_auto_until_request is added.
If the option is true (default), the behavior is as it was previously (auto release) until the exclusive lock is acquired via API. After this it is not automatically released any more, only via API; if an IO operation requires exclusive lock and it is currently released it fails with EINVAL (instead of auto acquiring the lock); acquiring the lock will block until other client releases the lock.

Contributor

trociny commented Jun 29, 2016

@dillaman here some working implementation (not sure I found all (and the best) places to hook the checks.

A new configuration option rbd_exclusive_lock_auto_until_request is added.
If the option is true (default), the behavior is as it was previously (auto release) until the exclusive lock is acquired via API. After this it is not automatically released any more, only via API; if an IO operation requires exclusive lock and it is currently released it fails with EINVAL (instead of auto acquiring the lock); acquiring the lock will block until other client releases the lock.

Show outdated Hide outdated src/librbd/ImageCtx.h
m_image_ctx->exclusive_lock->release_lock(nullptr);
ldout(m_image_ctx->cct, 20) << this << " " << __func__ << ": force=" << force
<< dendl;

This comment has been minimized.

@dillaman

dillaman Aug 9, 2016

Contributor

Might be nice to send a -EROFS error back to the requester of the lock -- otherwise they will keep trying forever and this process will keep ignoring them.

@dillaman

dillaman Aug 9, 2016

Contributor

Might be nice to send a -EROFS error back to the requester of the lock -- otherwise they will keep trying forever and this process will keep ignoring them.

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman Aug 9, 2016

Contributor

@trociny sorry for the delay --- commented

Contributor

dillaman commented Aug 9, 2016

@trociny sorry for the delay --- commented

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Aug 15, 2016

Contributor

@dillaman updated.

Not sure about a new policy method though, how understandable it would look for other people who read the code. May be it is just due to wrong naming?

Contributor

trociny commented Aug 15, 2016

@dillaman updated.

Not sure about a new policy method though, how understandable it would look for other people who read the code. May be it is just due to wrong naming?

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman Aug 16, 2016

Contributor

@trociny Perhaps "CooporativeReleasePolicy" vs "ManualReleasePolicy"?

Contributor

dillaman commented Aug 16, 2016

@trociny Perhaps "CooporativeReleasePolicy" vs "ManualReleasePolicy"?

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Aug 22, 2016

Contributor

@dillaman

Perhaps "CooporativeReleasePolicy" vs "ManualReleasePolicy"?

You are talking about renaming "AutomaticPolicy" to "CooperativeReleasePolicy" and "StandardPolicy" to "ManualReleasePolicy"?

It might be better names. Then may be rbd_auto_exclusive_lock_until_manual_request option should be differently named? E.g:

 rbd_exclusive_lock_cooperative_release_policy_until_manual_request

(thought it is too long...)

But my main concern is not policy names, but a new method Policy::is_auto() I have added.

It is used in librbd::lock_acquire():

   if (ictx->get_exclusive_lock_policy()->is_auto()) {
     ictx->set_exclusive_lock_policy(
       new exclusive_lock::StandardPolicy(ictx));
   }

or, after the policy rename:

   if (ictx->get_exclusive_lock_policy()->is_auto()) {
     ictx->set_exclusive_lock_policy(
       new exclusive_lock::ManualReleasePolicy(ictx));
   }

and in AioImageRequestWQ::queue():

  if (lock_required && !m_image_ctx.get_exclusive_lock_policy()->is_auto()) {
     lderr(cct) << "op requires exclusive lock" << dendl;
     req->fail(-EROFS);

I don't very like this. May by, after renaming the config option to rbd_exclusive_lock_cooperative_release_policy_until_manual_request, in librbd::lock_acquire() we should rather check that CooperativeReleasePolicy is still used (e.g. by introducing Policy::type() method, or using dynamic_cast trick)? So is_auto() method will be used only in AioImageRequestWQ::queue(), and renamed to something like may_request_lock(), may_auto_request_lock(), or may_request_lock_release()...

Contributor

trociny commented Aug 22, 2016

@dillaman

Perhaps "CooporativeReleasePolicy" vs "ManualReleasePolicy"?

You are talking about renaming "AutomaticPolicy" to "CooperativeReleasePolicy" and "StandardPolicy" to "ManualReleasePolicy"?

It might be better names. Then may be rbd_auto_exclusive_lock_until_manual_request option should be differently named? E.g:

 rbd_exclusive_lock_cooperative_release_policy_until_manual_request

(thought it is too long...)

But my main concern is not policy names, but a new method Policy::is_auto() I have added.

It is used in librbd::lock_acquire():

   if (ictx->get_exclusive_lock_policy()->is_auto()) {
     ictx->set_exclusive_lock_policy(
       new exclusive_lock::StandardPolicy(ictx));
   }

or, after the policy rename:

   if (ictx->get_exclusive_lock_policy()->is_auto()) {
     ictx->set_exclusive_lock_policy(
       new exclusive_lock::ManualReleasePolicy(ictx));
   }

and in AioImageRequestWQ::queue():

  if (lock_required && !m_image_ctx.get_exclusive_lock_policy()->is_auto()) {
     lderr(cct) << "op requires exclusive lock" << dendl;
     req->fail(-EROFS);

I don't very like this. May by, after renaming the config option to rbd_exclusive_lock_cooperative_release_policy_until_manual_request, in librbd::lock_acquire() we should rather check that CooperativeReleasePolicy is still used (e.g. by introducing Policy::type() method, or using dynamic_cast trick)? So is_auto() method will be used only in AioImageRequestWQ::queue(), and renamed to something like may_request_lock(), may_auto_request_lock(), or may_request_lock_release()...

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman Aug 23, 2016

Contributor

@trociny Yeah, I was on the fence about is_auto as well. I like your suggestion for may_request_lock or may_auto_request_lock.

I am fine keeping AutomaticPolicy now that I know it wasn't your original concern.

Contributor

dillaman commented Aug 23, 2016

@trociny Yeah, I was on the fence about is_auto as well. I like your suggestion for may_request_lock or may_auto_request_lock.

I am fine keeping AutomaticPolicy now that I know it wasn't your original concern.

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Aug 25, 2016

Contributor

@dillaman Thanks. So I have just renamed is_auto() to may_auto_request_lock().

Still, there is a case where it behaves not as I suppose it is expected (see the comment in TestLibRBD.ExclusiveLock unit test):

When one client opens the image and requests the lock explicitly via API, and then another client opens the image and tries to write, it blocks, though the suggested behavior would be to return -EROFS. It is because ImageWatcher<I>::handle_request_lock just reschedules request lock after the error.

Requesting the lock via API (when it is already locked via API by other client) will also block, but this is I think is desirable behavior.

Contributor

trociny commented Aug 25, 2016

@dillaman Thanks. So I have just renamed is_auto() to may_auto_request_lock().

Still, there is a case where it behaves not as I suppose it is expected (see the comment in TestLibRBD.ExclusiveLock unit test):

When one client opens the image and requests the lock explicitly via API, and then another client opens the image and tries to write, it blocks, though the suggested behavior would be to return -EROFS. It is because ImageWatcher<I>::handle_request_lock just reschedules request lock after the error.

Requesting the lock via API (when it is already locked via API by other client) will also block, but this is I think is desirable behavior.

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman Aug 25, 2016

Contributor

@trociny I think the blocking behavior is fine. Perhaps on the API-side we should have some way to force acquire (i.e. automatically blacklist current lock owner if any) ... probably could wait but would require a new API method when its added.

Contributor

dillaman commented Aug 25, 2016

@trociny I think the blocking behavior is fine. Perhaps on the API-side we should have some way to force acquire (i.e. automatically blacklist current lock owner if any) ... probably could wait but would require a new API method when its added.

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Aug 25, 2016

Contributor

In order not to provide a new API method when adding "force acquire" feature, we could either just add a new mode enum (RBD_LOCK_MODE_EXCLUSIVE_FORCE), or initially use flags instead of mode in our API:

typedef enum {
  RBD_LOCK_FLAGS_EXCLUSIVE = 1 << 0,
  RBD_LOCK_FLAGS_SHARED = 1 << 1
} rbd_lock_flags_t;

CEPH_RBD_API int rbd_lock_acquire(rbd_image_t image, rbd_lock_flags_t lock_flags);
CEPH_RBD_API int rbd_lock_release(rbd_image_t image);

So RBD_LOCK_FLAGS_FORCE could be added (somewhat similar to libc open(2)). I like the first variant (RBD_LOCK_MODE_EXCLUSIVE_FORCE mode) a little more.

@dillaman What do you think?

Contributor

trociny commented Aug 25, 2016

In order not to provide a new API method when adding "force acquire" feature, we could either just add a new mode enum (RBD_LOCK_MODE_EXCLUSIVE_FORCE), or initially use flags instead of mode in our API:

typedef enum {
  RBD_LOCK_FLAGS_EXCLUSIVE = 1 << 0,
  RBD_LOCK_FLAGS_SHARED = 1 << 1
} rbd_lock_flags_t;

CEPH_RBD_API int rbd_lock_acquire(rbd_image_t image, rbd_lock_flags_t lock_flags);
CEPH_RBD_API int rbd_lock_release(rbd_image_t image);

So RBD_LOCK_FLAGS_FORCE could be added (somewhat similar to libc open(2)). I like the first variant (RBD_LOCK_MODE_EXCLUSIVE_FORCE mode) a little more.

@dillaman What do you think?

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman Aug 26, 2016

Contributor

@trociny +1 to sticking with the enum. Does this still need the [RFC] prefix?

Contributor

dillaman commented Aug 26, 2016

@trociny +1 to sticking with the enum. Does this still need the [RFC] prefix?

Mykola Golub

@trociny trociny changed the title from [RFC] librbd: API methods to directly acquire and release the exclusive lock to librbd: API methods to directly acquire and release the exclusive lock Aug 26, 2016

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Aug 26, 2016

Contributor

@dillaman I have removed [RFC] prefix.

Contributor

trociny commented Aug 26, 2016

@dillaman I have removed [RFC] prefix.

@dillaman dillaman merged commit fdd2364 into ceph:master Sep 5, 2016

2 checks passed

Signed-off-by all commits in this PR are signed
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment