[v10,14/25] xfs: Remove xfs_trans_roll in xfs_attr_node_removename
diff mbox series

Message ID 20200625233018.14585-15-allison.henderson@oracle.com
State Superseded
Headers show
Series
  • xfs: Delay Ready Attributes
Related show

Commit Message

Allison Collins June 25, 2020, 11:30 p.m. UTC
The xfs_trans_roll in _removename is not needed because invalidating
blocks is an incore-only change.  This is analogous to the non-remote
remove case where an entry is removed and a potential dabtree join
occurs under the same transaction.

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

Comments

Brian Foster July 8, 2020, 12:42 p.m. UTC | #1
On Thu, Jun 25, 2020 at 04:30:07PM -0700, Allison Collins wrote:
> The xfs_trans_roll in _removename is not needed because invalidating
> blocks is an incore-only change.  This is analogous to the non-remote
> remove case where an entry is removed and a potential dabtree join
> occurs under the same transaction.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---

Ok, but I think we should be a bit more descriptive in the commit log so
the reasoning is available for historical reference. For example:

"A transaction roll is not necessary immediately after setting the
INCOMPLETE flag when removing a node xattr entry with remote value
blocks. The remote block invalidation that immediately follows setting
the flag is an in-core only change. The next step after that is to start
unmapping the remote blocks from the attr fork, but the xattr remove
transaction reservation includes reservation for full tree splits of the
dabtree and bmap tree. The remote block unmap code will roll the
transaction as extents are unmapped and freed."

With something like that in place:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_attr.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 1a78023..f1becca 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1148,10 +1148,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_invalidate(args);
>  		if (error)
>  			return error;
> -- 
> 2.7.4
>
Allison Collins July 9, 2020, 10:01 p.m. UTC | #2
On 7/8/20 5:42 AM, Brian Foster wrote:
> On Thu, Jun 25, 2020 at 04:30:07PM -0700, Allison Collins wrote:
>> The xfs_trans_roll in _removename is not needed because invalidating
>> blocks is an incore-only change.  This is analogous to the non-remote
>> remove case where an entry is removed and a potential dabtree join
>> occurs under the same transaction.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
> 
> Ok, but I think we should be a bit more descriptive in the commit log so
> the reasoning is available for historical reference. For example:
> 
> "A transaction roll is not necessary immediately after setting the
> INCOMPLETE flag when removing a node xattr entry with remote value
> blocks. The remote block invalidation that immediately follows setting
> the flag is an in-core only change. The next step after that is to start
> unmapping the remote blocks from the attr fork, but the xattr remove
> transaction reservation includes reservation for full tree splits of the
> dabtree and bmap tree. The remote block unmap code will roll the
> transaction as extents are unmapped and freed."
Ok, that is a lot more detailed.

> 
> With something like that in place:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Sure, will update.  Thanks!
Allison
> 
>>   fs/xfs/libxfs/xfs_attr.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 1a78023..f1becca 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -1148,10 +1148,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_invalidate(args);
>>   		if (error)
>>   			return error;
>> -- 
>> 2.7.4
>>
>

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 1a78023..f1becca 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1148,10 +1148,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_invalidate(args);
 		if (error)
 			return error;