[PATCHSET 0/3] xfs: small fixes for 5.12

[PATCHSET 0/3] xfs: small fixes for 5.12

[Darrick J. Wong]

Hi all,

Here's a handful of bug fixes for 5.12.  The first one fixes a bug in
quota accounting on idmapped mounts; the second avoids a buffer deadlock
in bulkstat/inumbers if the inobt is corrupt; and the third fixes a
mount hang if mount fails after creating (or otherwise dirtying) the
quota inodes.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes-5.12
---
 fs/xfs/xfs_inode.c   |   14 ++++++++------
 fs/xfs/xfs_itable.c  |   46 ++++++++++++++++++++++++++++++++++++++++++----
 fs/xfs/xfs_mount.c   |   10 ++++++++++
 fs/xfs/xfs_symlink.c |    3 ++-
 4 files changed, 62 insertions(+), 11 deletions(-)

[PATCH 1/3] xfs: fix quota accounting when a mount is idmapped

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Nowadays, we indirectly use the idmap-aware helper functions in the VFS
to set the initial uid and gid of a file being created.  Unfortunately,
we didn't convert the quota code, which means we attach the wrong dquots
to files created on an idmapped mount.

Fixes: f736d93d76d3 ("xfs: support idmapped mounts")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_inode.c   |   14 ++++++++------
 fs/xfs/xfs_symlink.c |    3 ++-
 2 files changed, 10 insertions(+), 7 deletions(-)


diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 46a861d55e48..f93370bd7b1e 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1007,9 +1007,10 @@ xfs_create(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
-					XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
-					&udqp, &gdqp, &pdqp);
+	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
+			fsgid_into_mnt(mnt_userns), prid,
+			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
+			&udqp, &gdqp, &pdqp);
 	if (error)
 		return error;
 
@@ -1157,9 +1158,10 @@ xfs_create_tmpfile(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
-				XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
-				&udqp, &gdqp, &pdqp);
+	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
+			fsgid_into_mnt(mnt_userns), prid,
+			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
+			&udqp, &gdqp, &pdqp);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 1379013d74b8..7f368b10ded1 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -182,7 +182,8 @@ xfs_symlink(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
+	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
+			fsgid_into_mnt(mnt_userns), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
 			&udqp, &gdqp, &pdqp);
 	if (error)

[PATCH 2/3] xfs: avoid buffer deadlocks in inumbers/bulkstat

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

When we're servicing an INUMBERS or BULKSTAT request, grab an empty
transaction so that we don't hit an ABBA buffer deadlock if the inode
btree contains a cycle.

Found by fuzzing an inode btree pointer to introduce a cycle into the
tree (xfs/365).

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_itable.c |   46 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index ca310a125d1e..2339a1874efa 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -19,6 +19,7 @@
 #include "xfs_error.h"
 #include "xfs_icache.h"
 #include "xfs_health.h"
+#include "xfs_trans.h"
 
 /*
  * Bulk Stat
@@ -166,6 +167,7 @@ xfs_bulkstat_one(
 		.formatter	= formatter,
 		.breq		= breq,
 	};
+	struct xfs_trans	*tp;
 	int			error;
 
 	ASSERT(breq->icount == 1);
@@ -175,9 +177,20 @@ xfs_bulkstat_one(
 	if (!bc.buf)
 		return -ENOMEM;
 
+	/*
+	 * Grab freeze protection and allocate an empty transaction so that
+	 * we avoid deadlocking if the inobt is corrupt.
+	 */
+	sb_start_write(breq->mp->m_super);
+	error = xfs_trans_alloc_empty(breq->mp, &tp);
+	if (error)
+		goto out;
+
 	error = xfs_bulkstat_one_int(breq->mp, breq->mnt_userns, NULL,
 				     breq->startino, &bc);
