linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] xfs: fix sf to block inode fork logging
@ 2019-10-07 13:19 Brian Foster
  2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Brian Foster @ 2019-10-07 13:19 UTC (permalink / raw)
  To: linux-xfs

Hi all,

Here's v2 of the directory inode shortform to block logging fixup. This
also addresses the similar attr fork conversion function. I put the
other cleanups into separate patches because it wasn't totally clear to
me if we wanted to add the log call to the conversion function given the
other callers log the inode outside of that function. IOW, we can either
keep or drop patch 3. I have no strong preference either way.

Brian

v2:
- Also fix up attr fork conversion.
- Add patches 2 and 3 for follow up cleanups.
v1: https://lore.kernel.org/linux-xfs/20191004125520.7857-1-bfoster@redhat.com/

Brian Foster (3):
  xfs: log the inode on directory sf to block format change
  xfs: remove broken error handling on failed attr sf to leaf change
  xfs: move local to extent inode logging into bmap helper

 fs/xfs/libxfs/xfs_attr_leaf.c  | 21 +++------------------
 fs/xfs/libxfs/xfs_bmap.c       |  6 ++++--
 fs/xfs/libxfs/xfs_bmap.h       |  3 ++-
 fs/xfs/libxfs/xfs_dir2_block.c |  2 +-
 4 files changed, 10 insertions(+), 22 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/3] xfs: log the inode on directory sf to block format change
  2019-10-07 13:19 [PATCH v2 0/3] xfs: fix sf to block inode fork logging Brian Foster
