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

[Brian Foster <bfoster@redhat.com>]

On Wed, Jun 23, 2021 at 08:42:47AM +1000, Dave Chinner wrote:
> On Tue, Jun 22, 2021 at 08:38:56AM -0400, Brian Foster wrote:
> > 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?
> 
> This was done in 2008. It's very likely that, at the time, nobody
> (including me) understood the iclog state machine well enough to
> determine if we could race with adding iclogs at this time. Maybe
> they did race and this was a bandaid over, say, a shutdown race condition.
> But, more likely, it was just defensive to try to prevent callbacks
> from being added before the iclog was marked ACTIVE again...
> 
> Really, though, nobody is going to be able to tell you why the code
> was written like this in the first place because even the author
> doesn't remember...
> 

Ok, just wanted to be sure there wasn't some context I was missing. The
patch seems fine to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>