| Message ID | 162360482438.1530792.18197198406001465325.stgit@locust (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: deferred inode inactivation | expand |
On Sun, Jun 13, 2021 at 10:20:24AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > During review of the v6 deferred inode inactivation patchset[1], Dave > commented that _cache_hit should have a clear separation between inode > selection criteria and actions performed on a selected inode. Move a > hunk to make this true, and compact the shrink cases in the function. > > [1] https://lore.kernel.org/linux-xfs/162310469340.3465262.504398465311182657.stgit@locust/T/#mca6d958521cb88bbc1bfe1a30767203328d410b5 > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/xfs_icache.c | 39 ++++++++++++++++----------------------- > 1 file changed, 16 insertions(+), 23 deletions(-) > > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > index 7939eced3a47..4002f0b84401 100644 > --- a/fs/xfs/xfs_icache.c > +++ b/fs/xfs/xfs_icache.c > @@ -562,13 +562,8 @@ xfs_iget_cache_hit( > * will not match, so check for that, too. > */ > spin_lock(&ip->i_flags_lock); > - if (ip->i_ino != ino) { > - trace_xfs_iget_skip(ip); > - XFS_STATS_INC(mp, xs_ig_frecycle); > - error = -EAGAIN; > - goto out_error; > - } > - > + if (ip->i_ino != ino) > + goto out_skip; > > /* > * If we are racing with another cache hit that is currently > @@ -580,12 +575,8 @@ xfs_iget_cache_hit( > * wait_on_inode to wait for these flags to be cleared > * instead of polling for it. > */ > - if (ip->i_flags & (XFS_INEW|XFS_IRECLAIM)) { > - trace_xfs_iget_skip(ip); > - XFS_STATS_INC(mp, xs_ig_frecycle); > - error = -EAGAIN; > - goto out_error; > - } > + if (ip->i_flags & (XFS_INEW | XFS_IRECLAIM)) > + goto out_skip; > > /* > * Check the inode free state is valid. This also detects lookup > @@ -595,23 +586,21 @@ xfs_iget_cache_hit( > if (error) > goto out_error; > > + /* Skip inodes that have no vfs state. */ > + if ((flags & XFS_IGET_INCORE) && > + (ip->i_flags & XFS_IRECLAIMABLE)) > + goto out_skip; > + > + /* The inode fits the selection criteria; process it. */ > if (ip->i_flags & XFS_IRECLAIMABLE) { > - if (flags & XFS_IGET_INCORE) { > - error = -EAGAIN; > - goto out_error; > - } > - > /* Drops i_flags_lock and RCU read lock. */ > error = xfs_iget_recycle(pag, ip); > if (error) > return error; > } else { > /* If the VFS inode is being torn down, pause and try again. */ > - if (!igrab(inode)) { > - trace_xfs_iget_skip(ip); > - error = -EAGAIN; > - goto out_error; > - } > + if (!igrab(inode)) > + goto out_skip; > > /* We've got a live one. */ > spin_unlock(&ip->i_flags_lock); > @@ -628,6 +617,10 @@ xfs_iget_cache_hit( > > return 0; > > +out_skip: > + trace_xfs_iget_skip(ip); > + XFS_STATS_INC(mp, xs_ig_frecycle); > + error = -EAGAIN; > out_error: > spin_unlock(&ip->i_flags_lock); > rcu_read_unlock(); >
On Sun, Jun 13, 2021 at 10:20:24AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > During review of the v6 deferred inode inactivation patchset[1], Dave > commented that _cache_hit should have a clear separation between inode > selection criteria and actions performed on a selected inode. Move a > hunk to make this true, and compact the shrink cases in the function. I'm not sure the backstore really belongs here vs describing what changes for what reason in the code. It might be worth that this is now consistently calling the tracepoints and incrementing xs_ig_frecycle, though. But the actual changes look like a really nice improvement, so: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index 7939eced3a47..4002f0b84401 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -562,13 +562,8 @@ xfs_iget_cache_hit( * will not match, so check for that, too. */ spin_lock(&ip->i_flags_lock); - if (ip->i_ino != ino) { - trace_xfs_iget_skip(ip); - XFS_STATS_INC(mp, xs_ig_frecycle); - error = -EAGAIN; - goto out_error; - } - + if (ip->i_ino != ino) + goto out_skip; /* * If we are racing with another cache hit that is currently @@ -580,12 +575,8 @@ xfs_iget_cache_hit( * wait_on_inode to wait for these flags to be cleared * instead of polling for it. */ - if (ip->i_flags & (XFS_INEW|XFS_IRECLAIM)) { - trace_xfs_iget_skip(ip); - XFS_STATS_INC(mp, xs_ig_frecycle); - error = -EAGAIN; - goto out_error; - } + if (ip->i_flags & (XFS_INEW | XFS_IRECLAIM)) + goto out_skip; /* * Check the inode free state is valid. This also detects lookup @@ -595,23 +586,21 @@ xfs_iget_cache_hit( if (error) goto out_error; + /* Skip inodes that have no vfs state. */ + if ((flags & XFS_IGET_INCORE) && + (ip->i_flags & XFS_IRECLAIMABLE)) + goto out_skip; + + /* The inode fits the selection criteria; process it. */ if (ip->i_flags & XFS_IRECLAIMABLE) { - if (flags & XFS_IGET_INCORE) { - error = -EAGAIN; - goto out_error; - } - /* Drops i_flags_lock and RCU read lock. */ error = xfs_iget_recycle(pag, ip); if (error) return error; } else { /* If the VFS inode is being torn down, pause and try again. */ - if (!igrab(inode)) { - trace_xfs_iget_skip(ip); - error = -EAGAIN; - goto out_error; - } + if (!igrab(inode)) + goto out_skip; /* We've got a live one. */ spin_unlock(&ip->i_flags_lock); @@ -628,6 +617,10 @@ xfs_iget_cache_hit( return 0; +out_skip: + trace_xfs_iget_skip(ip); + XFS_STATS_INC(mp, xs_ig_frecycle); + error = -EAGAIN; out_error: spin_unlock(&ip->i_flags_lock); rcu_read_unlock();