Re: [PATCH 7/7] xfs_repair: retain superblock buffer to avoid write hook deadlock

[Carlos Maiolino <cem@kernel.org>]

> Fix this by retaining a reference to the superblock buffer when possible
> so that the writeback hook doesn't have to access the buffer cache to
> set NEEDSREPAIR.

This is the same one you sent for 5.19 that opened a discussion between
retaining it at specific points, or at 'mount' time, wasn't it? Anyway, I think
this is ok, we can try to do it at mount time in the future.


> 
> Fixes: 3b7667cb ("xfs_repair: set NEEDSREPAIR the first time we write to a filesystem")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  libxfs/libxfs_api_defs.h |    2 +
>  libxfs/libxfs_io.h       |    1 +
>  libxfs/rdwr.c            |    8 +++++
>  repair/phase2.c          |    8 +++++
>  repair/protos.h          |    1 +
>  repair/xfs_repair.c      |   75 ++++++++++++++++++++++++++++++++++++++++------
>  6 files changed, 86 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index 2716a731bf..f8efcce777 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -53,9 +53,11 @@
>  #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
>  #define xfs_buf_get			libxfs_buf_get
>  #define xfs_buf_get_uncached		libxfs_buf_get_uncached
> +#define xfs_buf_lock			libxfs_buf_lock
>  #define xfs_buf_read			libxfs_buf_read
>  #define xfs_buf_read_uncached		libxfs_buf_read_uncached
>  #define xfs_buf_relse			libxfs_buf_relse
> +#define xfs_buf_unlock			libxfs_buf_unlock
>  #define xfs_bunmapi			libxfs_bunmapi
>  #define xfs_bwrite			libxfs_bwrite
>  #define xfs_calc_dquots_per_chunk	libxfs_calc_dquots_per_chunk
> diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> index 9c0e2704d1..fae8642720 100644
> --- a/libxfs/libxfs_io.h
> +++ b/libxfs/libxfs_io.h
> @@ -226,6 +226,7 @@ xfs_buf_hold(struct xfs_buf *bp)
>  }
> 
>  void xfs_buf_lock(struct xfs_buf *bp);
> +void xfs_buf_unlock(struct xfs_buf *bp);
> 
>  int libxfs_buf_get_uncached(struct xfs_buftarg *targ, size_t bblen, int flags,
>  		struct xfs_buf **bpp);
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 20e0793c2f..d5aad3ea21 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -384,6 +384,14 @@ xfs_buf_lock(
>  		pthread_mutex_lock(&bp->b_lock);
>  }
> 
> +void
> +xfs_buf_unlock(
> +	struct xfs_buf	*bp)
> +{
> +	if (use_xfs_buf_lock)
> +		pthread_mutex_unlock(&bp->b_lock);
> +}
> +
>  static int
>  __cache_lookup(
>  	struct xfs_bufkey	*key,
> diff --git a/repair/phase2.c b/repair/phase2.c
> index 56a39bb456..2ada95aefd 100644
> --- a/repair/phase2.c
> +++ b/repair/phase2.c
> @@ -370,6 +370,14 @@ phase2(
>  	} else
>  		do_log(_("Phase 2 - using internal log\n"));
> 
> +	/*
> +	 * Now that we've set up the buffer cache the way we want it, try to
> +	 * grab our own reference to the primary sb so that the hooks will not
> +	 * have to call out to the buffer cache.
> +	 */
> +	if (mp->m_buf_writeback_fn)
> +		retain_primary_sb(mp);
> +
>  	/* Zero log if applicable */
>  	do_log(_("        - zero log...\n"));
> 
> diff --git a/repair/protos.h b/repair/protos.h
> index 03ebae1413..83e471ff2a 100644
> --- a/repair/protos.h
> +++ b/repair/protos.h
> @@ -16,6 +16,7 @@ int	get_sb(xfs_sb_t			*sbp,
>  		xfs_off_t			off,
>  		int			size,
>  		xfs_agnumber_t		agno);
> +int retain_primary_sb(struct xfs_mount *mp);
>  void	write_primary_sb(xfs_sb_t	*sbp,
>  			int		size);
> 
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 871b428d7d..ff29bea974 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -749,6 +749,63 @@ check_fs_vs_host_sectsize(
>  	}
>  }
> 
> +/*
> + * If we set up a writeback function to set NEEDSREPAIR while the filesystem is
> + * dirty, there's a chance that calling libxfs_getsb could deadlock the buffer
> + * cache while trying to get the primary sb buffer if the first non-sb write to
> + * the filesystem is the result of a cache shake.  Retain a reference to the
> + * primary sb buffer to avoid all that.
> + */
> +static struct xfs_buf *primary_sb_bp;	/* buffer for superblock */
> +
> +int
> +retain_primary_sb(
> +	struct xfs_mount	*mp)
> +{
> +	int			error;
> +
> +	error = -libxfs_buf_read(mp->m_ddev_targp, XFS_SB_DADDR,
> +			XFS_FSS_TO_BB(mp, 1), 0, &primary_sb_bp,
> +			&xfs_sb_buf_ops);
> +	if (error)
> +		return error;
> +
> +	libxfs_buf_unlock(primary_sb_bp);
> +	return 0;
> +}
> +
> +static void
> +drop_primary_sb(void)
> +{
> +	if (!primary_sb_bp)
> +		return;
> +
> +	libxfs_buf_lock(primary_sb_bp);
> +	libxfs_buf_relse(primary_sb_bp);
> +	primary_sb_bp = NULL;
> +}
> +
> +static int
> +get_primary_sb(
> +	struct xfs_mount	*mp,
> +	struct xfs_buf		**bpp)
> +{
> +	int			error;
> +
> +	*bpp = NULL;
> +
> +	if (!primary_sb_bp) {
> +		error = retain_primary_sb(mp);
> +		if (error)
> +			return error;
> +	}
> +
> +	libxfs_buf_lock(primary_sb_bp);
> +	xfs_buf_hold(primary_sb_bp);
> +	*bpp = primary_sb_bp;
> +	return 0;
> +}
> +
>  /* Clear needsrepair after a successful repair run. */
>  static void
>  clear_needsrepair(
> @@ -769,15 +826,14 @@ clear_needsrepair(
>  		do_warn(
>  	_("Cannot clear needsrepair due to flush failure, err=%d.\n"),
>  			error);
> -		return;
> +		goto drop;
>  	}
> 
>  	/* Clear needsrepair from the superblock. */
> -	bp = libxfs_getsb(mp);
> -	if (!bp || bp->b_error) {
> +	error = get_primary_sb(mp, &bp);
> +	if (error) {
>  		do_warn(
> -	_("Cannot clear needsrepair from primary super, err=%d.\n"),
> -			bp ? bp->b_error : ENOMEM);
> +	_("Cannot clear needsrepair from primary super, err=%d.\n"), error);
>  	} else {
>  		mp->m_sb.sb_features_incompat &=
>  				~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> @@ -786,6 +842,8 @@ clear_needsrepair(
>  	}
>  	if (bp)
>  		libxfs_buf_relse(bp);
> +drop:
> +	drop_primary_sb();
>  }
> 
>  static void
> @@ -808,11 +866,10 @@ force_needsrepair(
>  	    xfs_sb_version_needsrepair(&mp->m_sb))
>  		return;
> 
> -	bp = libxfs_getsb(mp);
> -	if (!bp || bp->b_error) {
> +	error = get_primary_sb(mp, &bp);
> +	if (error) {
>  		do_log(
> -	_("couldn't get superblock to set needsrepair, err=%d\n"),
> -				bp ? bp->b_error : ENOMEM);
> +	_("couldn't get superblock to set needsrepair, err=%d\n"), error);
>  	} else {
>  		/*
>  		 * It's possible that we need to set NEEDSREPAIR before we've
> 

-- 
Carlos Maiolino