diff mbox series

xfs: drop extra transaction roll from inode extent truncate

Message ID 20200910132926.1147266-1-bfoster@redhat.com (mailing list archive)
State New
Headers show
Series xfs: drop extra transaction roll from inode extent truncate | expand

Commit Message

Brian Foster Sept. 10, 2020, 1:29 p.m. UTC
The inode extent truncate path unmaps extents from the inode block
mapping, finishes deferred ops to free the associated extents and
then explicitly rolls the transaction before processing the next
extent. The latter extent roll is spurious as xfs_defer_finish()
always returns a clean transaction and automatically relogs inodes
attached to the transaction (with lock_flags == 0). This can
unnecessarily increase the number of log ticket regrants that occur
during a long running truncate operation. Remove the explicit
transaction roll.

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

Just something I noticed when reading through the code based on Dave's
recent EFI recovery reservation patches..

Brian

 fs/xfs/xfs_inode.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Comments

Darrick J. Wong Sept. 16, 2020, 3:44 a.m. UTC | #1
On Thu, Sep 10, 2020 at 09:29:26AM -0400, Brian Foster wrote:
> The inode extent truncate path unmaps extents from the inode block
> mapping, finishes deferred ops to free the associated extents and
> then explicitly rolls the transaction before processing the next
> extent. The latter extent roll is spurious as xfs_defer_finish()
> always returns a clean transaction and automatically relogs inodes
> attached to the transaction (with lock_flags == 0). This can
> unnecessarily increase the number of log ticket regrants that occur
> during a long running truncate operation. Remove the explicit
> transaction roll.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> Just something I noticed when reading through the code based on Dave's
> recent EFI recovery reservation patches..
> 
> Brian
> 
>  fs/xfs/xfs_inode.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c06129cffba9..7af99c7a2821 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1532,17 +1532,10 @@ xfs_itruncate_extents_flags(
>  		if (error)
>  			goto out;
>  
> -		/*
> -		 * Duplicate the transaction that has the permanent
> -		 * reservation and commit the old transaction.
> -		 */
> +		/* free the just unmapped extents */
>  		error = xfs_defer_finish(&tp);
>  		if (error)
>  			goto out;
> -
> -		error = xfs_trans_roll_inode(&tp, ip);
> -		if (error)
> -			goto out;
>  	}
>  
>  	if (whichfork == XFS_DATA_FORK) {
> -- 
> 2.25.4
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c06129cffba9..7af99c7a2821 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1532,17 +1532,10 @@  xfs_itruncate_extents_flags(
 		if (error)
 			goto out;
 
-		/*
-		 * Duplicate the transaction that has the permanent
-		 * reservation and commit the old transaction.
-		 */
+		/* free the just unmapped extents */
 		error = xfs_defer_finish(&tp);
 		if (error)
 			goto out;
-
-		error = xfs_trans_roll_inode(&tp, ip);
-		if (error)
-			goto out;
 	}
 
 	if (whichfork == XFS_DATA_FORK) {