diff mbox series

[v2,3/3] xfs: move local to extent inode logging into bmap helper

Message ID 20191007131938.23839-4-bfoster@redhat.com (mailing list archive)
State Accepted
Headers show
Series xfs: fix sf to block inode fork logging | expand

Commit Message

Brian Foster Oct. 7, 2019, 1:19 p.m. UTC 
The callers of xfs_bmap_local_to_extents_empty() log the inode
external to the function, yet this function is where the on-disk
format value is updated. Push the inode logging down into the
function itself to help prevent future mistakes.

Note that internal bmap callers track the inode logging flags
independently and thus may log the inode core twice due to this
change. This is harmless, so leave this code around for consistency
with the other attr fork conversion functions.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c  | 3 +--
 fs/xfs/libxfs/xfs_bmap.c       | 6 ++++--
 fs/xfs/libxfs/xfs_bmap.h       | 3 ++-
 fs/xfs/libxfs/xfs_dir2_block.c | 3 +--
 4 files changed, 8 insertions(+), 7 deletions(-)

Comments

Christoph Hellwig Oct. 8, 2019, 7 a.m. UTC | #1 
On Mon, Oct 07, 2019 at 09:19:38AM -0400, Brian Foster wrote:
> The callers of xfs_bmap_local_to_extents_empty() log the inode
> external to the function, yet this function is where the on-disk
> format value is updated. Push the inode logging down into the
> function itself to help prevent future mistakes.
> 
> Note that internal bmap callers track the inode logging flags
> independently and thus may log the inode core twice due to this
> change. This is harmless, so leave this code around for consistency
> with the other attr fork conversion functions.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Oct. 8, 2019, 4:11 p.m. UTC | #2 
On Mon, Oct 07, 2019 at 09:19:38AM -0400, Brian Foster wrote:
> The callers of xfs_bmap_local_to_extents_empty() log the inode
> external to the function, yet this function is where the on-disk
> format value is updated. Push the inode logging down into the
> function itself to help prevent future mistakes.
> 
> Note that internal bmap callers track the inode logging flags
> independently and thus may log the inode core twice due to this
> change. This is harmless, so leave this code around for consistency
> with the other attr fork conversion functions.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

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

--D

> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c  | 3 +--
>  fs/xfs/libxfs/xfs_bmap.c       | 6 ++++--
>  fs/xfs/libxfs/xfs_bmap.h       | 3 ++-
>  fs/xfs/libxfs/xfs_dir2_block.c | 3 +--
>  4 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 1b956c313b6b..f0089e862216 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -826,8 +826,7 @@ xfs_attr_shortform_to_leaf(
>  	sf = (xfs_attr_shortform_t *)tmpbuffer;
>  
>  	xfs_idata_realloc(dp, -size, XFS_ATTR_FORK);
> -	xfs_bmap_local_to_extents_empty(dp, XFS_ATTR_FORK);
> -	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
> +	xfs_bmap_local_to_extents_empty(args->trans, dp, XFS_ATTR_FORK);
>  
>  	bp = NULL;
>  	error = xfs_da_grow_inode(args, &blkno);
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 4edc25a2ba80..02469d59c787 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -792,6 +792,7 @@ xfs_bmap_extents_to_btree(
>   */
>  void
>  xfs_bmap_local_to_extents_empty(
> +	struct xfs_trans	*tp,
>  	struct xfs_inode	*ip,
>  	int			whichfork)
>  {
> @@ -808,6 +809,7 @@ xfs_bmap_local_to_extents_empty(
>  	ifp->if_u1.if_root = NULL;
>  	ifp->if_height = 0;
>  	XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS);
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  }
>  
>  
> @@ -840,7 +842,7 @@ xfs_bmap_local_to_extents(
>  	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
>  
>  	if (!ifp->if_bytes) {
> -		xfs_bmap_local_to_extents_empty(ip, whichfork);
> +		xfs_bmap_local_to_extents_empty(tp, ip, whichfork);
>  		flags = XFS_ILOG_CORE;
>  		goto done;
>  	}
> @@ -887,7 +889,7 @@ xfs_bmap_local_to_extents(
>  
>  	/* account for the change in fork size */
>  	xfs_idata_realloc(ip, -ifp->if_bytes, whichfork);
> -	xfs_bmap_local_to_extents_empty(ip, whichfork);
> +	xfs_bmap_local_to_extents_empty(tp, ip, whichfork);
>  	flags |= XFS_ILOG_CORE;
>  
>  	ifp->if_u1.if_root = NULL;
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 5bb446d80542..e2798c6f3a5f 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -182,7 +182,8 @@ void	xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
>  		xfs_filblks_t len);
>  int	xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd);
>  int	xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version);
> -void	xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork);
> +void	xfs_bmap_local_to_extents_empty(struct xfs_trans *tp,
> +		struct xfs_inode *ip, int whichfork);
>  void	__xfs_bmap_add_free(struct xfs_trans *tp, xfs_fsblock_t bno,
>  		xfs_filblks_t len, const struct xfs_owner_info *oinfo,
>  		bool skip_discard);
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 3d1e5f6d64fd..49e4bc39e7bb 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -1096,9 +1096,8 @@ xfs_dir2_sf_to_block(
>  	memcpy(sfp, oldsfp, ifp->if_bytes);
>  
>  	xfs_idata_realloc(dp, -ifp->if_bytes, XFS_DATA_FORK);
> -	xfs_bmap_local_to_extents_empty(dp, XFS_DATA_FORK);
> +	xfs_bmap_local_to_extents_empty(tp, dp, XFS_DATA_FORK);
>  	dp->i_d.di_size = 0;
> -	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
>  
>  	/*
>  	 * Add block 0 to the inode.
> -- 
> 2.20.1
>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 1b956c313b6b..f0089e862216 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -826,8 +826,7 @@  xfs_attr_shortform_to_leaf(
 	sf = (xfs_attr_shortform_t *)tmpbuffer;
 
 	xfs_idata_realloc(dp, -size, XFS_ATTR_FORK);
-	xfs_bmap_local_to_extents_empty(dp, XFS_ATTR_FORK);
-	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
+	xfs_bmap_local_to_extents_empty(args->trans, dp, XFS_ATTR_FORK);
 
 	bp = NULL;
 	error = xfs_da_grow_inode(args, &blkno);
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 4edc25a2ba80..02469d59c787 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -792,6 +792,7 @@  xfs_bmap_extents_to_btree(
  */
 void
 xfs_bmap_local_to_extents_empty(
+	struct xfs_trans	*tp,
 	struct xfs_inode	*ip,
 	int			whichfork)
 {
@@ -808,6 +809,7 @@  xfs_bmap_local_to_extents_empty(
 	ifp->if_u1.if_root = NULL;
 	ifp->if_height = 0;
 	XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS);
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 }
 
 
@@ -840,7 +842,7 @@  xfs_bmap_local_to_extents(
 	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
 
 	if (!ifp->if_bytes) {
-		xfs_bmap_local_to_extents_empty(ip, whichfork);
+		xfs_bmap_local_to_extents_empty(tp, ip, whichfork);
 		flags = XFS_ILOG_CORE;
 		goto done;
 	}
@@ -887,7 +889,7 @@  xfs_bmap_local_to_extents(
 
 	/* account for the change in fork size */
 	xfs_idata_realloc(ip, -ifp->if_bytes, whichfork);
-	xfs_bmap_local_to_extents_empty(ip, whichfork);
+	xfs_bmap_local_to_extents_empty(tp, ip, whichfork);
 	flags |= XFS_ILOG_CORE;
 
 	ifp->if_u1.if_root = NULL;
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 5bb446d80542..e2798c6f3a5f 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -182,7 +182,8 @@  void	xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
 		xfs_filblks_t len);
 int	xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd);
 int	xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version);
-void	xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork);
+void	xfs_bmap_local_to_extents_empty(struct xfs_trans *tp,
+		struct xfs_inode *ip, int whichfork);
 void	__xfs_bmap_add_free(struct xfs_trans *tp, xfs_fsblock_t bno,
 		xfs_filblks_t len, const struct xfs_owner_info *oinfo,
 		bool skip_discard);
diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index 3d1e5f6d64fd..49e4bc39e7bb 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -1096,9 +1096,8 @@  xfs_dir2_sf_to_block(
 	memcpy(sfp, oldsfp, ifp->if_bytes);
 
 	xfs_idata_realloc(dp, -ifp->if_bytes, XFS_DATA_FORK);
-	xfs_bmap_local_to_extents_empty(dp, XFS_DATA_FORK);
+	xfs_bmap_local_to_extents_empty(tp, dp, XFS_DATA_FORK);
 	dp->i_d.di_size = 0;
-	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
 
 	/*
 	 * Add block 0 to the inode.