Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCHSET 0/3] xfs: small fixes for 5.12
@ 2021-03-02 22:28 Darrick J. Wong
  2021-03-02 22:28 ` [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped Darrick J. Wong
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Darrick J. Wong @ 2021-03-02 22:28 UTC (permalink / raw)
  To: djwong; +Cc: linux-xfs, hch, dchinner, christian.brauner

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped
  2021-03-02 22:28 [PATCHSET 0/3] xfs: small fixes for 5.12 Darrick J. Wong
@ 2021-03-02 22:28 ` Darrick J. Wong
  2021-03-03  6:43   ` Christoph Hellwig
  2021-03-03 14:44   ` Christian Brauner
  2021-03-02 22:28 ` [PATCH 2/3] xfs: avoid buffer deadlocks in inumbers/bulkstat Darrick J. Wong
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Darrick J. Wong @ 2021-03-02 22:28 UTC (permalink / raw)
  To: djwong; +Cc: linux-xfs, hch, dchinner, christian.brauner

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)


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/3] xfs: avoid buffer deadlocks in inumbers/bulkstat
  2021-03-02 22:28 [PATCHSET 0/3] xfs: small fixes for 5.12 Darrick J. Wong
  2021-03-02 22:28 ` [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped Darrick J. Wong
@ 2021-03-02 22:28 ` Darrick J. Wong
  2021-03-03  6:45   ` Christoph Hellwig
  2021-03-02 22:28 ` [PATCH 3/3] xfs: force log and push AIL to clear pinned inodes when aborting mount Darrick J. Wong
  2021-03-02 23:07 ` [PATCHSET 0/3] xfs: small fixes for 5.12 Christian Brauner
  3 siblings, 1 reply; 13+ messages in thread
From: Darrick J. Wong @ 2021-03-02 22:28 UTC (permalink / raw)
  To: djwong; +Cc: linux-xfs, hch, dchinner, christian.brauner

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] xfs: force log and push AIL to clear pinned inodes when aborting mount
  2021-03-02 22:28 [PATCHSET 0/3] xfs: small fixes for 5.12 Darrick J. Wong
  2021-03-02 22:28 ` [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped Darrick J. Wong
  2021-03-02 22:28 ` [PATCH 2/3] xfs: avoid buffer deadlocks in inumbers/bulkstat Darrick J. Wong
@ 2021-03-02 22:28 ` Darrick J. Wong
  2021-03-03  6:48   ` Christoph Hellwig
  2021-03-02 23:07 ` [PATCHSET 0/3] xfs: small fixes for 5.12 Christian Brauner
  3 siblings, 1 reply; 13+ messages in thread
From: Darrick J. Wong @ 2021-03-02 22:28 UTC (permalink / raw)
  To: djwong; +Cc: linux-xfs, hch, dchinner, christian.brauner

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHSET 0/3] xfs: small fixes for 5.12
  2021-03-02 22:28 [PATCHSET 0/3] xfs: small fixes for 5.12 Darrick J. Wong
                   ` (2 preceding siblings ...)
  2021-03-02 22:28 ` [PATCH 3/3] xfs: force log and push AIL to clear pinned inodes when aborting mount Darrick J. Wong
@ 2021-03-02 23:07 ` Christian Brauner
  3 siblings, 0 replies; 13+ messages in thread
From: Christian Brauner @ 2021-03-02 23:07 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, dchinner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped
  2021-03-02 22:28 ` [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped Darrick J. Wong
@ 2021-03-03  6:43   ` Christoph Hellwig
  2021-03-03 14:44   ` Christian Brauner
  1 sibling, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2021-03-03  6:43 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, dchinner, 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>

Looks good,

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] xfs: avoid buffer deadlocks in inumbers/bulkstat
  2021-03-02 22:28 ` [PATCH 2/3] xfs: avoid buffer deadlocks in inumbers/bulkstat Darrick J. Wong
@ 2021-03-03  6:45   ` Christoph Hellwig
  2021-03-03 18:57     ` Darrick J. Wong
  0 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2021-03-03  6:45 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, dchinner, christian.brauner

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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] xfs: force log and push AIL to clear pinned inodes when aborting mount
  2021-03-02 22:28 ` [PATCH 3/3] xfs: force log and push AIL to clear pinned inodes when aborting mount Darrick J. Wong
@ 2021-03-03  6:48   ` Christoph Hellwig
  2021-03-03 19:02     ` Darrick J. Wong
  0 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2021-03-03  6:48 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, dchinner, christian.brauner

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped
  2021-03-02 22:28 ` [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped Darrick J. Wong
  2021-03-03  6:43   ` Christoph Hellwig
@ 2021-03-03 14:44   ` Christian Brauner
  2021-03-03 18:33     ` Darrick J. Wong
  1 sibling, 1 reply; 13+ messages in thread
From: Christian Brauner @ 2021-03-03 14:44 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, dchinner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped
  2021-03-03 14:44   ` Christian Brauner
@ 2021-03-03 18:33     ` Darrick J. Wong
  2021-03-03 19:09       ` Christian Brauner
  0 siblings, 1 reply; 13+ messages in thread
From: Darrick J. Wong @ 2021-03-03 18:33 UTC (permalink / raw)
  To: Christian Brauner; +Cc: linux-xfs, hch, dchinner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] xfs: avoid buffer deadlocks in inumbers/bulkstat
  2021-03-03  6:45   ` Christoph Hellwig
@ 2021-03-03 18:57     ` Darrick J. Wong
  0 siblings, 0 replies; 13+ messages in thread
From: Darrick J. Wong @ 2021-03-03 18:57 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs, dchinner, christian.brauner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] xfs: force log and push AIL to clear pinned inodes when aborting mount
  2021-03-03  6:48   ` Christoph Hellwig
@ 2021-03-03 19:02     ` Darrick J. Wong
  0 siblings, 0 replies; 13+ messages in thread
From: Darrick J. Wong @ 2021-03-03 19:02 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs, dchinner, christian.brauner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped
  2021-03-03 18:33     ` Darrick J. Wong
@ 2021-03-03 19:09       ` Christian Brauner
  0 siblings, 0 replies; 13+ messages in thread
From: Christian Brauner @ 2021-03-03 19:09 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, hch, dchinner

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-02 22:28 [PATCHSET 0/3] xfs: small fixes for 5.12 Darrick J. Wong
2021-03-02 22:28 ` [PATCH 1/3] xfs: fix quota accounting when a mount is idmapped Darrick J. Wong
2021-03-03  6:43   ` Christoph Hellwig
2021-03-03 14:44   ` Christian Brauner
2021-03-03 18:33     ` Darrick J. Wong
2021-03-03 19:09       ` Christian Brauner
2021-03-02 22:28 ` [PATCH 2/3] xfs: avoid buffer deadlocks in inumbers/bulkstat Darrick J. Wong
2021-03-03  6:45   ` Christoph Hellwig
2021-03-03 18:57     ` Darrick J. Wong
2021-03-02 22:28 ` [PATCH 3/3] xfs: force log and push AIL to clear pinned inodes when aborting mount Darrick J. Wong
2021-03-03  6:48   ` Christoph Hellwig
2021-03-03 19:02     ` Darrick J. Wong
2021-03-02 23:07 ` [PATCHSET 0/3] xfs: small fixes for 5.12 Christian Brauner

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-xfs linux-xfs/ https://lore.kernel.org/linux-xfs \
		linux-xfs@vger.kernel.org
	public-inbox-index linux-xfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git