[Ocfs2-devel] [PATCH] ocfs2:fix memory leak in dlm_add_migration_mle

[Jeff Liu]

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 at 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 at 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 at oracle.com>

-Jeff