Re: [PATCH 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag}
From: Christoph Hellwig <hch@infradead.org>
Date: 2021-03-19 07:36:53
Subsystem:
filesystems (vfs and infrastructure), the rest, xfs filesystem · Maintainers:
Alexander Viro, Christian Brauner, Linus Torvalds, Carlos Maiolino
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. 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:
Something like this is probably going to work better (maybe split into two patches). I'm going to kick off some testing on it: --- 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