Re: [PATCH] xfs: quota radix tree allocations need to be NOFS on insert

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

On Fri, Mar 15, 2024 at 12:16:39PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> In converting the XFS code from GFP_NOFS to scoped contexts, we
> converted the quota radix tree to GFP_KERNEL. Unfortunately, it was
> not clearly documented that this set was because there is a
> dependency on the quotainfo->qi_tree_lock being taken in memory
> reclaim to remove dquots from the radix tree.
> 
> In hindsight this is obvious, but the radix tree allocations on
> insert are not immediately obvious, and we avoid this for the inode
> cache radix trees by using preloading and hence completely avoiding
> the radix tree node allocation under tree lock constraints.
> 
> Hence there are a few solutions here. The first is to reinstate
> GFP_NOFS for the radix tree and add a comment explaining why
> GFP_NOFS is used. The second is to use memalloc_nofs_save() on the
> radix tree insert context, which makes it obvious that the radix
> tree insert runs under GFP_NOFS constraints. The third option is to
> simply replace the radix tree and it's lock with an xarray which can
> do memory allocation safely in an insert context.
> 
> The first is OK, but not really the direction we want to head. The
> second is my preferred short term solution. The third - converting
> XFS radix trees to xarray - is the longer term solution.
> 
> Hence to fix the regression here, we take option 2 as it moves us in
> the direction we want to head with memory allocation and GFP_NOFS
> removal.
> 
> Reported-by: syzbot+8fdff861a781522bda4d@syzkaller.appspotmail.com
> Reported-by: syzbot+d247769793ec169e4bf9@syzkaller.appspotmail.com
> Fixes: 94a69db2367e ("xfs: use __GFP_NOLOCKDEP instead of GFP_NOFS")
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_dquot.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index d2c7fcc2ea6b..9c027e44d88f 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -811,6 +811,12 @@ xfs_qm_dqget_cache_lookup(
>   * caller should throw away the dquot and start over.  Otherwise, the dquot
>   * is returned locked (and held by the cache) as if there had been a cache
>   * hit.
> + *
> + * The insert needs to be done under memalloc_nofs context because the radix
> + * tree can do memory allocation during insert. The qi->qi_tree_lock is taken in
> + * memory reclaim when freeing unused dquots, so we cannot have the radix tree
> + * node allocation recursing into filesystem reclaim whilst we hold the
> + * qi_tree_lock.

Sounds reasonable to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>   */
>  static int
>  xfs_qm_dqget_cache_insert(
> @@ -820,25 +826,27 @@ xfs_qm_dqget_cache_insert(
>  	xfs_dqid_t		id,
>  	struct xfs_dquot	*dqp)
>  {
> +	unsigned int		nofs_flags;
>  	int			error;
>  
> +	nofs_flags = memalloc_nofs_save();
>  	mutex_lock(&qi->qi_tree_lock);
>  	error = radix_tree_insert(tree, id, dqp);
>  	if (unlikely(error)) {
>  		/* Duplicate found!  Caller must try again. */
> -		mutex_unlock(&qi->qi_tree_lock);
>  		trace_xfs_dqget_dup(dqp);
> -		return error;
> +		goto out_unlock;
>  	}
>  
>  	/* Return a locked dquot to the caller, with a reference taken. */
>  	xfs_dqlock(dqp);
>  	dqp->q_nrefs = 1;
> -
>  	qi->qi_dquots++;
> -	mutex_unlock(&qi->qi_tree_lock);
>  
> -	return 0;
> +out_unlock:
> +	mutex_unlock(&qi->qi_tree_lock);
> +	memalloc_nofs_restore(nofs_flags);
> +	return error;
>  }
>  
>  /* Check our input parameters. */
> -- 
> 2.43.0
> 
>