* [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.