ocfs2: clean up some data_ac usage
diff mbox series

Message ID 20180907122500.azbll3byq4hdnrlw@kili.mountain
State New
Headers show
Series
  • ocfs2: clean up some data_ac usage
Related show

Commit Message

Dan Carpenter Sept. 7, 2018, 12:25 p.m. UTC
I started looking at this code because of a Smatch false positive:

    fs/ocfs2/move_extents.c:347 ocfs2_defrag_extent()
    warn: variable dereferenced before check 'context->data_ac' (see line 304)

context->data_ac can't be NULL so I removed the check.  I changed the
next lines to use "data_ac" instead of "context->data_ac" for
consistency.  And we don't need to pass "data_ac" to
ocfs2_lock_meta_allocator_move_extents() any more so I removed that.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Comments

Andrew Morton Oct. 2, 2018, 10:53 p.m. UTC | #1
On Fri, 7 Sep 2018 15:25:01 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:

> I started looking at this code because of a Smatch false positive:
> 
>     fs/ocfs2/move_extents.c:347 ocfs2_defrag_extent()
>     warn: variable dereferenced before check 'context->data_ac' (see line 304)
> 
> context->data_ac can't be NULL so I removed the check.  I changed the
> next lines to use "data_ac" instead of "context->data_ac" for
> consistency.  And we don't need to pass "data_ac" to
> ocfs2_lock_meta_allocator_move_extents() any more so I removed that.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 

Please don't forget the ^---$ at end-of-changelog.

