diff mbox

ocfs2: free meta_ac and data_ac when ocfs2_start_trans fails in ocfs2_xattr_set()

Message ID 51FA2436.2050604@huawei.com
State New, archived
Headers show

Commit Message

Younger Liu Aug. 1, 2013, 9:02 a.m. UTC
In ocfs2_xattr_set(), if ocfs2_start_trans failed, 
meta_ac and data_ac should be free. 
Otherwise, It would lead to  memory leak.

Signed-off-by: Younger Liu <younger.liu@huawei.com>
Cc: Joseph Qi <joseph.qi@huawei.com>
---
 fs/ocfs2/xattr.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

jeff.liu Aug. 6, 2013, 10:44 p.m. UTC | #1
On 08/01/2013 05:02 PM, Younger Liu wrote:

> In ocfs2_xattr_set(), if ocfs2_start_trans failed, 
> meta_ac and data_ac should be free. 
> Otherwise, It would lead to  memory leak.
> 
> Signed-off-by: Younger Liu <younger.liu@huawei.com>
> Cc: Joseph Qi <joseph.qi@huawei.com>
> ---
>  fs/ocfs2/xattr.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 317ef0a..1c2a892 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -3505,7 +3505,7 @@ int ocfs2_xattr_set(struct inode *inode,
>  	int ret, credits, ref_meta = 0, ref_credits = 0;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  	struct inode *tl_inode = osb->osb_tl_inode;
> -	struct ocfs2_xattr_set_ctxt ctxt = { NULL, NULL, };
> +	struct ocfs2_xattr_set_ctxt ctxt = { NULL, NULL, NULL, };
>  	struct ocfs2_refcount_tree *ref_tree = NULL;
>  
>  	struct ocfs2_xattr_info xi = {
> @@ -3609,13 +3609,14 @@ int ocfs2_xattr_set(struct inode *inode,
>  	if (IS_ERR(ctxt.handle)) {
>  		ret = PTR_ERR(ctxt.handle);
>  		mlog_errno(ret);
> -		goto cleanup;
> +		goto out_free_ac;
>  	}
>  
>  	ret = __ocfs2_xattr_set_handle(inode, di, &xi, &xis, &xbs, &ctxt);
>  
>  	ocfs2_commit_trans(osb, ctxt.handle);
>  
> +out_free_ac:
>  	if (ctxt.data_ac)
>  		ocfs2_free_alloc_context(ctxt.data_ac);
>  	if (ctxt.meta_ac)

Yes, we should free the allocated contex in this case.
However, we don't performs the log truncation as well as the delayed allocation
business as per the old logic.

Srinivas,

How do you think about this fix?

Thanks,
-Jeff
Younger Liu Aug. 7, 2013, 2:03 a.m. UTC | #2
On 2013/8/7 6:44, Jeff Liu wrote:
> On 08/01/2013 05:02 PM, Younger Liu wrote:
> 
>> In ocfs2_xattr_set(), if ocfs2_start_trans failed, 
>> meta_ac and data_ac should be free. 
>> Otherwise, It would lead to  memory leak.
>>
>> Signed-off-by: Younger Liu <younger.liu@huawei.com>
>> Cc: Joseph Qi <joseph.qi@huawei.com>
>> ---
>>  fs/ocfs2/xattr.c |    5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
>> index 317ef0a..1c2a892 100644
>> --- a/fs/ocfs2/xattr.c
>> +++ b/fs/ocfs2/xattr.c
>> @@ -3505,7 +3505,7 @@ int ocfs2_xattr_set(struct inode *inode,
>>  	int ret, credits, ref_meta = 0, ref_credits = 0;
>>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>>  	struct inode *tl_inode = osb->osb_tl_inode;
>> -	struct ocfs2_xattr_set_ctxt ctxt = { NULL, NULL, };
>> +	struct ocfs2_xattr_set_ctxt ctxt = { NULL, NULL, NULL, };
>>  	struct ocfs2_refcount_tree *ref_tree = NULL;
>>  
>>  	struct ocfs2_xattr_info xi = {
>> @@ -3609,13 +3609,14 @@ int ocfs2_xattr_set(struct inode *inode,
>>  	if (IS_ERR(ctxt.handle)) {
>>  		ret = PTR_ERR(ctxt.handle);
>>  		mlog_errno(ret);
>> -		goto cleanup;
>> +		goto out_free_ac;
>>  	}
>>  
>>  	ret = __ocfs2_xattr_set_handle(inode, di, &xi, &xis, &xbs, &ctxt);
>>  
>>  	ocfs2_commit_trans(osb, ctxt.handle);
>>  
>> +out_free_ac:
>>  	if (ctxt.data_ac)
>>  		ocfs2_free_alloc_context(ctxt.data_ac);
>>  	if (ctxt.meta_ac)
> 
> Yes, we should free the allocated contex in this case.
> However, we don't performs the log truncation as well as the delayed allocation
> business as per the old logic.
> 
> Srinivas,
> 
> How do you think about this fix?
> 
> Thanks,
> -Jeff
> 
In ocfs2_init_xattr_set_ctxt(), the allocated contex is initialized, 
(&ctxt.dealloc)->c_global_allocator and (&ctxt.dealloc)->
c_first_suballocator are set to NULL.

After applying the patch, if ocfs2_start_trans fails, we would free 
meta_ac/data_ac, but not perform truncate_log_flush/ocfs2_run_deallocs.
So, it would take no side effect to the function.

Thanks.
	--Younger
jeff.liu Aug. 7, 2013, 7:21 a.m. UTC | #3
On 08/07/2013 10:03 AM, Younger Liu wrote:

> On 2013/8/7 6:44, Jeff Liu wrote:
>> On 08/01/2013 05:02 PM, Younger Liu wrote:
>>
>>> In ocfs2_xattr_set(), if ocfs2_start_trans failed, 
>>> meta_ac and data_ac should be free. 
>>> Otherwise, It would lead to  memory leak.
>>>
>>> Signed-off-by: Younger Liu <younger.liu@huawei.com>
>>> Cc: Joseph Qi <joseph.qi@huawei.com>
>>> ---
>>>  fs/ocfs2/xattr.c |    5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
>>> index 317ef0a..1c2a892 100644
>>> --- a/fs/ocfs2/xattr.c
>>> +++ b/fs/ocfs2/xattr.c
>>> @@ -3505,7 +3505,7 @@ int ocfs2_xattr_set(struct inode *inode,
>>>  	int ret, credits, ref_meta = 0, ref_credits = 0;
>>>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>>>  	struct inode *tl_inode = osb->osb_tl_inode;
>>> -	struct ocfs2_xattr_set_ctxt ctxt = { NULL, NULL, };
>>> +	struct ocfs2_xattr_set_ctxt ctxt = { NULL, NULL, NULL, };
>>>  	struct ocfs2_refcount_tree *ref_tree = NULL;
>>>  
>>>  	struct ocfs2_xattr_info xi = {
>>> @@ -3609,13 +3609,14 @@ int ocfs2_xattr_set(struct inode *inode,
>>>  	if (IS_ERR(ctxt.handle)) {
>>>  		ret = PTR_ERR(ctxt.handle);
>>>  		mlog_errno(ret);
>>> -		goto cleanup;
>>> +		goto out_free_ac;
>>>  	}
>>>  
>>>  	ret = __ocfs2_xattr_set_handle(inode, di, &xi, &xis, &xbs, &ctxt);
>>>  
>>>  	ocfs2_commit_trans(osb, ctxt.handle);
>>>  
>>> +out_free_ac:
>>>  	if (ctxt.data_ac)
>>>  		ocfs2_free_alloc_context(ctxt.data_ac);
>>>  	if (ctxt.meta_ac)
>>
>> Yes, we should free the allocated contex in this case.
>> However, we don't performs the log truncation as well as the delayed allocation

s/delayed allocation/delalloc/. Sorry, I habitually typed above wrong.

>> business as per the old logic.
>>
>> Srinivas,
>>
>> How do you think about this fix?
>>
>> Thanks,
>> -Jeff
>>
> In ocfs2_init_xattr_set_ctxt(), the allocated contex is initialized, 
> (&ctxt.dealloc)->c_global_allocator and (&ctxt.dealloc)->
> c_first_suballocator are set to NULL.
> 
> After applying the patch, if ocfs2_start_trans fails, we would free 
> meta_ac/data_ac, but not perform truncate_log_flush/ocfs2_run_deallocs.
> So, it would take no side effect to the function.

So this fix looks good, thanks.

Reviewed-by: Jie Liu <jeff.liu@oracle.com>
diff mbox

Patch

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 317ef0a..1c2a892 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3505,7 +3505,7 @@  int ocfs2_xattr_set(struct inode *inode,
 	int ret, credits, ref_meta = 0, ref_credits = 0;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct inode *tl_inode = osb->osb_tl_inode;
-	struct ocfs2_xattr_set_ctxt ctxt = { NULL, NULL, };
+	struct ocfs2_xattr_set_ctxt ctxt = { NULL, NULL, NULL, };
 	struct ocfs2_refcount_tree *ref_tree = NULL;
 
 	struct ocfs2_xattr_info xi = {
@@ -3609,13 +3609,14 @@  int ocfs2_xattr_set(struct inode *inode,
 	if (IS_ERR(ctxt.handle)) {
 		ret = PTR_ERR(ctxt.handle);
 		mlog_errno(ret);
-		goto cleanup;
+		goto out_free_ac;
 	}
 
 	ret = __ocfs2_xattr_set_handle(inode, di, &xi, &xis, &xbs, &ctxt);
 
 	ocfs2_commit_trans(osb, ctxt.handle);
 
+out_free_ac:
 	if (ctxt.data_ac)
 		ocfs2_free_alloc_context(ctxt.data_ac);
 	if (ctxt.meta_ac)