Re: [PATCH] rbd: prevent busy loop when requesting exclusive lock

[Dongsheng Yang <dongsheng.yang@easystack.cn>]

在 2023/8/2 星期三 下午 2:41, Ilya Dryomov 写道:
> On Wed, Aug 2, 2023 at 8:36 AM Dongsheng Yang
> <dongsheng.yang@easystack.cn> wrote:
>>
>> Hi Ilya
>>
>> 在 2023/8/2 星期三 上午 6:22, Ilya Dryomov 写道:
>>> Due to rbd_try_acquire_lock() effectively swallowing all but
>>> EBLOCKLISTED error from rbd_try_lock() ("request lock anyway") and
>>> rbd_request_lock() returning ETIMEDOUT error not only for an actual
>>> notify timeout but also when the lock owner doesn't respond, a busy
>>> loop inside of rbd_acquire_lock() between rbd_try_acquire_lock() and
>>> rbd_request_lock() is possible.
>>>
>>> Requesting the lock on EBUSY error (returned by get_lock_owner_info()
>>> if an incompatible lock or invalid lock owner is detected) makes very
>>> little sense.  The same goes for ETIMEDOUT error (might pop up pretty
>>> much anywhere if osd_request_timeout option is set) and many others.
>>>
>>> Just fail I/O requests on rbd_dev->acquiring_list immediately on any
>>> error from rbd_try_lock().
>>>
>>> Cc: stable@vger.kernel.org # 588159009d5b: rbd: retrieve and check lock owner twice before blocklisting
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>>> ---
>>>    drivers/block/rbd.c | 28 +++++++++++++++-------------
>>>    1 file changed, 15 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>>> index 24afcc93ac01..2328cc05be36 100644
>>> --- a/drivers/block/rbd.c
>>> +++ b/drivers/block/rbd.c
>>> @@ -3675,7 +3675,7 @@ static int rbd_lock(struct rbd_device *rbd_dev)
>>>        ret = ceph_cls_lock(osdc, &rbd_dev->header_oid, &rbd_dev->header_oloc,
>>>                            RBD_LOCK_NAME, CEPH_CLS_LOCK_EXCLUSIVE, cookie,
>>>                            RBD_LOCK_TAG, "", 0);
>>> -     if (ret)
>>> +     if (ret && ret != -EEXIST)
>>>                return ret;
>>>
>>>        __rbd_lock(rbd_dev, cookie);
>>
>> If we got -EEXIST here, we will call __rbd_lock() and return 0. -EEXIST
>> means lock is held by myself, is that necessary to call __rbd_lock()?
> 
> Hi Dongsheng,
> 
> Yes, because the reason rbd_lock() gets called in the first place is
> that the kernel client doesn't "know" that it's still holding the lock
> in RADOS.  This can happen if the unlock operation times out, for
> example.
> 
> Notice
> 
>          WARN_ON(__rbd_is_lock_owner(rbd_dev) ||
>                  rbd_dev->lock_cookie[0] != '\0');
> 
> at the top of rbd_lock().

Okey, then

Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
> 
> Thanks,
> 
>                  Ilya
> .
>