-
+	xfs_trans_cancel(tp);
+out:
+	sb_end_write(breq->mp->m_super);
 	kmem_free(bc.buf);
 
 	/*
@@ -241,6 +254,7 @@ xfs_bulkstat(
 		.formatter	= formatter,
 		.breq		= breq,
 	};
+	struct xfs_trans	*tp;
 	int			error;
 
 	if (breq->mnt_userns != &init_user_ns) {
@@ -256,9 +270,20 @@ xfs_bulkstat(
 	if (!bc.buf)
 		return -ENOMEM;
 
-	error = xfs_iwalk(breq->mp, NULL, breq->startino, breq->flags,
+	/*
+	 * Grab freeze protection and allocate an empty transaction so that
+	 * we avoid deadlocking if the inobt is corrupt.
+	 */
+	sb_start_write(breq->mp->m_super);
+	error = xfs_trans_alloc_empty(breq->mp, &tp);
+	if (error)
+		goto out;
+
+	error = xfs_iwalk(breq->mp, tp, breq->startino, breq->flags,
 			xfs_bulkstat_iwalk, breq->icount, &bc);
-
+	xfs_trans_cancel(tp);
+out:
+	sb_end_write(breq->mp->m_super);
 	kmem_free(bc.buf);
 
 	/*
@@ -371,13 +396,26 @@ xfs_inumbers(
 		.formatter	= formatter,
 		.breq		= breq,
 	};
+	struct xfs_trans	*tp;
 	int			error = 0;
 
 	if (xfs_bulkstat_already_done(breq->mp, breq->startino))
 		return 0;
 
-	error = xfs_inobt_walk(breq->mp, NULL, breq->startino, breq->flags,
+	/*
+	 * Grab freeze protection and allocate an empty transaction so that
+	 * we avoid deadlocking if the inobt is corrupt.
+	 */
+	sb_start_write(breq->mp->m_super);
+	error = xfs_trans_alloc_empty(breq->mp, &tp);
+	if (error)
+		goto out;
+
+	error = xfs_inobt_walk(breq->mp, tp, breq->startino, breq->flags,
 			xfs_inumbers_walk, breq->icount, &ic);
+	xfs_trans_cancel(tp);
+out:
+	sb_end_write(breq->mp->m_super);
 
 	/*
 	 * We found some inode groups, so clear the error status and return

[PATCH 3/3] xfs: force log and push AIL to clear pinned inodes when aborting mount

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If we allocate quota inodes in the process of mounting a filesystem but
then decide to abort the mount, it's possible that the quota inodes are
sitting around pinned by the log.  Now that inode reclaim relies on the
AIL to flush inodes, we have to force the log and push the AIL in
between releasing the quota inodes and kicking off reclaim to tear down
all the incore inodes.

This was originally found during a fuzz test of metadata directories
(xfs/1546), but the actual symptom was that reclaim hung up on the quota
inodes.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_mount.c |   10 ++++++++++
 1 file changed, 10 insertions(+)


diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 52370d0a3f43..6f445b611663 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1007,6 +1007,16 @@ xfs_mountfs(
 	xfs_irele(rip);
 	/* Clean out dquots that might be in memory after quotacheck. */
 	xfs_qm_unmount(mp);
