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

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

[Dave Chinner]

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.
  */
 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

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

[Darrick J. Wong]

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
> 
> 

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

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Dave Chinner]

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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 30d36596a2e4..c98cb468c357 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.
  */
 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

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

[Christoph Hellwig]

Didn't this just get merged into the for-next tree already?

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

[Chandan Babu R]

On Mon, Mar 18, 2024 at 11:37:07 PM -0700, Christoph Hellwig wrote:
> Didn't this just get merged into the for-next tree already?

Yes, This patch was merged into XFS's for-next branch on Monday.

-- 
Chandan