* [Ocfs2-devel] [PATCH] ocfs2:fix memory leak in dlm_add_migration_mle @ 2012-11-02 8:50 Xue jiufei 2012-11-02 14:52 ` Jeff Liu 0 siblings, 1 reply; 4+ messages in thread From: Xue jiufei @ 2012-11-02 8:50 UTC (permalink / raw) To: ocfs2-devel After some parallel mount/umount test on ocfs2, we got this: slab error in kmem_cache_destroy(): cache `o2dlm_mle': Can't free all objects. Then we found a memleak situation in dlm_add_migration_mle(). When a mle found, it will be removed from dlm->hlist. If there is no pointer to it at that moment, the mle will become an ?orphan mle? that no process can find and release. Signed-off-by: Xuejiufei <xuejiufei@huawei.com> --- fs/ocfs2/dlm/dlmmaster.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-) diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c index 005261c..20d2307 100644 --- a/fs/ocfs2/dlm/dlmmaster.c +++ b/fs/ocfs2/dlm/dlmmaster.c @@ -284,6 +284,7 @@ static void dlm_init_mle(struct dlm_master_list_entry *mle, mle->master = O2NM_MAX_NODES; mle->new_master = O2NM_MAX_NODES; mle->inuse = 0; + mle->assert_master = 0; BUG_ON(mle->type != DLM_MLE_BLOCK && mle->type != DLM_MLE_MASTER && @@ -1743,6 +1744,7 @@ int dlm_assert_master_handler(struct o2net_msg *msg, u32 len, void *data, u32 flags; int master_request = 0, have_lockres_ref = 0; int ret = 0; + int bit; if (!dlm_grab(dlm)) return 0; @@ -1770,7 +1772,11 @@ int dlm_assert_master_handler(struct o2net_msg *msg, u32 len, void *data, "MLE for it! (%.*s)\n", assert->node_idx, namelen, name); } else { - int bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); + spin_lock(&mle->spinlock); + mle->assert_master = 1; + spin_unlock(&mle->spinlock); + + bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); if (bit >= O2NM_MAX_NODES) { /* not necessarily an error, though less likely. * could be master just re-asserting. */ @@ -3081,6 +3087,8 @@ static int dlm_add_migration_mle(struct dlm_ctxt *dlm, /* remove it so that only one mle will be found */ __dlm_unlink_mle(dlm, tmp); __dlm_mle_detach_hb_events(dlm, tmp); + if (tmp->type == DLM_MLE_BLOCK && tmp->assert_master == 0) + __dlm_put_mle(tmp); ret = DLM_MIGRATE_RESPONSE_MASTERY_REF; mlog(0, "%s:%.*s: master=%u, newmaster=%u, " "telling master to get ref for cleared out mle " @@ -3173,6 +3181,7 @@ static void dlm_clean_block_mle(struct dlm_ctxt *dlm, wake_up(&mle->wq); /* Do not need events any longer, so detach from heartbeat */ + __dlm_unlink_mle(dlm, mle); __dlm_mle_detach_hb_events(dlm, mle); __dlm_put_mle(mle); } -- 1.7.8.6 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2:fix memory leak in dlm_add_migration_mle 2012-11-02 8:50 [Ocfs2-devel] [PATCH] ocfs2:fix memory leak in dlm_add_migration_mle Xue jiufei @ 2012-11-02 14:52 ` Jeff Liu 2012-11-05 8:45 ` Xue jiufei 0 siblings, 1 reply; 4+ messages in thread From: Jeff Liu @ 2012-11-02 14:52 UTC (permalink / raw) To: ocfs2-devel Hi Jiefei, On 11/02/2012 04:50 PM, Xue jiufei wrote: > After some parallel mount/umount test on ocfs2, we got this: slab error > in kmem_cache_destroy(): cache `o2dlm_mle': Can't free all objects. > > Then we found a memleak situation in dlm_add_migration_mle(). > When a mle found, it will be removed from dlm->hlist. If there is no > pointer to it at that moment, the mle will become an ?orphan mle? > that no process can find and release. > > Signed-off-by: Xuejiufei <xuejiufei@huawei.com> > --- > fs/ocfs2/dlm/dlmmaster.c | 11 ++++++++++- > 1 files changed, 10 insertions(+), 1 deletions(-) > > diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c > index 005261c..20d2307 100644 > --- a/fs/ocfs2/dlm/dlmmaster.c > +++ b/fs/ocfs2/dlm/dlmmaster.c > @@ -284,6 +284,7 @@ static void dlm_init_mle(struct dlm_master_list_entry *mle, > mle->master = O2NM_MAX_NODES; > mle->new_master = O2NM_MAX_NODES; > mle->inuse = 0; > + mle->assert_master = 0; > > BUG_ON(mle->type != DLM_MLE_BLOCK && > mle->type != DLM_MLE_MASTER && > @@ -1743,6 +1744,7 @@ int dlm_assert_master_handler(struct o2net_msg *msg, u32 len, void *data, > u32 flags; > int master_request = 0, have_lockres_ref = 0; > int ret = 0; > + int bit; > > if (!dlm_grab(dlm)) > return 0; > @@ -1770,7 +1772,11 @@ int dlm_assert_master_handler(struct o2net_msg *msg, u32 len, void *data, > "MLE for it! (%.*s)\n", assert->node_idx, > namelen, name); > } else { Looks good. I am being really picky, looks there is no need to move bit variable to be function level, i.e. int bit; spin_lock(&mle->spinlock); mle->assert_master = 1; spin_unlock(&mle->spinlock); bit = find_next_bit(mle->maybe_map, O2NM_MAX_NODES, 0); Thanks, -Jeff > - int bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); > + spin_lock(&mle->spinlock); > + mle->assert_master = 1; > + spin_unlock(&mle->spinlock); > + > + bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); > if (bit >= O2NM_MAX_NODES) { > /* not necessarily an error, though less likely. > * could be master just re-asserting. */ > @@ -3081,6 +3087,8 @@ static int dlm_add_migration_mle(struct dlm_ctxt *dlm, > /* remove it so that only one mle will be found */ > __dlm_unlink_mle(dlm, tmp); > __dlm_mle_detach_hb_events(dlm, tmp); > + if (tmp->type == DLM_MLE_BLOCK && tmp->assert_master == 0) > + __dlm_put_mle(tmp); > ret = DLM_MIGRATE_RESPONSE_MASTERY_REF; > mlog(0, "%s:%.*s: master=%u, newmaster=%u, " > "telling master to get ref for cleared out mle " > @@ -3173,6 +3181,7 @@ static void dlm_clean_block_mle(struct dlm_ctxt *dlm, > wake_up(&mle->wq); > > /* Do not need events any longer, so detach from heartbeat */ > + __dlm_unlink_mle(dlm, mle); > __dlm_mle_detach_hb_events(dlm, mle); > __dlm_put_mle(mle); > } > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2:fix memory leak in dlm_add_migration_mle 2012-11-02 14:52 ` Jeff Liu @ 2012-11-05 8:45 ` Xue jiufei 2012-11-05 9:03 ` Jeff Liu 0 siblings, 1 reply; 4+ messages in thread From: Xue jiufei @ 2012-11-05 8:45 UTC (permalink / raw) To: ocfs2-devel Hi Jeff ? 2012/11/2 22:52, Jeff Liu ??: > Hi Jiefei, > > On 11/02/2012 04:50 PM, Xue jiufei wrote: >> After some parallel mount/umount test on ocfs2, we got this: slab error >> in kmem_cache_destroy(): cache `o2dlm_mle': Can't free all objects. >> >> Then we found a memleak situation in dlm_add_migration_mle(). >> When a mle found, it will be removed from dlm->hlist. If there is no >> pointer to it at that moment, the mle will become an ?orphan mle? >> that no process can find and release. >> >> Signed-off-by: Xuejiufei <xuejiufei@huawei.com> >> --- >> fs/ocfs2/dlm/dlmmaster.c | 11 ++++++++++- >> 1 files changed, 10 insertions(+), 1 deletions(-) >> >> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c >> index 005261c..20d2307 100644 >> --- a/fs/ocfs2/dlm/dlmmaster.c >> +++ b/fs/ocfs2/dlm/dlmmaster.c >> @@ -284,6 +284,7 @@ static void dlm_init_mle(struct dlm_master_list_entry *mle, >> mle->master = O2NM_MAX_NODES; >> mle->new_master = O2NM_MAX_NODES; >> mle->inuse = 0; >> + mle->assert_master = 0; >> >> BUG_ON(mle->type != DLM_MLE_BLOCK && >> mle->type != DLM_MLE_MASTER && >> @@ -1743,6 +1744,7 @@ int dlm_assert_master_handler(struct o2net_msg *msg, u32 len, void *data, >> u32 flags; >> int master_request = 0, have_lockres_ref = 0; >> int ret = 0; >> + int bit; >> >> if (!dlm_grab(dlm)) >> return 0; >> @@ -1770,7 +1772,11 @@ int dlm_assert_master_handler(struct o2net_msg *msg, u32 len, void *data, >> "MLE for it! (%.*s)\n", assert->node_idx, >> namelen, name); >> } else { > Looks good. > I am being really picky, looks there is no need to move bit variable to > be function level, i.e. > > int bit; > > spin_lock(&mle->spinlock); > mle->assert_master = 1; > spin_unlock(&mle->spinlock); > bit = find_next_bit(mle->maybe_map, O2NM_MAX_NODES, 0); > > Thanks, > -Jeff >> - int bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); >> + spin_lock(&mle->spinlock); >> + mle->assert_master = 1; >> + spin_unlock(&mle->spinlock); >> + >> + bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); >> if (bit >= O2NM_MAX_NODES) { >> /* not necessarily an error, though less likely. >> * could be master just re-asserting. */ >> @@ -3081,6 +3087,8 @@ static int dlm_add_migration_mle(struct dlm_ctxt *dlm, >> /* remove it so that only one mle will be found */ >> __dlm_unlink_mle(dlm, tmp); >> __dlm_mle_detach_hb_events(dlm, tmp); >> + if (tmp->type == DLM_MLE_BLOCK && tmp->assert_master == 0) >> + __dlm_put_mle(tmp); >> ret = DLM_MIGRATE_RESPONSE_MASTERY_REF; >> mlog(0, "%s:%.*s: master=%u, newmaster=%u, " >> "telling master to get ref for cleared out mle " >> @@ -3173,6 +3181,7 @@ static void dlm_clean_block_mle(struct dlm_ctxt *dlm, >> wake_up(&mle->wq); >> >> /* Do not need events any longer, so detach from heartbeat */ >> + __dlm_unlink_mle(dlm, mle); >> __dlm_mle_detach_hb_events(dlm, mle); >> __dlm_put_mle(mle); >> } >> > > . > Thank you for the suggestion and I really agree with you. Here is the patch after modification: Signed-off-by: Xuejiufei <xuejiufei@huawei.com --- fs/ocfs2/dlm/dlmmaster.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-) diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c index 005261c..2458b29 100644 --- a/fs/ocfs2/dlm/dlmmaster.c +++ b/fs/ocfs2/dlm/dlmmaster.c @@ -284,6 +284,7 @@ static void dlm_init_mle(struct dlm_master_list_entry *mle, mle->master = O2NM_MAX_NODES; mle->new_master = O2NM_MAX_NODES; mle->inuse = 0; + mle->assert_master = 0; BUG_ON(mle->type != DLM_MLE_BLOCK && mle->type != DLM_MLE_MASTER && @@ -1770,7 +1771,12 @@ int dlm_assert_master_handler(struct o2net_msg *msg, u32 len, void *data, "MLE for it! (%.*s)\n", assert->node_idx, namelen, name); } else { - int bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); + int bit; + spin_lock(&mle->spinlock); + mle->assert_master = 1; + spin_unlock(&mle->spinlock); + + bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); if (bit >= O2NM_MAX_NODES) { /* not necessarily an error, though less likely. * could be master just re-asserting. */ @@ -3081,6 +3087,8 @@ static int dlm_add_migration_mle(struct dlm_ctxt *dlm, /* remove it so that only one mle will be found */ __dlm_unlink_mle(dlm, tmp); __dlm_mle_detach_hb_events(dlm, tmp); + if (tmp->type == DLM_MLE_BLOCK && tmp->assert_master == 0) + __dlm_put_mle(tmp); ret = DLM_MIGRATE_RESPONSE_MASTERY_REF; mlog(0, "%s:%.*s: master=%u, newmaster=%u, " "telling master to get ref for cleared out mle " @@ -3173,6 +3181,7 @@ static void dlm_clean_block_mle(struct dlm_ctxt *dlm, wake_up(&mle->wq); /* Do not need events any longer, so detach from heartbeat */ + __dlm_unlink_mle(dlm, mle); __dlm_mle_detach_hb_events(dlm, mle); __dlm_put_mle(mle); } -- 1.7.8.6 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2:fix memory leak in dlm_add_migration_mle 2012-11-05 8:45 ` Xue jiufei @ 2012-11-05 9:03 ` Jeff Liu 0 siblings, 0 replies; 4+ messages in thread From: Jeff Liu @ 2012-11-05 9:03 UTC (permalink / raw) To: ocfs2-devel On 11/05/2012 04:45 PM, Xue jiufei wrote: > Hi Jeff > ? 2012/11/2 22:52, Jeff Liu ??: >> Hi Jiefei, >> >> On 11/02/2012 04:50 PM, Xue jiufei wrote: >>> After some parallel mount/umount test on ocfs2, we got this: slab error >>> in kmem_cache_destroy(): cache `o2dlm_mle': Can't free all objects. >>> >>> Then we found a memleak situation in dlm_add_migration_mle(). >>> When a mle found, it will be removed from dlm->hlist. If there is no >>> pointer to it at that moment, the mle will become an ?orphan mle? >>> that no process can find and release. >>> >>> Signed-off-by: Xuejiufei <xuejiufei@huawei.com> >>> --- >>> fs/ocfs2/dlm/dlmmaster.c | 11 ++++++++++- >>> 1 files changed, 10 insertions(+), 1 deletions(-) >>> >>> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c >>> index 005261c..20d2307 100644 >>> --- a/fs/ocfs2/dlm/dlmmaster.c >>> +++ b/fs/ocfs2/dlm/dlmmaster.c >>> @@ -284,6 +284,7 @@ static void dlm_init_mle(struct dlm_master_list_entry *mle, >>> mle->master = O2NM_MAX_NODES; >>> mle->new_master = O2NM_MAX_NODES; >>> mle->inuse = 0; >>> + mle->assert_master = 0; >>> >>> BUG_ON(mle->type != DLM_MLE_BLOCK && >>> mle->type != DLM_MLE_MASTER && >>> @@ -1743,6 +1744,7 @@ int dlm_assert_master_handler(struct o2net_msg *msg, u32 len, void *data, >>> u32 flags; >>> int master_request = 0, have_lockres_ref = 0; >>> int ret = 0; >>> + int bit; >>> >>> if (!dlm_grab(dlm)) >>> return 0; >>> @@ -1770,7 +1772,11 @@ int dlm_assert_master_handler(struct o2net_msg *msg, u32 len, void *data, >>> "MLE for it! (%.*s)\n", assert->node_idx, >>> namelen, name); >>> } else { >> Looks good. >> I am being really picky, looks there is no need to move bit variable to >> be function level, i.e. >> >> int bit; >> >> spin_lock(&mle->spinlock); >> mle->assert_master = 1; >> spin_unlock(&mle->spinlock); >> bit = find_next_bit(mle->maybe_map, O2NM_MAX_NODES, 0); >> >> Thanks, >> -Jeff >>> - int bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); >>> + spin_lock(&mle->spinlock); >>> + mle->assert_master = 1; >>> + spin_unlock(&mle->spinlock); >>> + >>> + bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); >>> if (bit >= O2NM_MAX_NODES) { >>> /* not necessarily an error, though less likely. >>> * could be master just re-asserting. */ >>> @@ -3081,6 +3087,8 @@ static int dlm_add_migration_mle(struct dlm_ctxt *dlm, >>> /* remove it so that only one mle will be found */ >>> __dlm_unlink_mle(dlm, tmp); >>> __dlm_mle_detach_hb_events(dlm, tmp); >>> + if (tmp->type == DLM_MLE_BLOCK && tmp->assert_master == 0) >>> + __dlm_put_mle(tmp); >>> ret = DLM_MIGRATE_RESPONSE_MASTERY_REF; >>> mlog(0, "%s:%.*s: master=%u, newmaster=%u, " >>> "telling master to get ref for cleared out mle " >>> @@ -3173,6 +3181,7 @@ static void dlm_clean_block_mle(struct dlm_ctxt *dlm, >>> wake_up(&mle->wq); >>> >>> /* Do not need events any longer, so detach from heartbeat */ >>> + __dlm_unlink_mle(dlm, mle); >>> __dlm_mle_detach_hb_events(dlm, mle); >>> __dlm_put_mle(mle); >>> } >>> > > Thank you for the suggestion and I really agree with you. > Here is the patch after modification: > > Signed-off-by: Xuejiufei <xuejiufei@huawei.com > --- > fs/ocfs2/dlm/dlmmaster.c | 11 ++++++++++- > 1 files changed, 10 insertions(+), 1 deletions(-) > > diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c > index 005261c..2458b29 100644 > --- a/fs/ocfs2/dlm/dlmmaster.c > +++ b/fs/ocfs2/dlm/dlmmaster.c > @@ -284,6 +284,7 @@ static void dlm_init_mle(struct dlm_master_list_entry *mle, > mle->master = O2NM_MAX_NODES; > mle->new_master = O2NM_MAX_NODES; > mle->inuse = 0; > + mle->assert_master = 0; > > BUG_ON(mle->type != DLM_MLE_BLOCK && > mle->type != DLM_MLE_MASTER && > @@ -1770,7 +1771,12 @@ int dlm_assert_master_handler(struct o2net_msg *msg, u32 len, void *data, > "MLE for it! (%.*s)\n", assert->node_idx, > namelen, name); > } else { > - int bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); > + int bit; > + spin_lock(&mle->spinlock); > + mle->assert_master = 1; > + spin_unlock(&mle->spinlock); > + > + bit = find_next_bit (mle->maybe_map, O2NM_MAX_NODES, 0); > if (bit >= O2NM_MAX_NODES) { > /* not necessarily an error, though less likely. > * could be master just re-asserting. */ > @@ -3081,6 +3087,8 @@ static int dlm_add_migration_mle(struct dlm_ctxt *dlm, > /* remove it so that only one mle will be found */ > __dlm_unlink_mle(dlm, tmp); > __dlm_mle_detach_hb_events(dlm, tmp); > + if (tmp->type == DLM_MLE_BLOCK && tmp->assert_master == 0) > + __dlm_put_mle(tmp); > ret = DLM_MIGRATE_RESPONSE_MASTERY_REF; > mlog(0, "%s:%.*s: master=%u, newmaster=%u, " > "telling master to get ref for cleared out mle " > @@ -3173,6 +3181,7 @@ static void dlm_clean_block_mle(struct dlm_ctxt *dlm, > wake_up(&mle->wq); > > /* Do not need events any longer, so detach from heartbeat */ > + __dlm_unlink_mle(dlm, mle); > __dlm_mle_detach_hb_events(dlm, mle); > __dlm_put_mle(mle); > } Looks good to me, thank you! Reviewed-by: Jie Liu <jeff.liu@oracle.com> -Jeff ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-11-05 9:03 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-02 8:50 [Ocfs2-devel] [PATCH] ocfs2:fix memory leak in dlm_add_migration_mle Xue jiufei 2012-11-02 14:52 ` Jeff Liu 2012-11-05 8:45 ` Xue jiufei 2012-11-05 9:03 ` Jeff Liu
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.