ceph: trigger the reclaim work once there has enough pending caps
diff mbox series

Message ID 20191126085114.40326-1-xiubli@redhat.com
State New
Headers show
Series
  • ceph: trigger the reclaim work once there has enough pending caps
Related show

Commit Message

Xiubo Li Nov. 26, 2019, 8:51 a.m. UTC
From: Xiubo Li <xiubli@redhat.com>

The nr in ceph_reclaim_caps_nr() is very possibly larger than 1,
so we may miss it and the reclaim work couldn't triggered as expected.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/mds_client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Yan, Zheng Nov. 26, 2019, 9:49 a.m. UTC | #1
On Tue, Nov 26, 2019 at 4:57 PM <xiubli@redhat.com> wrote:
>
> From: Xiubo Li <xiubli@redhat.com>
>
> The nr in ceph_reclaim_caps_nr() is very possibly larger than 1,
> so we may miss it and the reclaim work couldn't triggered as expected.
>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/mds_client.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 08b70b5ee05e..547ffe16f91c 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2020,7 +2020,7 @@ void ceph_reclaim_caps_nr(struct ceph_mds_client *mdsc, int nr)
>         if (!nr)
>                 return;
>         val = atomic_add_return(nr, &mdsc->cap_reclaim_pending);
> -       if (!(val % CEPH_CAPS_PER_RELEASE)) {
> +       if (val / CEPH_CAPS_PER_RELEASE) {
>                 atomic_set(&mdsc->cap_reclaim_pending, 0);
>                 ceph_queue_cap_reclaim_work(mdsc);
>         }

this will call ceph_queue_cap_reclaim_work too frequently

> --
> 2.21.0
>
Xiubo Li Nov. 26, 2019, 10:01 a.m. UTC | #2
On 2019/11/26 17:49, Yan, Zheng wrote:
> On Tue, Nov 26, 2019 at 4:57 PM <xiubli@redhat.com> wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> The nr in ceph_reclaim_caps_nr() is very possibly larger than 1,
>> so we may miss it and the reclaim work couldn't triggered as expected.
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/mds_client.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>> index 08b70b5ee05e..547ffe16f91c 100644
>> --- a/fs/ceph/mds_client.c
>> +++ b/fs/ceph/mds_client.c
>> @@ -2020,7 +2020,7 @@ void ceph_reclaim_caps_nr(struct ceph_mds_client *mdsc, int nr)
>>          if (!nr)
>>                  return;
>>          val = atomic_add_return(nr, &mdsc->cap_reclaim_pending);
>> -       if (!(val % CEPH_CAPS_PER_RELEASE)) {
>> +       if (val / CEPH_CAPS_PER_RELEASE) {
>>                  atomic_set(&mdsc->cap_reclaim_pending, 0);
>>                  ceph_queue_cap_reclaim_work(mdsc);
>>          }
> this will call ceph_queue_cap_reclaim_work too frequently

No it won't, the '/' here equals to '>=' and then the 
"mdsc->cap_reclaim_pending" will be reset and it will increase from 0 again.

It will make sure that only when "mdsc->cap_reclaim_pending >= 
CEPH_CAPS_PER_RELEASE" will call the work queue.

>> --
>> 2.21.0
>>
Yan, Zheng Nov. 26, 2019, 11:03 a.m. UTC | #3
On 11/26/19 6:01 PM, Xiubo Li wrote:
> On 2019/11/26 17:49, Yan, Zheng wrote:
>> On Tue, Nov 26, 2019 at 4:57 PM <xiubli@redhat.com> wrote:
>>> From: Xiubo Li <xiubli@redhat.com>
>>>
>>> The nr in ceph_reclaim_caps_nr() is very possibly larger than 1,
>>> so we may miss it and the reclaim work couldn't triggered as expected.
>>>
>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>> ---
>>>   fs/ceph/mds_client.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>> index 08b70b5ee05e..547ffe16f91c 100644
>>> --- a/fs/ceph/mds_client.c
>>> +++ b/fs/ceph/mds_client.c
>>> @@ -2020,7 +2020,7 @@ void ceph_reclaim_caps_nr(struct 
>>> ceph_mds_client *mdsc, int nr)
>>>          if (!nr)
>>>                  return;
>>>          val = atomic_add_return(nr, &mdsc->cap_reclaim_pending);
>>> -       if (!(val % CEPH_CAPS_PER_RELEASE)) {
>>> +       if (val / CEPH_CAPS_PER_RELEASE) {
>>>                  atomic_set(&mdsc->cap_reclaim_pending, 0);
>>>                  ceph_queue_cap_reclaim_work(mdsc);
>>>          }
>> this will call ceph_queue_cap_reclaim_work too frequently
> 
> No it won't, the '/' here equals to '>=' and then the 
> "mdsc->cap_reclaim_pending" will be reset and it will increase from 0 
> again.
> 
> It will make sure that only when "mdsc->cap_reclaim_pending >= 
> CEPH_CAPS_PER_RELEASE" will call the work queue.

Work does not get executed immediately. call 
ceph_queue_cap_reclaim_work() when val == CEPH_CAPS_PER_RELEASE is 
enough. There is no point to call it too frequently


> 
>>> -- 
>>> 2.21.0
>>>
>
Xiubo Li Nov. 26, 2019, 11:25 a.m. UTC | #4
On 2019/11/26 19:03, Yan, Zheng wrote:
> On 11/26/19 6:01 PM, Xiubo Li wrote:
>> On 2019/11/26 17:49, Yan, Zheng wrote:
>>> On Tue, Nov 26, 2019 at 4:57 PM <xiubli@redhat.com> wrote:
>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>
>>>> The nr in ceph_reclaim_caps_nr() is very possibly larger than 1,
>>>> so we may miss it and the reclaim work couldn't triggered as expected.
>>>>
>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>> ---
>>>>   fs/ceph/mds_client.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>> index 08b70b5ee05e..547ffe16f91c 100644
>>>> --- a/fs/ceph/mds_client.c
>>>> +++ b/fs/ceph/mds_client.c
>>>> @@ -2020,7 +2020,7 @@ void ceph_reclaim_caps_nr(struct 
>>>> ceph_mds_client *mdsc, int nr)
>>>>          if (!nr)
>>>>                  return;
>>>>          val = atomic_add_return(nr, &mdsc->cap_reclaim_pending);
>>>> -       if (!(val % CEPH_CAPS_PER_RELEASE)) {
>>>> +       if (val / CEPH_CAPS_PER_RELEASE) {
>>>> atomic_set(&mdsc->cap_reclaim_pending, 0);
>>>>                  ceph_queue_cap_reclaim_work(mdsc);
>>>>          }
>>> this will call ceph_queue_cap_reclaim_work too frequently
>>
>> No it won't, the '/' here equals to '>=' and then the 
>> "mdsc->cap_reclaim_pending" will be reset and it will increase from 0 
>> again.
>>
>> It will make sure that only when "mdsc->cap_reclaim_pending >= 
>> CEPH_CAPS_PER_RELEASE" will call the work queue.
>
> Work does not get executed immediately. call 
> ceph_queue_cap_reclaim_work() when val == CEPH_CAPS_PER_RELEASE is 
> enough. There is no point to call it too frequently
>
>
Yeah, it true and I am okay with this. Just going through the session 
release related code, and saw the "nr" parameter will be "ctx->used" in 
ceph_reclaim_caps_nr(mdsc, ctx->used), and in case there has many 
sessions with tremendous amount of caps. In corner case that we may 
always miss the condition that the "val == CEPH_CAPS_PER_RELEASE" here.

IMO, it wants to fire the work queue once "val >= 
CEPH_CAPS_PER_RELEASE", but it is not working like this, the val may 
just skip it without doing any thing.

Thanks


>>
>>>> -- 
>>>> 2.21.0
>>>>
>>
>
Yan, Zheng Nov. 26, 2019, 12:24 p.m. UTC | #5
On Tue, Nov 26, 2019 at 7:25 PM Xiubo Li <xiubli@redhat.com> wrote:
>
> On 2019/11/26 19:03, Yan, Zheng wrote:
> > On 11/26/19 6:01 PM, Xiubo Li wrote:
> >> On 2019/11/26 17:49, Yan, Zheng wrote:
> >>> On Tue, Nov 26, 2019 at 4:57 PM <xiubli@redhat.com> wrote:
> >>>> From: Xiubo Li <xiubli@redhat.com>
> >>>>
> >>>> The nr in ceph_reclaim_caps_nr() is very possibly larger than 1,
> >>>> so we may miss it and the reclaim work couldn't triggered as expected.
> >>>>
> >>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> >>>> ---
> >>>>   fs/ceph/mds_client.c | 2 +-
> >>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> >>>> index 08b70b5ee05e..547ffe16f91c 100644
> >>>> --- a/fs/ceph/mds_client.c
> >>>> +++ b/fs/ceph/mds_client.c
> >>>> @@ -2020,7 +2020,7 @@ void ceph_reclaim_caps_nr(struct
> >>>> ceph_mds_client *mdsc, int nr)
> >>>>          if (!nr)
> >>>>                  return;
> >>>>          val = atomic_add_return(nr, &mdsc->cap_reclaim_pending);
> >>>> -       if (!(val % CEPH_CAPS_PER_RELEASE)) {
> >>>> +       if (val / CEPH_CAPS_PER_RELEASE) {
> >>>> atomic_set(&mdsc->cap_reclaim_pending, 0);
> >>>>                  ceph_queue_cap_reclaim_work(mdsc);
> >>>>          }
> >>> this will call ceph_queue_cap_reclaim_work too frequently
> >>
> >> No it won't, the '/' here equals to '>=' and then the
> >> "mdsc->cap_reclaim_pending" will be reset and it will increase from 0
> >> again.
> >>
> >> It will make sure that only when "mdsc->cap_reclaim_pending >=
> >> CEPH_CAPS_PER_RELEASE" will call the work queue.
> >
> > Work does not get executed immediately. call
> > ceph_queue_cap_reclaim_work() when val == CEPH_CAPS_PER_RELEASE is
> > enough. There is no point to call it too frequently
> >
> >
> Yeah, it true and I am okay with this. Just going through the session
> release related code, and saw the "nr" parameter will be "ctx->used" in
> ceph_reclaim_caps_nr(mdsc, ctx->used), and in case there has many
> sessions with tremendous amount of caps. In corner case that we may
> always miss the condition that the "val == CEPH_CAPS_PER_RELEASE" here.
>

good catch. But the test should be something like

"if ((val % CEPH_CAPS_PER_RELEASE) < nr)"

> IMO, it wants to fire the work queue once "val >=
> CEPH_CAPS_PER_RELEASE", but it is not working like this, the val may
> just skip it without doing any thing.
>
> Thanks
>
>
> >>
> >>>> --
> >>>> 2.21.0
> >>>>
> >>
> >
>
Xiubo Li Nov. 26, 2019, 12:27 p.m. UTC | #6
On 2019/11/26 20:24, Yan, Zheng wrote:
> On Tue, Nov 26, 2019 at 7:25 PM Xiubo Li <xiubli@redhat.com> wrote:
>> On 2019/11/26 19:03, Yan, Zheng wrote:
>>> On 11/26/19 6:01 PM, Xiubo Li wrote:
>>>> On 2019/11/26 17:49, Yan, Zheng wrote:
>>>>> On Tue, Nov 26, 2019 at 4:57 PM <xiubli@redhat.com> wrote:
>>>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>>>
>>>>>> The nr in ceph_reclaim_caps_nr() is very possibly larger than 1,
>>>>>> so we may miss it and the reclaim work couldn't triggered as expected.
>>>>>>
>>>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>>>> ---
>>>>>>    fs/ceph/mds_client.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>>>> index 08b70b5ee05e..547ffe16f91c 100644
>>>>>> --- a/fs/ceph/mds_client.c
>>>>>> +++ b/fs/ceph/mds_client.c
>>>>>> @@ -2020,7 +2020,7 @@ void ceph_reclaim_caps_nr(struct
>>>>>> ceph_mds_client *mdsc, int nr)
>>>>>>           if (!nr)
>>>>>>                   return;
>>>>>>           val = atomic_add_return(nr, &mdsc->cap_reclaim_pending);
>>>>>> -       if (!(val % CEPH_CAPS_PER_RELEASE)) {
>>>>>> +       if (val / CEPH_CAPS_PER_RELEASE) {
>>>>>> atomic_set(&mdsc->cap_reclaim_pending, 0);
>>>>>>                   ceph_queue_cap_reclaim_work(mdsc);
>>>>>>           }
>>>>> this will call ceph_queue_cap_reclaim_work too frequently
>>>> No it won't, the '/' here equals to '>=' and then the
>>>> "mdsc->cap_reclaim_pending" will be reset and it will increase from 0
>>>> again.
>>>>
>>>> It will make sure that only when "mdsc->cap_reclaim_pending >=
>>>> CEPH_CAPS_PER_RELEASE" will call the work queue.
>>> Work does not get executed immediately. call
>>> ceph_queue_cap_reclaim_work() when val == CEPH_CAPS_PER_RELEASE is
>>> enough. There is no point to call it too frequently
>>>
>>>
>> Yeah, it true and I am okay with this. Just going through the session
>> release related code, and saw the "nr" parameter will be "ctx->used" in
>> ceph_reclaim_caps_nr(mdsc, ctx->used), and in case there has many
>> sessions with tremendous amount of caps. In corner case that we may
>> always miss the condition that the "val == CEPH_CAPS_PER_RELEASE" here.
>>
> good catch. But the test should be something like
>
> "if ((val % CEPH_CAPS_PER_RELEASE) < nr)"

Sure, this looks a bit more graceful.

Will fix it.

Thanks Yan

BRs


>> IMO, it wants to fire the work queue once "val >=
>> CEPH_CAPS_PER_RELEASE", but it is not working like this, the val may
>> just skip it without doing any thing.
>>
>> Thanks
>>
>>
>>>>>> --
>>>>>> 2.21.0
>>>>>>

Patch
diff mbox series

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 08b70b5ee05e..547ffe16f91c 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2020,7 +2020,7 @@  void ceph_reclaim_caps_nr(struct ceph_mds_client *mdsc, int nr)
 	if (!nr)
 		return;
 	val = atomic_add_return(nr, &mdsc->cap_reclaim_pending);
-	if (!(val % CEPH_CAPS_PER_RELEASE)) {
+	if (val / CEPH_CAPS_PER_RELEASE) {
 		atomic_set(&mdsc->cap_reclaim_pending, 0);
 		ceph_queue_cap_reclaim_work(mdsc);
 	}