* [PATCH][TRIVIAL] ceph: Free the previous caps if alloc failed.
@ 2013-06-25 7:27 majianpeng
2013-06-25 15:27 ` Sage Weil
0 siblings, 1 reply; 4+ messages in thread
From: majianpeng @ 2013-06-25 7:27 UTC (permalink / raw)
To: sage; +Cc: ceph-devel
Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
---
fs/ceph/caps.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index da0f9b8..626b0ec 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -203,6 +203,11 @@ out_alloc_count:
/* we didn't manage to reserve as much as we needed */
pr_warning("reserve caps ctx=%p ENOMEM need=%d got=%d\n",
ctx, need, have);
+ if (alloc) {
+ struct ceph_cap *tmp;
+ list_for_each_entry_safe(cap, tmp, &newcaps, caps_item)
+ kmem_cache_free(ceph_cap_cachep, cap);
+ }
return ret;
}
--
1.8.1.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][TRIVIAL] ceph: Free the previous caps if alloc failed.
2013-06-25 7:27 [PATCH][TRIVIAL] ceph: Free the previous caps if alloc failed majianpeng
@ 2013-06-25 15:27 ` Sage Weil
2013-06-26 0:29 ` majianpeng
0 siblings, 1 reply; 4+ messages in thread
From: Sage Weil @ 2013-06-25 15:27 UTC (permalink / raw)
To: majianpeng; +Cc: ceph-devel
I think in this case we don't want to drop other caps. This basically
means we weren't able to maintain the desired level of reserves, but we
will continue to try to fill it periodically, and not reaching the desired
point is not a reason to throw out what we have. You'll note that the one
caller ignores the return value..
sage
On Tue, 25 Jun 2013, majianpeng wrote:
> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
> ---
> fs/ceph/caps.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index da0f9b8..626b0ec 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -203,6 +203,11 @@ out_alloc_count:
> /* we didn't manage to reserve as much as we needed */
> pr_warning("reserve caps ctx=%p ENOMEM need=%d got=%d\n",
> ctx, need, have);
> + if (alloc) {
> + struct ceph_cap *tmp;
> + list_for_each_entry_safe(cap, tmp, &newcaps, caps_item)
> + kmem_cache_free(ceph_cap_cachep, cap);
> + }
> return ret;
> }
>
> --
> 1.8.1.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [PATCH][TRIVIAL] ceph: Free the previous caps if alloc failed.
2013-06-25 15:27 ` Sage Weil
@ 2013-06-26 0:29 ` majianpeng
2013-06-26 0:34 ` Sage Weil
0 siblings, 1 reply; 4+ messages in thread
From: majianpeng @ 2013-06-26 0:29 UTC (permalink / raw)
To: sage; +Cc: ceph-devel
>I think in this case we don't want to drop other caps. This basically
>means we weren't able to maintain the desired level of reserves, but we
>will continue to try to fill it periodically, and not reaching the desired
>point is not a reason to throw out what we have. You'll note that the one
>caller ignores the return value..
>
I see.But the code don't.
> for (i = have; i < need; i++) {
> cap = kmem_cache_alloc(ceph_cap_cachep, GFP_NOFS);
> if (!cap) {
> ret = -ENOMEM;
> goto out_alloc_count;
> }
> list_add(&cap->caps_item, &newcaps);
> alloc++;
> }
> BUG_ON(have + alloc != need);
For the caps which allocated, those can't add into '&mdsc->caps_list'.
So if allocate failed,it will cause memory leak.
By your thought, the code maybe like
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index da0f9b8..d5d5eca 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -176,12 +176,11 @@ int ceph_reserve_caps(struct ceph_mds_client *mdsc,
cap = kmem_cache_alloc(ceph_cap_cachep, GFP_NOFS);
if (!cap) {
ret = -ENOMEM;
- goto out_alloc_count;
+ break;
}
list_add(&cap->caps_item, &newcaps);
alloc++;
}
- BUG_ON(have + alloc != need);
spin_lock(&mdsc->caps_list_lock);
mdsc->caps_total_count += alloc;
BTY, The function which call ceph_reserve_caps don't care the result.
And from you comment, this will periodically do.So i think the function proto will be
viod ceph_reserve_caps()
{
}
How about this?
Thanks!
Jianpeng Ma
>sage
>
>
>On Tue, 25 Jun 2013, majianpeng wrote:
>
>> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
>> ---
>> fs/ceph/caps.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>> index da0f9b8..626b0ec 100644
>> --- a/fs/ceph/caps.c
>> +++ b/fs/ceph/caps.c
>> @@ -203,6 +203,11 @@ out_alloc_count:
>> /* we didn't manage to reserve as much as we needed */
>> pr_warning("reserve caps ctx=%p ENOMEM need=%d got=%d\n",
>> ctx, need, have);
>> + if (alloc) {
>> + struct ceph_cap *tmp;
>> + list_for_each_entry_safe(cap, tmp, &newcaps, caps_item)
>> + kmem_cache_free(ceph_cap_cachep, cap);
>> + }
>> return ret;
>> }
>>
>> --
>> 1.8.1.2
>>
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Re: [PATCH][TRIVIAL] ceph: Free the previous caps if alloc failed.
2013-06-26 0:29 ` majianpeng
@ 2013-06-26 0:34 ` Sage Weil
0 siblings, 0 replies; 4+ messages in thread
From: Sage Weil @ 2013-06-26 0:34 UTC (permalink / raw)
To: majianpeng; +Cc: ceph-devel
On Wed, 26 Jun 2013, majianpeng wrote:
> >I think in this case we don't want to drop other caps. This basically
> >means we weren't able to maintain the desired level of reserves, but we
> >will continue to try to fill it periodically, and not reaching the desired
> >point is not a reason to throw out what we have. You'll note that the one
> >caller ignores the return value..
> >
> I see.But the code don't.
> > for (i = have; i < need; i++) {
> > cap = kmem_cache_alloc(ceph_cap_cachep, GFP_NOFS);
> > if (!cap) {
> > ret = -ENOMEM;
> > goto out_alloc_count;
> > }
> > list_add(&cap->caps_item, &newcaps);
> > alloc++;
> > }
> > BUG_ON(have + alloc != need);
> For the caps which allocated, those can't add into '&mdsc->caps_list'.
> So if allocate failed,it will cause memory leak.
Ah, I see.
> By your thought, the code maybe like
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index da0f9b8..d5d5eca 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -176,12 +176,11 @@ int ceph_reserve_caps(struct ceph_mds_client *mdsc,
> cap = kmem_cache_alloc(ceph_cap_cachep, GFP_NOFS);
> if (!cap) {
> ret = -ENOMEM;
> - goto out_alloc_count;
> + break;
> }
> list_add(&cap->caps_item, &newcaps);
> alloc++;
> }
> - BUG_ON(have + alloc != need);
>
> spin_lock(&mdsc->caps_list_lock);
> mdsc->caps_total_count += alloc;
>
That looks good. Can we add a check later though that if have + alloc <
need we still pr_warning?
> BTY, The function which call ceph_reserve_caps don't care the result.
> And from you comment, this will periodically do.So i think the function proto will be
> viod ceph_reserve_caps()
> {
> }
> How about this?
Yeah, sounds good to me.
Thanks!
sage
>
> Thanks!
> Jianpeng Ma
>
>
>
> >sage
> >
> >
> >On Tue, 25 Jun 2013, majianpeng wrote:
> >
> >> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
> >> ---
> >> fs/ceph/caps.c | 5 +++++
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> >> index da0f9b8..626b0ec 100644
> >> --- a/fs/ceph/caps.c
> >> +++ b/fs/ceph/caps.c
> >> @@ -203,6 +203,11 @@ out_alloc_count:
> >> /* we didn't manage to reserve as much as we needed */
> >> pr_warning("reserve caps ctx=%p ENOMEM need=%d got=%d\n",
> >> ctx, need, have);
> >> + if (alloc) {
> >> + struct ceph_cap *tmp;
> >> + list_for_each_entry_safe(cap, tmp, &newcaps, caps_item)
> >> + kmem_cache_free(ceph_cap_cachep, cap);
> >> + }
> >> return ret;
> >> }
> >>
> >> --
> >> 1.8.1.2
> >>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-06-26 0:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-25 7:27 [PATCH][TRIVIAL] ceph: Free the previous caps if alloc failed majianpeng
2013-06-25 15:27 ` Sage Weil
2013-06-26 0:29 ` majianpeng
2013-06-26 0:34 ` Sage Weil
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.