| Message ID | 51FA2436.2050604@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 --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)
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(-)