* [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 related [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 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 related [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 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
* [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 related [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 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
* [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 related [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 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: [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
end of thread, other threads:[~2021-03-04 0:18 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).