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

["Darrick J. Wong" <djwong@kernel.org>]

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
>