* [PATCH 0/3] rbd: exclusive mapping (-o exclusive) fixes
@ 2024-07-24 6:29 Ilya Dryomov
2024-07-24 6:29 ` [PATCH 1/3] rbd: rename RBD_LOCK_STATE_RELEASING and releasing_wait Ilya Dryomov
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ilya Dryomov @ 2024-07-24 6:29 UTC (permalink / raw)
To: ceph-devel; +Cc: Dongsheng Yang
Hello,
This addresses incorrect assumptions on rbd_dev->lock_state in
rbd_img_exclusive_lock() and rbd_add_acquire_lock() which could lead to
issues with exclusive mappings in the face of watch errors.
Thanks,
Ilya
Ilya Dryomov (3):
rbd: rename RBD_LOCK_STATE_RELEASING and releasing_wait
rbd: don't assume RBD_LOCK_STATE_LOCKED for exclusive mappings
rbd: don't assume rbd_is_lock_owner() for exclusive mappings
drivers/block/rbd.c | 35 +++++++++++++++--------------------
1 file changed, 15 insertions(+), 20 deletions(-)
--
2.45.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] rbd: rename RBD_LOCK_STATE_RELEASING and releasing_wait
2024-07-24 6:29 [PATCH 0/3] rbd: exclusive mapping (-o exclusive) fixes Ilya Dryomov
@ 2024-07-24 6:29 ` Ilya Dryomov
2024-07-25 8:46 ` Dongsheng Yang
2024-07-24 6:29 ` [PATCH 2/3] rbd: don't assume RBD_LOCK_STATE_LOCKED for exclusive mappings Ilya Dryomov
2024-07-24 6:29 ` [PATCH 3/3] rbd: don't assume rbd_is_lock_owner() " Ilya Dryomov
2 siblings, 1 reply; 9+ messages in thread
From: Ilya Dryomov @ 2024-07-24 6:29 UTC (permalink / raw)
To: ceph-devel; +Cc: Dongsheng Yang
... to RBD_LOCK_STATE_QUIESCING to quiescing_wait to recognize that
this state and the associated completion are backing rbd_quiesce_lock(),
which isn't specific to releasing the lock.
While exclusive lock does get quiesced before it's released, it also
gets quiesced before an attempt to update the cookie is made and there
the lock is not released as long as ceph_cls_set_cookie() succeeds.
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
drivers/block/rbd.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 9c6cff54831f..77a9f19a0035 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -362,7 +362,7 @@ enum rbd_watch_state {
enum rbd_lock_state {
RBD_LOCK_STATE_UNLOCKED,
RBD_LOCK_STATE_LOCKED,
- RBD_LOCK_STATE_RELEASING,
+ RBD_LOCK_STATE_QUIESCING,
};
/* WatchNotify::ClientId */
@@ -422,7 +422,7 @@ struct rbd_device {
struct list_head running_list;
struct completion acquire_wait;
int acquire_err;
- struct completion releasing_wait;
+ struct completion quiescing_wait;
spinlock_t object_map_lock;
u8 *object_map;
@@ -525,7 +525,7 @@ static bool __rbd_is_lock_owner(struct rbd_device *rbd_dev)
lockdep_assert_held(&rbd_dev->lock_rwsem);
return rbd_dev->lock_state == RBD_LOCK_STATE_LOCKED ||
- rbd_dev->lock_state == RBD_LOCK_STATE_RELEASING;
+ rbd_dev->lock_state == RBD_LOCK_STATE_QUIESCING;
}
static bool rbd_is_lock_owner(struct rbd_device *rbd_dev)
@@ -3458,12 +3458,12 @@ static void rbd_lock_del_request(struct rbd_img_request *img_req)
spin_lock(&rbd_dev->lock_lists_lock);
if (!list_empty(&img_req->lock_item)) {
list_del_init(&img_req->lock_item);
- need_wakeup = (rbd_dev->lock_state == RBD_LOCK_STATE_RELEASING &&
+ need_wakeup = (rbd_dev->lock_state == RBD_LOCK_STATE_QUIESCING &&
list_empty(&rbd_dev->running_list));
}
spin_unlock(&rbd_dev->lock_lists_lock);
if (need_wakeup)
- complete(&rbd_dev->releasing_wait);
+ complete(&rbd_dev->quiescing_wait);
}
static int rbd_img_exclusive_lock(struct rbd_img_request *img_req)
@@ -4181,16 +4181,16 @@ static bool rbd_quiesce_lock(struct rbd_device *rbd_dev)
/*
* Ensure that all in-flight IO is flushed.
*/
- rbd_dev->lock_state = RBD_LOCK_STATE_RELEASING;
- rbd_assert(!completion_done(&rbd_dev->releasing_wait));
+ rbd_dev->lock_state = RBD_LOCK_STATE_QUIESCING;
+ rbd_assert(!completion_done(&rbd_dev->quiescing_wait));
if (list_empty(&rbd_dev->running_list))
return true;
up_write(&rbd_dev->lock_rwsem);
- wait_for_completion(&rbd_dev->releasing_wait);
+ wait_for_completion(&rbd_dev->quiescing_wait);
down_write(&rbd_dev->lock_rwsem);
- if (rbd_dev->lock_state != RBD_LOCK_STATE_RELEASING)
+ if (rbd_dev->lock_state != RBD_LOCK_STATE_QUIESCING)
return false;
rbd_assert(list_empty(&rbd_dev->running_list));
@@ -5383,7 +5383,7 @@ static struct rbd_device *__rbd_dev_create(struct rbd_spec *spec)
INIT_LIST_HEAD(&rbd_dev->acquiring_list);
INIT_LIST_HEAD(&rbd_dev->running_list);
init_completion(&rbd_dev->acquire_wait);
- init_completion(&rbd_dev->releasing_wait);
+ init_completion(&rbd_dev->quiescing_wait);
spin_lock_init(&rbd_dev->object_map_lock);
--
2.45.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] rbd: don't assume RBD_LOCK_STATE_LOCKED for exclusive mappings
2024-07-24 6:29 [PATCH 0/3] rbd: exclusive mapping (-o exclusive) fixes Ilya Dryomov
2024-07-24 6:29 ` [PATCH 1/3] rbd: rename RBD_LOCK_STATE_RELEASING and releasing_wait Ilya Dryomov
@ 2024-07-24 6:29 ` Ilya Dryomov
2024-07-24 6:29 ` [PATCH 3/3] rbd: don't assume rbd_is_lock_owner() " Ilya Dryomov
2 siblings, 0 replies; 9+ messages in thread
From: Ilya Dryomov @ 2024-07-24 6:29 UTC (permalink / raw)
To: ceph-devel; +Cc: Dongsheng Yang
Every time a watch is reestablished after getting lost, we need to
update the cookie which involves quiescing exclusive lock. For this,
we transition from RBD_LOCK_STATE_LOCKED to RBD_LOCK_STATE_QUIESCING
roughly for the duration of rbd_reacquire_lock() call. If the mapping
is exclusive and I/O happens to arrive in this time window, it's failed
with EROFS (later translated to EIO) based on the wrong assumption in
rbd_img_exclusive_lock() -- "lock got released?" check there stopped
making sense with commit a2b1da09793d ("rbd: lock should be quiesced on
reacquire").
To make it worse, any such I/O is added to the acquiring list before
EROFS is returned and this sets up for violating rbd_lock_del_request()
precondition that the request is either on the running list or not on
any list at all -- see commit ded080c86b3f ("rbd: don't move requests
to the running list on errors"). rbd_lock_del_request() ends up
processing these requests as if they were on the running list which
screws up quiescing_wait completion counter and ultimately leads to
rbd_assert(!completion_done(&rbd_dev->quiescing_wait));
being triggered on the next watch error.
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
drivers/block/rbd.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 77a9f19a0035..dc4ddae4f7eb 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3457,6 +3457,7 @@ static void rbd_lock_del_request(struct rbd_img_request *img_req)
lockdep_assert_held(&rbd_dev->lock_rwsem);
spin_lock(&rbd_dev->lock_lists_lock);
if (!list_empty(&img_req->lock_item)) {
+ rbd_assert(!list_empty(&rbd_dev->running_list));
list_del_init(&img_req->lock_item);
need_wakeup = (rbd_dev->lock_state == RBD_LOCK_STATE_QUIESCING &&
list_empty(&rbd_dev->running_list));
@@ -3476,11 +3477,6 @@ static int rbd_img_exclusive_lock(struct rbd_img_request *img_req)
if (rbd_lock_add_request(img_req))
return 1;
- if (rbd_dev->opts->exclusive) {
- WARN_ON(1); /* lock got released? */
- return -EROFS;
- }
-
/*
* Note the use of mod_delayed_work() in rbd_acquire_lock()
* and cancel_delayed_work() in wake_lock_waiters().
@@ -4601,6 +4597,10 @@ static void rbd_reacquire_lock(struct rbd_device *rbd_dev)
rbd_warn(rbd_dev, "failed to update lock cookie: %d",
ret);
+ if (rbd_dev->opts->exclusive)
+ rbd_warn(rbd_dev,
+ "temporarily releasing lock on exclusive mapping");
+
/*
* Lock cookie cannot be updated on older OSDs, so do
* a manual release and queue an acquire.
--
2.45.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] rbd: don't assume rbd_is_lock_owner() for exclusive mappings
2024-07-24 6:29 [PATCH 0/3] rbd: exclusive mapping (-o exclusive) fixes Ilya Dryomov
2024-07-24 6:29 ` [PATCH 1/3] rbd: rename RBD_LOCK_STATE_RELEASING and releasing_wait Ilya Dryomov
2024-07-24 6:29 ` [PATCH 2/3] rbd: don't assume RBD_LOCK_STATE_LOCKED for exclusive mappings Ilya Dryomov
@ 2024-07-24 6:29 ` Ilya Dryomov
2024-07-25 8:45 ` Dongsheng Yang
2 siblings, 1 reply; 9+ messages in thread
From: Ilya Dryomov @ 2024-07-24 6:29 UTC (permalink / raw)
To: ceph-devel; +Cc: Dongsheng Yang
Expanding on the previous commit, assuming that rbd_is_lock_owner()
always returns true (i.e. that we are either in RBD_LOCK_STATE_LOCKED
or RBD_LOCK_STATE_QUIESCING) if the mapping is exclusive is wrong too.
In case ceph_cls_set_cookie() fails, the lock would be temporarily
released even if the mapping is exclusive, meaning that we can end up
even in RBD_LOCK_STATE_UNLOCKED.
IOW, exclusive mappings are really "just" about disabling automatic
lock transitions (as documented in the man page), not about grabbing
the lock and holding on to it whatever it takes.
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
drivers/block/rbd.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index dc4ddae4f7eb..b8e6700d65f8 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -6589,11 +6589,6 @@ static int rbd_add_acquire_lock(struct rbd_device *rbd_dev)
if (ret)
return ret;
- /*
- * The lock may have been released by now, unless automatic lock
- * transitions are disabled.
- */
- rbd_assert(!rbd_dev->opts->exclusive || rbd_is_lock_owner(rbd_dev));
return 0;
}
--
2.45.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] rbd: don't assume rbd_is_lock_owner() for exclusive mappings
2024-07-24 6:29 ` [PATCH 3/3] rbd: don't assume rbd_is_lock_owner() " Ilya Dryomov
@ 2024-07-25 8:45 ` Dongsheng Yang
2024-07-25 9:31 ` Ilya Dryomov
0 siblings, 1 reply; 9+ messages in thread
From: Dongsheng Yang @ 2024-07-25 8:45 UTC (permalink / raw)
To: Ilya Dryomov, ceph-devel
在 2024/7/24 星期三 下午 2:29, Ilya Dryomov 写道:
> Expanding on the previous commit, assuming that rbd_is_lock_owner()
> always returns true (i.e. that we are either in RBD_LOCK_STATE_LOCKED
> or RBD_LOCK_STATE_QUIESCING) if the mapping is exclusive is wrong too.
> In case ceph_cls_set_cookie() fails, the lock would be temporarily
> released even if the mapping is exclusive, meaning that we can end up
> even in RBD_LOCK_STATE_UNLOCKED.
>
> IOW, exclusive mappings are really "just" about disabling automatic
> lock transitions (as documented in the man page), not about grabbing
> the lock and holding on to it whatever it takes.
Hi Ilya,
Could you explain more about "disabling atomic lock transitions"? To be
honest, I was thinking --exclusive means "grabbing
the lock and holding on to it whatever it takes."
Thanx
>
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
> drivers/block/rbd.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index dc4ddae4f7eb..b8e6700d65f8 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -6589,11 +6589,6 @@ static int rbd_add_acquire_lock(struct rbd_device *rbd_dev)
> if (ret)
> return ret;
>
> - /*
> - * The lock may have been released by now, unless automatic lock
> - * transitions are disabled.
> - */
> - rbd_assert(!rbd_dev->opts->exclusive || rbd_is_lock_owner(rbd_dev));
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] rbd: rename RBD_LOCK_STATE_RELEASING and releasing_wait
2024-07-24 6:29 ` [PATCH 1/3] rbd: rename RBD_LOCK_STATE_RELEASING and releasing_wait Ilya Dryomov
@ 2024-07-25 8:46 ` Dongsheng Yang
0 siblings, 0 replies; 9+ messages in thread
From: Dongsheng Yang @ 2024-07-25 8:46 UTC (permalink / raw)
To: Ilya Dryomov, ceph-devel
在 2024/7/24 星期三 下午 2:29, Ilya Dryomov 写道:
> ... to RBD_LOCK_STATE_QUIESCING to quiescing_wait to recognize that
Hi Ilya,
s/to quiescing_wait/and quiescing_wait
Otherwise:
Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
> this state and the associated completion are backing rbd_quiesce_lock(),
> which isn't specific to releasing the lock.
>
> While exclusive lock does get quiesced before it's released, it also
> gets quiesced before an attempt to update the cookie is made and there
> the lock is not released as long as ceph_cls_set_cookie() succeeds.
>
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
> drivers/block/rbd.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 9c6cff54831f..77a9f19a0035 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -362,7 +362,7 @@ enum rbd_watch_state {
> enum rbd_lock_state {
> RBD_LOCK_STATE_UNLOCKED,
> RBD_LOCK_STATE_LOCKED,
> - RBD_LOCK_STATE_RELEASING,
> + RBD_LOCK_STATE_QUIESCING,
> };
>
> /* WatchNotify::ClientId */
> @@ -422,7 +422,7 @@ struct rbd_device {
> struct list_head running_list;
> struct completion acquire_wait;
> int acquire_err;
> - struct completion releasing_wait;
> + struct completion quiescing_wait;
>
> spinlock_t object_map_lock;
> u8 *object_map;
> @@ -525,7 +525,7 @@ static bool __rbd_is_lock_owner(struct rbd_device *rbd_dev)
> lockdep_assert_held(&rbd_dev->lock_rwsem);
>
> return rbd_dev->lock_state == RBD_LOCK_STATE_LOCKED ||
> - rbd_dev->lock_state == RBD_LOCK_STATE_RELEASING;
> + rbd_dev->lock_state == RBD_LOCK_STATE_QUIESCING;
> }
>
> static bool rbd_is_lock_owner(struct rbd_device *rbd_dev)
> @@ -3458,12 +3458,12 @@ static void rbd_lock_del_request(struct rbd_img_request *img_req)
> spin_lock(&rbd_dev->lock_lists_lock);
> if (!list_empty(&img_req->lock_item)) {
> list_del_init(&img_req->lock_item);
> - need_wakeup = (rbd_dev->lock_state == RBD_LOCK_STATE_RELEASING &&
> + need_wakeup = (rbd_dev->lock_state == RBD_LOCK_STATE_QUIESCING &&
> list_empty(&rbd_dev->running_list));
> }
> spin_unlock(&rbd_dev->lock_lists_lock);
> if (need_wakeup)
> - complete(&rbd_dev->releasing_wait);
> + complete(&rbd_dev->quiescing_wait);
> }
>
> static int rbd_img_exclusive_lock(struct rbd_img_request *img_req)
> @@ -4181,16 +4181,16 @@ static bool rbd_quiesce_lock(struct rbd_device *rbd_dev)
> /*
> * Ensure that all in-flight IO is flushed.
> */
> - rbd_dev->lock_state = RBD_LOCK_STATE_RELEASING;
> - rbd_assert(!completion_done(&rbd_dev->releasing_wait));
> + rbd_dev->lock_state = RBD_LOCK_STATE_QUIESCING;
> + rbd_assert(!completion_done(&rbd_dev->quiescing_wait));
> if (list_empty(&rbd_dev->running_list))
> return true;
>
> up_write(&rbd_dev->lock_rwsem);
> - wait_for_completion(&rbd_dev->releasing_wait);
> + wait_for_completion(&rbd_dev->quiescing_wait);
>
> down_write(&rbd_dev->lock_rwsem);
> - if (rbd_dev->lock_state != RBD_LOCK_STATE_RELEASING)
> + if (rbd_dev->lock_state != RBD_LOCK_STATE_QUIESCING)
> return false;
>
> rbd_assert(list_empty(&rbd_dev->running_list));
> @@ -5383,7 +5383,7 @@ static struct rbd_device *__rbd_dev_create(struct rbd_spec *spec)
> INIT_LIST_HEAD(&rbd_dev->acquiring_list);
> INIT_LIST_HEAD(&rbd_dev->running_list);
> init_completion(&rbd_dev->acquire_wait);
> - init_completion(&rbd_dev->releasing_wait);
> + init_completion(&rbd_dev->quiescing_wait);
>
> spin_lock_init(&rbd_dev->object_map_lock);
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] rbd: don't assume rbd_is_lock_owner() for exclusive mappings
2024-07-25 8:45 ` Dongsheng Yang
@ 2024-07-25 9:31 ` Ilya Dryomov
2024-07-25 10:08 ` Dongsheng Yang
0 siblings, 1 reply; 9+ messages in thread
From: Ilya Dryomov @ 2024-07-25 9:31 UTC (permalink / raw)
To: Dongsheng Yang; +Cc: ceph-devel
On Thu, Jul 25, 2024 at 10:45 AM Dongsheng Yang
<dongsheng.yang@linux.dev> wrote:
>
>
>
> 在 2024/7/24 星期三 下午 2:29, Ilya Dryomov 写道:
> > Expanding on the previous commit, assuming that rbd_is_lock_owner()
> > always returns true (i.e. that we are either in RBD_LOCK_STATE_LOCKED
> > or RBD_LOCK_STATE_QUIESCING) if the mapping is exclusive is wrong too.
> > In case ceph_cls_set_cookie() fails, the lock would be temporarily
> > released even if the mapping is exclusive, meaning that we can end up
> > even in RBD_LOCK_STATE_UNLOCKED.
> >
> > IOW, exclusive mappings are really "just" about disabling automatic
> > lock transitions (as documented in the man page), not about grabbing
> > the lock and holding on to it whatever it takes.
>
> Hi Ilya,
> Could you explain more about "disabling atomic lock transitions"? To be
> honest, I was thinking --exclusive means "grabbing
> the lock and holding on to it whatever it takes."
Hi Dongsheng,
Here are the relevant excerpts from the documentation [1]:
> To maintain multi-client access, the exclusive-lock feature
> implements automatic cooperative lock transitions between clients.
>
> Whenever a client that holds an exclusive lock on an RBD image gets
> a request to release the lock, it stops handling writes, flushes its
> caches and releases the lock.
>
> By default, the exclusive-lock feature does not prevent two or more
> concurrently running clients from opening the same RBD image and
> writing to it in turns (whether on the same node or not). In effect,
> their writes just get linearized as the lock is automatically
> transitioned back and forth in a cooperative fashion.
>
> To disable automatic lock transitions between clients, the
> RBD_LOCK_MODE_EXCLUSIVE flag may be specified when acquiring the
> exclusive lock. This is exposed by the --exclusive option for rbd
> device map command.
This is mostly equivalent to "grab the lock and hold on to it", but
it's not guaranteed that the lock would never be released. If a watch
error occurs, the lock cookie needs to be updated after the watch is
reestablished. If ceph_cls_set_cookie() fails, we have no choice but
to release the lock and immediately attempt to reacquire it because
otherwise the lock cookie would disagree with that of the new watch.
The code in question has always behaved this way. Prior to commit
14bb211d324d ("rbd: support updating the lock cookie without releasing
the lock"), the lock was (briefly) released on watch errors
unconditionally.
[1] https://docs.ceph.com/en/latest/rbd/rbd-exclusive-locks/
Thanks,
Ilya
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] rbd: don't assume rbd_is_lock_owner() for exclusive mappings
2024-07-25 9:31 ` Ilya Dryomov
@ 2024-07-25 10:08 ` Dongsheng Yang
2024-07-25 10:22 ` Ilya Dryomov
0 siblings, 1 reply; 9+ messages in thread
From: Dongsheng Yang @ 2024-07-25 10:08 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: ceph-devel
在 2024/7/25 星期四 下午 5:31, Ilya Dryomov 写道:
> On Thu, Jul 25, 2024 at 10:45 AM Dongsheng Yang
> <dongsheng.yang@linux.dev> wrote:
>>
>>
>>
>> 在 2024/7/24 星期三 下午 2:29, Ilya Dryomov 写道:
>>> Expanding on the previous commit, assuming that rbd_is_lock_owner()
>>> always returns true (i.e. that we are either in RBD_LOCK_STATE_LOCKED
>>> or RBD_LOCK_STATE_QUIESCING) if the mapping is exclusive is wrong too.
>>> In case ceph_cls_set_cookie() fails, the lock would be temporarily
>>> released even if the mapping is exclusive, meaning that we can end up
>>> even in RBD_LOCK_STATE_UNLOCKED.
>>>
>>> IOW, exclusive mappings are really "just" about disabling automatic
>>> lock transitions (as documented in the man page), not about grabbing
>>> the lock and holding on to it whatever it takes.
>>
>> Hi Ilya,
>> Could you explain more about "disabling atomic lock transitions"? To be
>> honest, I was thinking --exclusive means "grabbing
>> the lock and holding on to it whatever it takes."
>
> Hi Dongsheng,
>
> Here are the relevant excerpts from the documentation [1]:
>
>> To maintain multi-client access, the exclusive-lock feature
>> implements automatic cooperative lock transitions between clients.
>>
>> Whenever a client that holds an exclusive lock on an RBD image gets
>> a request to release the lock, it stops handling writes, flushes its
>> caches and releases the lock.
>>
>> By default, the exclusive-lock feature does not prevent two or more
>> concurrently running clients from opening the same RBD image and
>> writing to it in turns (whether on the same node or not). In effect,
>> their writes just get linearized as the lock is automatically
>> transitioned back and forth in a cooperative fashion.
>>
>> To disable automatic lock transitions between clients, the
>> RBD_LOCK_MODE_EXCLUSIVE flag may be specified when acquiring the
>> exclusive lock. This is exposed by the --exclusive option for rbd
>> device map command.
>
> This is mostly equivalent to "grab the lock and hold on to it", but
> it's not guaranteed that the lock would never be released. If a watch
> error occurs, the lock cookie needs to be updated after the watch is
> reestablished. If ceph_cls_set_cookie() fails, we have no choice but
> to release the lock and immediately attempt to reacquire it because
> otherwise the lock cookie would disagree with that of the new watch.
>
> The code in question has always behaved this way. Prior to commit
> 14bb211d324d ("rbd: support updating the lock cookie without releasing
> the lock"), the lock was (briefly) released on watch errors
> unconditionally.
Thanx Ilya, it clarify things. then
Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
For all of these 3 patches.
Thanx
>
> [1] https://docs.ceph.com/en/latest/rbd/rbd-exclusive-locks/
>
> Thanks,
>
> Ilya
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] rbd: don't assume rbd_is_lock_owner() for exclusive mappings
2024-07-25 10:08 ` Dongsheng Yang
@ 2024-07-25 10:22 ` Ilya Dryomov
0 siblings, 0 replies; 9+ messages in thread
From: Ilya Dryomov @ 2024-07-25 10:22 UTC (permalink / raw)
To: Dongsheng Yang; +Cc: ceph-devel
On Thu, Jul 25, 2024 at 12:08 PM Dongsheng Yang
<dongsheng.yang@linux.dev> wrote:
>
>
>
> 在 2024/7/25 星期四 下午 5:31, Ilya Dryomov 写道:
> > On Thu, Jul 25, 2024 at 10:45 AM Dongsheng Yang
> > <dongsheng.yang@linux.dev> wrote:
> >>
> >>
> >>
> >> 在 2024/7/24 星期三 下午 2:29, Ilya Dryomov 写道:
> >>> Expanding on the previous commit, assuming that rbd_is_lock_owner()
> >>> always returns true (i.e. that we are either in RBD_LOCK_STATE_LOCKED
> >>> or RBD_LOCK_STATE_QUIESCING) if the mapping is exclusive is wrong too.
> >>> In case ceph_cls_set_cookie() fails, the lock would be temporarily
> >>> released even if the mapping is exclusive, meaning that we can end up
> >>> even in RBD_LOCK_STATE_UNLOCKED.
> >>>
> >>> IOW, exclusive mappings are really "just" about disabling automatic
> >>> lock transitions (as documented in the man page), not about grabbing
> >>> the lock and holding on to it whatever it takes.
> >>
> >> Hi Ilya,
> >> Could you explain more about "disabling atomic lock transitions"? To be
> >> honest, I was thinking --exclusive means "grabbing
> >> the lock and holding on to it whatever it takes."
> >
> > Hi Dongsheng,
> >
> > Here are the relevant excerpts from the documentation [1]:
> >
> >> To maintain multi-client access, the exclusive-lock feature
> >> implements automatic cooperative lock transitions between clients.
> >>
> >> Whenever a client that holds an exclusive lock on an RBD image gets
> >> a request to release the lock, it stops handling writes, flushes its
> >> caches and releases the lock.
> >>
> >> By default, the exclusive-lock feature does not prevent two or more
> >> concurrently running clients from opening the same RBD image and
> >> writing to it in turns (whether on the same node or not). In effect,
> >> their writes just get linearized as the lock is automatically
> >> transitioned back and forth in a cooperative fashion.
> >>
> >> To disable automatic lock transitions between clients, the
> >> RBD_LOCK_MODE_EXCLUSIVE flag may be specified when acquiring the
> >> exclusive lock. This is exposed by the --exclusive option for rbd
> >> device map command.
> >
> > This is mostly equivalent to "grab the lock and hold on to it", but
> > it's not guaranteed that the lock would never be released. If a watch
> > error occurs, the lock cookie needs to be updated after the watch is
> > reestablished. If ceph_cls_set_cookie() fails, we have no choice but
> > to release the lock and immediately attempt to reacquire it because
> > otherwise the lock cookie would disagree with that of the new watch.
> >
> > The code in question has always behaved this way. Prior to commit
> > 14bb211d324d ("rbd: support updating the lock cookie without releasing
> > the lock"), the lock was (briefly) released on watch errors
> > unconditionally.
>
> Thanx Ilya, it clarify things. then
>
> Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
>
> For all of these 3 patches.
Thanks for the speedy review! I have fixed the typo in the description
of patch 1 and also appended stable tags to patches 2 and 3.
Ilya
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-07-25 10:23 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-24 6:29 [PATCH 0/3] rbd: exclusive mapping (-o exclusive) fixes Ilya Dryomov
2024-07-24 6:29 ` [PATCH 1/3] rbd: rename RBD_LOCK_STATE_RELEASING and releasing_wait Ilya Dryomov
2024-07-25 8:46 ` Dongsheng Yang
2024-07-24 6:29 ` [PATCH 2/3] rbd: don't assume RBD_LOCK_STATE_LOCKED for exclusive mappings Ilya Dryomov
2024-07-24 6:29 ` [PATCH 3/3] rbd: don't assume rbd_is_lock_owner() " Ilya Dryomov
2024-07-25 8:45 ` Dongsheng Yang
2024-07-25 9:31 ` Ilya Dryomov
2024-07-25 10:08 ` Dongsheng Yang
2024-07-25 10:22 ` Ilya Dryomov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox