On Thu, Feb 01, 2024 at 11:30:13AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> We need the XFS_NEED_INACTIVE flag to correspond to whether the
> inode is on the inodegc queues so that we can then use this state
> for lazy removal.
>
> To do this, move the addition of the inode to the inodegc queue
> under the ip->i_flags_lock so that it is atomic w.r.t. setting
> the XFS_NEED_INACTIVE flag.
>
> Then, when we remove the inode from the inodegc list to actually run
> inactivation, clear the XFS_NEED_INACTIVE at the same time we are
> setting XFS_INACTIVATING to indicate that inactivation is in
> progress.
>
> These changes result in all the state changes and inodegc queuing
> being atomic w.r.t. each other and inode lookups via the use of the
> ip->i_flags lock.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_icache.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 06046827b5fe..425b55526386 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -1875,7 +1875,12 @@ xfs_inodegc_worker(
> llist_for_each_entry_safe(ip, n, node, i_gclist) {
> int error;
>
> - xfs_iflags_set(ip, XFS_INACTIVATING);
> + /* Switch state to inactivating. */
> + spin_lock(&ip->i_flags_lock);
> + ip->i_flags |= XFS_INACTIVATING;
> + ip->i_flags &= ~XFS_NEED_INACTIVE;
The comment for XFS_INACTIVATING ought to be updated to state that
NEED_INACTIVE is cleared at the same time that INACTIVATING is set.
> + spin_unlock(&ip->i_flags_lock);
> +
> error = xfs_inodegc_inactivate(ip);
> if (error && !gc->error)
> gc->error = error;
> @@ -2068,9 +2073,13 @@ xfs_inodegc_queue(
> unsigned long queue_delay = 1;
>
> trace_xfs_inode_set_need_inactive(ip);
> +
> + /*
> + * Put the addition of the inode to the gc list under the
> + * ip->i_flags_lock so that the state change and list addition are
> + * atomic w.r.t. lookup operations under the ip->i_flags_lock.
> + */
> spin_lock(&ip->i_flags_lock);
> - ip->i_flags |= XFS_NEED_INACTIVE;
> - spin_unlock(&ip->i_flags_lock);
>
> cpu_nr = get_cpu();
> gc = this_cpu_ptr(mp->m_inodegc);
> @@ -2079,6 +2088,9 @@ xfs_inodegc_queue(
> WRITE_ONCE(gc->items, items + 1);
> shrinker_hits = READ_ONCE(gc->shrinker_hits);
>
> + ip->i_flags |= XFS_NEED_INACTIVE;
> + spin_unlock(&ip->i_flags_lock);
This change mostly makes sense to me, but is it necessary to move the
line that sets XFS_NEED_INACTIVE? This change extends the critical
section so that the llist_add and the flags update are atomic, so
couldn't this change reduce down to moving the spin_unlock call?
(IOWs I'm not sure if there's a subtlety here or if this is merely rough
draft syndrome.)
--D
> +
> /*
> * Ensure the list add is always seen by anyone who finds the cpumask
> * bit set. This effectively gives the cpumask bit set operation
> --
> 2.43.0
>
>
