On Fri, Mar 26, 2021 at 06:48:09AM +0000, Christoph Hellwig wrote: > On Thu, Mar 25, 2021 at 05:21:46PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > > > In preparation for adding another incore inode tree tag, refactor the > > code that sets and clears tags from the per-AG inode tree and the tree > > of per-AG structures, and remove the open-coded versions used by the > > blockgc code. > > > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx> > > --- > > fs/xfs/xfs_icache.c | 127 ++++++++++++++++++++++++--------------------------- > > fs/xfs/xfs_icache.h | 2 - > > fs/xfs/xfs_super.c | 2 - > > fs/xfs/xfs_trace.h | 6 +- > > 4 files changed, 65 insertions(+), 72 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > > index 2b25fe679b0e..4c124bc98f39 100644 > > --- a/fs/xfs/xfs_icache.c > > +++ b/fs/xfs/xfs_icache.c > > @@ -29,6 +29,7 @@ > > /* Forward declarations to reduce indirect calls */ > > static int xfs_blockgc_scan_inode(struct xfs_inode *ip, > > struct xfs_eofblocks *eofb); > > +static inline void xfs_blockgc_queue(struct xfs_perag *pag); > > static bool xfs_reclaim_inode_grab(struct xfs_inode *ip); > > static void xfs_reclaim_inode(struct xfs_inode *ip, struct xfs_perag *pag); > > > > @@ -163,46 +164,78 @@ xfs_reclaim_work_queue( > > rcu_read_unlock(); > > } > > > > +/* Set a tag on both the AG incore inode tree and the AG radix tree. */ > > static void > > +xfs_perag_set_ici_tag( > > + struct xfs_perag *pag, > > + xfs_agino_t agino, > > + unsigned int tag) > > Looking at the callers - I think the logic to lookup the pag and set the > inode flag should also go in here. I deliberately didn't do that here because of what happens in the deferred inactivation patch. After calling xfs_inactive, we have to transition the inode from INACTIVATING to RECLAIMABLE (along with the radix tree tags) without anybody being able to see intermediate state: /* * Move the inode from the inactivation phase to the reclamation phase * by clearing both inactivation inode state flags and marking the * inode reclaimable. Schedule background reclaim to run later. */ spin_lock(&pag->pag_ici_lock); spin_lock(&ip->i_flags_lock); ip->i_flags &= ~(XFS_NEED_INACTIVE | XFS_INACTIVATING); ip->i_flags |= XFS_IRECLAIMABLE; xfs_perag_clear_ici_tag(pag, agino, XFS_ICI_INODEGC_TAG); xfs_perag_set_ici_tag(pag, agino, XFS_ICI_RECLAIM_TAG); spin_unlock(&ip->i_flags_lock); spin_unlock(&pag->pag_ici_lock); Which is why the xfs_perag_*_ici_tag callers are left in charge of looking up the pag and taking locks as needed. > Currently only xfs_inode_destroy > nests i_Flags log inside the pag_ici_lock, but I don't see how that > would harm the xfs_blockgc_set_iflag case. The other wart is that IEOFBLOCKS and ICOWBLOCKS share the same radix tree tag, which complicates the clearing logic, and I thought it best to let the callers deal with that. > I suspect the unlocked > check in xfs_blockgc_set_iflag would harm in the reclaim case either. "wouldn't"? > > > void > > +xfs_inode_destroy( > > I find this new name a little confusing. What about > xfs_inode_mark_reclaimable? Fixed. > But overall this new scheme looks nice to me. Thanks! --D