+
+	/*
+	 * It's possible that we modified some inodes as part of setting up
+	 * quotas or initializing filesystem metadata.  These inodes could be
+	 * pinned in the log, so force the log and push the AIL to unpin them
+	 * so that we can reclaim them.
+	 */
+	xfs_log_force(mp, XFS_LOG_SYNC);
+	xfs_ail_push_all_sync(mp->m_ail);
+
 	/*
 	 * Cancel all delayed reclaim work and reclaim the inodes directly.
 	 * We have to do this /after/ rtunmount and qm_unmount because those

Re: [PATCHSET 0/3] xfs: small fixes for 5.12

[Christian Brauner]

On Tue, Mar 2, 2021 at 11:28 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> Hi all,
>
> Here's a handful of bug fixes for 5.12.  The first one fixes a bug in
> quota accounting on idmapped mounts; the second avoids a buffer deadlock
> in bulkstat/inumbers if the inobt is corrupt; and the third fixes a
> mount hang if mount fails after creating (or otherwise dirtying) the
> quota inodes.
>
> If you're going to start using this mess, you probably ought to just
> pull from my git trees, which are linked below.
>
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.

Thanks for doing this, Darrick.
I haven't forgotten the xfstests addition for quotas I promised yesterday.
While writing the tests I got bitten by the swapfile bug (Yes yes, I
was insane enough
to run -rc1 for the first time at that.) and now I'm in the process of
recovering my
development machine, emails and all. I'll try to review this tomorrow!

Thanks again!
Christian

>
> --D
>
> kernel git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes-5.12
> ---
>  fs/xfs/xfs_inode.c   |   14 ++++++++------
>  fs/xfs/xfs_itable.c  |   46 ++++++++++++++++++++++++++++++++++++++++++----
>  fs/xfs/xfs_mount.c   |   10 ++++++++++
>  fs/xfs/xfs_symlink.c |    3 ++-
>  4 files changed, 62 insertions(+), 11 deletions(-)
>

Re: [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped

[Christoph Hellwig]

On Tue, Mar 02, 2021 at 02:28:22PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Nowadays, we indirectly use the idmap-aware helper functions in the VFS
> to set the initial uid and gid of a file being created.  Unfortunately,
> we didn't convert the quota code, which means we attach the wrong dquots
> to files created on an idmapped mount.
> 
> Fixes: f736d93d76d3 ("xfs: support idmapped mounts")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good,

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

Re: [PATCH 2/3] xfs: avoid buffer deadlocks in inumbers/bulkstat

[Christoph Hellwig]

On Tue, Mar 02, 2021 at 02:28:28PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> When we're servicing an INUMBERS or BULKSTAT request, grab an empty
> transaction so that we don't hit an ABBA buffer deadlock if the inode
> btree contains a cycle.
> 
> Found by fuzzing an inode btree pointer to introduce a cycle into the
> tree (xfs/365).
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

So basically you want to piggy back on the per-trans recursion using
xfs_trans_buf_item_match?  Why do we need the sb-counter for that?
Can the comments be a little more clear?  Why don't we want that
elsewhere where we're walking the btrees?

Re: [PATCH 3/3] xfs: force log and push AIL to clear pinned inodes when aborting mount

[Christoph Hellwig]

On Tue, Mar 02, 2021 at 02:28:34PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If we allocate quota inodes in the process of mounting a filesystem but
> then decide to abort the mount, it's possible that the quota inodes are
> sitting around pinned by the log.  Now that inode reclaim relies on the
> AIL to flush inodes, we have to force the log and push the AIL in
> between releasing the quota inodes and kicking off reclaim to tear down
> all the incore inodes.
> 
> This was originally found during a fuzz test of metadata directories
> (xfs/1546), but the actual symptom was that reclaim hung up on the quota
> inodes.

This looks ok, but I'm a little worried about sprinkling these log
forces and AIL pushes around.  We have a similar one but split int
the regular unmount path, and I wonder if we just need to regularize
that.

Re: [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped

[Christian Brauner]

On Tue, Mar 02, 2021 at 02:28:22PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Nowadays, we indirectly use the idmap-aware helper functions in the VFS
> to set the initial uid and gid of a file being created.  Unfortunately,
> we didn't convert the quota code, which means we attach the wrong dquots
> to files created on an idmapped mount.
> 
> Fixes: f736d93d76d3 ("xfs: support idmapped mounts")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

This looks good but it misses one change, I think. The
xfs_ioctl_setattr() needs to take the mapping into account as well:

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 99dfe89a8d08..067366bf9a59 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1460,8 +1460,8 @@ xfs_ioctl_setattr(
         * because the i_*dquot fields will get updated anyway.
         */
        if (XFS_IS_QUOTA_ON(mp)) {
-               error = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
-                               VFS_I(ip)->i_gid, fa->fsx_projid,
+               error = xfs_qm_vop_dqalloc(ip, i_uid_into_mnt(mnt_userns, VFS_I(ip)),
+                               i_gid_into_mnt(mnt_userns, VFS_I(ip)), fa->fsx_projid,
                                XFS_QMOPT_PQUOTA, NULL, NULL, &pdqp);
                if (error)
                        return error;

This should cover all callsites.

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

>  fs/xfs/xfs_inode.c   |   14 ++++++++------
>  fs/xfs/xfs_symlink.c |    3 ++-
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 46a861d55e48..f93370bd7b1e 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1007,9 +1007,10 @@ xfs_create(
>  	/*
>  	 * Make sure that we have allocated dquot(s) on disk.
>  	 */
> -	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> -					XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> -					&udqp, &gdqp, &pdqp);
> +	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
> +			fsgid_into_mnt(mnt_userns), prid,
> +			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> +			&udqp, &gdqp, &pdqp);
>  	if (error)
>  		return error;
>  
> @@ -1157,9 +1158,10 @@ xfs_create_tmpfile(
>  	/*
>  	 * Make sure that we have allocated dquot(s) on disk.
>  	 */
> -	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> -				XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> -				&udqp, &gdqp, &pdqp);
> +	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
> +			fsgid_into_mnt(mnt_userns), prid,
> +			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> +			&udqp, &gdqp, &pdqp);
>  	if (error)
>  		return error;
>  
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 1379013d74b8..7f368b10ded1 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -182,7 +182,8 @@ xfs_symlink(
>  	/*
>  	 * Make sure that we have allocated dquot(s) on disk.
>  	 */
> -	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> +	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
> +			fsgid_into_mnt(mnt_userns), prid,
>  			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
>  			&udqp, &gdqp, &pdqp);
>  	if (error)
> 

