Re: [PATCH 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag}
From: "Darrick J. Wong" <djwong@kernel.org>
Date: 2021-03-19 16:44:56
On Fri, Mar 19, 2021 at 06:25:01AM +0000, Christoph Hellwig wrote:
On Thu, Mar 18, 2021 at 03:33:45PM -0700, Darrick J. Wong wrote:quoted
From: Darrick J. Wong <djwong@kernel.org> It turns out that there is a 1:1 mapping between the execute and tag parameters that are passed to xfs_inode_walk_ag: xfs_dqrele_inode => XFS_ICI_NO_TAG xfs_blockgc_scan_inode => XFS_ICI_BLOCKGC_TAG The radix tree tags are an implementation detail of the inode cache, which means that callers outside of xfs_icache.c have no business passing in radix tree tags. Since we're about to get rid of the indirect calls in the BLOCKGC case, eliminate the extra argument in favor of computing the ICI tag from the execute argument passed into the function.This seems backwards to me. I'd rather deduce the function from the talk, which seems like a more sensible pattern.
Fair enough.
That being said, the quota inode walk is a little different in that it doesn't use any tags, so switching it to a plain list_for_each_entry_safe on sb->s_inodes would seems more sensible, something like this untested patch:
Hmm, well, I look forward to hearing the results of your testing. :) I /think/ this will work, since quotaoff doesn't touch inodes that can't be igrabbed (i.e. their VFS state is gone), so walking sb->s_inodes /should/ be the same. The only thing I'm not sure about is that the vfs removes the inode from the sb list before clear_inode sets I_FREEING (to prevent further igrab), which /could/ introduce a behavioral change? Though I think even if quotaoff ends up racing with evict_inodes, the xfs_fs_destroy_inode call will inactivate the inode and drop the dquots (before the next patchset) or queue the inode for inactivation and detach the dquots. One thing that occurs to me -- do the quota and rt metadata inodes end up on the sb inode list? The rt metadata inodes definitely contribute to the root dquot's inode counts. --D
quoted hunk ↗ jump to hunk
--- From 9ae07b6bf8c6b1337a627c8f0ad619c56511b343 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Fri, 19 Mar 2021 07:16:31 +0100 Subject: xfs: use s_inodes in xfs_qm_dqrele_all_inodes Using xfs_inode_walk in xfs_qm_dqrele_all_inodes is complete overkill, given that function simplify wants to iterate all live inodes known to the VFS. Just iterate over the s_inodes list. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_qm_syscalls.c | 50 +++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 21 deletions(-)diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c index 11f1e2fbf22f44..4e33919ed04b56 100644 --- a/fs/xfs/xfs_qm_syscalls.c +++ b/fs/xfs/xfs_qm_syscalls.c@@ -748,41 +748,27 @@ xfs_qm_scall_getquota_next( return error; } -STATIC int +static void xfs_dqrele_inode( struct xfs_inode *ip, - void *args) + uint flags) { - uint *flags = args; - - /* skip quota inodes */ - if (ip == ip->i_mount->m_quotainfo->qi_uquotaip || - ip == ip->i_mount->m_quotainfo->qi_gquotaip || - ip == ip->i_mount->m_quotainfo->qi_pquotaip) { - ASSERT(ip->i_udquot == NULL); - ASSERT(ip->i_gdquot == NULL); - ASSERT(ip->i_pdquot == NULL); - return 0; - } - xfs_ilock(ip, XFS_ILOCK_EXCL); - if ((*flags & XFS_UQUOTA_ACCT) && ip->i_udquot) { + if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) { xfs_qm_dqrele(ip->i_udquot); ip->i_udquot = NULL; } - if ((*flags & XFS_GQUOTA_ACCT) && ip->i_gdquot) { + if ((flags & XFS_GQUOTA_ACCT) && ip->i_gdquot) { xfs_qm_dqrele(ip->i_gdquot); ip->i_gdquot = NULL; } - if ((*flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) { + if ((flags & XFS_PQUOTA_ACCT) && ip->i_pdquot) { xfs_qm_dqrele(ip->i_pdquot); ip->i_pdquot = NULL; } xfs_iunlock(ip, XFS_ILOCK_EXCL); - return 0; } - /* * Go thru all the inodes in the file system, releasing their dquots. *@@ -794,7 +780,29 @@ xfs_qm_dqrele_all_inodes( struct xfs_mount *mp, uint flags) { + struct super_block *sb = mp->m_super; + struct inode *inode, *old_inode = NULL; + ASSERT(mp->m_quotainfo); - xfs_inode_walk(mp, XFS_INODE_WALK_INEW_WAIT, xfs_dqrele_inode, - &flags, XFS_ICI_NO_TAG); + + spin_lock(&sb->s_inode_list_lock); + list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { + struct xfs_inode *ip = XFS_I(inode); + + if (xfs_is_quota_inode(&mp->m_sb, ip->i_ino)) + continue; + if (!igrab(inode)) + continue; + spin_unlock(&sb->s_inode_list_lock); + + iput(old_inode); + old_inode = inode; + + xfs_dqrele_inode(ip, flags); + + spin_lock(&sb->s_inode_list_lock); + } + spin_unlock(&sb->s_inode_list_lock); + + iput(old_inode); }-- 2.30.1