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

[Brian Foster <bfoster@redhat.com>]

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>
> ---
>  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.
> -	 */

This makes sense wrt to the current locking, but I'd like to better
understand what's being removed. When would we add callbacks to an iclog
that's made it to this stage (i.e., already completed I/O)? Is this some
historical case or attempt at defensive logic?

Brian

> -	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
>