ceph: Free the previous caps if alloc failed.
PATCH 1/1 ceph: Free the previous caps if alloc failed.
--- fs/ceph/caps.c | 5 +++++ 1 file changed, 5 insertions(+)
fs/ceph/caps.c +5 -0
@@ -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; }
Signature · 1 line
1.8.1.2
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
Quoted 24 lines · majianpeng
Signed-off-by Jianpeng Ma <[email protected]>--- fs/ceph/caps.c | 5 +++++ 1 file changed, 5 insertions(+)fs/ceph/caps.c +5
-0@@ -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; }Signature · 1 line
1.8.1.2
Quoted 6 lines
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.
Quoted 10 lines
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
fs/ceph/caps.c +1 -2
@@ -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
Quoted 29 lines
sage
Quoted 24 lines · majianpeng
Signed-off-by Jianpeng Ma <[email protected]>--- fs/ceph/caps.c | 5 +++++ 1 file changed, 5 insertions(+)fs/ceph/caps.c +5
-0@@ -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; }Signature · 1 line
1.8.1.2
Quoted 17 lines · majianpeng
Quoted 6 lines
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.
Quoted 10 lines
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);
Quoted 2 lines
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.
Quoted 11 lines
By your thought, the code maybe like
fs/ceph/caps.c +0
-0@@ -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 ;
fs/ceph/caps.c int ceph_reserve_caps(struct ceph_mds_client *mdsc, · line 186
Quoted 10 lines
fs/ceph/caps.c +1
-2- 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?
Quoted 6 lines
BTY, The function which call
ceph_reserve_capsdon't care the result. And from you comment, this will periodically do.So i think the function proto will be viodceph_reserve_caps()
{
}
How about this?
Yeah, sounds good to me.
Thanks!
sage
Quoted 35 lines
Thanks!
Jianpeng MaQuoted 29 lines
sage
Quoted 24 lines · majianpeng
Signed-off-by Jianpeng Ma <[email protected]>--- fs/ceph/caps.c | 5 +++++ 1 file changed, 5 insertions(+)fs/ceph/caps.c +5
-0@@ -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; }Signature · 1 line
1.8.1.2