On Fri, Mar 15, 2024 at 12:16:39PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: syzbot+d247769793ec169e4bf9@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 94a69db2367e ("xfs: use __GFP_NOLOCKDEP instead of GFP_NOFS")
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--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
>
>
