| Message ID | 20240724062914.667734-4-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rbd: exclusive mapping (-o exclusive) fixes | expand |
在 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; > } > >
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
在 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 >
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
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; }
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(-)