| Message ID | 20191126085114.40326-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: trigger the reclaim work once there has enough pending caps | expand |
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 >
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 >>
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 >>> >
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 >>>> >> >
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 > >>>> > >> > > >
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 >>>>>>
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); }