Re: [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped

[Darrick J. Wong]

On Wed, Mar 03, 2021 at 02:44:01PM +0000, Christian Brauner wrote:
> On Tue, Mar 02, 2021 at 02:28:22PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Nowadays, we indirectly use the idmap-aware helper functions in the VFS
> > to set the initial uid and gid of a file being created.  Unfortunately,
> > we didn't convert the quota code, which means we attach the wrong dquots
> > to files created on an idmapped mount.
> > 
> > Fixes: f736d93d76d3 ("xfs: support idmapped mounts")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> This looks good but it misses one change, I think. The
> xfs_ioctl_setattr() needs to take the mapping into account as well:
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 99dfe89a8d08..067366bf9a59 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1460,8 +1460,8 @@ xfs_ioctl_setattr(
>          * because the i_*dquot fields will get updated anyway.
>          */
>         if (XFS_IS_QUOTA_ON(mp)) {
> -               error = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
> -                               VFS_I(ip)->i_gid, fa->fsx_projid,
> +               error = xfs_qm_vop_dqalloc(ip, i_uid_into_mnt(mnt_userns, VFS_I(ip)),
> +                               i_gid_into_mnt(mnt_userns, VFS_I(ip)), fa->fsx_projid,
>                                 XFS_QMOPT_PQUOTA, NULL, NULL, &pdqp);

The uid/gid parameters here don't matter because the "XFS_QMOPT_PQUOTA"
flag by itself here means that _dqalloc only looks at the project id
argument.

>                 if (error)
>                         return error;
> 
> This should cover all callsites.
> 
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Thanks!

--D

> >  fs/xfs/xfs_inode.c   |   14 ++++++++------
> >  fs/xfs/xfs_symlink.c |    3 ++-
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 46a861d55e48..f93370bd7b1e 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -1007,9 +1007,10 @@ xfs_create(
> >  	/*
> >  	 * Make sure that we have allocated dquot(s) on disk.
> >  	 */
> > -	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> > -					XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> > -					&udqp, &gdqp, &pdqp);
> > +	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
> > +			fsgid_into_mnt(mnt_userns), prid,
> > +			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> > +			&udqp, &gdqp, &pdqp);
> >  	if (error)
> >  		return error;
> >  
> > @@ -1157,9 +1158,10 @@ xfs_create_tmpfile(
> >  	/*
> >  	 * Make sure that we have allocated dquot(s) on disk.
> >  	 */
> > -	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> > -				XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> > -				&udqp, &gdqp, &pdqp);
> > +	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
> > +			fsgid_into_mnt(mnt_userns), prid,
> > +			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> > +			&udqp, &gdqp, &pdqp);
> >  	if (error)
> >  		return error;
> >  
> > diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> > index 1379013d74b8..7f368b10ded1 100644
> > --- a/fs/xfs/xfs_symlink.c
> > +++ b/fs/xfs/xfs_symlink.c
> > @@ -182,7 +182,8 @@ xfs_symlink(
> >  	/*
> >  	 * Make sure that we have allocated dquot(s) on disk.
> >  	 */
> > -	error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> > +	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
> > +			fsgid_into_mnt(mnt_userns), prid,
> >  			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> >  			&udqp, &gdqp, &pdqp);
> >  	if (error)
> > 

Re: [PATCH 2/3] xfs: avoid buffer deadlocks in inumbers/bulkstat

[Darrick J. Wong]

On Wed, Mar 03, 2021 at 07:45:56AM +0100, Christoph Hellwig wrote:
> On Tue, Mar 02, 2021 at 02:28:28PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > When we're servicing an INUMBERS or BULKSTAT request, grab an empty
> > transaction so that we don't hit an ABBA buffer deadlock if the inode
> > btree contains a cycle.
> > 
> > Found by fuzzing an inode btree pointer to introduce a cycle into the
> > tree (xfs/365).
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> 
> So basically you want to piggy back on the per-trans recursion using
> xfs_trans_buf_item_match?

Right.

> Why do we need the sb-counter for that?

I was about to say that we need freeze protection to prevent fsfreeze
and inumbers/bulkstat stalling each other out on the buffer LRU, but
then I remembered that Brian changed freeze to wait for IO to complete
without dumping the buffer cache this cycle.  So, I don't think freeze
protection is necessary anymore.

> Can the comments be a little more clear?

/*
 * Grab an empty transaction so that we can use the recursive locking
 * support to detect tree cycles instead of deadlocking.
 */

How does that sound?

> Why don't we want that elsewhere where we're walking the btrees?

I'm merely playing whackamole with tree walks here.  I would guess that
quotacheck will be next for evaluation.

It also occurs to me that inumbers/bulkstat should probably not be
calling copy_to_user() separately for every single record.

--D

Re: [PATCH 3/3] xfs: force log and push AIL to clear pinned inodes when aborting mount

[Darrick J. Wong]

On Wed, Mar 03, 2021 at 07:48:12AM +0100, Christoph Hellwig wrote:
> On Tue, Mar 02, 2021 at 02:28:34PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > If we allocate quota inodes in the process of mounting a filesystem but
> > then decide to abort the mount, it's possible that the quota inodes are
> > sitting around pinned by the log.  Now that inode reclaim relies on the
> > AIL to flush inodes, we have to force the log and push the AIL in
> > between releasing the quota inodes and kicking off reclaim to tear down
> > all the incore inodes.
> > 
> > This was originally found during a fuzz test of metadata directories
> > (xfs/1546), but the actual symptom was that reclaim hung up on the quota
> > inodes.
> 
> This looks ok, but I'm a little worried about sprinkling these log
> forces and AIL pushes around.  We have a similar one but split int
> the regular unmount path, and I wonder if we just need to regularize
> that.

Hmm, the unmount path splits the log force and the ail split while we
wait for free extents to get discarded.  Log replay can free extents, so
at the very least I think we need to do that here too.

--D

Re: [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped

[Christian Brauner]

On Wed, Mar 03, 2021 at 10:33:52AM -0800, Darrick J. Wong wrote:
> On Wed, Mar 03, 2021 at 02:44:01PM +0000, Christian Brauner wrote:
> > On Tue, Mar 02, 2021 at 02:28:22PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Nowadays, we indirectly use the idmap-aware helper functions in the VFS
> > > to set the initial uid and gid of a file being created.  Unfortunately,
> > > we didn't convert the quota code, which means we attach the wrong dquots
> > > to files created on an idmapped mount.
> > > 
> > > Fixes: f736d93d76d3 ("xfs: support idmapped mounts")
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > 
> > This looks good but it misses one change, I think. The
> > xfs_ioctl_setattr() needs to take the mapping into account as well:
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 99dfe89a8d08..067366bf9a59 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1460,8 +1460,8 @@ xfs_ioctl_setattr(
> >          * because the i_*dquot fields will get updated anyway.
> >          */
> >         if (XFS_IS_QUOTA_ON(mp)) {
> > -               error = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
> > -                               VFS_I(ip)->i_gid, fa->fsx_projid,
> > +               error = xfs_qm_vop_dqalloc(ip, i_uid_into_mnt(mnt_userns, VFS_I(ip)),
> > +                               i_gid_into_mnt(mnt_userns, VFS_I(ip)), fa->fsx_projid,
> >                                 XFS_QMOPT_PQUOTA, NULL, NULL, &pdqp);
> 
> The uid/gid parameters here don't matter because the "XFS_QMOPT_PQUOTA"
> flag by itself here means that _dqalloc only looks at the project id
> argument.

Ah, thanks! (Maybe a comment would be good or just idmaped them anyway.)

Thanks!
Christian