Re: [PATCH 06/12] xfs: use an empty transaction for fstrim

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

On Tue, Jan 16, 2024 at 09:59:44AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We currently use a btree walk in the fstrim code. This requires a
> btree cursor and btree cursors are only used inside transactions
> except for the fstrim code. This means that all the btree operations
> that allocate memory operate in both GFP_KERNEL and GFP_NOFS
> contexts.
> 
> This causes problems with lockdep being unable to determine the
> difference between objects that are safe to lock both above and
> below memory reclaim. Free space btree buffers are definitely locked
> both above and below reclaim and that means we have to mark all
> btree infrastructure allocations with GFP_NOFS to avoid potential
> lockdep false positives.
> 
> If we wrap this btree walk in an empty cursor, all btree walks are
> now done under transaction context and so all allocations inherit
> GFP_NOFS context from the tranaction. This enables us to move all
> the btree allocations to GFP_KERNEL context and hence help remove
> the explicit use of GFP_NOFS in XFS.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

LOL I just wrote this exact patch to shut up lockdep.

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

--D

> ---
>  fs/xfs/xfs_discard.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index 8539f5c9a774..299b8f907292 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -8,6 +8,7 @@
>  #include "xfs_format.h"
>  #include "xfs_log_format.h"
>  #include "xfs_trans_resv.h"
> +#include "xfs_trans.h"
>  #include "xfs_mount.h"
>  #include "xfs_btree.h"
>  #include "xfs_alloc_btree.h"
> @@ -120,7 +121,7 @@ xfs_discard_extents(
>  		error = __blkdev_issue_discard(mp->m_ddev_targp->bt_bdev,
>  				XFS_AGB_TO_DADDR(mp, busyp->agno, busyp->bno),
>  				XFS_FSB_TO_BB(mp, busyp->length),
> -				GFP_NOFS, &bio);
> +				GFP_KERNEL, &bio);
>  		if (error && error != -EOPNOTSUPP) {
>  			xfs_info(mp,
>  	 "discard failed for extent [0x%llx,%u], error %d",
> @@ -155,6 +156,7 @@ xfs_trim_gather_extents(
>  	uint64_t		*blocks_trimmed)
>  {
>  	struct xfs_mount	*mp = pag->pag_mount;
> +	struct xfs_trans	*tp;
>  	struct xfs_btree_cur	*cur;
>  	struct xfs_buf		*agbp;
>  	int			error;
> @@ -168,11 +170,15 @@ xfs_trim_gather_extents(
>  	 */
>  	xfs_log_force(mp, XFS_LOG_SYNC);
>  
> -	error = xfs_alloc_read_agf(pag, NULL, 0, &agbp);
> +	error = xfs_trans_alloc_empty(mp, &tp);
>  	if (error)
>  		return error;
>  
> -	cur = xfs_allocbt_init_cursor(mp, NULL, agbp, pag, XFS_BTNUM_CNT);
> +	error = xfs_alloc_read_agf(pag, tp, 0, &agbp);
> +	if (error)
> +		goto out_trans_cancel;
> +
> +	cur = xfs_allocbt_init_cursor(mp, tp, agbp, pag, XFS_BTNUM_CNT);
>  
>  	/*
>  	 * Look up the extent length requested in the AGF and start with it.
> @@ -279,7 +285,8 @@ xfs_trim_gather_extents(
>  		xfs_extent_busy_clear(mp, &extents->extent_list, false);
>  out_del_cursor:
>  	xfs_btree_del_cursor(cur, error);
> -	xfs_buf_relse(agbp);
> +out_trans_cancel:
> +	xfs_trans_cancel(tp);
>  	return error;
>  }
>  
> -- 
> 2.43.0
> 
>