@ 2019-10-07 13:19 ` Brian Foster
  2019-10-08  6:59   ` Christoph Hellwig
  2019-10-08 16:11   ` Darrick J. Wong
  2019-10-07 13:19 ` [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change Brian Foster
  2019-10-07 13:19 ` [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper Brian Foster
  2 siblings, 2 replies; 10+ messages in thread
From: Brian Foster @ 2019-10-07 13:19 UTC (permalink / raw)
  To: linux-xfs

When a directory changes from shortform (sf) to block format, the sf
format is copied to a temporary buffer, the inode format is modified
and the updated format filled with the dentries from the temporary
buffer. If the inode format is modified and attempt to grow the
inode fails (due to I/O error, for example), it is possible to
return an error while leaving the directory in an inconsistent state
and with an otherwise clean transaction. This results in corruption
of the associated directory and leads to xfs_dabuf_map() errors as
subsequent lookups cannot accurately determine the format of the
directory. This problem is reproduced occasionally by generic/475.

The fundamental problem is that xfs_dir2_sf_to_block() changes the
on-disk inode format without logging the inode. The inode is
eventually logged by the bmapi layer in the common case, but error
checking introduces the possibility of failing the high level
request before this happens.

Update both of the dir2 and attr callers of
xfs_bmap_local_to_extents_empty() to log the inode core as
consistent with the bmap local to extent format change codepath.
This ensures that any subsequent errors after the format has changed
cause the transaction to abort.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c  | 1 +
 fs/xfs/libxfs/xfs_dir2_block.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index b9f019603d0b..36c0a32cefcf 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -827,6 +827,7 @@ xfs_attr_shortform_to_leaf(
 
 	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);
 
 	bp = NULL;
 	error = xfs_da_grow_inode(args, &blkno);
diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index 9595ced393dc..3d1e5f6d64fd 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -1098,6 +1098,7 @@ xfs_dir2_sf_to_block(
 	xfs_idata_realloc(dp, -ifp->if_bytes, XFS_DATA_FORK);
 	xfs_bmap_local_to_extents_empty(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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change
  2019-10-07 13:19 [PATCH v2 0/3] xfs: fix sf to block inode fork logging Brian Foster
  2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster
@ 2019-10-07 13:19 ` Brian Foster
  2019-10-08  7:00   ` Christoph Hellwig
  2019-10-08 16:11   ` Darrick J. Wong
  2019-10-07 13:19 ` [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper Brian Foster
  2 siblings, 2 replies; 10+ messages in thread
From: Brian Foster @ 2019-10-07 13:19 UTC (permalink / raw)
  To: linux-xfs

xfs_attr_shortform_to_leaf() attempts to put the shortform fork back
together after a failed attempt to convert from shortform to leaf
format. While this code reallocates and copies back the shortform
attr fork data, it never resets the inode format field back to local
format. Further, now that the inode is properly logged after the
initial switch from local format, any error that triggers the
recovery code will eventually abort the transaction and shutdown the
fs. Therefore, remove the broken and unnecessary error handling
code.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 36c0a32cefcf..1b956c313b6b 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -831,28 +831,13 @@ xfs_attr_shortform_to_leaf(
 
 	bp = NULL;
 	error = xfs_da_grow_inode(args, &blkno);
-	if (error) {
-		/*
-		 * If we hit an IO error middle of the transaction inside
-		 * grow_inode(), we may have inconsistent data. Bail out.
-		 */
-		if (error == -EIO)
-			goto out;
-		xfs_idata_realloc(dp, size, XFS_ATTR_FORK);	/* try to put */
-		memcpy(ifp->if_u1.if_data, tmpbuffer, size);	/* it back */
+	if (error)
 		goto out;
-	}
 
 	ASSERT(blkno == 0);
 	error = xfs_attr3_leaf_create(args, blkno, &bp);
-	if (error) {
-		/* xfs_attr3_leaf_create may not have instantiated a block */
-		if (bp && (xfs_da_shrink_inode(args, 0, bp) != 0))
-			goto out;
-		xfs_idata_realloc(dp, size, XFS_ATTR_FORK);	/* try to put */
-		memcpy(ifp->if_u1.if_data, tmpbuffer, size);	/* it back */
+	if (error)
 		goto out;
-	}
 
 	memset((char *)&nargs, 0, sizeof(nargs));
 	nargs.dp = dp;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper
  2019-10-07 13:19 [PATCH v2 0/3] xfs: fix sf to block inode fork logging Brian Foster
  2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster
  2019-10-07 13:19 ` [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change Brian Foster
@ 2019-10-07 13:19 ` Brian Foster
  2019-10-08  7:00   ` Christoph Hellwig
  2019-10-08 16:11   ` Darrick J. Wong
  2 siblings, 2 replies; 10+ messages in thread
From: Brian Foster @ 2019-10-07 13:19 UTC (permalink / raw)
  To: linux-xfs

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(-)

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] xfs: log the inode on directory sf to block format change
  2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster
@ 2019-10-08  6:59   ` Christoph Hellwig
  2019-10-08 16:11   ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2019-10-08  6:59 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Mon, Oct 07, 2019 at 09:19:36AM -0400, Brian Foster wrote:
> When a directory changes from shortform (sf) to block format, the sf
> format is copied to a temporary buffer, the inode format is modified
> and the updated format filled with the dentries from the temporary
> buffer. If the inode format is modified and attempt to grow the
> inode fails (due to I/O error, for example), it is possible to
> return an error while leaving the directory in an inconsistent state
> and with an otherwise clean transaction. This results in corruption
> of the associated directory and leads to xfs_dabuf_map() errors as
> subsequent lookups cannot accurately determine the format of the
> directory. This problem is reproduced occasionally by generic/475.
> 
> The fundamental problem is that xfs_dir2_sf_to_block() changes the
> on-disk inode format without logging the inode. The inode is
> eventually logged by the bmapi layer in the common case, but error
> checking introduces the possibility of failing the high level
> request before this happens.
> 
> Update both of the dir2 and attr callers of
> xfs_bmap_local_to_extents_empty() to log the inode core as
> consistent with the bmap local to extent format change codepath.
> This ensures that any subsequent errors after the format has changed
> cause the transaction to abort.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change
  2019-10-07 13:19 ` [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change Brian Foster
@ 2019-10-08  7:00   ` Christoph Hellwig
  2019-10-08 16:11   ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2019-10-08  7:00 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Mon, Oct 07, 2019 at 09:19:37AM -0400, Brian Foster wrote:
> xfs_attr_shortform_to_leaf() attempts to put the shortform fork back
> together after a failed attempt to convert from shortform to leaf
> format. While this code reallocates and copies back the shortform
> attr fork data, it never resets the inode format field back to local
> format. Further, now that the inode is properly logged after the
> initial switch from local format, any error that triggers the
> recovery code will eventually abort the transaction and shutdown the
> fs. Therefore, remove the broken and unnecessary error handling
> code.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper
  2019-10-07 13:19 ` [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper Brian Foster
@ 2019-10-08  7:00   ` Christoph Hellwig
  2019-10-08 16:11   ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2019-10-08  7:00 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] xfs: log the inode on directory sf to block format change
  2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster
  2019-10-08  6:59   ` Christoph Hellwig
@ 2019-10-08 16:11   ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2019-10-08 16:11 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Mon, Oct 07, 2019 at 09:19:36AM -0400, Brian Foster wrote:
> When a directory changes from shortform (sf) to block format, the sf
> format is copied to a temporary buffer, the inode format is modified
> and the updated format filled with the dentries from the temporary
> buffer. If the inode format is modified and attempt to grow the
> inode fails (due to I/O error, for example), it is possible to
> return an error while leaving the directory in an inconsistent state
> and with an otherwise clean transaction. This results in corruption
> of the associated directory and leads to xfs_dabuf_map() errors as
> subsequent lookups cannot accurately determine the format of the
> directory. This problem is reproduced occasionally by generic/475.
> 
> The fundamental problem is that xfs_dir2_sf_to_block() changes the
> on-disk inode format without logging the inode. The inode is
> eventually logged by the bmapi layer in the common case, but error
> checking introduces the possibility of failing the high level
> request before this happens.
> 
> Update both of the dir2 and attr callers of
> xfs_bmap_local_to_extents_empty() to log the inode core as
> consistent with the bmap local to extent format change codepath.
> This ensures that any subsequent errors after the format has changed
> cause the transaction to abort.
> 
> 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  | 1 +
>  fs/xfs/libxfs/xfs_dir2_block.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index b9f019603d0b..36c0a32cefcf 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -827,6 +827,7 @@ xfs_attr_shortform_to_leaf(
>  
>  	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);
>  
>  	bp = NULL;
>  	error = xfs_da_grow_inode(args, &blkno);
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 9595ced393dc..3d1e5f6d64fd 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -1098,6 +1098,7 @@ xfs_dir2_sf_to_block(
>  	xfs_idata_realloc(dp, -ifp->if_bytes, XFS_DATA_FORK);
>  	xfs_bmap_local_to_extents_empty(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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change
  2019-10-07 13:19 ` [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change Brian Foster
  2019-10-08  7:00   ` Christoph Hellwig
@ 2019-10-08 16:11   ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2019-10-08 16:11 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

On Mon, Oct 07, 2019 at 09:19:37AM -0400, Brian Foster wrote:
> xfs_attr_shortform_to_leaf() attempts to put the shortform fork back
> together after a failed attempt to convert from shortform to leaf
> format. While this code reallocates and copies back the shortform
> attr fork data, it never resets the inode format field back to local
> format. Further, now that the inode is properly logged after the
> initial switch from local format, any error that triggers the
> recovery code will eventually abort the transaction and shutdown the
> fs. Therefore, remove the broken and unnecessary error handling
> code.
> 
> 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 | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 36c0a32cefcf..1b956c313b6b 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -831,28 +831,13 @@ xfs_attr_shortform_to_leaf(
>  
>  	bp = NULL;
>  	error = xfs_da_grow_inode(args, &blkno);
> -	if (error) {
> -		/*
> -		 * If we hit an IO error middle of the transaction inside
> -		 * grow_inode(), we may have inconsistent data. Bail out.
> -		 */
> -		if (error == -EIO)
> -			goto out;
> -		xfs_idata_realloc(dp, size, XFS_ATTR_FORK);	/* try to put */
> -		memcpy(ifp->if_u1.if_data, tmpbuffer, size);	/* it back */
> +	if (error)
>  		goto out;
> -	}
>  
>  	ASSERT(blkno == 0);
>  	error = xfs_attr3_leaf_create(args, blkno, &bp);
> -	if (error) {
> -		/* xfs_attr3_leaf_create may not have instantiated a block */
> -		if (bp && (xfs_da_shrink_inode(args, 0, bp) != 0))
> -			goto out;
> -		xfs_idata_realloc(dp, size, XFS_ATTR_FORK);	/* try to put */
> -		memcpy(ifp->if_u1.if_data, tmpbuffer, size);	/* it back */
> +	if (error)
>  		goto out;
> -	}
>  
>  	memset((char *)&nargs, 0, sizeof(nargs));
>  	nargs.dp = dp;
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper
  2019-10-07 13:19 ` [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper Brian Foster
  2019-10-08  7:00   ` Christoph Hellwig
@ 2019-10-08 16:11   ` Darrick J. Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2019-10-08 16:11 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-xfs

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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-10-08 16:12 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07 13:19 [PATCH v2 0/3] xfs: fix sf to block inode fork logging Brian Foster
2019-10-07 13:19 ` [PATCH v2 1/3] xfs: log the inode on directory sf to block format change Brian Foster
2019-10-08  6:59   ` Christoph Hellwig
2019-10-08 16:11   ` Darrick J. Wong
2019-10-07 13:19 ` [PATCH v2 2/3] xfs: remove broken error handling on failed attr sf to leaf change Brian Foster
2019-10-08  7:00   ` Christoph Hellwig
2019-10-08 16:11   ` Darrick J. Wong
2019-10-07 13:19 ` [PATCH v2 3/3] xfs: move local to extent inode logging into bmap helper Brian Foster
2019-10-08  7:00   ` Christoph Hellwig
2019-10-08 16:11   ` Darrick J. Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).