> --- a/fs/ocfs2/move_extents.c
> +++ b/fs/ocfs2/move_extents.c
> @@ -160,15 +160,12 @@ static int __ocfs2_move_extent(handle_t *handle,
>   * lock allocators, and reserving appropriate number of bits for
>   * meta blocks and data clusters.
>   *
> - * in some cases, we don't need to reserve clusters, just let data_ac
> - * be NULL.
>   */
>  static int ocfs2_lock_meta_allocator_move_extents(struct inode *inode,
>  					struct ocfs2_extent_tree *et,
>  					u32 clusters_to_move,
>  					u32 extents_to_split,
>  					struct ocfs2_alloc_context **meta_ac,
> -					struct ocfs2_alloc_context **data_ac,
>  					int extra_blocks,
>  					int *credits)
>  {
> @@ -255,7 +252,6 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
>  	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
>  						*len, 1,
>  						&context->meta_ac,
> -						&context->data_ac,
>  						extra_blocks, &credits);
>  	if (ret) {
>  		mlog_errno(ret);
> @@ -344,10 +340,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
>  		mlog_errno(ret);
>  
>  out_commit:
> -	if (need_free && context->data_ac) {
> +	if (need_free) {

Current mainline is different here.

>  		struct ocfs2_alloc_context *data_ac = context->data_ac;
>  
> -		if (context->data_ac->ac_which == OCFS2_AC_USE_LOCAL)
> +		if (data_ac->ac_which == OCFS2_AC_USE_LOCAL)
>  			ocfs2_free_local_alloc_bits(osb, handle, data_ac,
>  					new_phys_cpos, new_len);
>  		else
> @@ -629,7 +625,7 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
>  	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
>  						len, 1,
>  						&context->meta_ac,
> -						NULL, extra_blocks, &credits);
> +						extra_blocks, &credits);
>  	if (ret) {
>  		mlog_errno(ret);
>  		goto out;
Dan Carpenter Oct. 11, 2018, 9:59 a.m. UTC | #2
On Tue, Oct 02, 2018 at 03:53:17PM -0700, Andrew Morton wrote:
> On Fri, 7 Sep 2018 15:25:01 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > I started looking at this code because of a Smatch false positive:
> > 
> >     fs/ocfs2/move_extents.c:347 ocfs2_defrag_extent()
> >     warn: variable dereferenced before check 'context->data_ac' (see line 304)
> > 
> > context->data_ac can't be NULL so I removed the check.  I changed the
> > next lines to use "data_ac" instead of "context->data_ac" for
> > consistency.  And we don't need to pass "data_ac" to
> > ocfs2_lock_meta_allocator_move_extents() any more so I removed that.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> 
> Please don't forget the ^---$ at end-of-changelog.
> 
> > --- a/fs/ocfs2/move_extents.c
> > +++ b/fs/ocfs2/move_extents.c
> > @@ -160,15 +160,12 @@ static int __ocfs2_move_extent(handle_t *handle,
> >   * lock allocators, and reserving appropriate number of bits for
> >   * meta blocks and data clusters.
> >   *
> > - * in some cases, we don't need to reserve clusters, just let data_ac
> > - * be NULL.
> >   */
> >  static int ocfs2_lock_meta_allocator_move_extents(struct inode *inode,
> >  					struct ocfs2_extent_tree *et,
> >  					u32 clusters_to_move,
> >  					u32 extents_to_split,
> >  					struct ocfs2_alloc_context **meta_ac,
> > -					struct ocfs2_alloc_context **data_ac,
> >  					int extra_blocks,
> >  					int *credits)
> >  {
> > @@ -255,7 +252,6 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
> >  	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
> >  						*len, 1,
> >  						&context->meta_ac,
> > -						&context->data_ac,
> >  						extra_blocks, &credits);
> >  	if (ret) {
> >  		mlog_errno(ret);
> > @@ -344,10 +340,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
> >  		mlog_errno(ret);
> >  
> >  out_commit:
> > -	if (need_free && context->data_ac) {
> > +	if (need_free) {
> 
> Current mainline is different here.
> 

Huh...  I wrote it against linux-next, on top of commit 66897d21fa76
("ocfs2: fix clusters leak in ocfs2_defrag_extent()").  But now git says
that patch was applied on Oct 4, two days later after your reply.

Anway, it should apply now again.  Would you like me to resend?

regarsd,
dan carpenter

> >  		struct ocfs2_alloc_context *data_ac = context->data_ac;
> >  
> > -		if (context->data_ac->ac_which == OCFS2_AC_USE_LOCAL)
> > +		if (data_ac->ac_which == OCFS2_AC_USE_LOCAL)
> >  			ocfs2_free_local_alloc_bits(osb, handle, data_ac,
> >  					new_phys_cpos, new_len);
> >  		else
> > @@ -629,7 +625,7 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
> >  	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
> >  						len, 1,
> >  						&context->meta_ac,
> > -						NULL, extra_blocks, &credits);
> > +						extra_blocks, &credits);
> >  	if (ret) {
> >  		mlog_errno(ret);
> >  		goto out;
Andrew Morton Oct. 12, 2018, 11:04 p.m. UTC | #3
On Thu, 11 Oct 2018 12:59:50 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:

> > >  out_commit:
> > > -	if (need_free && context->data_ac) {
> > > +	if (need_free) {
> > 
> > Current mainline is different here.
> > 
> 
> Huh...  I wrote it against linux-next, on top of commit 66897d21fa76
> ("ocfs2: fix clusters leak in ocfs2_defrag_extent()").  But now git says
> that patch was applied on Oct 4, two days later after your reply.
> 
> Anway, it should apply now again.  Would you like me to resend?

Yes please.  I've managed to lose the original email thanks to
ocfs2-devel's tendency to think it knows best about where emails should
be sent :(

Patch
diff mbox series

diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c
index 099016c9644e..9ee89d6816bc 100644
--- a/fs/ocfs2/move_extents.c
+++ b/fs/ocfs2/move_extents.c
@@ -160,15 +160,12 @@  static int __ocfs2_move_extent(handle_t *handle,
  * lock allocators, and reserving appropriate number of bits for
  * meta blocks and data clusters.
  *
- * in some cases, we don't need to reserve clusters, just let data_ac
- * be NULL.
  */
 static int ocfs2_lock_meta_allocator_move_extents(struct inode *inode,
 					struct ocfs2_extent_tree *et,
 					u32 clusters_to_move,
 					u32 extents_to_split,
 					struct ocfs2_alloc_context **meta_ac,
-					struct ocfs2_alloc_context **data_ac,
 					int extra_blocks,
 					int *credits)
 {
@@ -255,7 +252,6 @@  static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
 	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
 						*len, 1,
 						&context->meta_ac,
-						&context->data_ac,
 						extra_blocks, &credits);
 	if (ret) {
 		mlog_errno(ret);
@@ -344,10 +340,10 @@  static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
 		mlog_errno(ret);
 
 out_commit:
-	if (need_free && context->data_ac) {
+	if (need_free) {
 		struct ocfs2_alloc_context *data_ac = context->data_ac;
 
-		if (context->data_ac->ac_which == OCFS2_AC_USE_LOCAL)
+		if (data_ac->ac_which == OCFS2_AC_USE_LOCAL)
 			ocfs2_free_local_alloc_bits(osb, handle, data_ac,
 					new_phys_cpos, new_len);
 		else
@@ -629,7 +625,7 @@  static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
 	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
 						len, 1,
 						&context->meta_ac,
-						NULL, extra_blocks, &credits);
+						extra_blocks, &credits);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;