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