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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
