Re: [PATCH 1/4] xfs: don't nest icloglock inside ic_callback_lock

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jun 22, 2021 at 02:06:01PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> It's completely unnecessary because callbacks are added to iclogs
> without holding the icloglock, hence no amount of ordering between
> the icloglock and ic_callback_lock will order the removal of
> callbacks from the iclog.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Seems reasonable so far -- we don't tangle the icloglock with the
callback lock anywhere like this.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index e93cac6b5378..bb4390942275 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2773,11 +2773,8 @@ static void
>  xlog_state_do_iclog_callbacks(
>  	struct xlog		*log,
>  	struct xlog_in_core	*iclog)
> -		__releases(&log->l_icloglock)
> -		__acquires(&log->l_icloglock)
>  {
>  	trace_xlog_iclog_callbacks_start(iclog, _RET_IP_);
> -	spin_unlock(&log->l_icloglock);
>  	spin_lock(&iclog->ic_callback_lock);
>  	while (!list_empty(&iclog->ic_callbacks)) {
>  		LIST_HEAD(tmp);
> @@ -2789,12 +2786,6 @@ xlog_state_do_iclog_callbacks(
>  		spin_lock(&iclog->ic_callback_lock);
>  	}
>  
> -	/*
> -	 * Pick up the icloglock while still holding the callback lock so we
> -	 * serialise against anyone trying to add more callbacks to this iclog
> -	 * now we've finished processing.
> -	 */
> -	spin_lock(&log->l_icloglock);
>  	spin_unlock(&iclog->ic_callback_lock);
>  	trace_xlog_iclog_callbacks_done(iclog, _RET_IP_);
>  }
> @@ -2836,13 +2827,12 @@ xlog_state_do_callback(
>  				iclog = iclog->ic_next;
>  				continue;
>  			}
> +			spin_unlock(&log->l_icloglock);
>  
> -			/*
> -			 * Running callbacks will drop the icloglock which means
> -			 * we'll have to run at least one more complete loop.
> -			 */
> -			cycled_icloglock = true;
>  			xlog_state_do_iclog_callbacks(log, iclog);
> +			cycled_icloglock = true;
> +
> +			spin_lock(&log->l_icloglock);
>  			if (XLOG_FORCED_SHUTDOWN(log))
>  				wake_up_all(&iclog->ic_force_wait);
>  			else
> -- 
> 2.31.1
>