From: Christoph Hellwig <hch@infradead.org>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org, hch@infradead.org
Subject: Re: [PATCH 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag}
Date: Fri, 19 Mar 2021 07:35:23 +0000 [thread overview]
Message-ID: <20210319073523.GA980561@infradead.org> (raw)
In-Reply-To: <20210319062501.GC955126@infradead.org>
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:
> > 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
next prev parent reply other threads:[~2021-03-19 7:36 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-18 22:33 [PATCHSET 0/3] xfs: reduce indirect function calls in inode walk Darrick J. Wong
2021-03-18 22:33 ` [PATCH 1/3] xfs: remove tag parameter from xfs_inode_walk{,_ag} Darrick J. Wong
2021-03-19 6:25 ` Christoph Hellwig
2021-03-19 7:35 ` Christoph Hellwig [this message]
2021-03-19 16:43 ` Darrick J. Wong
2021-03-19 16:48 ` Christoph Hellwig
2021-03-23 18:35 ` Christoph Hellwig
2021-03-18 22:33 ` [PATCH 2/3] xfs: reduce indirect calls in xfs_inode_walk{,_ag} Darrick J. Wong
2021-03-18 22:33 ` [PATCH 3/3] xfs: remove iter_flags parameter from xfs_inode_walk_* Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210319073523.GA980561@infradead.org \
--to=hch@infradead.org \
--cc=djwong@kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).