Re: [PATCH 09/12] xfs: place intent recovery under NOFS allocation context

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

On Tue, Jan 16, 2024 at 09:59:47AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When recovery starts processing intents, all of the initial intent
> allocations are done outside of transaction contexts. That means
> they need to specifically use GFP_NOFS as we do not want memory
> reclaim to attempt to run direct reclaim of filesystem objects while
> we have lots of objects added into deferred operations.
> 
> Rather than use GFP_NOFS for these specific allocations, just place
> the entire intent recovery process under NOFS context and we can
> then just use GFP_KERNEL for these allocations.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Hooray!  This finally goes away...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_attr_item.c     |  2 +-
>  fs/xfs/xfs_bmap_item.c     |  3 ++-
>  fs/xfs/xfs_log_recover.c   | 18 ++++++++++++++----
>  fs/xfs/xfs_refcount_item.c |  2 +-
>  fs/xfs/xfs_rmap_item.c     |  2 +-
>  5 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 0bf25a2ba3b6..e14e229fc712 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -513,7 +513,7 @@ xfs_attri_recover_work(
>  		return ERR_PTR(error);
>  
>  	attr = kzalloc(sizeof(struct xfs_attr_intent) +
> -			sizeof(struct xfs_da_args), GFP_NOFS | __GFP_NOFAIL);
> +			sizeof(struct xfs_da_args), GFP_KERNEL | __GFP_NOFAIL);
>  	args = (struct xfs_da_args *)(attr + 1);
>  
>  	attr->xattri_da_args = args;
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 029a6a8d0efd..e3c58090e976 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -445,7 +445,8 @@ xfs_bui_recover_work(
>  	if (error)
>  		return ERR_PTR(error);
>  
> -	bi = kmem_cache_zalloc(xfs_bmap_intent_cache, GFP_NOFS | __GFP_NOFAIL);
> +	bi = kmem_cache_zalloc(xfs_bmap_intent_cache,
> +			GFP_KERNEL | __GFP_NOFAIL);
>  	bi->bi_whichfork = (map->me_flags & XFS_BMAP_EXTENT_ATTR_FORK) ?
>  			XFS_ATTR_FORK : XFS_DATA_FORK;
>  	bi->bi_type = map->me_flags & XFS_BMAP_EXTENT_TYPE_MASK;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e9ed43a833af..8c1d260bb9e1 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3443,12 +3443,19 @@ xlog_recover(
>   * part of recovery so that the root and real-time bitmap inodes can be read in
>   * from disk in between the two stages.  This is necessary so that we can free
>   * space in the real-time portion of the file system.
> + *
> + * We run this whole process under GFP_NOFS allocation context. We do a
> + * combination of non-transactional and transactional work, yet we really don't
> + * want to recurse into the filesystem from direct reclaim during any of this
> + * processing. This allows all the recovery code run here not to care about the
> + * memory allocation context it is running in.
>   */
>  int
>  xlog_recover_finish(
>  	struct xlog	*log)
>  {
> -	int	error;
> +	unsigned int	nofs_flags = memalloc_nofs_save();
> +	int		error;
>  
>  	error = xlog_recover_process_intents(log);
>  	if (error) {
> @@ -3462,7 +3469,7 @@ xlog_recover_finish(
>  		xlog_recover_cancel_intents(log);
>  		xfs_alert(log->l_mp, "Failed to recover intents");
>  		xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
> -		return error;
> +		goto out_error;
>  	}
>  
>  	/*
> @@ -3483,7 +3490,7 @@ xlog_recover_finish(
>  		if (error < 0) {
>  			xfs_alert(log->l_mp,
>  	"Failed to clear log incompat features on recovery");
> -			return error;
> +			goto out_error;
>  		}
>  	}
>  
> @@ -3508,9 +3515,12 @@ xlog_recover_finish(
>  		 * and AIL.
>  		 */
>  		xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
> +		goto out_error;
>  	}
>  
> -	return 0;
> +out_error:
> +	memalloc_nofs_restore(nofs_flags);
> +	return error;
>  }
>  
>  void
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index d850b9685f7f..14919b33e4fe 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -425,7 +425,7 @@ xfs_cui_recover_work(
>  	struct xfs_refcount_intent	*ri;
>  
>  	ri = kmem_cache_alloc(xfs_refcount_intent_cache,
> -			GFP_NOFS | __GFP_NOFAIL);
> +			GFP_KERNEL | __GFP_NOFAIL);
>  	ri->ri_type = pmap->pe_flags & XFS_REFCOUNT_EXTENT_TYPE_MASK;
>  	ri->ri_startblock = pmap->pe_startblock;
>  	ri->ri_blockcount = pmap->pe_len;
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index a40b92ac81e8..e473124e29cc 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -455,7 +455,7 @@ xfs_rui_recover_work(
>  {
>  	struct xfs_rmap_intent		*ri;
>  
> -	ri = kmem_cache_alloc(xfs_rmap_intent_cache, GFP_NOFS | __GFP_NOFAIL);
> +	ri = kmem_cache_alloc(xfs_rmap_intent_cache, GFP_KERNEL | __GFP_NOFAIL);
>  
>  	switch (map->me_flags & XFS_RMAP_EXTENT_TYPE_MASK) {
>  	case XFS_RMAP_EXTENT_MAP:
> -- 
> 2.43.0
> 
>