* simplify the blockgc iwalk infrastructure
@ 2021-03-24 7:03 Christoph Hellwig
2021-03-24 7:03 ` [PATCH 1/3] xfs: use s_inodes in xfs_qm_dqrele_all_inodes Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Christoph Hellwig @ 2021-03-24 7:03 UTC (permalink / raw)
To: linux-xfs
Hi all,
this series switches quotaoff to use the VFS s_inodes list and then
simplifies the iwalk infrastructure for its only remaining user.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] xfs: use s_inodes in xfs_qm_dqrele_all_inodes
2021-03-24 7:03 simplify the blockgc iwalk infrastructure Christoph Hellwig
@ 2021-03-24 7:03 ` Christoph Hellwig
2021-03-24 17:50 ` Darrick J. Wong
2021-03-24 7:03 ` [PATCH 2/3] xfs: simplify the perage inode walk infrastructure Christoph Hellwig
2021-03-24 7:03 ` [PATCH 3/3] xfs: pass struct xfs_eofblocks to the inode scan callback Christoph Hellwig
2 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2021-03-24 7:03 UTC (permalink / raw)
To: linux-xfs
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_icache.c | 2 +-
fs/xfs/xfs_icache.h | 2 ++
fs/xfs/xfs_qm_syscalls.c | 56 +++++++++++++++++++++++++---------------
3 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 1d7720a0c068b7..c0d084e3526a9c 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -233,7 +233,7 @@ xfs_inode_clear_reclaim_tag(
xfs_perag_clear_reclaim_tag(pag);
}
-static void
+void
xfs_inew_wait(
struct xfs_inode *ip)
{
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index d1fddb1524200b..d70d93c2bb1084 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -78,4 +78,6 @@ int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
void xfs_blockgc_stop(struct xfs_mount *mp);
void xfs_blockgc_start(struct xfs_mount *mp);
+void xfs_inew_wait(struct xfs_inode *ip);
+
#endif
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index ca1b57d291dc90..88dfd520ae91de 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,35 @@ 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_FORCED_SHUTDOWN(mp))
+ break;
+ if (xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
+ continue;
+ if (xfs_iflags_test(ip, XFS_IRECLAIMABLE | XFS_IRECLAIM))
+ continue;
+ if (!igrab(inode))
+ continue;
+ spin_unlock(&sb->s_inode_list_lock);
+
+ iput(old_inode);
+ old_inode = inode;
+
+ if (xfs_iflags_test(ip, XFS_INEW))
+ xfs_inew_wait(ip);
+ 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] xfs: simplify the perage inode walk infrastructure
2021-03-24 7:03 simplify the blockgc iwalk infrastructure Christoph Hellwig
2021-03-24 7:03 ` [PATCH 1/3] xfs: use s_inodes in xfs_qm_dqrele_all_inodes Christoph Hellwig
@ 2021-03-24 7:03 ` Christoph Hellwig
2021-03-24 17:57 ` Darrick J. Wong
2021-03-24 7:03 ` [PATCH 3/3] xfs: pass struct xfs_eofblocks to the inode scan callback Christoph Hellwig
2 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2021-03-24 7:03 UTC (permalink / raw)
To: linux-xfs
Remove the generic xfs_inode_walk and just open code the only caller.
Note that this leads to a somewhat ugly forward delcaration for
xfs_blockgc_scan_inode, but with other changes in that area pending
it seems like the lesser evil.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_icache.c | 103 +++++++++++++-------------------------------
fs/xfs/xfs_icache.h | 11 -----
2 files changed, 30 insertions(+), 84 deletions(-)
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index c0d084e3526a9c..7fdf77df66269c 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -728,11 +728,9 @@ xfs_icache_inode_is_allocated(
*/
STATIC bool
xfs_inode_walk_ag_grab(
- struct xfs_inode *ip,
- int flags)
+ struct xfs_inode *ip)
{
struct inode *inode = VFS_I(ip);
- bool newinos = !!(flags & XFS_INODE_WALK_INEW_WAIT);
ASSERT(rcu_read_lock_held());
@@ -742,8 +740,7 @@ xfs_inode_walk_ag_grab(
goto out_unlock_noent;
/* avoid new or reclaimable inodes. Leave for reclaim code to flush */
- if ((!newinos && __xfs_iflags_test(ip, XFS_INEW)) ||
- __xfs_iflags_test(ip, XFS_IRECLAIMABLE | XFS_IRECLAIM))
+ if (__xfs_iflags_test(ip, XFS_INEW | XFS_IRECLAIMABLE | XFS_IRECLAIM))
goto out_unlock_noent;
spin_unlock(&ip->i_flags_lock);
@@ -763,17 +760,19 @@ xfs_inode_walk_ag_grab(
return false;
}
+static int
+xfs_blockgc_scan_inode(
+ struct xfs_inode *ip,
+ void *args);
+
/*
* For a given per-AG structure @pag, grab, @execute, and rele all incore
* inodes with the given radix tree @tag.
*/
STATIC int
-xfs_inode_walk_ag(
+xfs_blockgc_free_space_ag(
struct xfs_perag *pag,
- int iter_flags,
- int (*execute)(struct xfs_inode *ip, void *args),
- void *args,
- int tag)
+ struct xfs_eofblocks *eofb)
{
struct xfs_mount *mp = pag->pag_mount;
uint32_t first_index;
@@ -793,17 +792,9 @@ xfs_inode_walk_ag(
int i;
rcu_read_lock();
-
- if (tag == XFS_ICI_NO_TAG)
- nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
- (void **)batch, first_index,
- XFS_LOOKUP_BATCH);
- else
- nr_found = radix_tree_gang_lookup_tag(
- &pag->pag_ici_root,
- (void **) batch, first_index,
- XFS_LOOKUP_BATCH, tag);
-
+ nr_found = radix_tree_gang_lookup_tag(&pag->pag_ici_root,
+ (void **) batch, first_index,
+ XFS_LOOKUP_BATCH, XFS_ICI_BLOCKGC_TAG);
if (!nr_found) {
rcu_read_unlock();
break;
@@ -816,7 +807,7 @@ xfs_inode_walk_ag(
for (i = 0; i < nr_found; i++) {
struct xfs_inode *ip = batch[i];
- if (done || !xfs_inode_walk_ag_grab(ip, iter_flags))
+ if (done || !xfs_inode_walk_ag_grab(ip))
batch[i] = NULL;
/*
@@ -844,10 +835,7 @@ xfs_inode_walk_ag(
for (i = 0; i < nr_found; i++) {
if (!batch[i])
continue;
- if ((iter_flags & XFS_INODE_WALK_INEW_WAIT) &&
- xfs_iflags_test(batch[i], XFS_INEW))
- xfs_inew_wait(batch[i]);
- error = execute(batch[i], args);
+ error = xfs_blockgc_scan_inode(batch[i], eofb);
xfs_irele(batch[i]);
if (error == -EAGAIN) {
skipped++;
@@ -872,49 +860,6 @@ xfs_inode_walk_ag(
return last_error;
}
-/* Fetch the next (possibly tagged) per-AG structure. */
-static inline struct xfs_perag *
-xfs_inode_walk_get_perag(
- struct xfs_mount *mp,
- xfs_agnumber_t agno,
- int tag)
-{
- if (tag == XFS_ICI_NO_TAG)
- return xfs_perag_get(mp, agno);
- return xfs_perag_get_tag(mp, agno, tag);
-}
-
-/*
- * Call the @execute function on all incore inodes matching the radix tree
- * @tag.
- */
-int
-xfs_inode_walk(
- struct xfs_mount *mp,
- int iter_flags,
- int (*execute)(struct xfs_inode *ip, void *args),
- void *args,
- int tag)
-{
- struct xfs_perag *pag;
- int error = 0;
- int last_error = 0;
- xfs_agnumber_t ag;
-
- ag = 0;
- while ((pag = xfs_inode_walk_get_perag(mp, ag, tag))) {
- ag = pag->pag_agno + 1;
- error = xfs_inode_walk_ag(pag, iter_flags, execute, args, tag);
- xfs_perag_put(pag);
- if (error) {
- last_error = error;
- if (error == -EFSCORRUPTED)
- break;
- }
- }
- return last_error;
-}
-
/*
* Grab the inode for reclaim exclusively.
*
@@ -1617,8 +1562,7 @@ xfs_blockgc_worker(
if (!sb_start_write_trylock(mp->m_super))
return;
- error = xfs_inode_walk_ag(pag, 0, xfs_blockgc_scan_inode, NULL,
- XFS_ICI_BLOCKGC_TAG);
+ error = xfs_blockgc_free_space_ag(pag, NULL);
if (error)
xfs_info(mp, "AG %u preallocation gc worker failed, err=%d",
pag->pag_agno, error);
@@ -1634,10 +1578,23 @@ xfs_blockgc_free_space(
struct xfs_mount *mp,
struct xfs_eofblocks *eofb)
{
+ struct xfs_perag *pag;
+ xfs_agnumber_t ag = 0;
+ int error = 0, last_error = 0;
+
trace_xfs_blockgc_free_space(mp, eofb, _RET_IP_);
- return xfs_inode_walk(mp, 0, xfs_blockgc_scan_inode, eofb,
- XFS_ICI_BLOCKGC_TAG);
+ while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_BLOCKGC_TAG))) {
+ ag = pag->pag_agno + 1;
+ error = xfs_blockgc_free_space_ag(pag, eofb);
+ xfs_perag_put(pag);
+ if (error) {
+ if (error == -EFSCORRUPTED)
+ return error;
+ last_error = error;
+ }
+ }
+ return last_error;
}
/*
diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
index d70d93c2bb1084..da390097546c67 100644
--- a/fs/xfs/xfs_icache.h
+++ b/fs/xfs/xfs_icache.h
@@ -20,8 +20,6 @@ struct xfs_eofblocks {
/*
* tags for inode radix tree
*/
-#define XFS_ICI_NO_TAG (-1) /* special flag for an untagged lookup
- in xfs_inode_walk */
#define XFS_ICI_RECLAIM_TAG 0 /* inode is to be reclaimed */
/* Inode has speculative preallocations (posteof or cow) to clean. */
#define XFS_ICI_BLOCKGC_TAG 1
@@ -34,11 +32,6 @@ struct xfs_eofblocks {
#define XFS_IGET_DONTCACHE 0x4
#define XFS_IGET_INCORE 0x8 /* don't read from disk or reinit */
-/*
- * flags for AG inode iterator
- */
-#define XFS_INODE_WALK_INEW_WAIT 0x1 /* wait on new inodes */
-
int xfs_iget(struct xfs_mount *mp, struct xfs_trans *tp, xfs_ino_t ino,
uint flags, uint lock_flags, xfs_inode_t **ipp);
@@ -68,10 +61,6 @@ void xfs_inode_clear_cowblocks_tag(struct xfs_inode *ip);
void xfs_blockgc_worker(struct work_struct *work);
-int xfs_inode_walk(struct xfs_mount *mp, int iter_flags,
- int (*execute)(struct xfs_inode *ip, void *args),
- void *args, int tag);
-
int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
xfs_ino_t ino, bool *inuse);
--
2.30.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] xfs: pass struct xfs_eofblocks to the inode scan callback
2021-03-24 7:03 simplify the blockgc iwalk infrastructure Christoph Hellwig
2021-03-24 7:03 ` [PATCH 1/3] xfs: use s_inodes in xfs_qm_dqrele_all_inodes Christoph Hellwig
2021-03-24 7:03 ` [PATCH 2/3] xfs: simplify the perage inode walk infrastructure Christoph Hellwig
@ 2021-03-24 7:03 ` Christoph Hellwig
2021-03-24 18:03 ` Darrick J. Wong
2 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2021-03-24 7:03 UTC (permalink / raw)
To: linux-xfs
Pass the actual structure instead of a void pointer here now
that none of the functions is used as a callback.
Signed-off-by: Christoph Hellwig <hch@lst.de>
xfs_inode_free_cowblocks(
---
fs/xfs/xfs_icache.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 7fdf77df66269c..06286b5b613252 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -763,7 +763,7 @@ xfs_inode_walk_ag_grab(
static int
xfs_blockgc_scan_inode(
struct xfs_inode *ip,
- void *args);
+ struct xfs_eofblocks *eofb);
/*
* For a given per-AG structure @pag, grab, @execute, and rele all incore
@@ -1228,10 +1228,9 @@ xfs_reclaim_worker(
STATIC int
xfs_inode_free_eofblocks(
struct xfs_inode *ip,
- void *args,
+ struct xfs_eofblocks *eofb,
unsigned int *lockflags)
{
- struct xfs_eofblocks *eofb = args;
bool wait;
wait = eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC);
@@ -1436,10 +1435,9 @@ xfs_prep_free_cowblocks(
STATIC int
xfs_inode_free_cowblocks(
struct xfs_inode *ip,
- void *args,
+ struct xfs_eofblocks *eofb,
unsigned int *lockflags)
{
- struct xfs_eofblocks *eofb = args;
bool wait;
int ret = 0;
@@ -1534,16 +1532,16 @@ xfs_blockgc_start(
static int
xfs_blockgc_scan_inode(
struct xfs_inode *ip,
- void *args)
+ struct xfs_eofblocks *eofb)
{
unsigned int lockflags = 0;
int error;
- error = xfs_inode_free_eofblocks(ip, args, &lockflags);
+ error = xfs_inode_free_eofblocks(ip, eofb, &lockflags);
if (error)
goto unlock;
- error = xfs_inode_free_cowblocks(ip, args, &lockflags);
+ error = xfs_inode_free_cowblocks(ip, eofb, &lockflags);
unlock:
if (lockflags)
xfs_iunlock(ip, lockflags);
--
2.30.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] xfs: use s_inodes in xfs_qm_dqrele_all_inodes
2021-03-24 7:03 ` [PATCH 1/3] xfs: use s_inodes in xfs_qm_dqrele_all_inodes Christoph Hellwig
@ 2021-03-24 17:50 ` Darrick J. Wong
0 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2021-03-24 17:50 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Wed, Mar 24, 2021 at 08:03:05AM +0100, Christoph Hellwig wrote:
> 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>
Seems fine to me...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
> ---
> fs/xfs/xfs_icache.c | 2 +-
> fs/xfs/xfs_icache.h | 2 ++
> fs/xfs/xfs_qm_syscalls.c | 56 +++++++++++++++++++++++++---------------
> 3 files changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 1d7720a0c068b7..c0d084e3526a9c 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -233,7 +233,7 @@ xfs_inode_clear_reclaim_tag(
> xfs_perag_clear_reclaim_tag(pag);
> }
>
> -static void
> +void
> xfs_inew_wait(
> struct xfs_inode *ip)
> {
> diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
> index d1fddb1524200b..d70d93c2bb1084 100644
> --- a/fs/xfs/xfs_icache.h
> +++ b/fs/xfs/xfs_icache.h
> @@ -78,4 +78,6 @@ int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
> void xfs_blockgc_stop(struct xfs_mount *mp);
> void xfs_blockgc_start(struct xfs_mount *mp);
>
> +void xfs_inew_wait(struct xfs_inode *ip);
> +
> #endif
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index ca1b57d291dc90..88dfd520ae91de 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,35 @@ 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_FORCED_SHUTDOWN(mp))
> + break;
> + if (xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
> + continue;
> + if (xfs_iflags_test(ip, XFS_IRECLAIMABLE | XFS_IRECLAIM))
> + continue;
> + if (!igrab(inode))
> + continue;
> + spin_unlock(&sb->s_inode_list_lock);
> +
> + iput(old_inode);
> + old_inode = inode;
> +
> + if (xfs_iflags_test(ip, XFS_INEW))
> + xfs_inew_wait(ip);
> + 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] xfs: simplify the perage inode walk infrastructure
2021-03-24 7:03 ` [PATCH 2/3] xfs: simplify the perage inode walk infrastructure Christoph Hellwig
@ 2021-03-24 17:57 ` Darrick J. Wong
2021-03-24 17:59 ` Christoph Hellwig
0 siblings, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2021-03-24 17:57 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Wed, Mar 24, 2021 at 08:03:06AM +0100, Christoph Hellwig wrote:
> Remove the generic xfs_inode_walk and just open code the only caller.
This is going in the wrong direction for me. Maybe.
I was planning to combine the reclaim inode walk into this function, and
later on share it with inactivation. This made for one switch-happy
iteration function, but it meant there was only one loop.
OFC maybe the point that you and/or Dave were trying to make is that I
should be doing the opposite, and combining the inactivation loop into
what is now the (badly misnamed) xfs_reclaim_inodes_ag? And leave this
blockgc loop alone?
<shrug>
--D
> Note that this leads to a somewhat ugly forward delcaration for
> xfs_blockgc_scan_inode, but with other changes in that area pending
> it seems like the lesser evil.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_icache.c | 103 +++++++++++++-------------------------------
> fs/xfs/xfs_icache.h | 11 -----
> 2 files changed, 30 insertions(+), 84 deletions(-)
>
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index c0d084e3526a9c..7fdf77df66269c 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -728,11 +728,9 @@ xfs_icache_inode_is_allocated(
> */
> STATIC bool
> xfs_inode_walk_ag_grab(
> - struct xfs_inode *ip,
> - int flags)
> + struct xfs_inode *ip)
> {
> struct inode *inode = VFS_I(ip);
> - bool newinos = !!(flags & XFS_INODE_WALK_INEW_WAIT);
>
> ASSERT(rcu_read_lock_held());
>
> @@ -742,8 +740,7 @@ xfs_inode_walk_ag_grab(
> goto out_unlock_noent;
>
> /* avoid new or reclaimable inodes. Leave for reclaim code to flush */
> - if ((!newinos && __xfs_iflags_test(ip, XFS_INEW)) ||
> - __xfs_iflags_test(ip, XFS_IRECLAIMABLE | XFS_IRECLAIM))
> + if (__xfs_iflags_test(ip, XFS_INEW | XFS_IRECLAIMABLE | XFS_IRECLAIM))
> goto out_unlock_noent;
> spin_unlock(&ip->i_flags_lock);
>
> @@ -763,17 +760,19 @@ xfs_inode_walk_ag_grab(
> return false;
> }
>
> +static int
> +xfs_blockgc_scan_inode(
> + struct xfs_inode *ip,
> + void *args);
> +
> /*
> * For a given per-AG structure @pag, grab, @execute, and rele all incore
> * inodes with the given radix tree @tag.
> */
> STATIC int
> -xfs_inode_walk_ag(
> +xfs_blockgc_free_space_ag(
> struct xfs_perag *pag,
> - int iter_flags,
> - int (*execute)(struct xfs_inode *ip, void *args),
> - void *args,
> - int tag)
> + struct xfs_eofblocks *eofb)
> {
> struct xfs_mount *mp = pag->pag_mount;
> uint32_t first_index;
> @@ -793,17 +792,9 @@ xfs_inode_walk_ag(
> int i;
>
> rcu_read_lock();
> -
> - if (tag == XFS_ICI_NO_TAG)
> - nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
> - (void **)batch, first_index,
> - XFS_LOOKUP_BATCH);
> - else
> - nr_found = radix_tree_gang_lookup_tag(
> - &pag->pag_ici_root,
> - (void **) batch, first_index,
> - XFS_LOOKUP_BATCH, tag);
> -
> + nr_found = radix_tree_gang_lookup_tag(&pag->pag_ici_root,
> + (void **) batch, first_index,
> + XFS_LOOKUP_BATCH, XFS_ICI_BLOCKGC_TAG);
> if (!nr_found) {
> rcu_read_unlock();
> break;
> @@ -816,7 +807,7 @@ xfs_inode_walk_ag(
> for (i = 0; i < nr_found; i++) {
> struct xfs_inode *ip = batch[i];
>
> - if (done || !xfs_inode_walk_ag_grab(ip, iter_flags))
> + if (done || !xfs_inode_walk_ag_grab(ip))
> batch[i] = NULL;
>
> /*
> @@ -844,10 +835,7 @@ xfs_inode_walk_ag(
> for (i = 0; i < nr_found; i++) {
> if (!batch[i])
> continue;
> - if ((iter_flags & XFS_INODE_WALK_INEW_WAIT) &&
> - xfs_iflags_test(batch[i], XFS_INEW))
> - xfs_inew_wait(batch[i]);
> - error = execute(batch[i], args);
> + error = xfs_blockgc_scan_inode(batch[i], eofb);
> xfs_irele(batch[i]);
> if (error == -EAGAIN) {
> skipped++;
> @@ -872,49 +860,6 @@ xfs_inode_walk_ag(
> return last_error;
> }
>
> -/* Fetch the next (possibly tagged) per-AG structure. */
> -static inline struct xfs_perag *
> -xfs_inode_walk_get_perag(
> - struct xfs_mount *mp,
> - xfs_agnumber_t agno,
> - int tag)
> -{
> - if (tag == XFS_ICI_NO_TAG)
> - return xfs_perag_get(mp, agno);
> - return xfs_perag_get_tag(mp, agno, tag);
> -}
> -
> -/*
> - * Call the @execute function on all incore inodes matching the radix tree
> - * @tag.
> - */
> -int
> -xfs_inode_walk(
> - struct xfs_mount *mp,
> - int iter_flags,
> - int (*execute)(struct xfs_inode *ip, void *args),
> - void *args,
> - int tag)
> -{
> - struct xfs_perag *pag;
> - int error = 0;
> - int last_error = 0;
> - xfs_agnumber_t ag;
> -
> - ag = 0;
> - while ((pag = xfs_inode_walk_get_perag(mp, ag, tag))) {
> - ag = pag->pag_agno + 1;
> - error = xfs_inode_walk_ag(pag, iter_flags, execute, args, tag);
> - xfs_perag_put(pag);
> - if (error) {
> - last_error = error;
> - if (error == -EFSCORRUPTED)
> - break;
> - }
> - }
> - return last_error;
> -}
> -
> /*
> * Grab the inode for reclaim exclusively.
> *
> @@ -1617,8 +1562,7 @@ xfs_blockgc_worker(
>
> if (!sb_start_write_trylock(mp->m_super))
> return;
> - error = xfs_inode_walk_ag(pag, 0, xfs_blockgc_scan_inode, NULL,
> - XFS_ICI_BLOCKGC_TAG);
> + error = xfs_blockgc_free_space_ag(pag, NULL);
> if (error)
> xfs_info(mp, "AG %u preallocation gc worker failed, err=%d",
> pag->pag_agno, error);
> @@ -1634,10 +1578,23 @@ xfs_blockgc_free_space(
> struct xfs_mount *mp,
> struct xfs_eofblocks *eofb)
> {
> + struct xfs_perag *pag;
> + xfs_agnumber_t ag = 0;
> + int error = 0, last_error = 0;
> +
> trace_xfs_blockgc_free_space(mp, eofb, _RET_IP_);
>
> - return xfs_inode_walk(mp, 0, xfs_blockgc_scan_inode, eofb,
> - XFS_ICI_BLOCKGC_TAG);
> + while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_BLOCKGC_TAG))) {
> + ag = pag->pag_agno + 1;
> + error = xfs_blockgc_free_space_ag(pag, eofb);
> + xfs_perag_put(pag);
> + if (error) {
> + if (error == -EFSCORRUPTED)
> + return error;
> + last_error = error;
> + }
> + }
> + return last_error;
> }
>
> /*
> diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
> index d70d93c2bb1084..da390097546c67 100644
> --- a/fs/xfs/xfs_icache.h
> +++ b/fs/xfs/xfs_icache.h
> @@ -20,8 +20,6 @@ struct xfs_eofblocks {
> /*
> * tags for inode radix tree
> */
> -#define XFS_ICI_NO_TAG (-1) /* special flag for an untagged lookup
> - in xfs_inode_walk */
> #define XFS_ICI_RECLAIM_TAG 0 /* inode is to be reclaimed */
> /* Inode has speculative preallocations (posteof or cow) to clean. */
> #define XFS_ICI_BLOCKGC_TAG 1
> @@ -34,11 +32,6 @@ struct xfs_eofblocks {
> #define XFS_IGET_DONTCACHE 0x4
> #define XFS_IGET_INCORE 0x8 /* don't read from disk or reinit */
>
> -/*
> - * flags for AG inode iterator
> - */
> -#define XFS_INODE_WALK_INEW_WAIT 0x1 /* wait on new inodes */
> -
> int xfs_iget(struct xfs_mount *mp, struct xfs_trans *tp, xfs_ino_t ino,
> uint flags, uint lock_flags, xfs_inode_t **ipp);
>
> @@ -68,10 +61,6 @@ void xfs_inode_clear_cowblocks_tag(struct xfs_inode *ip);
>
> void xfs_blockgc_worker(struct work_struct *work);
>
> -int xfs_inode_walk(struct xfs_mount *mp, int iter_flags,
> - int (*execute)(struct xfs_inode *ip, void *args),
> - void *args, int tag);
> -
> int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
> xfs_ino_t ino, bool *inuse);
>
> --
> 2.30.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] xfs: simplify the perage inode walk infrastructure
2021-03-24 17:57 ` Darrick J. Wong
@ 2021-03-24 17:59 ` Christoph Hellwig
2021-03-25 4:30 ` Darrick J. Wong
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2021-03-24 17:59 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs
On Wed, Mar 24, 2021 at 10:57:35AM -0700, Darrick J. Wong wrote:
> On Wed, Mar 24, 2021 at 08:03:06AM +0100, Christoph Hellwig wrote:
> > Remove the generic xfs_inode_walk and just open code the only caller.
>
> This is going in the wrong direction for me. Maybe.
>
> I was planning to combine the reclaim inode walk into this function, and
> later on share it with inactivation. This made for one switch-happy
> iteration function, but it meant there was only one loop.
Ok, we can skip this for now if this gets in your way. Or I can resend
a different patch 2 that just removes the no tag case for now.
> OFC maybe the point that you and/or Dave were trying to make is that I
> should be doing the opposite, and combining the inactivation loop into
> what is now the (badly misnamed) xfs_reclaim_inodes_ag? And leave this
> blockgc loop alone?
That is my gut feeling. No guarantee it actually works out, and given
that I've lead you down the wrong road a few times I already feel guily
ahead of time..
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] xfs: pass struct xfs_eofblocks to the inode scan callback
2021-03-24 7:03 ` [PATCH 3/3] xfs: pass struct xfs_eofblocks to the inode scan callback Christoph Hellwig
@ 2021-03-24 18:03 ` Darrick J. Wong
0 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2021-03-24 18:03 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Wed, Mar 24, 2021 at 08:03:07AM +0100, Christoph Hellwig wrote:
> Pass the actual structure instead of a void pointer here now
> that none of the functions is used as a callback.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> xfs_inode_free_cowblocks(
Assuming this ends up fitting in somewhere,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
> ---
> fs/xfs/xfs_icache.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 7fdf77df66269c..06286b5b613252 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -763,7 +763,7 @@ xfs_inode_walk_ag_grab(
> static int
> xfs_blockgc_scan_inode(
> struct xfs_inode *ip,
> - void *args);
> + struct xfs_eofblocks *eofb);
>
> /*
> * For a given per-AG structure @pag, grab, @execute, and rele all incore
> @@ -1228,10 +1228,9 @@ xfs_reclaim_worker(
> STATIC int
> xfs_inode_free_eofblocks(
> struct xfs_inode *ip,
> - void *args,
> + struct xfs_eofblocks *eofb,
> unsigned int *lockflags)
> {
> - struct xfs_eofblocks *eofb = args;
> bool wait;
>
> wait = eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC);
> @@ -1436,10 +1435,9 @@ xfs_prep_free_cowblocks(
> STATIC int
> xfs_inode_free_cowblocks(
> struct xfs_inode *ip,
> - void *args,
> + struct xfs_eofblocks *eofb,
> unsigned int *lockflags)
> {
> - struct xfs_eofblocks *eofb = args;
> bool wait;
> int ret = 0;
>
> @@ -1534,16 +1532,16 @@ xfs_blockgc_start(
> static int
> xfs_blockgc_scan_inode(
> struct xfs_inode *ip,
> - void *args)
> + struct xfs_eofblocks *eofb)
> {
> unsigned int lockflags = 0;
> int error;
>
> - error = xfs_inode_free_eofblocks(ip, args, &lockflags);
> + error = xfs_inode_free_eofblocks(ip, eofb, &lockflags);
> if (error)
> goto unlock;
>
> - error = xfs_inode_free_cowblocks(ip, args, &lockflags);
> + error = xfs_inode_free_cowblocks(ip, eofb, &lockflags);
> unlock:
> if (lockflags)
> xfs_iunlock(ip, lockflags);
> --
> 2.30.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] xfs: simplify the perage inode walk infrastructure
2021-03-24 17:59 ` Christoph Hellwig
@ 2021-03-25 4:30 ` Darrick J. Wong
0 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2021-03-25 4:30 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Wed, Mar 24, 2021 at 06:59:37PM +0100, Christoph Hellwig wrote:
> On Wed, Mar 24, 2021 at 10:57:35AM -0700, Darrick J. Wong wrote:
> > On Wed, Mar 24, 2021 at 08:03:06AM +0100, Christoph Hellwig wrote:
> > > Remove the generic xfs_inode_walk and just open code the only caller.
> >
> > This is going in the wrong direction for me. Maybe.
> >
> > I was planning to combine the reclaim inode walk into this function, and
> > later on share it with inactivation. This made for one switch-happy
> > iteration function, but it meant there was only one loop.
>
> Ok, we can skip this for now if this gets in your way. Or I can resend
> a different patch 2 that just removes the no tag case for now.
>
> > OFC maybe the point that you and/or Dave were trying to make is that I
> > should be doing the opposite, and combining the inactivation loop into
> > what is now the (badly misnamed) xfs_reclaim_inodes_ag? And leave this
> > blockgc loop alone?
>
> That is my gut feeling. No guarantee it actually works out, and given
> that I've lead you down the wrong road a few times I already feel guily
> ahead of time..
Actually, collapsing all of the tag walkers into xfs_inode_walk was
pretty straightforward, and in the end I just borrowed bits and pieces
from patches 2 and 3 to make it happen and clean up the arguments. The
net change is 55 lines deleted and ~1k less code (granted with all the
debugging and ubsan crud turned on).
--D
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-25 4:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-24 7:03 simplify the blockgc iwalk infrastructure Christoph Hellwig
2021-03-24 7:03 ` [PATCH 1/3] xfs: use s_inodes in xfs_qm_dqrele_all_inodes Christoph Hellwig
2021-03-24 17:50 ` Darrick J. Wong
2021-03-24 7:03 ` [PATCH 2/3] xfs: simplify the perage inode walk infrastructure Christoph Hellwig
2021-03-24 17:57 ` Darrick J. Wong
2021-03-24 17:59 ` Christoph Hellwig
2021-03-25 4:30 ` Darrick J. Wong
2021-03-24 7:03 ` [PATCH 3/3] xfs: pass struct xfs_eofblocks to the inode scan callback Christoph Hellwig
2021-03-24 18:03 ` Darrick J. Wong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.