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

PATCH New Younger Liu 2 participants 4 messages Aug 1, 2013 – Aug 7, 2013 OCFS2

Share & tools

Younger Liu

author

2013-08-01 09:02 UTC #0 lore raw

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

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 <[email protected]>

Cc Joseph Qi <[email protected]>

---
 fs/ocfs2/xattr.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/ocfs2/xattr.c +3 -2

index 317ef0a..1c2a892 100644
@@ -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)

Signature · 1 line

1.7.9.7

Jeff Liu

2013-08-06 22:44 UTC #1 lore raw

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

Quoted 29 lines · Younger Liu

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 <[email protected]>

Cc Joseph Qi <[email protected]>

---
 fs/ocfs2/xattr.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/ocfs2/xattr.c +2 -2

index 317ef0a..1c2a892 100644
@@ -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;

fs/ocfs2/xattr.c int ocfs2_xattr_set(struct inode *inode, · line 3622

Quoted 10 lines

fs/ocfs2/xattr.c +1 -0

 	}
 
 	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

author

2013-08-07 02:03 UTC #2 lore raw

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

Quoted 46 lines · Jeff Liu

Quoted 29 lines · Younger Liu
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 <[email protected]>
Cc Joseph Qi <[email protected]>
---
 fs/ocfs2/xattr.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/ocfs2/xattr.c +2 -2
index 317ef0a..1c2a892 100644
@@ -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;
fs/ocfs2/xattr.c int ocfs2_xattr_set(struct inode *inode, · line 3622
Quoted 10 lines
fs/ocfs2/xattr.c +1 -0
 	}
 
 	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.

Quoted 7 lines

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

2013-08-07 07:21 UTC #3 lore raw

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

Quoted 38 lines · Younger Liu

Quoted 37 lines · Jeff Liu

Quoted 35 lines · Younger Liu

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 <[email protected]>

Cc Joseph Qi <[email protected]>

---
 fs/ocfs2/xattr.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/ocfs2/xattr.c +2 -2

index 317ef0a..1c2a892 100644
@@ -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);

Quoted 7 lines

Quoted 7 lines
fs/ocfs2/xattr.c int ocfs2_xattr_set(struct inode *inode, · line 3622
Quoted 4 lines
fs/ocfs2/xattr.c +1 -0
+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.

Quoted 9 lines

Quoted 9 lines
business as per the old logic.
Srinivas,
How do you think about this fix?
Thanks,
-Jeff

Quoted 7 lines

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 <[email protected]>