[v8,12/20] xfs: Removed unneeded xfs_trans_roll_inode calls
diff mbox series

Message ID 20200403221229.4995-13-allison.henderson@oracle.com
State New
Headers show
Series
  • xfs: Delay Ready Attributes
Related show

Commit Message

Allison Collins April 3, 2020, 10:12 p.m. UTC
Some calls to xfs_trans_roll_inode in the *_addname routines are not
needed. If they are the last operations executed in these functions, and
no further changes are made, then higher level routines will roll or
commit the tranactions. The xfs_trans_roll in _removename is also not
needed because invalidating blocks is not an incore change.

Signed-off-by: Allison Collins <allison.henderson@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

Comments

Brian Foster April 7, 2020, 2:17 p.m. UTC | #1
On Fri, Apr 03, 2020 at 03:12:21PM -0700, Allison Collins wrote:
> Some calls to xfs_trans_roll_inode in the *_addname routines are not
> needed. If they are the last operations executed in these functions, and
> no further changes are made, then higher level routines will roll or
> commit the tranactions. The xfs_trans_roll in _removename is also not
> needed because invalidating blocks is not an incore change.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 30 ------------------------------
>  1 file changed, 30 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 27a9bb5..4225a94 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -700,11 +700,6 @@ xfs_attr_leaf_addname(
>  				return error;
>  		}
>  
> -		/*
> -		 * Commit the remove and start the next trans in series.
> -		 */
> -		error = xfs_trans_roll_inode(&args->trans, dp);
> -

Ok, so it looks like the only caller immediately rolls again.

>  	} else if (args->rmtblkno > 0) {
>  		/*
>  		 * Added a "remote" value, just clear the incomplete flag.
> @@ -712,12 +707,6 @@ xfs_attr_leaf_addname(
>  		error = xfs_attr3_leaf_clearflag(args);
>  		if (error)
>  			return error;
> -
> -		/*
> -		 * Commit the flag value change and start the next trans in
> -		 * series.
> -		 */
> -		error = xfs_trans_roll_inode(&args->trans, args->dp);
>  	}

Same logic applies here. Makes sense.

>  	return error;
>  }
> @@ -1069,13 +1058,6 @@ xfs_attr_node_addname(
>  				goto out;
>  		}
>  
> -		/*
> -		 * Commit and start the next trans in the chain.
> -		 */
> -		error = xfs_trans_roll_inode(&args->trans, dp);
> -		if (error)
> -			goto out;
> -

From here, we log the inode and commit, so that seems safe.

BTW, doesn't that mean the defer_finish() just above (after the
da3_join()) is unnecessary as well?

>  	} else if (args->rmtblkno > 0) {
>  		/*
>  		 * Added a "remote" value, just clear the incomplete flag.
> @@ -1083,14 +1065,6 @@ xfs_attr_node_addname(
>  		error = xfs_attr3_leaf_clearflag(args);
>  		if (error)
>  			goto out;
> -
> -		 /*
> -		  * Commit the flag value change and start the next trans in
> -		  * series.
> -		  */
> -		error = xfs_trans_roll_inode(&args->trans, args->dp);
> -		if (error)
> -			goto out;
>  	}
>  	retval = error = 0;
>  
> @@ -1189,10 +1163,6 @@ xfs_attr_node_removename(
>  		if (error)
>  			goto out;
>  
> -		error = xfs_trans_roll_inode(&args->trans, args->dp);
> -		if (error)
> -			goto out;
> -

Hmm, not sure I follow this one. Don't we want to commit the incomplete
flag before proceeding? Or are we just saying it can be combined with
the first bunmap since that's going to roll anyways..?

BTW, the commit log refers to the invalidation as "not incore," which I
think is opposite. :P xfs_attr_rmtval_invalidate() is incore-only in the
sense that it doesn't seem to use the transaction. Is that what you
mean?

Brian

>  		error = xfs_attr_rmtval_remove(args);
>  		if (error)
>  			goto out;
> -- 
> 2.7.4
>
Allison Collins April 7, 2020, 9:53 p.m. UTC | #2
On 4/7/20 7:17 AM, Brian Foster wrote:
> On Fri, Apr 03, 2020 at 03:12:21PM -0700, Allison Collins wrote:
>> Some calls to xfs_trans_roll_inode in the *_addname routines are not
>> needed. If they are the last operations executed in these functions, and
>> no further changes are made, then higher level routines will roll or
>> commit the tranactions. The xfs_trans_roll in _removename is also not
>> needed because invalidating blocks is not an incore change.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 30 ------------------------------
>>   1 file changed, 30 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 27a9bb5..4225a94 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -700,11 +700,6 @@ xfs_attr_leaf_addname(
>>   				return error;
>>   		}
>>   
>> -		/*
>> -		 * Commit the remove and start the next trans in series.
>> -		 */
>> -		error = xfs_trans_roll_inode(&args->trans, dp);
>> -
> 
> Ok, so it looks like the only caller immediately rolls again.
> 
>>   	} else if (args->rmtblkno > 0) {
>>   		/*
>>   		 * Added a "remote" value, just clear the incomplete flag.
>> @@ -712,12 +707,6 @@ xfs_attr_leaf_addname(
>>   		error = xfs_attr3_leaf_clearflag(args);
>>   		if (error)
>>   			return error;
>> -
>> -		/*
>> -		 * Commit the flag value change and start the next trans in
>> -		 * series.
>> -		 */
>> -		error = xfs_trans_roll_inode(&args->trans, args->dp);
>>   	}
> 
> Same logic applies here. Makes sense.
> 
>>   	return error;
>>   }
>> @@ -1069,13 +1058,6 @@ xfs_attr_node_addname(
>>   				goto out;
>>   		}
>>   
>> -		/*
>> -		 * Commit and start the next trans in the chain.
>> -		 */
>> -		error = xfs_trans_roll_inode(&args->trans, dp);
>> -		if (error)
>> -			goto out;
>> -
> 
>  From here, we log the inode and commit, so that seems safe.
> 
> BTW, doesn't that mean the defer_finish() just above (after the
> da3_join()) is unnecessary as well?
Hmm, I think you're right.  Will clean out here too.

