[PATCH] xfs: consider shutdown in bmapbt cursor delete assert

[PATCH] xfs: consider shutdown in bmapbt cursor delete assert

[Brian Foster]

The assert in xfs_btree_del_cursor() checks that the bmapbt block
allocation field has been handled correctly before the cursor is
freed. This field is used for accurate calculation of indirect block
reservation requirements (for delayed allocations), for example.
generic/019 reproduces a scenario where this assert fails because
the filesystem has shutdown while in the middle of a bmbt record
insertion. This occurs after a bmbt block has been allocated via the
cursor but before the higher level bmap function (i.e.
xfs_bmap_add_extent_hole_real()) completes and resets the field.

Update the assert to accommodate the transient state if the
filesystem has shutdown. While here, clean up the indentation and
comments in the function.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_btree.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index c4d7a9241dc3..b56ff451adce 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -353,20 +353,17 @@ xfs_btree_free_block(
  */
 void
 xfs_btree_del_cursor(
-	xfs_btree_cur_t	*cur,		/* btree cursor */
-	int		error)		/* del because of error */
+	struct xfs_btree_cur	*cur,		/* btree cursor */
+	int			error)		/* del because of error */
 {
-	int		i;		/* btree level */
+	int			i;		/* btree level */
 
 	/*
-	 * Clear the buffer pointers, and release the buffers.
-	 * If we're doing this in the face of an error, we
-	 * need to make sure to inspect all of the entries
-	 * in the bc_bufs array for buffers to be unlocked.
-	 * This is because some of the btree code works from
-	 * level n down to 0, and if we get an error along
-	 * the way we won't have initialized all the entries
-	 * down to 0.
+	 * Clear the buffer pointers and release the buffers. If we're doing
+	 * this because of an error, inspect all of the entries in the bc_bufs
+	 * array for buffers to be unlocked. This is because some of the btree
+	 * code works from level n down to 0, and if we get an error along the
+	 * way we won't have initialized all the entries down to 0.
 	 */
 	for (i = 0; i < cur->bc_nlevels; i++) {
 		if (cur->bc_bufs[i])
@@ -374,17 +371,11 @@ xfs_btree_del_cursor(
 		else if (!error)
 			break;
 	}
-	/*
-	 * Can't free a bmap cursor without having dealt with the
-	 * allocated indirect blocks' accounting.
-	 */
-	ASSERT(cur->bc_btnum != XFS_BTNUM_BMAP ||
-	       cur->bc_ino.allocated == 0);
-	/*
-	 * Free the cursor.
-	 */
+
+	ASSERT(cur->bc_btnum != XFS_BTNUM_BMAP || cur->bc_ino.allocated == 0 ||
+	       XFS_FORCED_SHUTDOWN(cur->bc_mp));
 	if (unlikely(cur->bc_flags & XFS_BTREE_STAGING))
-		kmem_free((void *)cur->bc_ops);
+		kmem_free(cur->bc_ops);
 	kmem_cache_free(xfs_btree_cur_zone, cur);
 }
 
-- 
2.26.2

Re: [PATCH] xfs: consider shutdown in bmapbt cursor delete assert

[Darrick J. Wong]

On Thu, Feb 11, 2021 at 09:39:11AM -0500, Brian Foster wrote:
> The assert in xfs_btree_del_cursor() checks that the bmapbt block
> allocation field has been handled correctly before the cursor is
> freed. This field is used for accurate calculation of indirect block
> reservation requirements (for delayed allocations), for example.
> generic/019 reproduces a scenario where this assert fails because
> the filesystem has shutdown while in the middle of a bmbt record
> insertion. This occurs after a bmbt block has been allocated via the
> cursor but before the higher level bmap function (i.e.
> xfs_bmap_add_extent_hole_real()) completes and resets the field.
> 
> Update the assert to accommodate the transient state if the
> filesystem has shutdown. While here, clean up the indentation and
> comments in the function.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Seems like a lot of cleanup for the amount of actual code change; you
might've sent them separately but I'll merge it anyway...

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_btree.c | 33 ++++++++++++---------------------
>  1 file changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index c4d7a9241dc3..b56ff451adce 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -353,20 +353,17 @@ xfs_btree_free_block(
>   */
>  void
>  xfs_btree_del_cursor(
> -	xfs_btree_cur_t	*cur,		/* btree cursor */
> -	int		error)		/* del because of error */
> +	struct xfs_btree_cur	*cur,		/* btree cursor */
> +	int			error)		/* del because of error */
>  {
> -	int		i;		/* btree level */
> +	int			i;		/* btree level */
>  
>  	/*
> -	 * Clear the buffer pointers, and release the buffers.
> -	 * If we're doing this in the face of an error, we
> -	 * need to make sure to inspect all of the entries
> -	 * in the bc_bufs array for buffers to be unlocked.
> -	 * This is because some of the btree code works from
> -	 * level n down to 0, and if we get an error along
> -	 * the way we won't have initialized all the entries
> -	 * down to 0.
> +	 * Clear the buffer pointers and release the buffers. If we're doing
> +	 * this because of an error, inspect all of the entries in the bc_bufs
> +	 * array for buffers to be unlocked. This is because some of the btree
> +	 * code works from level n down to 0, and if we get an error along the
> +	 * way we won't have initialized all the entries down to 0.
>  	 */
>  	for (i = 0; i < cur->bc_nlevels; i++) {
>  		if (cur->bc_bufs[i])
> @@ -374,17 +371,11 @@ xfs_btree_del_cursor(
>  		else if (!error)
>  			break;
>  	}
> -	/*
> -	 * Can't free a bmap cursor without having dealt with the
> -	 * allocated indirect blocks' accounting.
> -	 */
> -	ASSERT(cur->bc_btnum != XFS_BTNUM_BMAP ||
> -	       cur->bc_ino.allocated == 0);
> -	/*
> -	 * Free the cursor.
> -	 */
> +
> +	ASSERT(cur->bc_btnum != XFS_BTNUM_BMAP || cur->bc_ino.allocated == 0 ||
> +	       XFS_FORCED_SHUTDOWN(cur->bc_mp));
>  	if (unlikely(cur->bc_flags & XFS_BTREE_STAGING))
> -		kmem_free((void *)cur->bc_ops);
> +		kmem_free(cur->bc_ops);
>  	kmem_cache_free(xfs_btree_cur_zone, cur);
>  }
>  
> -- 
> 2.26.2
>