> 
>>   	} else if (args->rmtblkno > 0) {
>>   		/*
>>   		 * Added a "remote" value, just clear the incomplete flag.
>> @@ -1083,14 +1065,6 @@ xfs_attr_node_addname(
>>   		error = xfs_attr3_leaf_clearflag(args);
>>   		if (error)
>>   			goto out;
>> -
>> -		 /*
>> -		  * Commit the flag value change and start the next trans in
>> -		  * series.
>> -		  */
>> -		error = xfs_trans_roll_inode(&args->trans, args->dp);
>> -		if (error)
>> -			goto out;
>>   	}
>>   	retval = error = 0;
>>   
>> @@ -1189,10 +1163,6 @@ xfs_attr_node_removename(
>>   		if (error)
>>   			goto out;
>>   
>> -		error = xfs_trans_roll_inode(&args->trans, args->dp);
>> -		if (error)
>> -			goto out;
>> -
> 
> Hmm, not sure I follow this one. Don't we want to commit the incomplete
> flag before proceeding? Or are we just saying it can be combined with
> the first bunmap since that's going to roll anyways..?
> 
> BTW, the commit log refers to the invalidation as "not incore," which I
> think is opposite. :P xfs_attr_rmtval_invalidate() is incore-only in the
> sense that it doesn't seem to use the transaction. Is that what you
> mean?
Yes, sorry, the commit should say in-core only.  We pulled this one out 
in v5 for that reason.  It used to belong to the later "Add delay ready 
attr remove routines" patch in v6 and v7. Then when we decided to clean 
out these extra transaction rolls, because it simplifies the top level 
transaction rolling.  So I decided to do the removename clean out in 
this patch because it seemed appropriate.  In any case, will amend the 
commit message.  Discussion thread just for brain refresh:

https://lore.kernel.org/linux-xfs/27f6d1f2-8d7e-c759-31e1-6c4ac8c7ccad@oracle.com/

Thanks for the reviews!  Will update!

Allison


> 
> Brian
> 
>>   		error = xfs_attr_rmtval_remove(args);
>>   		if (error)
>>   			goto out;
>> -- 
>> 2.7.4
>>
>

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 27a9bb5..4225a94 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -700,11 +700,6 @@  xfs_attr_leaf_addname(
 				return error;
 		}
 
-		/*
-		 * Commit the remove and start the next trans in series.
-		 */
-		error = xfs_trans_roll_inode(&args->trans, dp);
-
 	} else if (args->rmtblkno > 0) {
 		/*
 		 * Added a "remote" value, just clear the incomplete flag.
@@ -712,12 +707,6 @@  xfs_attr_leaf_addname(
 		error = xfs_attr3_leaf_clearflag(args);
 		if (error)
 			return error;
-
-		/*
-		 * Commit the flag value change and start the next trans in
-		 * series.
-		 */
-		error = xfs_trans_roll_inode(&args->trans, args->dp);
 	}
 	return error;
 }
@@ -1069,13 +1058,6 @@  xfs_attr_node_addname(
 				goto out;
 		}
 
-		/*
-		 * Commit and start the next trans in the chain.
-		 */
-		error = xfs_trans_roll_inode(&args->trans, dp);
-		if (error)
-			goto out;
-
 	} else if (args->rmtblkno > 0) {
 		/*
 		 * Added a "remote" value, just clear the incomplete flag.
@@ -1083,14 +1065,6 @@  xfs_attr_node_addname(
 		error = xfs_attr3_leaf_clearflag(args);
 		if (error)
 			goto out;
-
-		 /*
-		  * Commit the flag value change and start the next trans in
-		  * series.
-		  */
-		error = xfs_trans_roll_inode(&args->trans, args->dp);
-		if (error)
-			goto out;
 	}
 	retval = error = 0;
 
@@ -1189,10 +1163,6 @@  xfs_attr_node_removename(
 		if (error)
 			goto out;
 
-		error = xfs_trans_roll_inode(&args->trans, args->dp);
-		if (error)
-			goto out;
-
 		error = xfs_attr_rmtval_remove(args);
 		if (error)
 			goto out;